Hi Angelo,
On 26 September 2018 at 11:53, Angelo Dureghello <ang...@sysam.it> wrote:
> Hi Simon,
>
> thanks for the review.
>
> On Tue, Sep 25, 2018 at 10:42:08PM -0700, Simon Glass wrote:
>> Hi Angelo,
>>
>> On 20 September 2018 at 15:07, Angelo Dureghello <ang...@sysam.it> wrote:
>> > This patch converts cf_spi.c to DM and to read driver
>> > platform data from flat devicetree.
>> >
>> > ---
>> > Changes from v1:
>> > - split into 2 patches
>> >
>> > Signed-off-by: Angelo Dureghello <ang...@sysam.it>
>> > ---
>> > drivers/spi/Kconfig | 18 +-
>> > drivers/spi/cf_spi.c | 495 ++++++++++++++++--------
>> > include/dm/platform_data/spi_coldfire.h | 25 ++
>> > 3 files changed, 369 insertions(+), 169 deletions(-)
>> > create mode 100644 include/dm/platform_data/spi_coldfire.h
>> >
>>
>> Good to see this.
>>
>> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> > index dcd719ff0a..974c5bbed8 100644
>> > --- a/drivers/spi/Kconfig
>> > +++ b/drivers/spi/Kconfig
>> > @@ -80,6 +80,12 @@ config CADENCE_QSPI
>> > used to access the SPI NOR flash on platforms embedding this
>> > Cadence IP core.
>> >
>> > +config CF_SPI
>> > + bool "ColdFire SPI driver"
>> > + help
>> > + Enable the ColdFire SPI driver. This driver can be used on
>> > + some m68k SoCs.
>> > +
>> > config DESIGNWARE_SPI
>> > bool "Designware SPI driver"
>> > help
>> > @@ -244,18 +250,18 @@ config ZYNQMP_GQSPI
>> >
>> > endif # if DM_SPI
>> >
>> > -config SOFT_SPI
>> > - bool "Soft SPI driver"
>> > - help
>> > - Enable Soft SPI driver. This driver is to use GPIO simulate
>> > - the SPI protocol.
>>
>> How come this is changing? That should be a separate patch.
>>
> I just respected Kconfig alphabetical order, SOFT_SPI is just moved after.
OK, well I do think this should be in a separate patch.
>
>> > -
>> > config CF_SPI
>> > bool "ColdFire SPI driver"
>> > help
>> > Enable the ColdFire SPI driver. This driver can be used on
>> > some m68k SoCs.
>> >
>> > +config SOFT_SPI
>> > + bool "Soft SPI driver"
>> > + help
>> > + Enable Soft SPI driver. This driver is to use GPIO simulate
>> > + the SPI protocol.
>> > +
>> > config FSL_ESPI
>> > bool "Freescale eSPI driver"
>> > help
>> > diff --git a/drivers/spi/cf_spi.c b/drivers/spi/cf_spi.c
>> > index 522631cbbf..11a11f79c4 100644
>> > --- a/drivers/spi/cf_spi.c
>> > +++ b/drivers/spi/cf_spi.c
>> > @@ -6,16 +6,27 @@
>> > *
>> > * Copyright (C) 2004-2009 Freescale Semiconductor, Inc.
>> > * TsiChung Liew (tsi-chung.l...@freescale.com)
>> > + *
>> > + * Support for device model:
>> > + * Copyright (C) 2018 Angelo Dureghello <ang...@sysam.it>
>> > + *
>> > */
>> >
>> > #include <common.h>
>> > +#include <dm.h>
>> > +#include <dm/platform_data/spi_coldfire.h>
>> > #include <spi.h>
>> > #include <malloc.h>
>> > #include <asm/immap.h>
>> > +#include <asm/io.h>
>> >
>> > -struct cf_spi_slave {
>> > +struct coldfire_spi_priv {
>> > +#ifndef CONFIG_DM_SPI
>> > struct spi_slave slave;
>> > +#endif
>> > + struct dspi *regs;
>> > uint baudrate;
>> > + int mode;
>> > int charbit;
>> > };
>> >
>> > @@ -38,14 +49,14 @@ DECLARE_GLOBAL_DATA_PTR;
>> > #define SPI_MODE_MOD 0x00200000
>> > #define SPI_DBLRATE 0x00100000
>> >
>> > -static inline struct cf_spi_slave *to_cf_spi_slave(struct spi_slave
>> > *slave)
>> > -{
>> > - return container_of(slave, struct cf_spi_slave, slave);
>> > -}
>> > +/* Default values */
>> > +#define MCF_DSPI_DEFAULT_SCK_FREQ 10000000
>> > +#define MCF_DSPI_MAX_CHIPSELECTS 4
>> > +#define MCF_DSPI_MODE 0
>> >
>> > -static void cfspi_init(void)
>> > +static void __spi_init(struct coldfire_spi_priv *cfspi)
>> > {
>> > - volatile dspi_t *dspi = (dspi_t *) MMAP_DSPI;
>> > + struct dspi *dspi = cfspi->regs;
>> >
>> > cfspi_port_conf(); /* port configuration */
>> >
>> > @@ -56,125 +67,32 @@ static void cfspi_init(void)
>> >
>> > /* Default setting in platform configuration */
>> > #ifdef CONFIG_SYS_DSPI_CTAR0
>> > - dspi->ctar[0] = CONFIG_SYS_DSPI_CTAR0;
>> > + writel(CONFIG_SYS_DSPI_CTAR0, &dspi->ctar[0]);
>>
>> What is going on here? I think these CONFIG options are addresses? If
>> so, they should be read from the DT, not a CONFIG.
>>
>
> These are just default settings for each channel (bus), actually coming
> from the include/configs/boardxxx.h. Their speed an mode bitfields are
> rewritten later, with values coming from devicetree.
> Some driver #define the default value inside the driver itself, in case
> i may change in this way. No one seems reading them from device tree.
OK, can we remove these? At least they should not have a CONFIG_
prefix, so we can remove them from the whitelist.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
