| Message ID | 20231214114600.2451162-11-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand |
On 12/14/23 2:45 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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 <claudiu.beznea.uj@bp.renesas.com> [...] > 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 <claudiu.beznea.uj@bp.renesas.com> > > 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 <claudiu.beznea.uj@bp.renesas.com> [...] > 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 <claudiu.beznea.uj@bp.renesas.com> >> >> 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 <claudiu.beznea.uj@bp.renesas.com> > [...] > >> 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 <claudiu.beznea.uj@bp.renesas.com> >>> >>> 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 <claudiu.beznea.uj@bp.renesas.com> >> [...] >> >>> 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 <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> 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 <claudiu.beznea.uj@bp.renesas.com> >>> [...] >>> >>>> 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
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;