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

[Marek Vasut <marek.vasut@mailbox.org>]

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.