Re: [U-Boot Patch v2 2/4] spi: fu540: add claim and release method to spi-sifive.c
Hi Sagar,
On Fri, Feb 7, 2020 at 2:25 AM Sagar Kadam wrote:
>
> Hello Bin,
>
> > -Original Message-
> > From: Bin Meng
> > Sent: Tuesday, February 4, 2020 5:43 PM
> > To: Sagar Kadam
> > Cc: U-Boot Mailing List ; Rick Chen
> > ; Paul Walmsley ( Sifive) ;
> > Jagan Teki ; Anup Patel
> >
> > 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
> > 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
> > > ---
> > > 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
> > 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
> > Tested-by: Jagan Teki # 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
RE: [U-Boot Patch v2 2/4] spi: fu540: add claim and release method to spi-sifive.c
Hello Bin,
> -Original Message-
> From: Bin Meng
> Sent: Tuesday, February 4, 2020 5:43 PM
> To: Sagar Kadam
> Cc: U-Boot Mailing List ; Rick Chen
> ; Paul Walmsley ( Sifive) ;
> Jagan Teki ; Anup Patel
>
> 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
> 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
> > ---
> > 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
> 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
> Tested-by: Jagan Teki # 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 -dts file and c
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
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?
>
> Signed-off-by: Sagar Shrikant Kadam
> ---
> 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
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
Tested-by: Jagan Teki # SoPine
Adding check in the sifive spi driver seems to unnecessary. Could you
please confirm?
> + 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?
> +
> 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?
> +
> 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
[U-Boot Patch v2 2/4] spi: fu540: add claim and release method to spi-sifive.c
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.
Signed-off-by: Sagar Shrikant Kadam
---
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)) {
+ 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;
+}
+
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;
+
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[] = {
--
2.7.4
