[U-Boot] [PATCH 2/7] drivers: spi: cf_spi: migrate to DM and DT

Angelo Dureghello
2018-09-26T18:53:35Z

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 <angelo@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 <angelo@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.

> > -
> > 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.Liew at freescale.com)
> > + *
> > + * Support for device model:
> > + * Copyright (C) 2018 Angelo Dureghello <angelo@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.

> > #endif
> > #ifdef CONFIG_SYS_DSPI_CTAR1
> > - dspi->ctar[1] = CONFIG_SYS_DSPI_CTAR1;
> > + writel(CONFIG_SYS_DSPI_CTAR1, &dspi->ctar[1]);
> > #endif
> > #ifdef CONFIG_SYS_DSPI_CTAR2
> > - dspi->ctar[2] = CONFIG_SYS_DSPI_CTAR2;
> > + writel(CONFIG_SYS_DSPI_CTAR2, &dspi->ctar[2]);
> > #endif
> > #ifdef CONFIG_SYS_DSPI_CTAR3
> > - dspi->ctar[3] = CONFIG_SYS_DSPI_CTAR3;
> > + writel(CONFIG_SYS_DSPI_CTAR3, &dspi->ctar[3]);
> > #endif
> > #ifdef CONFIG_SYS_DSPI_CTAR4
> > - dspi->ctar[4] = CONFIG_SYS_DSPI_CTAR4;
> > + writel(CONFIG_SYS_DSPI_CTAR4, &dspi->ctar[4]);
> > #endif
> > #ifdef CONFIG_SYS_DSPI_CTAR5
> > - dspi->ctar[5] = CONFIG_SYS_DSPI_CTAR5;
> > + writel(CONFIG_SYS_DSPI_CTAR5, &dspi->ctar[5]);
> > #endif
> > #ifdef CONFIG_SYS_DSPI_CTAR6
> > - dspi->ctar[6] = CONFIG_SYS_DSPI_CTAR6;
> > + writel(CONFIG_SYS_DSPI_CTAR6, &dspi->ctar[6]);
> > #endif
> > #ifdef CONFIG_SYS_DSPI_CTAR7
> > - dspi->ctar[7] = CONFIG_SYS_DSPI_CTAR7;
> > + writel(CONFIG_SYS_DSPI_CTAR7, &dspi->ctar[7]);
> > #endif
> > }
> >
>
> [..]
>
> Regards,
> Simon

Regards,
Angelo