Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Geert Uytterhoeven]

Hi Biju,

CC hyperbus, spi

On Wed, Aug 30, 2023 at 4:58 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 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>

Thanks for your patch!

I guess this issue is not unique to Renesas platforms...

> --- 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 &&

Wouldn't this apply to non-RZ/G2L systems, too?

> +           of_device_is_compatible(flash, "micron,mt25qu512a"))
> +               rpc->io3_fv = RPCIF_CMNCR_IO3FV(1);
> +       else
> +               rpc->io3_fv = RPCIF_CMNCR_IO3FV(3);
> +
>         return 0;
>  }

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

RE: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Biju Das]

Hi Geert Uytterhoeven,

Thanks for the feedback.

> Subject: Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash
> type
> 
> Hi Biju,
> 
> CC hyperbus, spi
> 
> On Wed, Aug 30, 2023 at 4:58 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > 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>
> 
> Thanks for your patch!
> 
> I guess this issue is not unique to Renesas platforms...
> 
> > --- 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 &&
> 
> 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

Cheers,
Biju

> 
> > +           of_device_is_compatible(flash, "micron,mt25qu512a"))
> > +               rpc->io3_fv = RPCIF_CMNCR_IO3FV(1);
> > +       else
> > +               rpc->io3_fv = RPCIF_CMNCR_IO3FV(3);
> > +
> >         return 0;
> >  }
> 
> 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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Krzysztof Kozlowski]

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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Geert Uytterhoeven]

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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Miquel Raynal]

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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Geert Uytterhoeven]

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").

-- 
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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Miquel Raynal]

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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Geert Uytterhoeven]

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

-- 
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

RE: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Biju Das]

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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Michael Walle]

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

RE: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Biju Das]

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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Michael Walle]

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

RE: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Biju Das]

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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Michael Walle]

>> >> >> > 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

Re: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Michael Walle]

>> 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

RE: [PATCH] memory: renesas-rpc-if: Fix IO state based on flash type

[Biju Das]

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