* [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched
@ 2025-09-18 13:44 Marek Vasut
2025-09-20 4:26 ` Wolfram Sang
2025-09-22 11:35 ` Geert Uytterhoeven
0 siblings, 2 replies; 6+ messages in thread
From: Marek Vasut @ 2025-09-18 13:44 UTC (permalink / raw)
To: linux-clk
Cc: Marek Vasut, Geert Uytterhoeven, Michael Turquette, Stephen Boyd,
linux-renesas-soc
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
---
V2: Factor out the writel-then-readl code into cpg_mssr_writel_with_latch()
and clean the code up a bit
---
drivers/clk/renesas/renesas-cpg-mssr.c | 45 ++++++++++++--------------
1 file changed, 20 insertions(+), 25 deletions(-)
diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index 65dfaceea71f..cfb7f2a59c6b 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -676,18 +676,31 @@ static int __init cpg_mssr_add_clk_domain(struct device *dev,
#define rcdev_to_priv(x) container_of(x, struct cpg_mssr_priv, rcdev)
-static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
- unsigned long id)
+static int cpg_mssr_writel_with_latch(struct reset_controller_dev *rcdev,
+ char *func, bool set, unsigned long id)
{
struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
unsigned int reg = id / 32;
unsigned int bit = id % 32;
+ const u16 reset_reg = set ? priv->reset_regs[reg] : priv->reset_clear_regs[reg];
u32 bitmask = BIT(bit);
- dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
+ if (func)
+ dev_dbg(priv->dev, "%s %u%02u\n", func, reg, bit);
+
+ writel(bitmask, priv->pub.base0 + reset_reg);
+ readl(priv->pub.base0 + reset_reg);
+
+ return 0;
+}
+
+static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
/* Reset module */
- writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
+ cpg_mssr_writel_with_latch(rcdev, "reset", true, id);
/*
* On R-Car Gen4, delay after SRCR has been written is 1ms.
@@ -700,36 +713,18 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
usleep_range(35, 1000);
/* Release module from reset state */
- writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
-
- return 0;
+ return cpg_mssr_writel_with_latch(rcdev, NULL, false, id);
}
static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id)
{
- struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
- unsigned int reg = id / 32;
- unsigned int bit = id % 32;
- u32 bitmask = BIT(bit);
-
- dev_dbg(priv->dev, "assert %u%02u\n", reg, bit);
-
- writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
- return 0;
+ return cpg_mssr_writel_with_latch(rcdev, "assert", true, id);
}
static int cpg_mssr_deassert(struct reset_controller_dev *rcdev,
unsigned long id)
{
- struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
- unsigned int reg = id / 32;
- unsigned int bit = id % 32;
- u32 bitmask = BIT(bit);
-
- dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit);
-
- writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
- return 0;
+ return cpg_mssr_writel_with_latch(rcdev, "deassert", false, id);
}
static int cpg_mssr_status(struct reset_controller_dev *rcdev,
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched
2025-09-18 13:44 [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched Marek Vasut
@ 2025-09-20 4:26 ` Wolfram Sang
2025-09-22 11:35 ` Geert Uytterhoeven
1 sibling, 0 replies; 6+ messages in thread
From: Wolfram Sang @ 2025-09-20 4:26 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-clk, Geert Uytterhoeven, Michael Turquette, Stephen Boyd,
linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]
On Thu, Sep 18, 2025 at 03:44:41PM +0200, Marek Vasut 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>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched
2025-09-18 13:44 [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched Marek Vasut
2025-09-20 4:26 ` Wolfram Sang
@ 2025-09-22 11:35 ` Geert Uytterhoeven
2025-09-22 16:24 ` Marek Vasut
1 sibling, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2025-09-22 11:35 UTC (permalink / raw)
To: Marek Vasut; +Cc: linux-clk, Michael Turquette, Stephen Boyd, linux-renesas-soc
Hi Marek,
On Thu, 18 Sept 2025 at 15:45, 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>
Thanks for your patch, which fixes the PCIe SError for me, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -676,18 +676,31 @@ static int __init cpg_mssr_add_clk_domain(struct device *dev,
>
> #define rcdev_to_priv(x) container_of(x, struct cpg_mssr_priv, rcdev)
>
> -static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> - unsigned long id)
> +static int cpg_mssr_writel_with_latch(struct reset_controller_dev *rcdev,
> + char *func, bool set, unsigned long id)
This function does a bit more than writel()-with-latch, so please find
a more suitable name. Or... continue reading.
> {
> struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> unsigned int reg = id / 32;
> unsigned int bit = id % 32;
> + const u16 reset_reg = set ? priv->reset_regs[reg] : priv->reset_clear_regs[reg];
> u32 bitmask = BIT(bit);
>
> - dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
> + if (func)
> + dev_dbg(priv->dev, "%s %u%02u\n", func, reg, bit);
> +
> + writel(bitmask, priv->pub.base0 + reset_reg);
> + readl(priv->pub.base0 + reset_reg);
> +
> + return 0;
> +}
Now, do we want a special de(reset)-with-latch() function (which does
reduce code duplication), or would a simpler variant be more useful?
After this, we have three different "dummy read" mechanisms in this
driver:
1. Clock enable/disable and resume on RZ/A:
writeb(value, priv->pub.base0 + priv->control_regs[reg]);
/* dummy read to ensure write has completed */
readb(priv->pub.base0 + priv->control_regs[reg]);
barrier_data(priv->pub.base0 + priv->control_regs[reg]);
2. Reset handling on R-Car:
writel(bitmask, priv->pub.base0 + reset_reg);
readl(priv->pub.base0 + reset_reg);
3. Reset release on RZ/T2H and RZ/N2H:
writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
/*
* To secure processing after release from a module reset, dummy read
* the same register at least seven times.
*/
for (i = 0; i < 7; i++)
readl(priv->pub.base0 + priv->reset_regs[reg]);
So perhaps a simple helper like
void writel_with_latch(u32 val, volatile void __iomem *addr, unsigned int n)
{
writel(val, addr);
while (n-- > 0)
readl(addr);
}
? Do we need barrier_data(), like on RZ/A?
Unfortunately RZ/A uses byte-wide registers, so that one needs another
copy.
> +
> +static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
"priv" is unused (no compiler warning on your side?)
>
> /* Reset module */
> - writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> + cpg_mssr_writel_with_latch(rcdev, "reset", true, id);
>
> /*
> * On R-Car Gen4, delay after SRCR has been written is 1ms.
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched
2025-09-22 11:35 ` Geert Uytterhoeven
@ 2025-09-22 16:24 ` Marek Vasut
2025-09-23 7:11 ` Geert Uytterhoeven
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2025-09-22 16:24 UTC (permalink / raw)
To: Geert Uytterhoeven, Marek Vasut
Cc: linux-clk, Michael Turquette, Stephen Boyd, linux-renesas-soc
On 9/22/25 1:35 PM, Geert Uytterhoeven wrote:
Hello Geert,
>> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
>> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
>> @@ -676,18 +676,31 @@ static int __init cpg_mssr_add_clk_domain(struct device *dev,
>>
>> #define rcdev_to_priv(x) container_of(x, struct cpg_mssr_priv, rcdev)
>>
>> -static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>> - unsigned long id)
>> +static int cpg_mssr_writel_with_latch(struct reset_controller_dev *rcdev,
>> + char *func, bool set, unsigned long id)
>
> This function does a bit more than writel()-with-latch, so please find
> a more suitable name. Or... continue reading.
I did so in V4.
>> {
>> struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
>> unsigned int reg = id / 32;
>> unsigned int bit = id % 32;
>> + const u16 reset_reg = set ? priv->reset_regs[reg] : priv->reset_clear_regs[reg];
>> u32 bitmask = BIT(bit);
>>
>> - dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
>> + if (func)
>> + dev_dbg(priv->dev, "%s %u%02u\n", func, reg, bit);
>> +
>> + writel(bitmask, priv->pub.base0 + reset_reg);
>> + readl(priv->pub.base0 + reset_reg);
>> +
>> + return 0;
>> +}
>
> Now, do we want a special de(reset)-with-latch() function (which does
> reduce code duplication), or would a simpler variant be more useful?
> After this, we have three different "dummy read" mechanisms in this
> driver:
>
> 1. Clock enable/disable and resume on RZ/A:
>
> writeb(value, priv->pub.base0 + priv->control_regs[reg]);
>
> /* dummy read to ensure write has completed */
> readb(priv->pub.base0 + priv->control_regs[reg]);
> barrier_data(priv->pub.base0 + priv->control_regs[reg]);
>
> 2. Reset handling on R-Car:
>
> writel(bitmask, priv->pub.base0 + reset_reg);
> readl(priv->pub.base0 + reset_reg);
>
> 3. Reset release on RZ/T2H and RZ/N2H:
Maybe T2H support is not yet upstream , even in next ?
In any case, 2 and 3 could be merged into single write-and-latch function.
> writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
>
> /*
> * To secure processing after release from a module reset, dummy read
> * the same register at least seven times.
> */
> for (i = 0; i < 7; i++)
> readl(priv->pub.base0 + priv->reset_regs[reg]);
>
> So perhaps a simple helper like
>
> void writel_with_latch(u32 val, volatile void __iomem *addr, unsigned int n)
> {
> writel(val, addr);
> while (n-- > 0)
> readl(addr);
> }
>
> ? Do we need barrier_data(), like on RZ/A?
I think so.
> Unfortunately RZ/A uses byte-wide registers, so that one needs another
> copy.
>
>> +
>> +static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
>> + unsigned long id)
>> +{
>> + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
>
> "priv" is unused (no compiler warning on your side?)
I also have [PATCH] clk: renesas: cpg-mssr: Add missing 1ms delay into
reset toggle callback applied in tree, rebase was not accurate, dropped
in V4.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched
2025-09-22 16:24 ` Marek Vasut
@ 2025-09-23 7:11 ` Geert Uytterhoeven
2025-09-23 9:08 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2025-09-23 7:11 UTC (permalink / raw)
To: Marek Vasut
Cc: Marek Vasut, linux-clk, Michael Turquette, Stephen Boyd,
linux-renesas-soc
Hi Marek,
On Mon, 22 Sept 2025 at 18:24, Marek Vasut <marek.vasut@mailbox.org> wrote:
> On 9/22/25 1:35 PM, Geert Uytterhoeven wrote:
> >> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> >> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> >> @@ -676,18 +676,31 @@ static int __init cpg_mssr_add_clk_domain(struct device *dev,
> >>
> >> #define rcdev_to_priv(x) container_of(x, struct cpg_mssr_priv, rcdev)
> >>
> >> -static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> >> - unsigned long id)
> >> +static int cpg_mssr_writel_with_latch(struct reset_controller_dev *rcdev,
> >> + char *func, bool set, unsigned long id)
> >
> > This function does a bit more than writel()-with-latch, so please find
> > a more suitable name. Or... continue reading.
>
> I did so in V4.
>
> >> {
> >> struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> >> unsigned int reg = id / 32;
> >> unsigned int bit = id % 32;
> >> + const u16 reset_reg = set ? priv->reset_regs[reg] : priv->reset_clear_regs[reg];
> >> u32 bitmask = BIT(bit);
> >>
> >> - dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
> >> + if (func)
> >> + dev_dbg(priv->dev, "%s %u%02u\n", func, reg, bit);
> >> +
> >> + writel(bitmask, priv->pub.base0 + reset_reg);
> >> + readl(priv->pub.base0 + reset_reg);
> >> +
> >> + return 0;
> >> +}
> >
> > Now, do we want a special de(reset)-with-latch() function (which does
> > reduce code duplication), or would a simpler variant be more useful?
> > After this, we have three different "dummy read" mechanisms in this
> > driver:
> >
> > 1. Clock enable/disable and resume on RZ/A:
> >
> > writeb(value, priv->pub.base0 + priv->control_regs[reg]);
> >
> > /* dummy read to ensure write has completed */
> > readb(priv->pub.base0 + priv->control_regs[reg]);
> > barrier_data(priv->pub.base0 + priv->control_regs[reg]);
> >
> > 2. Reset handling on R-Car:
> >
> > writel(bitmask, priv->pub.base0 + reset_reg);
> > readl(priv->pub.base0 + reset_reg);
> >
> > 3. Reset release on RZ/T2H and RZ/N2H:
>
> Maybe T2H support is not yet upstream , even in next ?
My bad, that is still under review (and I didn't even have the latest
version in my local tree). Latest version is
"[PATCH v3] clk: renesas: cpg-mssr: Add module reset support for RZ/T2H"
https://lore.kernel.org/all/20250905114558.1602756-1-prabhakar.mahadev-lad.rj@bp.renesas.com
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched
2025-09-23 7:11 ` Geert Uytterhoeven
@ 2025-09-23 9:08 ` Marek Vasut
0 siblings, 0 replies; 6+ messages in thread
From: Marek Vasut @ 2025-09-23 9:08 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Marek Vasut, linux-clk, Michael Turquette, Stephen Boyd,
linux-renesas-soc
On 9/23/25 9:11 AM, Geert Uytterhoeven wrote:
Hello Geert,
>>> 3. Reset release on RZ/T2H and RZ/N2H:
>>
>> Maybe T2H support is not yet upstream , even in next ?
>
> My bad, that is still under review (and I didn't even have the latest
> version in my local tree). Latest version is
> "[PATCH v3] clk: renesas: cpg-mssr: Add module reset support for RZ/T2H"
> https://lore.kernel.org/all/20250905114558.1602756-1-prabhakar.mahadev-lad.rj@bp.renesas.com
Let's see how the T2H/N2H reset fares and which patch lands first, then
we can unify the accessors one way or the other ?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-09-23 9:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-18 13:44 [PATCH v2] clk: renesas: cpg-mssr: Read back reset registers to assure values latched Marek Vasut
2025-09-20 4:26 ` Wolfram Sang
2025-09-22 11:35 ` Geert Uytterhoeven
2025-09-22 16:24 ` Marek Vasut
2025-09-23 7:11 ` Geert Uytterhoeven
2025-09-23 9:08 ` Marek Vasut
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox