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

Simon Glass sjg at chromium.org
Tue Oct 2 11:21:59 UTC 2018


Hi Angelo,

On 28 September 2018 at 04:22, Angelo Dureghello <angelo at sysam.it> wrote:
> 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 at 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 at 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 at 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 at 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 ?

Yes you can migrate them forcibly since the alternative is presumably
to delete their support from mainline.

Regards,
Simon


More information about the U-Boot mailing list