[PATCH v2 1/2] mtd: fsl-quadspi: Update slave device hwcap based on mode provided in dtsi file
Yogesh Narayan Gaur
yogeshnarayan.gaur at nxp.com
Mon Feb 12 22:45:21 PST 2018
Hi Cyrille,
> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen at wedev4u.fr]
> Sent: Tuesday, February 06, 2018 2:19 AM
> To: Yogesh Narayan Gaur <yogeshnarayan.gaur at nxp.com>; linux-
> mtd at lists.infradead.org; devicetree at vger.kernel.org; robh at kernel.org;
> mark.rutland at arm.com; shawnguo at kernel.org
> Cc: linux-arm-kernel at lists.infradead.org; boris.brezillon at free-electrons.com;
> computersforpeace at gmail.com; Han Xu <han.xu at nxp.com>;
> festevam at gmail.com; Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>;
> Suresh Gupta <suresh.gupta at nxp.com>
> Subject: Re: [PATCH v2 1/2] mtd: fsl-quadspi: Update slave device hwcap based
> on mode provided in dtsi file
>
> Hi Yogesh,
>
> Le 02/02/2018 à 06:31, Yogesh Gaur a écrit :
> > FSL QuadSPI controller supports Single, dual, quad modes of operation.
> > Mode information is available via "spi-tx-bus-width" and
> > "spi-rx-bus-width" nodes of device tree for the connected slave device.
> >
> > Update read and write hwcap capability for slave device by reading
> > "spi-rx-bus-width" and "spi-tx-bus-width" respectively.
> > Assign hwcaps mask to minimal caps for the slave node i.e.
> > SNOR_HWCAPS_READ | SNOR_HWCAPS_PP
> >
> > If value not provided in device tree file, then fallback to default
> > mode i.e. SNOR_HWCAPS_READ_1_1_4 and SNOR_HWCAPS_PP.
> >
> > Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>
> > Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur at nxp.com>
> > ---
> > Changes for v2:
> > - None
> >
> > drivers/mtd/spi-nor/fsl-quadspi.c | 56
> > +++++++++++++++++++++++++++++++++++----
> > 1 file changed, 51 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c
> > b/drivers/mtd/spi-nor/fsl-quadspi.c
> > index b9c5918..1503e0c 100644
> > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > @@ -994,17 +994,14 @@ static void fsl_qspi_unprep(struct spi_nor *nor,
> > enum spi_nor_ops ops)
> >
> > static int fsl_qspi_probe(struct platform_device *pdev) {
> > - const struct spi_nor_hwcaps hwcaps = {
> > - .mask = SNOR_HWCAPS_READ_1_1_4 |
> > - SNOR_HWCAPS_PP,
> > - };
> > + struct spi_nor_hwcaps hwcaps;
> > struct device_node *np = pdev->dev.of_node;
> > struct device *dev = &pdev->dev;
> > struct fsl_qspi *q;
> > struct resource *res;
> > struct spi_nor *nor;
> > struct mtd_info *mtd;
> > - int ret, i = 0;
> > + int ret, i = 0, value;
> >
> > q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
> > if (!q)
> > @@ -1077,6 +1074,10 @@ static int fsl_qspi_probe(struct
> > platform_device *pdev)
> >
> > /* iterate the subnodes. */
> > for_each_available_child_of_node(dev->of_node, np) {
> > + /* Reset hwcaps mask to minimal caps for the slave node. */
> > + hwcaps.mask = SNOR_HWCAPS_READ | SNOR_HWCAPS_PP;
> > + value = 0;
> > +
> > /* skip the holes */
> > if (!q->has_second_chip)
> > i *= 2;
> > @@ -1106,6 +1107,51 @@ static int fsl_qspi_probe(struct platform_device
> *pdev)
> > /* set the chip address for READID */
> > fsl_qspi_set_base_addr(q, nor);
> >
> > + /*
> > + * If spi-rx-bus-width and spi-tx-bus-width not defined assign
> > + * default hardware capabilities SNOR_HWCAPS_READ_1_1_4
> and
> > + * SNOR_HWCAPS_PP supported by the Quad-SPI controller.
> > + */
> > + if (!of_property_read_u32(np, "spi-rx-bus-width", &value)) {
> > + switch (value) {
> > + case 1:
> > + hwcaps.mask |= SNOR_HWCAPS_READ |
> > + SNOR_HWCAPS_READ_FAST;
> > + break;
> > + case 2:
> > + hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2
> |
> > + SNOR_HWCAPS_READ_1_2_2;
>
> I guess there is a misunderstanding in the meaning of spi-rx-bus-width and spi-
> tx-bus_width.
>
> spi-rx-bus-width = <N>, resp. spi-tx-bus-width = <N>, means the SPI controller
> can receive, resp. send, data on N I/O Lines.
>
> Then let's take the example of some Fast Read X-Y-Z command:
>
> the op code is *sent* on X I/O lines: you need to check spi-tx-bus-width the
> address/mode/dummy bits are *sent* on Y I/O lines: check spi-tx-bus-width too
> the data bits are *received* on Z I/O lines: check spi-rx-bus_width.
>
> > + break;
> > + case 4:
> > + hwcaps.mask |= SNOR_HWCAPS_READ_1_1_4
> |
> > + SNOR_HWCAPS_READ_1_4_4;
>
> The same here: to allow SPI 1-4-4, you have to check that spi-tx-bus-width == 4.
>
> So your usage of spi-{tx|rx}-bus-width is not consistent with the usage done in
> the SPI subsystem. Please have a look at drivers/mtd/devices/m25p80.c,
> especially how the SPI_RX_QUAD and SPI_TX_QUAD flags are used in the
> m25p_probe() function to have a better idea of how it should work.
>
> Also have a look at of_spi_parse_dt() in drivers/spi/spi.c, to figure out how the
> SPI_{TX|RX}_{DUAL|QUAD} flags are set according to the spi-{tx|rx}-bus-width
> DT properties.
>
Thanks for review, would incorporate your review comments and would have usage of spi-{rx/tx}-bus-width as being done in SPI subsystem.
> > + break;
> > + default:
> > + dev_err(dev,
> > + "spi-rx-bus-width %d not supported\n",
> > + value);
> > + break;
> > + }
> > + } else
> > + hwcaps.mask |= SNOR_HWCAPS_READ_1_1_4;
>
> Not sure whether it passes checkpatch, you may have to add { } for the else
> statement even if there is a single line as the if statement use { }.
>
I have run ' ./scripts/checkpatch.pl' present in linux-mtd code base and it hasn't reported any warning or error messages.
Meanwhile, would add {} braces for this else statement also.
--
Regards
Yogesh Gaur.
> Personally I don't mind that much. Please just check that your patch passes
> checkpatch verification and it will be okay for me :)
>
> Best regards,
>
> Cyrille
>
> > +
> > + if (!of_property_read_u32(np, "spi-tx-bus-width", &value)) {
> > + switch (value) {
> > + case 1:
> > + hwcaps.mask |= SNOR_HWCAPS_PP;
> > + break;
> > + case 4:
> > + hwcaps.mask |= SNOR_HWCAPS_PP_1_1_4 |
> > + SNOR_HWCAPS_PP_1_4_4;
> > + break;
> > + default:
> > + dev_err(dev,
> > + "spi-tx-bus-width %d not supported\n",
> > + value);
> > + break;
> > + }
> > + }
> > +
> > ret = spi_nor_scan(nor, NULL, &hwcaps);
> > if (ret)
> > goto mutex_failed;
> >
More information about the linux-arm-kernel
mailing list