On 11/23/23 8:04 PM, claudiu beznea wrote:
[...]
>>> From: Claudiu Beznea <[email protected]>
>>
>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>> Ethernet module). For this commit adds runtime PM support which
>>> relies on PM domain to enable/disable Ethernet clocks.
>>
>> That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>> RPM calls already in the probe() and remove() methods...
>> And the sh_eth driver
>> has RPM support since 2009...
>>
>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>
>> I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>> it doesn't seem hardly needed...
Does seem, sorry. :-)
>>> off the Ethernet clocks (if no other request arrives at the driver).
>>> After that if the interface is brought up (though ravb_open()) then
>>> the clocks remain enabled until interface is brought down (operation
>>> done though ravb_close()).
>>>
>>> If any request arrives to the driver while the interface is down the
>>> clocks are enabled to serve the request and then disabled.
>>>
>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>> ---
>>> drivers/net/ethernet/renesas/ravb.h | 1 +
>>> drivers/net/ethernet/renesas/ravb_main.c | 99 ++++++++++++++++++++++--
>>> 2 files changed, 93 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>> index c2d8d890031f..50f358472aab 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>> unsigned magic_pkt:1; /* E-MAC supports magic packet detection */
>>> unsigned half_duplex:1; /* E-MAC supports half duplex mode */
>>> unsigned refclk_in_pd:1; /* Reference clock is part of a power domain. */
>>> + unsigned rpm:1; /* Runtime PM available. */
>>
>> No, I don't think this flag makes any sense. We should support RPM
>> unconditionally...
If RPM calls work in the probe()/remove() methods, they should work
in the ndo_{open|stop}() methods, right?
> The reasons I've limited only to RZ/G3S are:
> 1/ I don't have all the platforms to test it
That's a usual problem with the kernel development...
> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
> platform at hand, only remotely, and is hardly to debug once the
> ethernet fails to work: probe is working(), open is executed, PHY is
> initialized and then TX/RX is not working... don't know why ATM.
That's why we have the long bug fixing period after -rc1...
[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index f4634ac0c972..d70ed7e5f7f6 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -145,12 +145,41 @@ static void ravb_read_mac_address(struct device_node *np,
[...]
>>> +}
>>> +
>>> static void ravb_mdio_ctrl(struct mdiobb_ctrl *ctrl, u32 mask, int set)
>>> {
>>> struct ravb_private *priv = container_of(ctrl, struct ravb_private,
>>> mdiobb);
>>> + int ret;
>>> +
>>> + ret = ravb_pm_runtime_get(priv);
>>> + if (ret < 0)
>>> + return;
>>>
>>> ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);
>>> +
>>> + ravb_pm_runtime_put(priv);
>>
>> Hmm, does this even work? :-/ Do the MDIO bits retain the values while
>> the AVB core is not clocked or even powered down?
>
> This actually is not needed. It's a leftover. I double checked with
> mii-tools to access the device while the interface is down and the IOCTL is
> blocked in this case by
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/renesas/ravb_main.c#L2266
Have you tested with ethtool as well?
>> Note that the sh_eth driver has RPM calls in the {read|write}_c{22?45}()
s/?/|/,
>> methods which do the full register read/write while the core is powere up
Powered.
>> and clocked...
>>
>> [...]
>>> @@ -2064,6 +2107,11 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>>> struct ravb_private *priv = netdev_priv(ndev);
>>> const struct ravb_hw_info *info = priv->info;
>>> struct net_device_stats *nstats, *stats0, *stats1;
>>> + int ret;
>>> +
>>> + ret = ravb_pm_runtime_get(priv);
>>> + if (ret < 0)
>>> + return NULL;
>>
>> Hm, sh_eth.c doesn't have any RPM calls in this method. Again, do
>
> In setups where systemd is enabled, user space calls this method in
> different stages (e.g. at boot time or when running ifconfig ethX, even if
> interface is down). W/o runtime resuming here the system will fail to boot.
>
> The other approach I wanted to take was to:
>
> if (!netif_running(dev))
> return &ndev->stats;
>
> But I didn't choose this path as there are some counters updated to nstat
> only in this function, e.g. nstats->tx_dropped += ravb_read(ndev, TROCR);
> and wanted an opinion about it.
Have you seen the following commit (that I've already posted for you on
IRC)?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7fa2955ff70ce4532f144d26b8a087095f9c9ffc
Looks like the RPM calls won't do here...
>> the hardware counters remain valid across powering the MAC core down?
>
> The power domain that the Ethernet clocks of RZ/G3S belong disables the
> clock and switches the Ethernet module to standby. There is no information
> in HW manual that the content of registers will be lost.
That's what your current PD driver does... isn't it possible that
in some new SoCs the PD would be completely powered off?
[...]
>>> @@ -2115,11 +2165,18 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>>> {
>>> struct ravb_private *priv = netdev_priv(ndev);
>>> unsigned long flags;
>>> + int ret;
>>> +
>>> + ret = ravb_pm_runtime_get(priv);
>>> + if (ret < 0)
>>> + return;
>>
>> Hm, sh_eth.c doesn't have any RPM calls in this method either.
>> Does changing the promiscous mode have sense for an offlined interface?
>
> I've added it for scenarios when the interface is down and user tries to
> configure it. I don't know to answer your question. W/o RPM resume here
> user space blocks if tries to access it and interface is down. I can just
> return if interface is down. Let me know if you prefer this way.
Looking at __dev_set_rx_mode(), the method gets only called when
(dev->flags & IFF_UP) is true -- but that contradicts your experience,
it seems... However, looking at net/core/dev_addr_lists.c, that function
is called from the atomic contexts, so please just return early.
>> [...]
>>> @@ -2187,6 +2244,11 @@ static int ravb_close(struct net_device *ndev)
>>> if (info->nc_queues)
>>> ravb_ring_free(ndev, RAVB_NC);
>>>
>>> + /* Note that if RPM is enabled on plaforms with ccc_gac=1 this needs to be
>>
>> It's "platforms". :-)
>>
>>> skipped and
>>
>> Overly long line?
>
> Not more than 100 chars. Do you want it to 80?
Yes, it's not the code, no need to go beyond 80 cols, I think...
[...]
MBR, Sergey
On 23.11.2023 21:19, Sergey Shtylyov wrote:
> On 11/23/23 8:04 PM, claudiu beznea wrote:
>
> [...]
>
>>>> From: Claudiu Beznea <[email protected]>
>>>
>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>> Ethernet module). For this commit adds runtime PM support which
>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>
>>> That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>> RPM calls already in the probe() and remove() methods...
>>> And the sh_eth driver
>>> has RPM support since 2009...
>>>
>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>
>>> I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>> it doesn't seem hardly needed...
>
> Does seem, sorry. :-)
>
>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>> After that if the interface is brought up (though ravb_open()) then
>>>> the clocks remain enabled until interface is brought down (operation
>>>> done though ravb_close()).
>>>>
>>>> If any request arrives to the driver while the interface is down the
>>>> clocks are enabled to serve the request and then disabled.
>>>>
>>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>>> ---
>>>> drivers/net/ethernet/renesas/ravb.h | 1 +
>>>> drivers/net/ethernet/renesas/ravb_main.c | 99 ++++++++++++++++++++++--
>>>> 2 files changed, 93 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>>> index c2d8d890031f..50f358472aab 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>>> unsigned magic_pkt:1; /* E-MAC supports magic packet detection */
>>>> unsigned half_duplex:1; /* E-MAC supports half duplex mode */
>>>> unsigned refclk_in_pd:1; /* Reference clock is part of a power domain. */
>>>> + unsigned rpm:1; /* Runtime PM available. */
>>>
>>> No, I don't think this flag makes any sense. We should support RPM
>>> unconditionally...
>
> If RPM calls work in the probe()/remove() methods, they should work
> in the ndo_{open|stop}() methods, right?
It might depend on hardware support... E.g.
I debugged it further the issue I had with this implementation on other
SoCs and it seems we cannot do RPM for those w/o reworking way the driver
is configured.
I wiped out the RPM code from this patch and just called:
pm_runtime_put_sync(); // [1]
usleep_range(300000, 400000); // [2]
pm_runtime_get_sync(); // [3]
at the end of ravb_probe(); with this the interfaces fails to work. I
continue debugging it and interrogated CSR and this returns RESET after
[3]. I tried to switched it back to configuration mode after [3] but fails
to restore to a proper working state.
Then continued to debug it further to see what happens on the clock driver.
The clk enable/disable reaches function at [4] which sets control_regs[reg]
which is one of the System module stop control registers. Setting this
activates module standby (AFICT). Switch to reset state on Ethernet IP
might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
chapter of the G1H HW manual (which I don't fully understand).
Also, the manual of G1H states from some IPs that register state is
preserved in standby mode but not for AVB.
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/renesas/renesas-cpg-mssr.c#n190
>
>> The reasons I've limited only to RZ/G3S are:
>> 1/ I don't have all the platforms to test it
>
> That's a usual problem with the kernel development...
>
>> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
>> platform at hand, only remotely, and is hardly to debug once the
>> ethernet fails to work: probe is working(), open is executed, PHY is
>> initialized and then TX/RX is not working... don't know why ATM.
>
> That's why we have the long bug fixing period after -rc1...
I prefer to not introduce any bug by intention.
>
> [...]
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index f4634ac0c972..d70ed7e5f7f6 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -145,12 +145,41 @@ static void ravb_read_mac_address(struct device_node *np,
> [...]
>>>> +}
>>>> +
>>>> static void ravb_mdio_ctrl(struct mdiobb_ctrl *ctrl, u32 mask, int set)
>>>> {
>>>> struct ravb_private *priv = container_of(ctrl, struct ravb_private,
>>>> mdiobb);
>>>> + int ret;
>>>> +
>>>> + ret = ravb_pm_runtime_get(priv);
>>>> + if (ret < 0)
>>>> + return;
>>>>
>>>> ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);
>>>> +
>>>> + ravb_pm_runtime_put(priv);
>>>
>>> Hmm, does this even work? :-/ Do the MDIO bits retain the values while
>>> the AVB core is not clocked or even powered down?
>>
>> This actually is not needed. It's a leftover. I double checked with
>> mii-tools to access the device while the interface is down and the IOCTL is
>> blocked in this case by
>> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/renesas/ravb_main.c#L2266
>
> Have you tested with ethtool as well?
>
>>> Note that the sh_eth driver has RPM calls in the {read|write}_c{22?45}()
>
> s/?/|/,
>
>>> methods which do the full register read/write while the core is powere up
>
> Powered.
>
>>> and clocked...
>>>
>>> [...]
>>>> @@ -2064,6 +2107,11 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev)
>>>> struct ravb_private *priv = netdev_priv(ndev);
>>>> const struct ravb_hw_info *info = priv->info;
>>>> struct net_device_stats *nstats, *stats0, *stats1;
>>>> + int ret;
>>>> +
>>>> + ret = ravb_pm_runtime_get(priv);
>>>> + if (ret < 0)
>>>> + return NULL;
>>>
>>> Hm, sh_eth.c doesn't have any RPM calls in this method. Again, do
>>
>> In setups where systemd is enabled, user space calls this method in
>> different stages (e.g. at boot time or when running ifconfig ethX, even if
>> interface is down). W/o runtime resuming here the system will fail to boot.
>>
>> The other approach I wanted to take was to:
>>
>> if (!netif_running(dev))
>> return &ndev->stats;
>>
>> But I didn't choose this path as there are some counters updated to nstat
>> only in this function, e.g. nstats->tx_dropped += ravb_read(ndev, TROCR);
>> and wanted an opinion about it.
>
> Have you seen the following commit (that I've already posted for you on
> IRC)?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7fa2955ff70ce4532f144d26b8a087095f9c9ffc
>
> Looks like the RPM calls won't do here...
>
>>> the hardware counters remain valid across powering the MAC core down?
>>
>> The power domain that the Ethernet clocks of RZ/G3S belong disables the
>> clock and switches the Ethernet module to standby. There is no information
>> in HW manual that the content of registers will be lost.
>
> That's what your current PD driver does... isn't it possible that
> in some new SoCs the PD would be completely powered off?
>
> [...]
>>>> @@ -2115,11 +2165,18 @@ static void ravb_set_rx_mode(struct net_device *ndev)
>>>> {
>>>> struct ravb_private *priv = netdev_priv(ndev);
>>>> unsigned long flags;
>>>> + int ret;
>>>> +
>>>> + ret = ravb_pm_runtime_get(priv);
>>>> + if (ret < 0)
>>>> + return;
>>>
>>> Hm, sh_eth.c doesn't have any RPM calls in this method either.
>>> Does changing the promiscous mode have sense for an offlined interface?
>>
>> I've added it for scenarios when the interface is down and user tries to
>> configure it. I don't know to answer your question. W/o RPM resume here
>> user space blocks if tries to access it and interface is down. I can just
>> return if interface is down. Let me know if you prefer this way.
>
> Looking at __dev_set_rx_mode(), the method gets only called when
> (dev->flags & IFF_UP) is true -- but that contradicts your experience,
> it seems... However, looking at net/core/dev_addr_lists.c, that function
> is called from the atomic contexts, so please just return early.
>
>>> [...]
>>>> @@ -2187,6 +2244,11 @@ static int ravb_close(struct net_device *ndev)
>>>> if (info->nc_queues)
>>>> ravb_ring_free(ndev, RAVB_NC);
>>>>
>>>> + /* Note that if RPM is enabled on plaforms with ccc_gac=1 this needs to be
>>>
>>> It's "platforms". :-)
>>>
>>>> skipped and
>>>
>>> Overly long line?
>>
>> Not more than 100 chars. Do you want it to 80?
>
> Yes, it's not the code, no need to go beyond 80 cols, I think...
>
> [...]
>
> MBR, Sergey
Hi Claudiu,
On Sat, Nov 25, 2023 at 12:00 AM claudiu beznea
<[email protected]> wrote:
> On 23.11.2023 21:19, Sergey Shtylyov wrote:
> > On 11/23/23 8:04 PM, claudiu beznea wrote:
> >>>> From: Claudiu Beznea <[email protected]>
> >>>
> >>>> RZ/G3S supports enabling/disabling clocks for its modules (including
> >>>> Ethernet module). For this commit adds runtime PM support which
> >>>> relies on PM domain to enable/disable Ethernet clocks.
> >>>
> >>> That's not exactly something new in RZ/G3S. The ravb driver has unconditional
> >>> RPM calls already in the probe() and remove() methods...
> >>> And the sh_eth driver
> >>> has RPM support since 2009...
> >>>
> >>>> At the end of probe ravb_pm_runtime_put() is called which will turn
> >>>
> >>> I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
> >>>> off the Ethernet clocks (if no other request arrives at the driver).
> >>>> After that if the interface is brought up (though ravb_open()) then
> >>>> the clocks remain enabled until interface is brought down (operation
> >>>> done though ravb_close()).
> >>>>
> >>>> If any request arrives to the driver while the interface is down the
> >>>> clocks are enabled to serve the request and then disabled.
> >>>>
> >>>> Signed-off-by: Claudiu Beznea <[email protected]>
> >>>> --- a/drivers/net/ethernet/renesas/ravb.h
> >>>> +++ b/drivers/net/ethernet/renesas/ravb.h
> >>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
> >>>> unsigned magic_pkt:1; /* E-MAC supports magic packet detection */
> >>>> unsigned half_duplex:1; /* E-MAC supports half duplex mode */
> >>>> unsigned refclk_in_pd:1; /* Reference clock is part of a power domain. */
> >>>> + unsigned rpm:1; /* Runtime PM available. */
> >>>
> >>> No, I don't think this flag makes any sense. We should support RPM
> >>> unconditionally...
> >
> > If RPM calls work in the probe()/remove() methods, they should work
> > in the ndo_{open|stop}() methods, right?
>
> It might depend on hardware support... E.g.
>
> I debugged it further the issue I had with this implementation on other
> SoCs and it seems we cannot do RPM for those w/o reworking way the driver
> is configured.
>
> I wiped out the RPM code from this patch and just called:
>
> pm_runtime_put_sync(); // [1]
> usleep_range(300000, 400000); // [2]
> pm_runtime_get_sync(); // [3]
>
> at the end of ravb_probe(); with this the interfaces fails to work. I
> continue debugging it and interrogated CSR and this returns RESET after
> [3]. I tried to switched it back to configuration mode after [3] but fails
> to restore to a proper working state.
>
> Then continued to debug it further to see what happens on the clock driver.
> The clk enable/disable reaches function at [4] which sets control_regs[reg]
> which is one of the System module stop control registers. Setting this
> activates module standby (AFICT). Switch to reset state on Ethernet IP
> might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
> chapter of the G1H HW manual (which I don't fully understand).
You mean 37A.3.1.3 (2) "Transition during power-off by module standby"?
The AVB-DMAC completes the bus master access in progress,
and then shifts to reset mode. At this time, the operating mode
configuration bits in the AVB-DMAC mode register (CCC.OPC) are
set to B'00.
"reset mode" could be interpreted as "register contents are reset (lost)".
However, the R-Car Gen3 documentation contains the same paragraph,
and register contents are known not to be lost...
37.7.2 for Ether ("sh-eth") states:
After returning from the standby state, the ether should be reset
and initialized.
Sergey: does sh_eth.c really reinitialize the hardware completely after
pm_runtime_get_sync()?
> Also, the manual of G1H states from some IPs that register state is
> preserved in standby mode but not for AVB.
Indeed, AFAIK all modules on SH/R-Mobile, R-Car, and RZ/G SoCs keep
their register contents when in standby-mode (module standby enabled).
On modules in a (not always-on) power area (e.g. SH/R-Mobile), register
contents are lost when the power area is powered down.
So I'd be surprised if EtherAVB behaves differently. Of course that
is still possible, there are big difference between EtherAVB in R-Car
Gen2 and RZ/G1, and the revision found in later SoC families.
> >> The reasons I've limited only to RZ/G3S are:
> >> 1/ I don't have all the platforms to test it
> >
> > That's a usual problem with the kernel development...
> >
> >> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
> >> platform at hand, only remotely, and is hardly to debug once the
> >> ethernet fails to work: probe is working(), open is executed, PHY is
> >> initialized and then TX/RX is not working... don't know why ATM.
> >
> > That's why we have the long bug fixing period after -rc1...
>
> I prefer to not introduce any bug by intention.
Iff register contents are lost on RZ/G1H, I'd rather add
an extra clk_prepare_enable(priv->clk) to ravb_probe() on
"renesas,etheravb-rcar-gen2"....
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On 11/27/23 5:05 PM, Geert Uytterhoeven wrote:
[...]
>>>>>> From: Claudiu Beznea <[email protected]>
>>>>>
>>>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>>>> Ethernet module). For this commit adds runtime PM support which
>>>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>>>
>>>>> That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>>>> RPM calls already in the probe() and remove() methods...
>>>>> And the sh_eth driver
>>>>> has RPM support since 2009...
>>>>>
>>>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>>>
>>>>> I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>>>> After that if the interface is brought up (though ravb_open()) then
>>>>>> the clocks remain enabled until interface is brought down (operation
>>>>>> done though ravb_close()).
>>>>>>
>>>>>> If any request arrives to the driver while the interface is down the
>>>>>> clocks are enabled to serve the request and then disabled.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <[email protected]>
[...]
> Sergey: does sh_eth.c really reinitialize the hardware completely after
> pm_runtime_get_sync()?
Well, even with the original Magnus' commit that added the RPM support (bcd5149ded6b2edbf3732fa1483600a716b1cba6) it wasn't so -- sh_eth_open()
indeed seemed to re-init everything (but not TSU!) but sh_eth_get_stats()
surely didn't (the RPM calls there have been removed since); other RPM
"wrappers" have been added to the driver methods since -- which also
don't init anything... thus the comment in sh_eth_runtime_nop(() seems
to be wrong from the very start...
[...]
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergey
Hi, Geert,
On 27.11.2023 16:05, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Sat, Nov 25, 2023 at 12:00 AM claudiu beznea
> <[email protected]> wrote:
>> On 23.11.2023 21:19, Sergey Shtylyov wrote:
>>> On 11/23/23 8:04 PM, claudiu beznea wrote:
>>>>>> From: Claudiu Beznea <[email protected]>
>>>>>
>>>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>>>> Ethernet module). For this commit adds runtime PM support which
>>>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>>>
>>>>> That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>>>> RPM calls already in the probe() and remove() methods...
>>>>> And the sh_eth driver
>>>>> has RPM support since 2009...
>>>>>
>>>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>>>
>>>>> I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>>>> After that if the interface is brought up (though ravb_open()) then
>>>>>> the clocks remain enabled until interface is brought down (operation
>>>>>> done though ravb_close()).
>>>>>>
>>>>>> If any request arrives to the driver while the interface is down the
>>>>>> clocks are enabled to serve the request and then disabled.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <[email protected]>
>
>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>>>>> unsigned magic_pkt:1; /* E-MAC supports magic packet detection */
>>>>>> unsigned half_duplex:1; /* E-MAC supports half duplex mode */
>>>>>> unsigned refclk_in_pd:1; /* Reference clock is part of a power domain. */
>>>>>> + unsigned rpm:1; /* Runtime PM available. */
>>>>>
>>>>> No, I don't think this flag makes any sense. We should support RPM
>>>>> unconditionally...
>>>
>>> If RPM calls work in the probe()/remove() methods, they should work
>>> in the ndo_{open|stop}() methods, right?
>>
>> It might depend on hardware support... E.g.
>>
>> I debugged it further the issue I had with this implementation on other
>> SoCs and it seems we cannot do RPM for those w/o reworking way the driver
>> is configured.
>>
>> I wiped out the RPM code from this patch and just called:
>>
>> pm_runtime_put_sync(); // [1]
>> usleep_range(300000, 400000); // [2]
>> pm_runtime_get_sync(); // [3]
>>
>> at the end of ravb_probe(); with this the interfaces fails to work. I
>> continue debugging it and interrogated CSR and this returns RESET after
>> [3]. I tried to switched it back to configuration mode after [3] but fails
>> to restore to a proper working state.
>>
>> Then continued to debug it further to see what happens on the clock driver.
>> The clk enable/disable reaches function at [4] which sets control_regs[reg]
>> which is one of the System module stop control registers. Setting this
>> activates module standby (AFICT). Switch to reset state on Ethernet IP
>> might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
>> chapter of the G1H HW manual (which I don't fully understand).
>
> You mean 37A.3.1.3 (2) "Transition during power-off by module standby"?
Yes!
>
> The AVB-DMAC completes the bus master access in progress,
> and then shifts to reset mode. At this time, the operating mode
> configuration bits in the AVB-DMAC mode register (CCC.OPC) are
> set to B'00.
>
> "reset mode" could be interpreted as "register contents are reset (lost)".
> However, the R-Car Gen3 documentation contains the same paragraph,
> and register contents are known not to be lost...
I remember (from the debugging session I've run few weeks ago) that I
checked on G1H an Ethernet register before point [1] and after point [3]
and the values were the same (but I may be wrong, I need to double check it).
I will double check also the value of MSTOP for Ethernet on RZ/G3S (though
I checked that this worked on my code), maybe RZ/G3S doesn't go to standby,
I have a bug in my code and that's why it works for RZ/G3S...
Also, I see that the STANDBY state is missing from CCC.OPC documentation
(chapter "37A.3.1 AVB-DMAC Operating Modes" on RZ/G1H vs "31.5.1 DMAC
Operating Modes" on RZ/G3S).
>
> 37.7.2 for Ether ("sh-eth") states:
>
> After returning from the standby state, the ether should be reset
> and initialized.
Ok, I found that one in my G1H manual. It is not available on RZ/G3S
manual, though.
>
> Sergey: does sh_eth.c really reinitialize the hardware completely after
> pm_runtime_get_sync()?
>
>> Also, the manual of G1H states from some IPs that register state is
>> preserved in standby mode but not for AVB.
>
> Indeed, AFAIK all modules on SH/R-Mobile, R-Car, and RZ/G SoCs keep
> their register contents when in standby-mode (module standby enabled).
> On modules in a (not always-on) power area (e.g. SH/R-Mobile), register
> contents are lost when the power area is powered down.
> So I'd be surprised if EtherAVB behaves differently. Of course that
> is still possible, there are big difference between EtherAVB in R-Car
> Gen2 and RZ/G1, and the revision found in later SoC families.
>
>>>> The reasons I've limited only to RZ/G3S are:
>>>> 1/ I don't have all the platforms to test it
>>>
>>> That's a usual problem with the kernel development...
>>>
>>>> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
>>>> platform at hand, only remotely, and is hardly to debug once the
>>>> ethernet fails to work: probe is working(), open is executed, PHY is
>>>> initialized and then TX/RX is not working... don't know why ATM.
>>>
>>> That's why we have the long bug fixing period after -rc1...
>>
>> I prefer to not introduce any bug by intention.
>
> Iff register contents are lost on RZ/G1H, I'd rather add
> an extra clk_prepare_enable(priv->clk) to ravb_probe() on
> "renesas,etheravb-rcar-gen2"....
This should work, though I would go with a pm_runtime_put_noidle().
Thank you,
Claudiu Beznea
>
> Gr{oetje,eeting}s,
>
> Geert
>
Hi, Geert,
On 27.11.2023 16:46, claudiu beznea wrote:
> Hi, Geert,
>
> On 27.11.2023 16:05, Geert Uytterhoeven wrote:
>> Hi Claudiu,
>>
>> On Sat, Nov 25, 2023 at 12:00 AM claudiu beznea
>> <[email protected]> wrote:
>>> On 23.11.2023 21:19, Sergey Shtylyov wrote:
>>>> On 11/23/23 8:04 PM, claudiu beznea wrote:
>>>>>>> From: Claudiu Beznea <[email protected]>
>>>>>>
>>>>>>> RZ/G3S supports enabling/disabling clocks for its modules (including
>>>>>>> Ethernet module). For this commit adds runtime PM support which
>>>>>>> relies on PM domain to enable/disable Ethernet clocks.
>>>>>>
>>>>>> That's not exactly something new in RZ/G3S. The ravb driver has unconditional
>>>>>> RPM calls already in the probe() and remove() methods...
>>>>>> And the sh_eth driver
>>>>>> has RPM support since 2009...
>>>>>>
>>>>>>> At the end of probe ravb_pm_runtime_put() is called which will turn
>>>>>>
>>>>>> I'd suggest a shorter name, like ravb_rpm_put() but (looking at this function)
>>>>>>> off the Ethernet clocks (if no other request arrives at the driver).
>>>>>>> After that if the interface is brought up (though ravb_open()) then
>>>>>>> the clocks remain enabled until interface is brought down (operation
>>>>>>> done though ravb_close()).
>>>>>>>
>>>>>>> If any request arrives to the driver while the interface is down the
>>>>>>> clocks are enabled to serve the request and then disabled.
>>>>>>>
>>>>>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>
>>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>>>>> @@ -1044,6 +1044,7 @@ struct ravb_hw_info {
>>>>>>> unsigned magic_pkt:1; /* E-MAC supports magic packet detection */
>>>>>>> unsigned half_duplex:1; /* E-MAC supports half duplex mode */
>>>>>>> unsigned refclk_in_pd:1; /* Reference clock is part of a power domain. */
>>>>>>> + unsigned rpm:1; /* Runtime PM available. */
>>>>>>
>>>>>> No, I don't think this flag makes any sense. We should support RPM
>>>>>> unconditionally...
>>>>
>>>> If RPM calls work in the probe()/remove() methods, they should work
>>>> in the ndo_{open|stop}() methods, right?
>>>
>>> It might depend on hardware support... E.g.
>>>
>>> I debugged it further the issue I had with this implementation on other
>>> SoCs and it seems we cannot do RPM for those w/o reworking way the driver
>>> is configured.
>>>
>>> I wiped out the RPM code from this patch and just called:
>>>
>>> pm_runtime_put_sync(); // [1]
>>> usleep_range(300000, 400000); // [2]
>>> pm_runtime_get_sync(); // [3]
>>>
>>> at the end of ravb_probe(); with this the interfaces fails to work. I
>>> continue debugging it and interrogated CSR and this returns RESET after
>>> [3]. I tried to switched it back to configuration mode after [3] but fails
>>> to restore to a proper working state.
>>>
>>> Then continued to debug it further to see what happens on the clock driver.
>>> The clk enable/disable reaches function at [4] which sets control_regs[reg]
>>> which is one of the System module stop control registers. Setting this
>>> activates module standby (AFICT). Switch to reset state on Ethernet IP
>>> might be backed by note (2) on "Operating Mode Transitions Due to Hardware"
>>> chapter of the G1H HW manual (which I don't fully understand).
>>
>> You mean 37A.3.1.3 (2) "Transition during power-off by module standby"?
>
> Yes!
>
>>
>> The AVB-DMAC completes the bus master access in progress,
>> and then shifts to reset mode. At this time, the operating mode
>> configuration bits in the AVB-DMAC mode register (CCC.OPC) are
>> set to B'00.
>>
>> "reset mode" could be interpreted as "register contents are reset (lost)".
>> However, the R-Car Gen3 documentation contains the same paragraph,
>> and register contents are known not to be lost...
>
> I remember (from the debugging session I've run few weeks ago) that I
> checked on G1H an Ethernet register before point [1] and after point [3]
> and the values were the same (but I may be wrong, I need to double check it).
I checked again DBAT before point [1] and after point [3]. Before point [1]
DBAT=0x6c040000, after point [3] DBAT=0x00000000.
However, if all the register settings done before point [1] are re-executed
after point [3] the Ethernet connection seems usable. I tried the above
settings after point [3] to confirm this:
ravb_set_config_mode(ndev);
usleep_range(1000, 2000);
pr_err("%s(): 2: mode=%08x\n", __func__, ravb_read(ndev, CSR));
if (info->gptp || info->ccc_gac) {
/* Set GTI value */
error = ravb_set_gti(ndev);
if (error)
goto out_disable_refclk;
/* Request GTI loading */
ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
}
if (info->internal_delay) {
ravb_parse_delay_mode(np, ndev);
ravb_set_delay_mode(ndev);
}
ravb_write(ndev, priv->desc_bat_dma, DBAT);
/* Initialise PTP Clock driver */
ravb_wait(ndev, GCCR, GCCR_TCR, GCCR_TCR_NOREQ);
ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
However, I don't have a PTP setup to check.
>
> I will double check also the value of MSTOP for Ethernet on RZ/G3S (though
> I checked that this worked on my code), maybe RZ/G3S doesn't go to standby,
> I have a bug in my code and that's why it works for RZ/G3S...
All is good in RZ/G3S. MSTOP is set accordingly and no issues.
Thank you,
Claudiu Beznea
>
> Also, I see that the STANDBY state is missing from CCC.OPC documentation
> (chapter "37A.3.1 AVB-DMAC Operating Modes" on RZ/G1H vs "31.5.1 DMAC
> Operating Modes" on RZ/G3S).
>
>>
>> 37.7.2 for Ether ("sh-eth") states:
>>
>> After returning from the standby state, the ether should be reset
>> and initialized.
>
> Ok, I found that one in my G1H manual. It is not available on RZ/G3S
> manual, though.
>
>>
>> Sergey: does sh_eth.c really reinitialize the hardware completely after
>> pm_runtime_get_sync()?
>>
>>> Also, the manual of G1H states from some IPs that register state is
>>> preserved in standby mode but not for AVB.
>>
>> Indeed, AFAIK all modules on SH/R-Mobile, R-Car, and RZ/G SoCs keep
>> their register contents when in standby-mode (module standby enabled).
>> On modules in a (not always-on) power area (e.g. SH/R-Mobile), register
>> contents are lost when the power area is powered down.
>> So I'd be surprised if EtherAVB behaves differently. Of course that
>> is still possible, there are big difference between EtherAVB in R-Car
>> Gen2 and RZ/G1, and the revision found in later SoC families.
>>
>>>>> The reasons I've limited only to RZ/G3S are:
>>>>> 1/ I don't have all the platforms to test it
>>>>
>>>> That's a usual problem with the kernel development...
>>>>
>>>>> 2/ on G1H this doesn't work. I tried to debugged it but I don't have a
>>>>> platform at hand, only remotely, and is hardly to debug once the
>>>>> ethernet fails to work: probe is working(), open is executed, PHY is
>>>>> initialized and then TX/RX is not working... don't know why ATM.
>>>>
>>>> That's why we have the long bug fixing period after -rc1...
>>>
>>> I prefer to not introduce any bug by intention.
>>
>> Iff register contents are lost on RZ/G1H, I'd rather add
>> an extra clk_prepare_enable(priv->clk) to ravb_probe() on
>> "renesas,etheravb-rcar-gen2"....
>
> This should work, though I would go with a pm_runtime_put_noidle().
>
> Thank you,
> Claudiu Beznea
>
>>
>> Gr{oetje,eeting}s,
>>
>> Geert
>>