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

Angelo Dureghello
2018-09-28T11:22:50Z

Hi Simon,

On Thu, Sep 27, 2018 at 06:41:37AM -0700, Simon Glass wrote:
> Hi Angelo,
>
> On 26 September 2018 at 11:53, Angelo Dureghello <angelo@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 <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.
>
> OK, well I do think this should be in a separate patch.
>
this is done, ready into a v2

> >
> >> > -
> >> > 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.
>
> OK, can we remove these? At least they should not have a CONFIG_
> prefix, so we can remove them from the whitelist.
>

I verified that in particular 1 m68k board (ls1012aqds.h) wants different
defaults as cs-clock delays. This is something that atually can only be
done by those CONFIG_SYS_DSPI_CTARX.

This settings may be moved into DT but all the related boards should have
been moved to use a dts. Not sure if i can do this now, since i cannot
test DT migration without owning the related physical board (hw).

How does it work in general ? Should i move al boards to dts, leaving
tests to board maintaners in the future ? Can we keep those
CONFIG_SYS_DSPI_CTARX in this way and perform the all-boards conversion
to dts in a later step ?

> Regards,
> Simon

Regards,
Angelo