[PATCH] PCI: rcar-gen4: Add missing 1ms delay after PWR reset assertion

[PATCH] PCI: rcar-gen4: Add missing 1ms delay after PWR reset assertion

[Marek Vasut]

R-Car V4H Reference Manual R19UH0186EJ0130 Rev.1.30 Apr. 21, 2025 page 585
Figure 9.3.2 Software Reset flow (B) indicates that for peripherals in HSC
domain, after reset has been asserted by writing a matching reset bit into
register SRCR, it is mandatory to wait 1ms.

Because it is the controller driver which can determine whether or not the
controller is in HSC domain based on its compatible string, add the missing
delay into the controller driver.

This 1ms delay is documented on R-Car V4H and V4M, it is currently unclear
whether S4 is affected as well. This patch does apply the extra delay on
R-Car S4 as well.

Fixes: 0d0c551011df ("PCI: rcar-gen4: Add R-Car Gen4 PCIe controller support for host mode")
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/pci/controller/dwc/pcie-rcar-gen4.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index de9fe2ed2423..8b39e014bc7c 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -182,8 +182,10 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
 		return ret;
 	}
 
-	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc))
+	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc)) {
 		reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc);
+		usleep_range(1000, 2000);
+	}
 
 	val = readl(rcar->base + PCIEMSR0);
 	if (rcar->drvdata->mode == DW_PCIE_RC_TYPE) {
-- 
2.51.0

Re: [PATCH] PCI: rcar-gen4: Add missing 1ms delay after PWR reset assertion

[Bjorn Helgaas]

On Thu, Sep 18, 2025 at 05:00:26AM +0200, Marek Vasut wrote:
> R-Car V4H Reference Manual R19UH0186EJ0130 Rev.1.30 Apr. 21, 2025 page 585
> Figure 9.3.2 Software Reset flow (B) indicates that for peripherals in HSC
> domain, after reset has been asserted by writing a matching reset bit into
> register SRCR, it is mandatory to wait 1ms.

> @@ -182,8 +182,10 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
>  		return ret;
>  	}
>  
> -	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc))
> +	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc)) {
>  		reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc);
> +		usleep_range(1000, 2000);

What would you think of "fsleep(1000)"?

I know there's controvery about fsleep(), but while the 1000 usec
lower bound is important, I think the selection of the 2000 usec upper
bound is pretty arbitrary and doesn't really justify spelling it out.

> +	}
>  
>  	val = readl(rcar->base + PCIEMSR0);
>  	if (rcar->drvdata->mode == DW_PCIE_RC_TYPE) {
> -- 
> 2.51.0
> 

Re: [PATCH] PCI: rcar-gen4: Add missing 1ms delay after PWR reset assertion

[Marek Vasut]

On 9/18/25 10:04 PM, Bjorn Helgaas wrote:
> On Thu, Sep 18, 2025 at 05:00:26AM +0200, Marek Vasut wrote:
>> R-Car V4H Reference Manual R19UH0186EJ0130 Rev.1.30 Apr. 21, 2025 page 585
>> Figure 9.3.2 Software Reset flow (B) indicates that for peripherals in HSC
>> domain, after reset has been asserted by writing a matching reset bit into
>> register SRCR, it is mandatory to wait 1ms.
> 
>> @@ -182,8 +182,10 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
>>   		return ret;
>>   	}
>>   
>> -	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc))
>> +	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc)) {
>>   		reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc);
>> +		usleep_range(1000, 2000);
> 
> What would you think of "fsleep(1000)"?
> 
> I know there's controvery about fsleep(), but while the 1000 usec
> lower bound is important, I think the selection of the 2000 usec upper
> bound is pretty arbitrary and doesn't really justify spelling it out.
The upper bound is arbitrary.

My reasoning for picking up usleep_range() is to give the kernel 
sufficient space to pick the best fitting delay in that 1..2 ms range, 
without constraining the timers too much. In other words, let the kernel 
pick the next easy to use timer tick which guarantees at least 1ms delay.

As far as I can tell, fsleep() in this case would add a bit of 
auto-detection overhead, and then select equivalent of 
usleep_range(1000, 1250) , wouldn't it ?

So I think using fsleep() here would add overhead, but not yield any 
actual benefit. Is my understanding and conclusions correct ?

Re: [PATCH] PCI: rcar-gen4: Add missing 1ms delay after PWR reset assertion

[Bjorn Helgaas]

