From: Claudiu Beznea <[email protected]>
Delay parse and set were done in the driver's probe API. As some IP
variants switch to reset mode (and thus registers' content is lost) when
setting clocks (due to module standby functionality) to be able to
implement runtime PM keep the delay parsing in the driver's probe function
and move the delay apply function to the driver's ndo_open API.
Signed-off-by: Claudiu Beznea <[email protected]>
---
Changes in v2:
- none; this patch is new
drivers/net/ethernet/renesas/ravb_main.c | 37 ++++++++++++++----------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5e01e03e1b43..04eaa1967651 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1795,6 +1795,21 @@ static int ravb_compute_gti(struct net_device *ndev)
return 0;
}
+static void ravb_set_delay_mode(struct net_device *ndev)
+{
+ struct ravb_private *priv = netdev_priv(ndev);
+ u32 set = 0;
+
+ if (!priv->info->internal_delay)
+ return;
+
+ if (priv->rxcidm)
+ set |= APSR_RDM;
+ if (priv->txcidm)
+ set |= APSR_TDM;
+ ravb_modify(ndev, APSR, APSR_RDM | APSR_TDM, set);
+}
+
/* Network device open function for Ethernet AVB */
static int ravb_open(struct net_device *ndev)
{
@@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
if (info->nc_queues)
napi_enable(&priv->napi[RAVB_NC]);
+ ravb_set_delay_mode(ndev);
+
/* Device init */
error = ravb_dmac_init(ndev);
if (error)
@@ -2530,6 +2547,9 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
bool explicit_delay = false;
u32 delay;
+ if (!priv->info->internal_delay)
+ return;
+
if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay)) {
/* Valid values are 0 and 1800, according to DT bindings */
priv->rxcidm = !!delay;
@@ -2679,18 +2699,6 @@ static int ravb_request_irqs(struct ravb_private *priv)
ndev, dev, "mgmt_a");
}
-static void ravb_set_delay_mode(struct net_device *ndev)
-{
- struct ravb_private *priv = netdev_priv(ndev);
- u32 set = 0;
-
- if (priv->rxcidm)
- set |= APSR_RDM;
- if (priv->txcidm)
- set |= APSR_TDM;
- ravb_modify(ndev, APSR, APSR_RDM | APSR_TDM, set);
-}
-
static int ravb_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
@@ -2818,10 +2826,7 @@ static int ravb_probe(struct platform_device *pdev)
if (error)
goto out_rpm_put;
- if (info->internal_delay) {
- ravb_parse_delay_mode(np, ndev);
- ravb_set_delay_mode(ndev);
- }
+ ravb_parse_delay_mode(np, ndev);
/* Allocate descriptor base address table */
priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
--
2.39.2
On 12/14/23 2:45 PM, Claudiu wrote:
> From: Claudiu Beznea <[email protected]>
>
> Delay parse and set were done in the driver's probe API. As some IP
Parsing and setting?
> variants switch to reset mode (and thus registers' content is lost) when
> setting clocks (due to module standby functionality) to be able to
> implement runtime PM keep the delay parsing in the driver's probe function
> and move the delay apply function to the driver's ndo_open API.
Applying?
> 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 5e01e03e1b43..04eaa1967651 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
> if (info->nc_queues)
> napi_enable(&priv->napi[RAVB_NC]);
>
> + ravb_set_delay_mode(ndev);
> +
I suspect this belongs in ravb_dmac_init() now...
> /* Device init */
> error = ravb_dmac_init(ndev);
> if (error)
[...]
MRB, Sergey
On 12/14/23 2:45 PM, Claudiu wrote:
> From: Claudiu Beznea <[email protected]>
>
> Delay parse and set were done in the driver's probe API. As some IP
Parsing and setting?
> variants switch to reset mode (and thus registers' content is lost) when
Register.
> setting clocks (due to module standby functionality) to be able to
> implement runtime PM keep the delay parsing in the driver's probe function
> and move the delay apply function to the driver's ndo_open API.
Applying?
> 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 5e01e03e1b43..04eaa1967651 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
> if (info->nc_queues)
> napi_enable(&priv->napi[RAVB_NC]);
>
> + ravb_set_delay_mode(ndev);
> +
I suspect this belongs in ravb_dmac_init() now...
> /* Device init */
> error = ravb_dmac_init(ndev);
> if (error)
[...]
MRB, Sergey
On 15.12.2023 21:58, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
>
>> From: Claudiu Beznea <[email protected]>
>>
>> Delay parse and set were done in the driver's probe API. As some IP
>
> Parsing and setting?
>
>> variants switch to reset mode (and thus registers' content is lost) when
>
> Register.
>
>> setting clocks (due to module standby functionality) to be able to
>> implement runtime PM keep the delay parsing in the driver's probe function
>> and move the delay apply function to the driver's ndo_open API.
>
> Applying?
>
>> 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 5e01e03e1b43..04eaa1967651 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
>> if (info->nc_queues)
>> napi_enable(&priv->napi[RAVB_NC]);
>>
>> + ravb_set_delay_mode(ndev);
>> +
>
> I suspect this belongs in ravb_dmac_init() now...
I'm confused... Why? To me this seems more like MAC-PHY interface related.
Though I'm not sure what ravb_dmac_init() purpose is.
>
>> /* Device init */
>> error = ravb_dmac_init(ndev);
>> if (error)
> [...]
>
> MRB, Sergey
On 12/17/23 3:49 PM, claudiu beznea wrote:
[...]
>>> From: Claudiu Beznea <[email protected]>
>>>
>>> Delay parse and set were done in the driver's probe API. As some IP
>>
>> Parsing and setting?
>>
>>> variants switch to reset mode (and thus registers' content is lost) when
>>
>> Register.
>>
>>> setting clocks (due to module standby functionality) to be able to
>>> implement runtime PM keep the delay parsing in the driver's probe function
>>> and move the delay apply function to the driver's ndo_open API.
>>
>> Applying?
>>
>>> 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 5e01e03e1b43..04eaa1967651 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> [...]
>>> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
>>> if (info->nc_queues)
>>> napi_enable(&priv->napi[RAVB_NC]);
>>>
>>> + ravb_set_delay_mode(ndev);
>>> +
>>
>> I suspect this belongs in ravb_dmac_init() now...
>
> I'm confused... Why? To me this seems more like MAC-PHY interface related.
APSR's full name is AVB-DMAC product specific register. :-)
> Though I'm not sure what ravb_dmac_init() purpose is.
To configure and start the AVB-DMAC, apparently. :-)
[...]
MRB, Sergey
On 19.12.2023 20:40, Sergey Shtylyov wrote:
> On 12/17/23 3:49 PM, claudiu beznea wrote:
>
> [...]
>>>> From: Claudiu Beznea <[email protected]>
>>>>
>>>> Delay parse and set were done in the driver's probe API. As some IP
>>>
>>> Parsing and setting?
>>>
>>>> variants switch to reset mode (and thus registers' content is lost) when
>>>
>>> Register.
>>>
>>>> setting clocks (due to module standby functionality) to be able to
>>>> implement runtime PM keep the delay parsing in the driver's probe function
>>>> and move the delay apply function to the driver's ndo_open API.
>>>
>>> Applying?
>>>
>>>> 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 5e01e03e1b43..04eaa1967651 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> [...]
>>>> @@ -1806,6 +1821,8 @@ static int ravb_open(struct net_device *ndev)
>>>> if (info->nc_queues)
>>>> napi_enable(&priv->napi[RAVB_NC]);
>>>>
>>>> + ravb_set_delay_mode(ndev);
>>>> +
>>>
>>> I suspect this belongs in ravb_dmac_init() now...
>>
>> I'm confused... Why? To me this seems more like MAC-PHY interface related.
>
> APSR's full name is AVB-DMAC product specific register. :-)
As ravb_dmac_init() is called in multiple places I don't think it worth
configuring delays more than once in ravb_open().
Moreover TX/RX delay is something specific to the MAC-PHY interface (and
could be influenced also by the wiring length b/w MAC and PHY).
Just because it is in the DMAC address space I don't think it worth having
it in ravb_dmac_init() (for the above mentioned reasons).
>
>> Though I'm not sure what ravb_dmac_init() purpose is.
>
> To configure and start the AVB-DMAC, apparently. :-)
>
> [...]
>
> MRB, Sergey