[PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Marek Vasut]

On R-Car V4H, the PCIEC controller DBI read would generate an SError
in case the controller reset is released by writing SRSTCLR register
first, and immediately afterward reading some PCIEC controller DBI
register. The issue triggers in rcar_gen4_pcie_additional_common_init()
on dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW), which on V4H is the first
read after reset_control_deassert(dw->core_rsts[DW_PCIE_PWR_RST].rstc).

The reset controller which contains the SRSTCLR register and the PCIEC
controller which contains the DBI register share the same root access
bus, but the bus then splits into separate segments before reaching
each IP. Even if the SRSTCLR write access was posted on the bus before
the DBI read access, it seems the DBI read access may reach the PCIEC
controller before the SRSTCLR write completed, and trigger the SError.

Mitigate the issue by adding a dummy SRSTCLR read, which assures the
SRSTCLR write completes fully and is latched into the reset controller,
before the PCIEC DBI read access can occur.

Fixes: 0ab55cf18341 ("clk: renesas: cpg-mssr: Add support for R-Car V4H")
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/clk/renesas/renesas-cpg-mssr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index 65dfaceea71f..7b52e8235984 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
 
 	/* Reset module */
 	writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
+	readl(priv->pub.base0 + priv->reset_regs[reg]);
 
 	/*
 	 * On R-Car Gen4, delay after SRCR has been written is 1ms.
@@ -701,6 +702,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
 
 	/* Release module from reset state */
 	writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
+	readl(priv->pub.base0 + priv->reset_clear_regs[reg]);
 
 	return 0;
 }
@@ -715,6 +717,7 @@ static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id)
 	dev_dbg(priv->dev, "assert %u%02u\n", reg, bit);
 
 	writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
+	readl(priv->pub.base0 + priv->reset_regs[reg]);
 	return 0;
 }
 
@@ -729,6 +732,7 @@ static int cpg_mssr_deassert(struct reset_controller_dev *rcdev,
 	dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit);
 
 	writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
+	readl(priv->pub.base0 + priv->reset_clear_regs[reg]);
 	return 0;
 }
 
-- 
2.51.0

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

Hi Marek,

thanks for your hard work on the PCIe issue.

>  	/* Reset module */
>  	writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> +	readl(priv->pub.base0 + priv->reset_regs[reg]);

Maybe it makes sense to introduce a light helper function
'writel_with_latch' or something?

a) we can have a central documentation point to describe why this is
needed and for which registers

b) this makes it hopefully more likely that future additions will use
the helper right away and not forget to latch their access

Opinions?

Happy hacking,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Lad, Prabhakar]

Hi Marek,

On Thu, Sep 18, 2025 at 4:07 AM Marek Vasut
<marek.vasut+renesas@mailbox.org> wrote:
>
> On R-Car V4H, the PCIEC controller DBI read would generate an SError
> in case the controller reset is released by writing SRSTCLR register
> first, and immediately afterward reading some PCIEC controller DBI
> register. The issue triggers in rcar_gen4_pcie_additional_common_init()
> on dw_pcie_readl_dbi(dw, PCIE_PORT_LANE_SKEW), which on V4H is the first
> read after reset_control_deassert(dw->core_rsts[DW_PCIE_PWR_RST].rstc).
>
> The reset controller which contains the SRSTCLR register and the PCIEC
> controller which contains the DBI register share the same root access
> bus, but the bus then splits into separate segments before reaching
> each IP. Even if the SRSTCLR write access was posted on the bus before
> the DBI read access, it seems the DBI read access may reach the PCIEC
> controller before the SRSTCLR write completed, and trigger the SError.
>
> Mitigate the issue by adding a dummy SRSTCLR read, which assures the
> SRSTCLR write completes fully and is latched into the reset controller,
> before the PCIEC DBI read access can occur.
>
> Fixes: 0ab55cf18341 ("clk: renesas: cpg-mssr: Add support for R-Car V4H")
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/clk/renesas/renesas-cpg-mssr.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 65dfaceea71f..7b52e8235984 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>
>         /* Reset module */
>         writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
to read the reset register `7` times and confirm [0] (as mentioned in
the HW manual). So after reading do we want to confirm the bit is
set/clear?