On Thu, Sep 18, 2025 at 10:35:08PM +0200, Marek Vasut wrote:
> On 9/18/25 10:04 PM, Bjorn Helgaas wrote:
> > On Thu, Sep 18, 2025 at 05:00:26AM +0200, Marek Vasut wrote:
> > > R-Car V4H Reference Manual R19UH0186EJ0130 Rev.1.30 Apr. 21, 2025 page 585
> > > Figure 9.3.2 Software Reset flow (B) indicates that for peripherals in HSC
> > > domain, after reset has been asserted by writing a matching reset bit into
> > > register SRCR, it is mandatory to wait 1ms.
> > 
> > > @@ -182,8 +182,10 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
> > >   		return ret;
> > >   	}
> > > -	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc))
> > > +	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc)) {
> > >   		reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc);
> > > +		usleep_range(1000, 2000);
> > 
> > What would you think of "fsleep(1000)"?
> > 
> > I know there's controvery about fsleep(), but while the 1000 usec
> > lower bound is important, I think the selection of the 2000 usec upper
> > bound is pretty arbitrary and doesn't really justify spelling it out.
>
> The upper bound is arbitrary.
> 
> My reasoning for picking up usleep_range() is to give the kernel
> sufficient space to pick the best fitting delay in that 1..2 ms
> range, without constraining the timers too much. In other words, let
> the kernel pick the next easy to use timer tick which guarantees at
> least 1ms delay.

Right, basically the same motivation as fsleep().

> As far as I can tell, fsleep() in this case would add a bit of
> auto-detection overhead, and then select equivalent of
> usleep_range(1000, 1250) , wouldn't it ?
> 
> So I think using fsleep() here would add overhead, but not yield any
> actual benefit. Is my understanding and conclusions correct ?

I think you're right that fsleep() will pick usleep_range(1000, 1250),
so it's less optimal in that sense, but I think optimization like that
would be better done inside fsleep() instead of everybody doing it ad
hoc at the call site.

I don't think fsleep() should add any overhead since it's inlined and
all the delays are constants.  But I haven't actually looked at the
generated code.

Re: [PATCH] PCI: rcar-gen4: Add missing 1ms delay after PWR reset assertion

[Marek Vasut]

On 9/18/25 10:44 PM, Bjorn Helgaas wrote:
> On Thu, Sep 18, 2025 at 10:35:08PM +0200, Marek Vasut wrote:
>> On 9/18/25 10:04 PM, Bjorn Helgaas wrote:
>>> On Thu, Sep 18, 2025 at 05:00:26AM +0200, Marek Vasut wrote:
>>>> R-Car V4H Reference Manual R19UH0186EJ0130 Rev.1.30 Apr. 21, 2025 page 585
>>>> Figure 9.3.2 Software Reset flow (B) indicates that for peripherals in HSC
>>>> domain, after reset has been asserted by writing a matching reset bit into
>>>> register SRCR, it is mandatory to wait 1ms.
>>>
>>>> @@ -182,8 +182,10 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar)
>>>>    		return ret;
>>>>    	}
>>>> -	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc))
>>>> +	if (!reset_control_status(dw->core_rsts[DW_PCIE_PWR_RST].rstc)) {
>>>>    		reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc);
>>>> +		usleep_range(1000, 2000);
>>>
>>> What would you think of "fsleep(1000)"?
>>>
>>> I know there's controvery about fsleep(), but while the 1000 usec
>>> lower bound is important, I think the selection of the 2000 usec upper
>>> bound is pretty arbitrary and doesn't really justify spelling it out.
>>
>> The upper bound is arbitrary.
>>
>> My reasoning for picking up usleep_range() is to give the kernel
>> sufficient space to pick the best fitting delay in that 1..2 ms
>> range, without constraining the timers too much. In other words, let
>> the kernel pick the next easy to use timer tick which guarantees at
>> least 1ms delay.
> 
> Right, basically the same motivation as fsleep().
> 
>> As far as I can tell, fsleep() in this case would add a bit of
>> auto-detection overhead, and then select equivalent of
>> usleep_range(1000, 1250) , wouldn't it ?
>>
>> So I think using fsleep() here would add overhead, but not yield any
>> actual benefit. Is my understanding and conclusions correct ?
> 
> I think you're right that fsleep() will pick usleep_range(1000, 1250),
> so it's less optimal in that sense, but I think optimization like that
> would be better done inside fsleep() instead of everybody doing it ad
> hoc at the call site.

Lemme do V2 with fsleep() then.

> I don't think fsleep() should add any overhead since it's inlined and
> all the delays are constants.  But I haven't actually looked at the
> generated code.

You're right about this part.