| Message ID | 1515685478-31457-1-git-send-email-fabio.estevam@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | [v4] mtd: fsl-quadspi: Distinguish the mtd device names | expand |
On Thu, 11 Jan 2018 13:44:38 -0200 Fabio Estevam <fabio.estevam@nxp.com> wrote: > Currently on a imx6sx-sdb board, which has two SPI NOR chips connected > to QSPI2 the following output from /proc/mtd is seen: > > # cat /proc/mtd > dev: size erasesize name > mtd0: 01000000 00010000 "21e4000.qspi" > mtd1: 01000000 00010000 "21e4000.qspi" > > Attempts to partition them on the kernel command line result in both > chips with identical (and identically named) partitions, which is > an inconvenient behavior. > > Assign a different mtd->name for each mtd device to avoid this problem. > > After this change the output from /proc/mtd becomes: > > # cat /proc/mtd > dev: size erasesize name > mtd0: 01000000 00010000 "21e4000.qspi-0" > mtd1: 01000000 00010000 "21e4000.qspi-1" > > Reported-by: David Wolfe <david.wolfe@nxp.com> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- > Changes since v1: > - Do not fail probe when reg is not present > > drivers/mtd/spi-nor/fsl-quadspi.c | 20 +++++++++++++++++++- Still missing the DT doc update. You can do it in a separate patch, but please make it part of the same series. > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c > index 2901c7b..8020295 100644 > --- a/drivers/mtd/spi-nor/fsl-quadspi.c > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c > @@ -967,7 +967,7 @@ static int fsl_qspi_probe(struct platform_device *pdev) > struct resource *res; > struct spi_nor *nor; > struct mtd_info *mtd; > - int ret, i = 0; > + int ret, i = 0, spiflash_idx; > > q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL); > if (!q) > @@ -1051,6 +1051,24 @@ static int fsl_qspi_probe(struct platform_device *pdev) > spi_nor_set_flash_node(nor, np); > nor->priv = q; > > + if (!mtd->name) { Move the spiflash_idx declaration here. > + ret = of_property_read_u32(np, "reg", &spiflash_idx); > + if (!ret) { > + mtd->name = devm_kasprintf(dev, GFP_KERNEL, > + "%s-%d", > + dev_name(dev), > + spiflash_idx); > + } else { > + mtd->name = dev_name(dev); > + dev_warn(dev, "reg property is missing\n"); > + } > + > + if (!mtd->name) { > + ret = -ENOMEM; > + goto mutex_failed; > + } > + } > + > /* fill the hooks */ > nor->read_reg = fsl_qspi_read_reg; > nor->write_reg = fsl_qspi_write_reg;
Hi Boris, On Thu, Jan 11, 2018 at 1:48 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Still missing the DT doc update. You can do it in a separate patch, but > please make it part of the same series. The dt-binding update part is not very clear to me. Currently on the imx6sx-sdb.dts we have: &qspi2 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_qspi2>; status = "okay"; flash0: n25q256a@0 { #address-cells = <1>; #size-cells = <1>; compatible = "micron,n25q256a", "jedec,spi-nor"; spi-max-frequency = <29000000>; reg = <0>; }; flash1: n25q256a@1 { #address-cells = <1>; #size-cells = <1>; compatible = "micron,n25q256a", "jedec,spi-nor"; spi-max-frequency = <29000000>; reg = <1>; }; }; The reg requirement is already present as part of Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt. Are you proposing that I add a comment to Documentation/devicetree/bindings/mtd/fsl-quadspi.txt saying that the Flash sub-nodes must contain a reg property? Please clarify. Thanks
On Thu, 11 Jan 2018 13:59:44 -0200 Fabio Estevam <festevam@gmail.com> wrote: > Hi Boris, > > On Thu, Jan 11, 2018 at 1:48 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > > Still missing the DT doc update. You can do it in a separate patch, but > > please make it part of the same series. > > The dt-binding update part is not very clear to me. > > Currently on the imx6sx-sdb.dts we have: > > &qspi2 { > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_qspi2>; > status = "okay"; > > flash0: n25q256a@0 { > #address-cells = <1>; > #size-cells = <1>; > compatible = "micron,n25q256a", "jedec,spi-nor"; > spi-max-frequency = <29000000>; > reg = <0>; > }; > > flash1: n25q256a@1 { > #address-cells = <1>; > #size-cells = <1>; > compatible = "micron,n25q256a", "jedec,spi-nor"; > spi-max-frequency = <29000000>; > reg = <1>; > }; > }; > > > The reg requirement is already present as part of > Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt. > > Are you proposing that I add a comment to > Documentation/devicetree/bindings/mtd/fsl-quadspi.txt saying that the > Flash sub-nodes must contain a reg property? Nope, I wasn't sure whether reg was mandatory or not since it was not used by the driver before your patch. This being said, that'd be good to update the example you have in the bindings do to fully describe a flash device. Anyway, if all existing DTs have a reg defined, even those where only one flash device is described, then your patch might break mtdparts users. And if this is not the case, and the reg property is really mandatory, then that means those dts are not compliant with the DT bindings and have to be patched :-). So, the solution of testing the presence of a reg property to choose the naming scheme is probably not appropriate.
On Thu, Jan 11, 2018 at 2:40 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Nope, I wasn't sure whether reg was mandatory or not since it was not > used by the driver before your patch. This being said, that'd be good to > update the example you have in the bindings do to fully describe a > flash device. Ok, I can update fsl-quadspi.txt to include an example for describing the flash devices. > Anyway, if all existing DTs have a reg defined, even those where only As far as I can see all DTs that use fsl-quadspi have a reg property in the SPI flash sub-nodes: arch/arm/boot/dts/imx6ul-14x14-evk.dts arch/arm/boot/dts/imx6sx-sdb.dts arch/arm/boot/dts/ls1021a-moxa-uc-8410a.dts arch/arm64/boot/dts/freescale/fsl-ls1046a-qds.dts > one flash device is described, then your patch might break mtdparts > users. And if this is not the case, and the reg property is really > mandatory, then that means those dts are not compliant with the DT > bindings and have to be patched :-). So, the solution of testing the > presence of a reg property to choose the naming scheme is probably not > appropriate. Any ideas on how we can properly solve this?
On Thu, 11 Jan 2018 14:58:39 -0200 Fabio Estevam <festevam@gmail.com> wrote: > On Thu, Jan 11, 2018 at 2:40 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > > Nope, I wasn't sure whether reg was mandatory or not since it was not > > used by the driver before your patch. This being said, that'd be good to > > update the example you have in the bindings do to fully describe a > > flash device. > > Ok, I can update fsl-quadspi.txt to include an example for describing > the flash devices. > > > Anyway, if all existing DTs have a reg defined, even those where only > > As far as I can see all DTs that use fsl-quadspi have a reg property > in the SPI flash sub-nodes: > > arch/arm/boot/dts/imx6ul-14x14-evk.dts > arch/arm/boot/dts/imx6sx-sdb.dts > arch/arm/boot/dts/ls1021a-moxa-uc-8410a.dts > arch/arm64/boot/dts/freescale/fsl-ls1046a-qds.dts > > > one flash device is described, then your patch might break mtdparts > > users. And if this is not the case, and the reg property is really > > mandatory, then that means those dts are not compliant with the DT > > bindings and have to be patched :-). So, the solution of testing the > > presence of a reg property to choose the naming scheme is probably not > > appropriate. > > Any ideas on how we can properly solve this? Nothing that I really like, sorry. One solution would be to use dev_name(&pdev->dev) when only one flash device is declared and use dev_name(&pdev->dev)-reg_val otherwise. Or you could leave the logic unchanged and force users to specify a label property when they have more than one device. The last solution would be to actually break mtdparts users so that you can start using a sane naming scheme.
Hi Boris, On Thu, Jan 11, 2018 at 6:15 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Nothing that I really like, sorry. One solution would be to use > dev_name(&pdev->dev) when only one flash device is declared and use > dev_name(&pdev->dev)-reg_val otherwise. Or you could leave the logic Ok, so I can do as you suggested above: --- a/drivers/mtd/spi-nor/fsl-quadspi.c +++ b/drivers/mtd/spi-nor/fsl-quadspi.c @@ -1051,6 +1051,24 @@ static int fsl_qspi_probe(struct platform_device *pdev) spi_nor_set_flash_node(nor, np); nor->priv = q; + if (q->nor_num > 1) { + int spiflash_idx; + + ret = of_property_read_u32(np, "reg", &spiflash_idx); + if (!ret) { + mtd->name = devm_kasprintf(dev, GFP_KERNEL, + "%s-%d", + dev_name(dev), + spiflash_idx); + if (!mtd->name) { + ret = -ENOMEM; + goto mutex_failed; + } + } else { + dev_warn(dev, "reg property is missing\n"); + } + } + /* fill the hooks */ nor->read_reg = fsl_qspi_read_reg; nor->write_reg = fsl_qspi_write_reg; And then I can do a separate patch adding the qspi flash examples into Documentation/devicetree/bindings/mtd/fsl-quadspi.txt Does this look good?
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c index 2901c7b..8020295 100644 --- a/drivers/mtd/spi-nor/fsl-quadspi.c +++ b/drivers/mtd/spi-nor/fsl-quadspi.c @@ -967,7 +967,7 @@ static int fsl_qspi_probe(struct platform_device *pdev) struct resource *res; struct spi_nor *nor; struct mtd_info *mtd; - int ret, i = 0; + int ret, i = 0, spiflash_idx; q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL); if (!q) @@ -1051,6 +1051,24 @@ static int fsl_qspi_probe(struct platform_device *pdev) spi_nor_set_flash_node(nor, np); nor->priv = q; + if (!mtd->name) { + ret = of_property_read_u32(np, "reg", &spiflash_idx); + if (!ret) { + mtd->name = devm_kasprintf(dev, GFP_KERNEL, + "%s-%d", + dev_name(dev), + spiflash_idx); + } else { + mtd->name = dev_name(dev); + dev_warn(dev, "reg property is missing\n"); + } + + if (!mtd->name) { + ret = -ENOMEM; + goto mutex_failed; + } + } + /* fill the hooks */ nor->read_reg = fsl_qspi_read_reg; nor->write_reg = fsl_qspi_write_reg;
Currently on a imx6sx-sdb board, which has two SPI NOR chips connected to QSPI2 the following output from /proc/mtd is seen: # cat /proc/mtd dev: size erasesize name mtd0: 01000000 00010000 "21e4000.qspi" mtd1: 01000000 00010000 "21e4000.qspi" Attempts to partition them on the kernel command line result in both chips with identical (and identically named) partitions, which is an inconvenient behavior. Assign a different mtd->name for each mtd device to avoid this problem. After this change the output from /proc/mtd becomes: # cat /proc/mtd dev: size erasesize name mtd0: 01000000 00010000 "21e4000.qspi-0" mtd1: 01000000 00010000 "21e4000.qspi-1" Reported-by: David Wolfe <david.wolfe@nxp.com> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> --- Changes since v1: - Do not fail probe when reg is not present drivers/mtd/spi-nor/fsl-quadspi.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)