[0] https://lore.kernel.org/all/20250905114558.1602756-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

>
>         /*
>          * On R-Car Gen4, delay after SRCR has been written is 1ms.
> @@ -701,6 +702,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>
>         /* Release module from reset state */
>         writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
> +       readl(priv->pub.base0 + priv->reset_clear_regs[reg]);
>
>         return 0;
>  }
> @@ -715,6 +717,7 @@ static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id)
>         dev_dbg(priv->dev, "assert %u%02u\n", reg, bit);
>
>         writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
>         return 0;
>  }
>
> @@ -729,6 +732,7 @@ static int cpg_mssr_deassert(struct reset_controller_dev *rcdev,
>         dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit);
>
>         writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
> +       readl(priv->pub.base0 + priv->reset_clear_regs[reg]);
>         return 0;
>  }
>
> --
> 2.51.0
>
>

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Marek Vasut]

On 9/18/25 8:08 AM, Wolfram Sang wrote:
> Hi Marek,

Hello Wolfram,

> thanks for your hard work on the PCIe issue.
> 
>>   	/* Reset module */
>>   	writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
>> +	readl(priv->pub.base0 + priv->reset_regs[reg]);
> 
> Maybe it makes sense to introduce a light helper function
> 'writel_with_latch' or something?

Sure, done in V2 with a bit more deduplication.

> a) we can have a central documentation point to describe why this is
> needed and for which registers

The part which worries me is that I don't have a clear information that 
my hypothesis is correct.

> b) this makes it hopefully more likely that future additions will use
> the helper right away and not forget to latch their access
This is fine by me.

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Marek Vasut]

On 9/18/25 1:11 PM, Lad, Prabhakar wrote:

Hello Prabhakar,

>> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
>> index 65dfaceea71f..7b52e8235984 100644
>> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
>> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
>> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>>
>>          /* Reset module */
>>          writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
>> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
> Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
> to read the reset register `7` times and confirm [0] (as mentioned in
> the HW manual). So after reading do we want to confirm the bit is
> set/clear?
This is interesting, I wonder if the readback is something more common 
to this reset controller.

Why 7 times ? Is this documented in one of the HW manuals ? Are those 
public and can you share a link to them , with the specific chapter or 
page I should read about this 7 times read requirement ?

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Lad, Prabhakar]

Hi Marek,

On Thu, Sep 18, 2025 at 2:42 PM Marek Vasut <marek.vasut@mailbox.org> wrote:
>
> On 9/18/25 1:11 PM, Lad, Prabhakar wrote:
>
> Hello Prabhakar,
>
> >> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> >> index 65dfaceea71f..7b52e8235984 100644
> >> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> >> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> >> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> >>
> >>          /* Reset module */
> >>          writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> >> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
> > Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
> > to read the reset register `7` times and confirm [0] (as mentioned in
> > the HW manual). So after reading do we want to confirm the bit is
> > set/clear?
> This is interesting, I wonder if the readback is something more common
> to this reset controller.
>
> Why 7 times ? Is this documented in one of the HW manuals ? Are those
> public and can you share a link to them , with the specific chapter or
> page I should read about this 7 times read requirement ?
>
Yes this is documented in the HW manual [0] section 6.5.1 Notes on
Module Reset Control Register Operation:

1. To secure processing after release from a module reset, dummy read
the same register at least seven times except RTC
and LCDC after writing to initiate release from the module reset, and
only then proceed with the subsequent processing.
For RTC, dummy read the same register at least 300 times and for LCDC,
at least 100 times.

2. When module is reset once and released again, make sure that the
target bit of module reset control register is set to 1 by
reading the register before releasing from a module reset. Then
release from a module reset


[0] https://www.renesas.com/en/document/mah/rzt2h-and-rzn2h-groups-users-manual-hardware?r=25567515

Cheers,
Prabhakar

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Marek Vasut]

On 9/18/25 5:05 PM, Lad, Prabhakar wrote:

Hello Prabhakar,

