Re: [PATCH] PCI: rcar-gen4: Fix inverted break condition in PHY initialization

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

On 9/17/25 10:00 AM, Geert Uytterhoeven wrote:

Hello Geert,

>> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
>> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
>> @@ -688,12 +688,14 @@ 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]);
>>
>>          /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
>>          udelay(35);
>>
>>          /* 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;
>>    }
>> @@ -708,6 +710,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;
>>    }
>>
>> @@ -722,6 +725,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;
>>    }
>>
> 
> Yes, reading the reset registers after writing works, too.
> 
> I just noticed the module reset operation in the R-Car V4H Hardware
> User's Manual has changed from R-Car Gen2/Gen3, and is now more complex,
> with three different reset types, depending on the module to be reset
> (see Section 9.3 "Operation" subsection (2) "Software Reset").
> The most striking difference is that there is no more mention of
> a single delay of 1 RCLK cycle (cfr. the udelay(35) above), but of
> much longer delays of 1 ms.
> 
> As drivers/pci/controller/dwc/pcie-rcar-gen4.c does not call the
> combined reset_control_reset() , but uses separate
> reset_control_assert() and reset_control_deassert() calls, the PCIe
> driver itself is responsible for enforcing this 1 ms delay.

Shouldn't we patch the reset driver and insert unconditional 1ms delay 
into reset_assert() callback ?

> Which is exactly what your introduction of mdelay(1) (just after the
> out-of-context call to reset_control_deassert()) does, so I believe
> we're all set?

No, I do not think so. Figure 9.3.2 Software Reset flow (B) on page 585 
does NOT describe 1ms delay after write into SRSTCLR register. It does 
describe 1ms delay after write into SRCR register. That means, we need 
1ms delay after reset_control_assert(), but it does NOT mean we need 1ms 
delay after reset_control_deassert() . That means the issue remains 
unexplained ?