[U-Boot Patch v2 2/4] spi: fu540: add claim and release method to spi-sifive.c

[Sagar Kadam]

Hello Bin,

> -----Original Message-----
> From: Bin Meng <bmeng.cn at gmail.com>
> Sent: Tuesday, February 4, 2020 5:43 PM
> To: Sagar Kadam <sagar.kadam at sifive.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Rick Chen
> <rick at andestech.com>; Paul Walmsley ( Sifive) <paul.walmsley at sifive.com>;
> Jagan Teki <jagan at amarulasolutions.com>; Anup Patel
> <anup.patel at wdc.com>
> Subject: Re: [U-Boot Patch v2 2/4] spi: fu540: add claim and release method
> to spi-sifive.c
> 
> Hi Sagar,
> 
> On Wed, Jan 29, 2020 at 2:02 AM Sagar Shrikant Kadam
> <sagar.kadam at sifive.com> wrote:
> >
> > Add missing bus claim/release method to spi driver for HiFive
> > Unleashed, and handle num_cs generously so that it generates an error
> > if invalid cs number is passed to sf probe.
> 
> Is adding the claim/release method the fix to the weird "sf probe 0:2/4/6/8"
> issue?
> 
Please see below.
> >
> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com>
> > ---
> >  drivers/spi/spi-sifive.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c index
> > 969bd4b..f990ad6 100644
> > --- a/drivers/spi/spi-sifive.c
> > +++ b/drivers/spi/spi-sifive.c
> > @@ -186,6 +186,36 @@ static void sifive_spi_tx(struct sifive_spi *spi, const
> u8 *tx_ptr)
> >         writel(tx_data, spi->regs + SIFIVE_SPI_REG_TXDATA);  }
> >
> > +static int sifive_spi_claim_bus(struct udevice *dev) {
> > +       int ret;
> > +       struct udevice *bus = dev->parent;
> > +       struct sifive_spi *spi = dev_get_priv(bus);
> > +       struct dm_spi_slave_platdata *slave =
> > +dev_get_parent_platdata(dev);
> > +
> > +       if (!(slave->cs < spi->num_cs)) {
> 
> slave->cs >= spi->num_cs
> 
> But there is already check added recently in the spi-uclass driver, see below:
> 
> commit 7bacce524d48594dae399f9ee9280ab105f6c8cf
> Author: Bin Meng <bmeng.cn at gmail.com>
> Date:   Mon Sep 9 06:00:02 2019 -0700
> 
>     dm: spi: Check cs number before accessing slaves
> 
>     Add chip select number check in spi_find_chip_select().
> 
>     Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>     Tested-by: Jagan Teki <jagan at amarulasolutions.com> # SoPine
> 
> Adding check in the sifive spi driver seems to unnecessary. Could you please
> confirm?
> 
Ahh!!. Thanks for the pointer Bin.
This V2 patch here is based on commit c05b38df477a    ("common: fix regression
on block cache init"), so didn't come across the patch you pointed out. 
So for now we can skip the check in sifive spi driver.

> > +               printf("Invalid cs number = %d\n", slave->cs);
> > +               return -EINVAL;
> > +       }
> > +
> > +       sifive_spi_prep_device(spi, slave);
> > +
> > +       ret = sifive_spi_set_cs(spi, slave);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> > +static int sifive_spi_release_bus(struct udevice *dev) {
> > +       struct sifive_spi *spi = dev_get_priv(dev->parent);
> > +
> > +       sifive_spi_clear_cs(spi);
> > +
> > +       return 0;
> > +}
> 
> What was done in the sifive_spi_claim_bus / sifive_spi_release_bus seems to
> be already done in sifive_spi_xfer(). See flags testing against SPI_XFER_BEGIN
> and SPI_XFER_END.
> 
> Could you please explain a little bit on why adding these 2 fixes things?
> 
What I saw was that sf probe was detecting flash even at wrong chip select inputs,
Like sf probe 0:2/4/6/.. this indicated that a check to validate chip selects needs to be
there.  The gist of adding the claim function was to introduce this chip select check.
The code within SPI_XFER_BEGIN and END functions missed this check. 
Now that the commit your pointed 7bacce524d48 handles this, I think
we can drop this check as of now. 

> > +
> >  static int sifive_spi_xfer(struct udevice *dev, unsigned int bitlen,
> >                            const void *dout, void *din, unsigned long
> > flags)  { @@ -345,6 +375,10 @@ static int sifive_spi_probe(struct
> > udevice *bus)
> >         /* init the sifive spi hw */
> >         sifive_spi_init_hw(spi);
> >
> > +       /* Fetch number of chip selects from DT if present */
> > +       ret = dev_read_u32_default(bus, "num-cs", spi->num_cs);
> > +       spi->num_cs = ret;
> 
> spi->num_cs is already assigned a value in sifive_spi_init_hw(), Why
> do we override the value using DT's?
>
Yes, spi_init_hw does initialise the spi->num_cs by reading the register.
For QSPI0 and QSPI2 it is set to 1 (as per the cs_width). But for QSPI1 it is
set to 4. Consider a case where user wishes to populate only 1 chip select for QSPI1
then it can be done in respective <board>-dts file and can be updated after sifive_spi_init_hw
is done. If the board device tree doesn't contain any such entry than above API will retain the
value of spi->num_cs populated in sifive_spi_init_hw().
So to summarize, we can drop claim/release addition from this patch and just 
keep the dev_read_u32_default function done above.
Please let me know your opinion here.
 
Thanks & BR,
Sagar

> > +
> >         return 0;
> >  }
> >
> > @@ -353,6 +387,8 @@ static const struct dm_spi_ops sifive_spi_ops = {
> >         .set_speed      = sifive_spi_set_speed,
> >         .set_mode       = sifive_spi_set_mode,
> >         .cs_info        = sifive_spi_cs_info,
> > +       .claim_bus      = sifive_spi_claim_bus,
> > +       .release_bus    = sifive_spi_release_bus,
> >  };
> >
> >  static const struct udevice_id sifive_spi_ids[] = {
> > --
> 
> Regards,
> Bin