>>>> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
>>>> index 65dfaceea71f..7b52e8235984 100644
>>>> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
>>>> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
>>>> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>>>>
>>>>           /* Reset module */
>>>>           writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
>>>> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
>>> Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
>>> to read the reset register `7` times and confirm [0] (as mentioned in
>>> the HW manual). So after reading do we want to confirm the bit is
>>> set/clear?
>> This is interesting, I wonder if the readback is something more common
>> to this reset controller.
>>
>> Why 7 times ? Is this documented in one of the HW manuals ? Are those
>> public and can you share a link to them , with the specific chapter or
>> page I should read about this 7 times read requirement ?
>>
> Yes this is documented in the HW manual [0] section 6.5.1 Notes on
> Module Reset Control Register Operation:
> 
> 1. To secure processing after release from a module reset, dummy read
> the same register at least seven times except RTC
> and LCDC after writing to initiate release from the module reset, and
> only then proceed with the subsequent processing.
> For RTC, dummy read the same register at least 300 times and for LCDC,
> at least 100 times.
> 
> 2. When module is reset once and released again, make sure that the
> target bit of module reset control register is set to 1 by
> reading the register before releasing from a module reset. Then
> release from a module reset

Thank you for sharing this, but it seems this is not the case for R-Car 
Gen4. I found out that V4H and V4M has additional "synchronized" and 
"asynchronized" reset types according to SRCRn_FSRCHRKRAn attachment to 
the V4H RM. The PCIe resets are "asynchronized". This extra readl() 
added in this patch is turning all the resets into "synchronized" and 
therefore makes them behave as expected.

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, 18 Sept 2025 at 17:05, Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Thu, Sep 18, 2025 at 2:42 PM Marek Vasut <marek.vasut@mailbox.org> wrote:
> > On 9/18/25 1:11 PM, Lad, Prabhakar wrote:
> > >> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> > >> index 65dfaceea71f..7b52e8235984 100644
> > >> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> > >> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> > >> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> > >>
> > >>          /* Reset module */
> > >>          writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> > >> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
> > > Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
> > > to read the reset register `7` times and confirm [0] (as mentioned in
> > > the HW manual). So after reading do we want to confirm the bit is
> > > set/clear?
> > This is interesting, I wonder if the readback is something more common
> > to this reset controller.
> >
> > Why 7 times ? Is this documented in one of the HW manuals ? Are those
> > public and can you share a link to them , with the specific chapter or
> > page I should read about this 7 times read requirement ?
> >
> Yes this is documented in the HW manual [0] section 6.5.1 Notes on
> Module Reset Control Register Operation:
>
> 1. To secure processing after release from a module reset, dummy read
> the same register at least seven times except RTC
> and LCDC after writing to initiate release from the module reset, and
> only then proceed with the subsequent processing.
> For RTC, dummy read the same register at least 300 times and for LCDC,
> at least 100 times.

Ugh, the number of times depends on the module to reset :-(
Do we need #reset-cells = <2> to encode the number?

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] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Lad, Prabhakar]

Hi Geert,

On Fri, Sep 19, 2025 at 8:21 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, 18 Sept 2025 at 17:05, Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Thu, Sep 18, 2025 at 2:42 PM Marek Vasut <marek.vasut@mailbox.org> wrote:
> > > On 9/18/25 1:11 PM, Lad, Prabhakar wrote:
> > > >> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> > > >> index 65dfaceea71f..7b52e8235984 100644
> > > >> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> > > >> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> > > >> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> > > >>
> > > >>          /* Reset module */
> > > >>          writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> > > >> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
> > > > Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
> > > > to read the reset register `7` times and confirm [0] (as mentioned in
> > > > the HW manual). So after reading do we want to confirm the bit is
> > > > set/clear?
> > > This is interesting, I wonder if the readback is something more common
> > > to this reset controller.
> > >
> > > Why 7 times ? Is this documented in one of the HW manuals ? Are those
> > > public and can you share a link to them , with the specific chapter or
> > > page I should read about this 7 times read requirement ?
> > >
> > Yes this is documented in the HW manual [0] section 6.5.1 Notes on
> > Module Reset Control Register Operation:
> >
> > 1. To secure processing after release from a module reset, dummy read
> > the same register at least seven times except RTC
> > and LCDC after writing to initiate release from the module reset, and
> > only then proceed with the subsequent processing.
> > For RTC, dummy read the same register at least 300 times and for LCDC,
> > at least 100 times.
>
> Ugh, the number of times depends on the module to reset :-(
> Do we need #reset-cells = <2> to encode the number?
>
There are no bits to reset RTC and LCDC in MRCTLA-MRCTLM registers,
Ive confirmed this with the HW team. (The manual will be updated in
the next version).

Cheers,
Prabhakar

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Lad, Prabhakar]

