[permalink] [raw]

Subject: Re: [PATCH net-next v3 08/19] net: ravb: Move the IRQs get and request in the probe function

On 14.01.2024 20:07, Sergey Shtylyov wrote:
> On 1/10/24 2:55 PM, claudiu beznea wrote:
>
> [...]
>
>>>>>> From: Claudiu Beznea <[email protected]>
>>>>>>
>>>>>> The runtime PM implementation will disable clocks at the end of
>>>>>> ravb_probe(). As some IP variants switch to reset mode as a result of
>>>>>> setting module standby through clock disable APIs, to implement runtime PM
>>>>>> the resource parsing and requesting are moved in the probe function and IP
>>>>>> settings are moved in the open function. This is done because at the end of
>>>>>> the probe some IP variants will switch anyway to reset mode and the
>>>>>> registers content is lost. Also keeping only register specific operations
>>>>>> in the ravb_open()/ravb_close() functions will make them faster.
>>>>>>
>>>>>> Commit moves IRQ requests to ravb_probe() to have all the IRQs ready when
>>>>>> the interface is open. As now IRQs gets and requests are in a single place
>>>>>> there is no need to keep intermediary data (like ravb_rx_irqs[] and
>>>>>> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
>>>>>
>>>>> There's one thing that you probably didn't take into account: after
>>>>> you call request_irq(), you should be able to handle your IRQ as it's
>>>>> automatically unmasked, unless you pass IRQF_NO_AUTOEN to request_irq().
>>>>> Your device may be held i reset or even powered off but if you pass
>>>>> IRQF_SHARED to request_irq() (you do in a single IRQ config), you must
>>>>> be prepared to get your device's registers read (in order to ascertain
>>>
>>> And, at least on arm32, reading a powered off (or not clocked?) device's
>>> register causes an imprecise external abort exception -- which results in a
>>> kernel oops...
>>>
>>>>> whether it's your IRQ or not). And you can't even pass IRQF_NO_AUTOEN
>>>>> along with IRQF_SHARED, according to my reading of the IRQ code...
>>>>
>>>> Good point!
>
> Iff we can come up with a robust check whether the device is powered on,
> we can overcome even the IRQF_SHARED issue though...
> I'm seeing pm_runtime_active() API and wondering whether we can use it
> from the IRQ context. Alternatively, we can add a is_opened flag, like
> sh_eth.c does...

The is_open flag should deal with this, too, AFAICT at the moment, and
should also cover your concerns about U-Boot.

Thank you,
Claudiu Beznea

>
>>>>>> This is a preparatory change to add runtime PM support for all IP variants.
>>>>>
>>>>> I don't readily see why this is necessary for the full-fledged RPM
>>>>> support...
>>>>
>>>> I tried to speed up the ravb_open()/ravb_close() but missed the IRQF_SHARED
>>>
>>> I doubt that optimizing ravb_{open,close}() is worth pursuing, frankly...
>
> OTOH, we'll get a simpler/cleaner code if we do this...
> Previously, I was under an impression that it's common behavior of
> the networking drivers to call request_irq() from their ndo_open() methods.
> Apparently, it's not true anymore (probably with the introduction of the
> managed device API) -- the newer drivers often call devm_request_irq()
> from their probe() methods instead...
>
>>>> IRQ. As there is only one IRQ requested w/ IRQF_SHARED, are you OK with
>>>> still keeping the rest of IRQs handled as proposed by this patch?
>>>
>>> I'm not, as this doesn't really seem necessary for your main goal.
>>> It's not clear in what state U-Boot leaves EtherAVB...
>
> This still seems an issue though... My prior experience with the R-Car
> MMC driver tells me that U-Boot may leave interrupts enabled... :-/
>
>> Ok. One other reason I did this is, as commit message states, to keep
>> resource parsing and allocation/freeing in probe/remove and hardware
>> settings in open/close.
>>
>> Anyway, I'll revert all the changes IRQ related.
>
> Now I've changed my mind -- let's retain your patch! It needs some work
> though...
>
>> Thank you,
>> Claudiu Beznea
>
> [...]
>
> MBR, Sergey

2024-01-23 20:07:25

by Sergey Shtylyov

[permalink] [raw]

Subject: Re: [PATCH net-next v3 07/19] net: ravb: Move reference clock enable/disable on runtime PM APIs

On 1/8/24 11:03 AM, claudiu beznea wrote:

Oops, looks like I forgot to reply to this one...

[...]
>>> From: Claudiu Beznea <[email protected]>
>>>
>>> Reference clock could be or not part of the power domain. If it is part of
>>> the power domain, the power domain takes care of propertly setting it. In
>>> case it is not part of the power domain and full runtime PM support is
>>> available in driver the clock will not be propertly disabled/enabled at
>>> runtime. For this, keep the prepare/unprepare operations in the driver's
>>> probe()/remove() functions and move the enable/disable in runtime PM
>>> functions.
>>>
>>> Along with it, the other clock request operations were moved close to
>>> reference clock request and prepare to have all the clock requests
>>> specific code grouped together.
>>>
>>> Reviewed-by: Sergey Shtylyov <[email protected]>

[...]

>>> Signed-off-by: Claudiu Beznea <[email protected]>
[...]

>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 844ac3306e93..4673cc2faec0 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>> + pm_runtime_enable(&pdev->dev);
>>> + error = pm_runtime_resume_and_get(&pdev->dev);
>>> + if (error < 0)
>>> + goto out_rpm_disable;
>>> +
>>> priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>> if (IS_ERR(priv->addr)) {
>>> error = PTR_ERR(priv->addr);
>>> - goto out_release;
>>> + goto out_rpm_put;
>>> }
>>>
>>> /* The Ether-specific entries in the device structure. */
[...]
>>> @@ -3060,21 +3058,27 @@ static int ravb_resume(struct device *dev)
>>> return ret;
>>> }
>>>
>>> -static int ravb_runtime_nop(struct device *dev)
>>> +static int ravb_runtime_suspend(struct device *dev)
>>> {
>>> - /* Runtime PM callback shared between ->runtime_suspend()
>>> - * and ->runtime_resume(). Simply returns success.
>>> - *
>>> - * This driver re-initializes all registers after
>>> - * pm_runtime_get_sync() anyway so there is no need
>>> - * to save and restore registers here.
>>> - */
>>
>> Perhaps even worth a separate patch to completely remove this function
>> which doesn't seem to make sense?
>
> Why? With that the refclk will not be properly enabled/disabled when it
> will not be part of the power domain. Take

That's what you are adding in this patch, right? Before this patch
this was ravb_runtime_nop(), always returning 0. Did it make any sense?

> https://elixir.bootlin.com/linux/v6.7/source/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi#L57
> as an example. Here refclk is from an external source (not part of power
> domain).
>
> Thank you,
> Claudiu Beznea

[...]

MBR, Sergey