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