| Message ID | 20230830145835.296690-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | memory: renesas-rpc-if: Fix IO state based on flash type | expand |
On 30/08/2023 17:18, Biju Das wrote: >>> regmap_update_bits(rpc->regmap, RPCIF_CMNCR, @@ -774,6 >>> +776,12 @@ static int rpcif_probe(struct platform_device *pdev) >>> return ret; >>> } >>> >>> + if (rpc->info->type == RPCIF_RZ_G2L && >> >> Wouldn't this apply to non-RZ/G2L systems, too? > > It applies, if the device uses the flash[1] or [2] and it needs > 4-bit tx support. > > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ – EBh/ECh > https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlks_u_512_aba_0.pdf?rev=3e5b2a574f7b4790b6e58dacf4c889b2 > > [2] section 8.14 > > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586 > Geert, Does it answer your comment or do you expect here some changes? Best regards, Krzysztof
Hi Krzysztof, CC Rob, Miquel On Thu, Sep 14, 2023 at 10:08 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 30/08/2023 17:18, Biju Das wrote: > >>> regmap_update_bits(rpc->regmap, RPCIF_CMNCR, @@ -774,6 > >>> +776,12 @@ static int rpcif_probe(struct platform_device *pdev) > >>> return ret; > >>> } > >>> > >>> + if (rpc->info->type == RPCIF_RZ_G2L && > >> > >> Wouldn't this apply to non-RZ/G2L systems, too? > > > > It applies, if the device uses the flash[1] or [2] and it needs > > 4-bit tx support. > > > > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ – EBh/ECh > > https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlks_u_512_aba_0.pdf?rev=3e5b2a574f7b4790b6e58dacf4c889b2 > > > > [2] section 8.14 > > > > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586 > > Geert, > > Does it answer your comment or do you expect here some changes? Well, now it has been confirmed this applies to non-RZ/G2L systems, too, the check for RPCIF_RZ_G2L should probably be removed. In upstream, only arch/arm64/boot/dts/renesas/rzg2l{,c}-smarc-som.dtsi have devices that are compatible with "micron,mt25qu512a", but obviously they can appear elsewhere, too. Now, the presence of that compatible value in rzg2l{,c}-smarc-som.dtsi currently causes a dtbs_check warning, as it is not documented. However, there has been some pushback against adding more compatible values, cfr. my patch to add mt25qu512a[1], and Miquel's commit [2]. But the issue Biju is seeing proves there is a need to add these. In addition, I had hoped to gather some feedback or guidance from the hyperbus and/or spi people, as issues w.r.t. pin states will eventually pop up on other systems, too, and thus may need handling in the core, instead of in each individual device driver. But of course that can be done later, when the need arises. Thanks! [1] "[PATCH] dt-bindings: mtd: jedec,spi-nor: Document support for more MT25QU parts" https://lore.kernel.org/all/363186079b4269891073f620e3e2353cf7d2559a.1669988238.git.geert+renesas@glider.be [2] 4b0cb4e7ab2f777c ("dt-bindings: mtd: spi-nor: clarify the need for spi-nor compatibles"). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, + Michael Walle geert@linux-m68k.org wrote on Thu, 14 Sep 2023 10:34:50 +0200: > Hi Krzysztof, > > CC Rob, Miquel > > On Thu, Sep 14, 2023 at 10:08 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > On 30/08/2023 17:18, Biju Das wrote: > > >>> regmap_update_bits(rpc->regmap, RPCIF_CMNCR, @@ -774,6 > > >>> +776,12 @@ static int rpcif_probe(struct platform_device *pdev) > > >>> return ret; > > >>> } > > >>> > > >>> + if (rpc->info->type == RPCIF_RZ_G2L && > > >> > > >> Wouldn't this apply to non-RZ/G2L systems, too? > > > > > > It applies, if the device uses the flash[1] or [2] and it needs > > > 4-bit tx support. > > > > > > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ – EBh/ECh > > > https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlks_u_512_aba_0.pdf?rev=3e5b2a574f7b4790b6e58dacf4c889b2 > > > > > > [2] section 8.14 > > > > > > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586 > > > > Geert, > > > > Does it answer your comment or do you expect here some changes? > > Well, now it has been confirmed this applies to non-RZ/G2L systems, too, > the check for RPCIF_RZ_G2L should probably be removed. In upstream, > only arch/arm64/boot/dts/renesas/rzg2l{,c}-smarc-som.dtsi have devices > that are compatible with "micron,mt25qu512a", but obviously they can > appear elsewhere, too. > > Now, the presence of that compatible value in rzg2l{,c}-smarc-som.dtsi > currently causes a dtbs_check warning, as it is not documented. > However, there has been some pushback against adding more compatible > values, cfr. my patch to add mt25qu512a[1], and Miquel's commit [2]. Just FYI, I sent [2] after an unsuccessful attempt to update that list too, see [3]. The idea is: if you don't have anything useful to add, just use the generic compatible. If you need specific changes, you can add an entry. [3] https://lore.kernel.org/linux-mtd/d816499e-baab-6200-0780-17a8205b252e@linaro.org/ > But the issue Biju is seeing proves there is a need to add these. > > In addition, I had hoped to gather some feedback or guidance from the > hyperbus and/or spi people, as issues w.r.t. pin states will eventually > pop up on other systems, too, and thus may need handling in the core, > instead of in each individual device driver. But of course that can > be done later, when the need arises. > > Thanks! > > [1] "[PATCH] dt-bindings: mtd: jedec,spi-nor: Document support for > more MT25QU parts" > https://lore.kernel.org/all/363186079b4269891073f620e3e2353cf7d2559a.1669988238.git.geert+renesas@glider.be > [2] 4b0cb4e7ab2f777c ("dt-bindings: mtd: spi-nor: clarify the need for > spi-nor compatibles"). > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds Thanks, Miquèl
Hi Miquel, On Thu, Sep 14, 2023 at 10:59 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > geert@linux-m68k.org wrote on Thu, 14 Sep 2023 10:34:50 +0200: > > On Thu, Sep 14, 2023 at 10:08 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > On 30/08/2023 17:18, Biju Das wrote: > > > >>> regmap_update_bits(rpc->regmap, RPCIF_CMNCR, @@ -774,6 > > > >>> +776,12 @@ static int rpcif_probe(struct platform_device *pdev) > > > >>> return ret; > > > >>> } > > > >>> > > > >>> + if (rpc->info->type == RPCIF_RZ_G2L && > > > >> > > > >> Wouldn't this apply to non-RZ/G2L systems, too? > > > > > > > > It applies, if the device uses the flash[1] or [2] and it needs > > > > 4-bit tx support. > > > > > > > > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ – EBh/ECh > > > > https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlks_u_512_aba_0.pdf?rev=3e5b2a574f7b4790b6e58dacf4c889b2 > > > > > > > > [2] section 8.14 > > > > > > > > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586 > > > > > > Geert, > > > > > > Does it answer your comment or do you expect here some changes? > > > > Well, now it has been confirmed this applies to non-RZ/G2L systems, too, > > the check for RPCIF_RZ_G2L should probably be removed. In upstream, > > only arch/arm64/boot/dts/renesas/rzg2l{,c}-smarc-som.dtsi have devices > > that are compatible with "micron,mt25qu512a", but obviously they can > > appear elsewhere, too. > > > > Now, the presence of that compatible value in rzg2l{,c}-smarc-som.dtsi > > currently causes a dtbs_check warning, as it is not documented. > > However, there has been some pushback against adding more compatible > > values, cfr. my patch to add mt25qu512a[1], and Miquel's commit [2]. > > Just FYI, I sent [2] after an unsuccessful attempt to update that list > too, see [3]. The idea is: if you don't have anything useful to add, Oh, I didn't know that. > just use the generic compatible. If you need specific changes, you can > add an entry. The problem is that usually these things are discovered too late, so the only prudent way is to be proactive, and always add them. Initially I thought that the different handling on RZ/G2L was due to a difference in the RPC-IF block. But now we know it's due to the type of FLASH attached. > [3] https://lore.kernel.org/linux-mtd/d816499e-baab-6200-0780-17a8205b252e@linaro.org/ > > > But the issue Biju is seeing proves there is a need to add these. > > > > In addition, I had hoped to gather some feedback or guidance from the > > hyperbus and/or spi people, as issues w.r.t. pin states will eventually > > pop up on other systems, too, and thus may need handling in the core, > > instead of in each individual device driver. But of course that can > > be done later, when the need arises. > > > > Thanks! > > > > [1] "[PATCH] dt-bindings: mtd: jedec,spi-nor: Document support for > > more MT25QU parts" > > https://lore.kernel.org/all/363186079b4269891073f620e3e2353cf7d2559a.1669988238.git.geert+renesas@glider.be > > [2] 4b0cb4e7ab2f777c ("dt-bindings: mtd: spi-nor: clarify the need for > > spi-nor compatibles").
Hi Geert, geert@linux-m68k.org wrote on Thu, 14 Sep 2023 11:04:01 +0200: > Hi Miquel, > > On Thu, Sep 14, 2023 at 10:59 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > geert@linux-m68k.org wrote on Thu, 14 Sep 2023 10:34:50 +0200: > > > On Thu, Sep 14, 2023 at 10:08 AM Krzysztof Kozlowski > > > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 30/08/2023 17:18, Biju Das wrote: > > > > >>> regmap_update_bits(rpc->regmap, RPCIF_CMNCR, @@ -774,6 > > > > >>> +776,12 @@ static int rpcif_probe(struct platform_device *pdev) > > > > >>> return ret; > > > > >>> } > > > > >>> > > > > >>> + if (rpc->info->type == RPCIF_RZ_G2L && > > > > >> > > > > >> Wouldn't this apply to non-RZ/G2L systems, too? > > > > > > > > > > It applies, if the device uses the flash[1] or [2] and it needs > > > > > 4-bit tx support. > > > > > > > > > > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ – EBh/ECh > > > > > https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlks_u_512_aba_0.pdf?rev=3e5b2a574f7b4790b6e58dacf4c889b2 > > > > > > > > > > [2] section 8.14 > > > > > > > > > > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586 > > > > > > > > Geert, > > > > > > > > Does it answer your comment or do you expect here some changes? > > > > > > Well, now it has been confirmed this applies to non-RZ/G2L systems, too, > > > the check for RPCIF_RZ_G2L should probably be removed. In upstream, > > > only arch/arm64/boot/dts/renesas/rzg2l{,c}-smarc-som.dtsi have devices > > > that are compatible with "micron,mt25qu512a", but obviously they can > > > appear elsewhere, too. > > > > > > Now, the presence of that compatible value in rzg2l{,c}-smarc-som.dtsi > > > currently causes a dtbs_check warning, as it is not documented. > > > However, there has been some pushback against adding more compatible > > > values, cfr. my patch to add mt25qu512a[1], and Miquel's commit [2]. > > > > Just FYI, I sent [2] after an unsuccessful attempt to update that list > > too, see [3]. The idea is: if you don't have anything useful to add, > > Oh, I didn't know that. > > > just use the generic compatible. If you need specific changes, you can > > add an entry. > > The problem is that usually these things are discovered too late, > so the only prudent way is to be proactive, and always add them. > Initially I thought that the different handling on RZ/G2L was due > to a difference in the RPC-IF block. But now we know it's due to the > type of FLASH attached. Actually what I say is wrong, we are not supposed to touch that list anymore and prefer to handle the issues in the drivers by auto-discovery. Can't we do that in your case? > > [3] https://lore.kernel.org/linux-mtd/d816499e-baab-6200-0780-17a8205b252e@linaro.org/ > > > > > But the issue Biju is seeing proves there is a need to add these. > > > > > > In addition, I had hoped to gather some feedback or guidance from the > > > hyperbus and/or spi people, as issues w.r.t. pin states will eventually > > > pop up on other systems, too, and thus may need handling in the core, > > > instead of in each individual device driver. But of course that can > > > be done later, when the need arises. > > > > > > Thanks! > > > > > > [1] "[PATCH] dt-bindings: mtd: jedec,spi-nor: Document support for > > > more MT25QU parts" > > > https://lore.kernel.org/all/363186079b4269891073f620e3e2353cf7d2559a.1669988238.git.geert+renesas@glider.be > > > [2] 4b0cb4e7ab2f777c ("dt-bindings: mtd: spi-nor: clarify the need for > > > spi-nor compatibles"). > Thanks, Miquèl
Hi Miquel, On Thu, Sep 14, 2023 at 11:12 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > geert@linux-m68k.org wrote on Thu, 14 Sep 2023 11:04:01 +0200: > > On Thu, Sep 14, 2023 at 10:59 AM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > geert@linux-m68k.org wrote on Thu, 14 Sep 2023 10:34:50 +0200: > > > > On Thu, Sep 14, 2023 at 10:08 AM Krzysztof Kozlowski > > > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > On 30/08/2023 17:18, Biju Das wrote: > > > > > >>> regmap_update_bits(rpc->regmap, RPCIF_CMNCR, @@ -774,6 > > > > > >>> +776,12 @@ static int rpcif_probe(struct platform_device *pdev) > > > > > >>> return ret; > > > > > >>> } > > > > > >>> > > > > > >>> + if (rpc->info->type == RPCIF_RZ_G2L && > > > > > >> > > > > > >> Wouldn't this apply to non-RZ/G2L systems, too? > > > > > > > > > > > > It applies, if the device uses the flash[1] or [2] and it needs > > > > > > 4-bit tx support. > > > > > > > > > > > > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ – EBh/ECh > > > > > > https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlks_u_512_aba_0.pdf?rev=3e5b2a574f7b4790b6e58dacf4c889b2 > > > > > > > > > > > > [2] section 8.14 > > > > > > > > > > > > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586 > > > > > > > > > > Geert, > > > > > > > > > > Does it answer your comment or do you expect here some changes? > > > > > > > > Well, now it has been confirmed this applies to non-RZ/G2L systems, too, > > > > the check for RPCIF_RZ_G2L should probably be removed. In upstream, > > > > only arch/arm64/boot/dts/renesas/rzg2l{,c}-smarc-som.dtsi have devices > > > > that are compatible with "micron,mt25qu512a", but obviously they can > > > > appear elsewhere, too. > > > > > > > > Now, the presence of that compatible value in rzg2l{,c}-smarc-som.dtsi > > > > currently causes a dtbs_check warning, as it is not documented. > > > > However, there has been some pushback against adding more compatible > > > > values, cfr. my patch to add mt25qu512a[1], and Miquel's commit [2]. > > > > > > Just FYI, I sent [2] after an unsuccessful attempt to update that list > > > too, see [3]. The idea is: if you don't have anything useful to add, > > > > Oh, I didn't know that. > > > > > just use the generic compatible. If you need specific changes, you can > > > add an entry. > > > > The problem is that usually these things are discovered too late, > > so the only prudent way is to be proactive, and always add them. > > Initially I thought that the different handling on RZ/G2L was due > > to a difference in the RPC-IF block. But now we know it's due to the > > type of FLASH attached. > > Actually what I say is wrong, we are not supposed to touch that list > anymore and prefer to handle the issues in the drivers by > auto-discovery. Can't we do that in your case? I'm not sure we can do that, as this code is part of the hardware initialization during probing. Biju: is this needed that early, or can it be done later, after the connected device has been identified? Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash > type > > Hi Miquel, > > On Thu, Sep 14, 2023 at 11:12 AM Miquel Raynal <miquel.raynal@bootlin.com> > wrote: > > geert@linux-m68k.org wrote on Thu, 14 Sep 2023 11:04:01 +0200: > > > On Thu, Sep 14, 2023 at 10:59 AM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > geert@linux-m68k.org wrote on Thu, 14 Sep 2023 10:34:50 +0200: > > > > > On Thu, Sep 14, 2023 at 10:08 AM Krzysztof Kozlowski > > > > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > On 30/08/2023 17:18, Biju Das wrote: > > > > > > >>> regmap_update_bits(rpc->regmap, > > > > > > >>> RPCIF_CMNCR, @@ -774,6 > > > > > > >>> +776,12 @@ static int rpcif_probe(struct platform_device > > > > > > >>> +*pdev) > > > > > > >>> return ret; > > > > > > >>> } > > > > > > >>> > > > > > > >>> + if (rpc->info->type == RPCIF_RZ_G2L && > > > > > > >> > > > > > > >> Wouldn't this apply to non-RZ/G2L systems, too? > > > > > > > > > > > > > > It applies, if the device uses the flash[1] or [2] and it > > > > > > > needs 4-bit tx support. > > > > > > > > > > > > > > > > > > > Geert, > > > > > > > > > > > > Does it answer your comment or do you expect here some changes? > > > > > > > > > > Well, now it has been confirmed this applies to non-RZ/G2L > > > > > systems, too, the check for RPCIF_RZ_G2L should probably be > > > > > removed. In upstream, only > > > > > arch/arm64/boot/dts/renesas/rzg2l{,c}-smarc-som.dtsi have > > > > > devices that are compatible with "micron,mt25qu512a", but obviously > they can appear elsewhere, too. > > > > > > > > > > Now, the presence of that compatible value in > > > > > rzg2l{,c}-smarc-som.dtsi currently causes a dtbs_check warning, as > it is not documented. > > > > > However, there has been some pushback against adding more > > > > > compatible values, cfr. my patch to add mt25qu512a[1], and Miquel's > commit [2]. > > > > > > > > Just FYI, I sent [2] after an unsuccessful attempt to update that > > > > list too, see [3]. The idea is: if you don't have anything useful > > > > to add, > > > > > > Oh, I didn't know that. > > > > > > > just use the generic compatible. If you need specific changes, you > > > > can add an entry. > > > > > > The problem is that usually these things are discovered too late, so > > > the only prudent way is to be proactive, and always add them. > > > Initially I thought that the different handling on RZ/G2L was due to > > > a difference in the RPC-IF block. But now we know it's due to the > > > type of FLASH attached. > > > > Actually what I say is wrong, we are not supposed to touch that list > > anymore and prefer to handle the issues in the drivers by > > auto-discovery. Can't we do that in your case? > > I'm not sure we can do that, as this code is part of the hardware > initialization during probing. > Biju: is this needed that early, or can it be done later, after the > connected device has been identified? I need to check that. You mean patch drivers/spi/spi-rpc-if.c to identify the flash type from sfdp info and pass as a parameter to rpcif_hw_init?? Cheers, Biju
Hi Biju, On Thu, Sep 14, 2023 at 11:37 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash > > type > > On Thu, Sep 14, 2023 at 11:12 AM Miquel Raynal <miquel.raynal@bootlin.com> > > wrote: > > > geert@linux-m68k.org wrote on Thu, 14 Sep 2023 11:04:01 +0200: > > > > On Thu, Sep 14, 2023 at 10:59 AM Miquel Raynal > > > > <miquel.raynal@bootlin.com> wrote: > > > > > geert@linux-m68k.org wrote on Thu, 14 Sep 2023 10:34:50 +0200: > > > > > > On Thu, Sep 14, 2023 at 10:08 AM Krzysztof Kozlowski > > > > > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > > On 30/08/2023 17:18, Biju Das wrote: > > > > > > > >>> regmap_update_bits(rpc->regmap, > > > > > > > >>> RPCIF_CMNCR, @@ -774,6 > > > > > > > >>> +776,12 @@ static int rpcif_probe(struct platform_device > > > > > > > >>> +*pdev) > > > > > > > >>> return ret; > > > > > > > >>> } > > > > > > > >>> > > > > > > > >>> + if (rpc->info->type == RPCIF_RZ_G2L && > > > > > > > >> > > > > > > > >> Wouldn't this apply to non-RZ/G2L systems, too? > > > > > > > > > > > > > > > > It applies, if the device uses the flash[1] or [2] and it > > > > > > > > needs 4-bit tx support. > > > > > > > > > > > > > > > > > > > > > > Geert, > > > > > > > > > > > > > > Does it answer your comment or do you expect here some changes? > > > > > > > > > > > > Well, now it has been confirmed this applies to non-RZ/G2L > > > > > > systems, too, the check for RPCIF_RZ_G2L should probably be > > > > > > removed. In upstream, only > > > > > > arch/arm64/boot/dts/renesas/rzg2l{,c}-smarc-som.dtsi have > > > > > > devices that are compatible with "micron,mt25qu512a", but obviously > > they can appear elsewhere, too. > > > > > > > > > > > > Now, the presence of that compatible value in > > > > > > rzg2l{,c}-smarc-som.dtsi currently causes a dtbs_check warning, as > > it is not documented. > > > > > > However, there has been some pushback against adding more > > > > > > compatible values, cfr. my patch to add mt25qu512a[1], and Miquel's > > commit [2]. > > > > > > > > > > Just FYI, I sent [2] after an unsuccessful attempt to update that > > > > > list too, see [3]. The idea is: if you don't have anything useful > > > > > to add, > > > > > > > > Oh, I didn't know that. > > > > > > > > > just use the generic compatible. If you need specific changes, you > > > > > can add an entry. > > > > > > > > The problem is that usually these things are discovered too late, so > > > > the only prudent way is to be proactive, and always add them. > > > > Initially I thought that the different handling on RZ/G2L was due to > > > > a difference in the RPC-IF block. But now we know it's due to the > > > > type of FLASH attached. > > > > > > Actually what I say is wrong, we are not supposed to touch that list > > > anymore and prefer to handle the issues in the drivers by > > > auto-discovery. Can't we do that in your case? > > > > I'm not sure we can do that, as this code is part of the hardware > > initialization during probing. > > Biju: is this needed that early, or can it be done later, after the > > connected device has been identified? > > I need to check that. > > You mean patch drivers/spi/spi-rpc-if.c > to identify the flash type from sfdp info and pass as a parameter to rpcif_hw_init?? Something like that. That configuration should be saved somewhere, as rpcif_hw_init() is also called from rpcif_resume(), and when recovering from an error in rpcif_manual_xfer(). Gr{oetje,eeting}s, Geert
Hi, >> > I'm not sure we can do that, as this code is part of the hardware >> > initialization during probing. >> > Biju: is this needed that early, or can it be done later, after the >> > connected device has been identified? >> >> I need to check that. >> >> You mean patch drivers/spi/spi-rpc-if.c >> to identify the flash type from sfdp info and pass as a parameter to >> rpcif_hw_init?? > > Something like that. > > That configuration should be saved somewhere, as rpcif_hw_init() is > also called from rpcif_resume(), and when recovering from an error > in rpcif_manual_xfer(). I'm not sure I follow everything here, but apparently you want to set the mode of the I/O pins of the controller, right? Shouldn't that depend on the spi-mem mode, i.e. the buswidth? Certainly not on the type of flash which is connected to the spi controller. What about dual mode? -michael
Hi Michael Walle, > Subject: Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash > type > > Hi, > > >> > I'm not sure we can do that, as this code is part of the hardware > >> > initialization during probing. > >> > Biju: is this needed that early, or can it be done later, after the > >> > connected device has been identified? > >> > >> I need to check that. > >> > >> You mean patch drivers/spi/spi-rpc-if.c to identify the flash type > >> from sfdp info and pass as a parameter to rpcif_hw_init?? > > > > Something like that. > > > > That configuration should be saved somewhere, as rpcif_hw_init() is > > also called from rpcif_resume(), and when recovering from an error in > > rpcif_manual_xfer(). > > I'm not sure I follow everything here, but apparently you want to set the > mode of the I/O pins of the controller, right? Shouldn't that depend on the > spi-mem mode, i.e. the buswidth? Certainly not on the type of flash which > is connected to the spi controller. How do you handle the IO states sections mentioned in the HW manual[1] and [2]? Without this setting flash detection/ read/write failing with tx in 4-bit mode. [1] Figure 20: QUAD INPUT/OUTPUT FAST READ - EBh/ECh https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlks_u_512_aba_0.pdf?rev=3e5b2a574f7b4790b6e58dacf4c889b2 [2] section 8.14 https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586 Cheers, Biju > What about dual mode? > > -michael
Hi, >> >> > I'm not sure we can do that, as this code is part of the hardware >> >> > initialization during probing. >> >> > Biju: is this needed that early, or can it be done later, after the >> >> > connected device has been identified? >> >> >> >> I need to check that. >> >> >> >> You mean patch drivers/spi/spi-rpc-if.c to identify the flash type >> >> from sfdp info and pass as a parameter to rpcif_hw_init?? >> > >> > Something like that. >> > >> > That configuration should be saved somewhere, as rpcif_hw_init() is >> > also called from rpcif_resume(), and when recovering from an error in >> > rpcif_manual_xfer(). >> >> I'm not sure I follow everything here, but apparently you want to set >> the >> mode of the I/O pins of the controller, right? Shouldn't that depend >> on the >> spi-mem mode, i.e. the buswidth? Certainly not on the type of flash >> which >> is connected to the spi controller. > > > How do you handle the IO states sections mentioned in the HW manual[1] > and [2]? What do you mean by "IO states" you don't configure anything on the SPI flash, do you? I guess you should have to configure your SoC SPI pins in your .exec_op() callback according to the buswidth property. Have a look at the other spi drivers. I'm not that familiar with the spi controller drivers. > Without this setting flash detection/ read/write failing with tx in > 4-bit mode. > > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ - EBh/ECh > > https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlks_u_512_aba_0.pdf?rev=3e5b2a574f7b4790b6e58dacf4c889b2 > > [2] section 8.14 > > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586 Section 8.14 shows a Read with Quad I/O and the flash will tri-state the I/O lines during the command and dummy phase and drive them during data phase (and expect an address from the SoC on all I/Os during address and mode phase). -michael
Hi Michael Walle, > Subject: Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash > type > > Hi, > > >> >> > I'm not sure we can do that, as this code is part of the > >> >> > hardware initialization during probing. > >> >> > Biju: is this needed that early, or can it be done later, after > >> >> > the connected device has been identified? > >> >> > >> >> I need to check that. > >> >> > >> >> You mean patch drivers/spi/spi-rpc-if.c to identify the flash type > >> >> from sfdp info and pass as a parameter to rpcif_hw_init?? > >> > > >> > Something like that. > >> > > >> > That configuration should be saved somewhere, as rpcif_hw_init() is > >> > also called from rpcif_resume(), and when recovering from an error > >> > in rpcif_manual_xfer(). > >> > >> I'm not sure I follow everything here, but apparently you want to set > >> the mode of the I/O pins of the controller, right? Shouldn't that > >> depend on the spi-mem mode, i.e. the buswidth? Certainly not on the > >> type of flash which is connected to the spi controller. > > > > > > How do you handle the IO states sections mentioned in the HW manual[1] > > and [2]? > > What do you mean by "IO states" you don't configure anything on the SPI > flash, do you? > > I guess you should have to configure your SoC SPI pins in your > .exec_op() > callback according to the buswidth property. Here, same 4-bit tx_mode IO pin (QSPIn_IO0 Fixed Value for 1-bit Size) to be configured based on flash type and bus width right? For eg: here Adesto flash requires HI-Z for IO3 pin and Micron flash requires setting "1" for IO3 pin for 4-bit mode to work. Have a look at the other spi > drivers. I'm not that familiar with the spi controller drivers. > > > Without this setting flash detection/ read/write failing with tx in > > 4-bit mode. > > > > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ - EBh/ECh > > > > [2] section 8.14 > > > > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608 > > 586 > > Section 8.14 shows a Read with Quad I/O and the flash will tri-state the > I/O lines during the command and dummy phase and drive them during data > phase (and expect an address from the SoC on all I/Os during address and > mode phase). I agree, What about micron flash?? Cheers, Biju
>> >> >> > I'm not sure we can do that, as this code is part of the >> >> >> > hardware initialization during probing. >> >> >> > Biju: is this needed that early, or can it be done later, after >> >> >> > the connected device has been identified? >> >> >> >> >> >> I need to check that. >> >> >> >> >> >> You mean patch drivers/spi/spi-rpc-if.c to identify the flash type >> >> >> from sfdp info and pass as a parameter to rpcif_hw_init?? >> >> > >> >> > Something like that. >> >> > >> >> > That configuration should be saved somewhere, as rpcif_hw_init() is >> >> > also called from rpcif_resume(), and when recovering from an error >> >> > in rpcif_manual_xfer(). >> >> >> >> I'm not sure I follow everything here, but apparently you want to set >> >> the mode of the I/O pins of the controller, right? Shouldn't that >> >> depend on the spi-mem mode, i.e. the buswidth? Certainly not on the >> >> type of flash which is connected to the spi controller. >> > >> > >> > How do you handle the IO states sections mentioned in the HW manual[1] >> > and [2]? >> >> What do you mean by "IO states" you don't configure anything on the >> SPI >> flash, do you? >> >> I guess you should have to configure your SoC SPI pins in your >> .exec_op() >> callback according to the buswidth property. > > Here, same 4-bit tx_mode IO pin (QSPIn_IO0 Fixed Value for 1-bit Size) > to be configured based on flash type and bus width right? Just bus width. There should be no dependency on the flash type. > For eg: here Adesto flash requires HI-Z for IO3 pin and Micron flash > requires setting "1" for IO3 pin for 4-bit mode to work. That is odd. You'd need to ask Micron, but I assume it is because IO3 is shared with hold# and reset#. And there is a note "For pin configurations that share the DQ3 pin with RESET#, the RESET# functionality is disabled in QIO-SPI mode". So I guess the reason why they asking for a '1' is because they don't want to reset the flash. I'm pretty sure, we don't really support this in linux, so you'd probably want to disable that feature, i.e. see Table 7, bit 4. You could also come around this by enabling a pull-up on that line (assuming the SPI controller 'drives' HiZ during command phase). > > Have a look at the other spi >> drivers. I'm not that familiar with the spi controller drivers. >> >> > Without this setting flash detection/ read/write failing with tx in >> > 4-bit mode. >> > >> > [1] Figure 20: QUAD INPUT/OUTPUT FAST READ - EBh/ECh >> > >> > [2] section 8.14 >> > >> > https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608 >> > 586 >> >> Section 8.14 shows a Read with Quad I/O and the flash will tri-state >> the >> I/O lines during the command and dummy phase and drive them during >> data >> phase (and expect an address from the SoC on all I/Os during address >> and >> mode phase). > > I agree, What about micron flash?? > > Cheers, > Biju
>> For eg: here Adesto flash requires HI-Z for IO3 pin and Micron flash >> requires setting "1" for IO3 pin for 4-bit mode to work. > > That is odd. You'd need to ask Micron, but I assume it is because > IO3 is shared with hold# and reset#. And there is a note "For pin > configurations that share the DQ3 pin with RESET#, the RESET# > functionality is disabled in QIO-SPI mode". So I guess the reason > why they asking for a '1' is because they don't want to reset the > flash. I'm pretty sure, we don't really support this in linux, so > you'd probably want to disable that feature, i.e. see Table 7, > bit 4. You could also come around this by enabling a pull-up on > that line (assuming the SPI controller 'drives' HiZ during command > phase). Oh and I forgot. You probably can do some kind of fixup (where you set this bit) for this flash in drivers/mtd/spi-nor/micron.c. -michael
Hi Michael Walle, Thanks for the feedback. > Subject: Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash > type > > > >> For eg: here Adesto flash requires HI-Z for IO3 pin and Micron flash > >> requires setting "1" for IO3 pin for 4-bit mode to work. > > > > That is odd. You'd need to ask Micron, but I assume it is because > > IO3 is shared with hold# and reset#. And there is a note "For pin > > configurations that share the DQ3 pin with RESET#, the RESET# > > functionality is disabled in QIO-SPI mode". So I guess the reason why > > they asking for a '1' is because they don't want to reset the flash. > > I'm pretty sure, we don't really support this in linux, so you'd > > probably want to disable that feature, i.e. see Table 7, bit 4. You > > could also come around this by enabling a pull-up on that line > > (assuming the SPI controller 'drives' HiZ during command phase). > > Oh and I forgot. You probably can do some kind of fixup (where you set > this bit) for this flash in drivers/mtd/spi-nor/micron.c. Ok will send an RFC patch. Cheers, Biju
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c index 9695b2d3ae59..68f2bb3f8e61 100644 --- a/drivers/memory/renesas-rpc-if.c +++ b/drivers/memory/renesas-rpc-if.c @@ -189,6 +189,7 @@ struct rpcif_priv { u32 enable; /* DRENR or SMENR */ u32 dummy; /* DRDMCR or SMDMCR */ u32 ddr; /* DRDRENR or SMDRENR */ + u32 io3_fv; }; static const struct rpcif_info rpcif_info_r8a7796 = { @@ -367,7 +368,8 @@ int rpcif_hw_init(struct device *dev, bool hyperflash) regmap_update_bits(rpc->regmap, RPCIF_CMNCR, RPCIF_CMNCR_MOIIO(3) | RPCIF_CMNCR_IOFV(3) | RPCIF_CMNCR_BSZ(3), - RPCIF_CMNCR_MOIIO(1) | RPCIF_CMNCR_IOFV(2) | + RPCIF_CMNCR_MOIIO(1) | RPCIF_CMNCR_IO0FV(2) | + RPCIF_CMNCR_IO2FV(3) | rpc->io3_fv | RPCIF_CMNCR_BSZ(hyperflash ? 1 : 0)); else regmap_update_bits(rpc->regmap, RPCIF_CMNCR, @@ -774,6 +776,12 @@ static int rpcif_probe(struct platform_device *pdev) return ret; } + if (rpc->info->type == RPCIF_RZ_G2L && + of_device_is_compatible(flash, "micron,mt25qu512a")) + rpc->io3_fv = RPCIF_CMNCR_IO3FV(1); + else + rpc->io3_fv = RPCIF_CMNCR_IO3FV(3); + return 0; }
Currently, RZ/G2L-alike SoCs use 2 different SPI serial flash memories 1) AT25QL128A embedded in RZ/{G2UL,Five} SMARC EVKs 2) MT25QU512AB embedded in RZ/{G2L,G2LC,V2L} SMARC EVKs As per section 8.14 on the AT25QL128A hardware manual, IO1..IO3 must be set to Hi-Z state for this flash for fast read quad IO. Snippet from HW manual section 8.14: The upper nibble of the Mode(M7-4) controls the length of the next FAST Read Quad IO instruction through the inclusion or exclusion of the first byte instruction code. The lower nibble bits of the Mode(M3-0) are don't care. However, the IO pins must be high-impedance before the falling edge of the first data out clock. As per the Figure 20: QUAD INPUT/OUTPUT FAST READ on MT25QU512AB mentions IO1..IO2 to be in Hi-Z state and IO3 in '1' state Add a variable io3_fv to struct rpcif_priv and check the child node compatible value to detect micron flash and set IO1..IO3 states based on flash type. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/memory/renesas-rpc-if.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)