Hi Marek,

On Fri, Sep 19, 2025 at 12:16 AM Marek Vasut <marek.vasut@mailbox.org> wrote:
>
> On 9/18/25 5:05 PM, Lad, Prabhakar wrote:
>
> Hello Prabhakar,
>
> >>>> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> >>>> index 65dfaceea71f..7b52e8235984 100644
> >>>> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> >>>> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> >>>> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> >>>>
> >>>>           /* Reset module */
> >>>>           writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> >>>> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
> >>> Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
> >>> to read the reset register `7` times and confirm [0] (as mentioned in
> >>> the HW manual). So after reading do we want to confirm the bit is
> >>> set/clear?
> >> This is interesting, I wonder if the readback is something more common
> >> to this reset controller.
> >>
> >> Why 7 times ? Is this documented in one of the HW manuals ? Are those
> >> public and can you share a link to them , with the specific chapter or
> >> page I should read about this 7 times read requirement ?
> >>
> > Yes this is documented in the HW manual [0] section 6.5.1 Notes on
> > Module Reset Control Register Operation:
> >
> > 1. To secure processing after release from a module reset, dummy read
> > the same register at least seven times except RTC
> > and LCDC after writing to initiate release from the module reset, and
> > only then proceed with the subsequent processing.
> > For RTC, dummy read the same register at least 300 times and for LCDC,
> > at least 100 times.
> >
> > 2. When module is reset once and released again, make sure that the
> > target bit of module reset control register is set to 1 by
> > reading the register before releasing from a module reset. Then
> > release from a module reset
>
> Thank you for sharing this, but it seems this is not the case for R-Car
> Gen4. I found out that V4H and V4M has additional "synchronized" and
> "asynchronized" reset types according to SRCRn_FSRCHRKRAn attachment to
> the V4H RM. The PCIe resets are "asynchronized". This extra readl()
> added in this patch is turning all the resets into "synchronized" and
> therefore makes them behave as expected.
>
I see, thank you for sharing it.

Cheers,
Prabhakar

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Marek Vasut]

On 9/19/25 9:21 AM, Geert Uytterhoeven wrote:

Hello Geert,

> On Thu, 18 Sept 2025 at 17:05, Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
>> On Thu, Sep 18, 2025 at 2:42 PM Marek Vasut <marek.vasut@mailbox.org> wrote:
>>> On 9/18/25 1:11 PM, Lad, Prabhakar wrote:
>>>>> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
>>>>> index 65dfaceea71f..7b52e8235984 100644
>>>>> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
>>>>> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
>>>>> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>>>>>
>>>>>           /* Reset module */
>>>>>           writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
>>>>> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
>>>> Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
>>>> to read the reset register `7` times and confirm [0] (as mentioned in
>>>> the HW manual). So after reading do we want to confirm the bit is
>>>> set/clear?
>>> This is interesting, I wonder if the readback is something more common
>>> to this reset controller.
>>>
>>> Why 7 times ? Is this documented in one of the HW manuals ? Are those
>>> public and can you share a link to them , with the specific chapter or
>>> page I should read about this 7 times read requirement ?
>>>
>> Yes this is documented in the HW manual [0] section 6.5.1 Notes on
>> Module Reset Control Register Operation:
>>
>> 1. To secure processing after release from a module reset, dummy read
>> the same register at least seven times except RTC
>> and LCDC after writing to initiate release from the module reset, and
>> only then proceed with the subsequent processing.
>> For RTC, dummy read the same register at least 300 times and for LCDC,
>> at least 100 times.
> 
> Ugh, the number of times depends on the module to reset :-(
> Do we need #reset-cells = <2> to encode the number?

This does seem like an reference manual issue and will be updated.

However, it also seems if this encoding of per-reset properties is 
necessary in the future, it would be better to have reset property 
tables (similar to clock property tables) in the kernel instead of 
encoding the per-reset properties in DT?

-- 
Best regards,
Marek Vasut

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Geert Uytterhoeven]

Hi Marek,

On Fri, 19 Sept 2025 at 14:19, Marek Vasut <marek.vasut@mailbox.org> wrote:
> On 9/19/25 9:21 AM, Geert Uytterhoeven wrote:
> > On Thu, 18 Sept 2025 at 17:05, Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> >> On Thu, Sep 18, 2025 at 2:42 PM Marek Vasut <marek.vasut@mailbox.org> wrote:
> >>> On 9/18/25 1:11 PM, Lad, Prabhakar wrote:
> >>>>> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> >>>>> index 65dfaceea71f..7b52e8235984 100644
> >>>>> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> >>>>> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> >>>>> @@ -688,6 +688,7 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> >>>>>
> >>>>>           /* Reset module */
> >>>>>           writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> >>>>> +       readl(priv->pub.base0 + priv->reset_regs[reg]);
> >>>> Fyi on the RZ/T2H and RZ/N2H SoCs which uses the same driver we need
> >>>> to read the reset register `7` times and confirm [0] (as mentioned in
> >>>> the HW manual). So after reading do we want to confirm the bit is
> >>>> set/clear?
> >>> This is interesting, I wonder if the readback is something more common
> >>> to this reset controller.
> >>>
> >>> Why 7 times ? Is this documented in one of the HW manuals ? Are those
> >>> public and can you share a link to them , with the specific chapter or
> >>> page I should read about this 7 times read requirement ?
> >>>
> >> Yes this is documented in the HW manual [0] section 6.5.1 Notes on
> >> Module Reset Control Register Operation:
> >>
> >> 1. To secure processing after release from a module reset, dummy read
> >> the same register at least seven times except RTC
> >> and LCDC after writing to initiate release from the module reset, and
> >> only then proceed with the subsequent processing.
> >> For RTC, dummy read the same register at least 300 times and for LCDC,
> >> at least 100 times.
> >
> > Ugh, the number of times depends on the module to reset :-(
> > Do we need #reset-cells = <2> to encode the number?
>
> This does seem like an reference manual issue and will be updated.
>
> However, it also seems if this encoding of per-reset properties is
> necessary in the future, it would be better to have reset property
> tables (similar to clock property tables) in the kernel instead of
> encoding the per-reset properties in DT?

Yes, the background task running in my brain had come to the
same conclusion ;-)

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] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Geert Uytterhoeven]

Hi Marek,

On Fri, 19 Sept 2025 at 01:16, Marek Vasut <marek.vasut@mailbox.org> wrote:
> Thank you for sharing this, but it seems this is not the case for R-Car
> Gen4. I found out that V4H and V4M has additional "synchronized" and
> "asynchronized" reset types according to SRCRn_FSRCHRKRAn attachment to
> the V4H RM. The PCIe resets are "asynchronized". This extra readl()
> added in this patch is turning all the resets into "synchronized" and
> therefore makes them behave as expected.

Interesting...

So e.g. MSIOF has synchronized resets, while I2C has not, although
both are connected to the peripheral AXI bus...

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] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]


> So e.g. MSIOF has synchronized resets, while I2C has not, although
> both are connected to the peripheral AXI bus...

Is this why we need 'read_poll_timeout_atomic' in 'rcar_i2c_do_reset()'
then? I have never received a reason besides "HW engineers said so".
This is pre-Gen4, though.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, 22 Sept 2025 at 15:57, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > So e.g. MSIOF has synchronized resets, while I2C has not, although
> > both are connected to the peripheral AXI bus...
>
> Is this why we need 'read_poll_timeout_atomic' in 'rcar_i2c_do_reset()'
> then? I have never received a reason besides "HW engineers said so".
> This is pre-Gen4, though.

Perhaps... IIRC there never was any mention of "(a)synchronized resets"
before.

Can you trigger an issue by removing the polling, and does Marek's
patch fix it?

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] clk: renesas: cpg-mssr: Read back reset registers to assure values latched

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 270 bytes --]


> Can you trigger an issue by removing the polling, and does Marek's
> patch fix it?

IIRC not with I2C, but SHDI needs this as well and there it surely
happened during development a few years ago. I need to work on SDHI soon
anyhow, I will see if I can reproduce it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]