| Message ID | 20231214114600.2451162-12-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On 12/14/23 2:45 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > DBAT setup was 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 > move the DBAT configuration in the driver's ndo_open API. > > This commit prepares the code for the addition of runtime PM. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 04eaa1967651..6b8ca08be35e 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev) > napi_enable(&priv->napi[RAVB_NC]); > > ravb_set_delay_mode(ndev); > + ravb_write(ndev, priv->desc_bat_dma, DBAT); > > /* Device init */ > error = ravb_dmac_init(ndev); > @@ -2841,7 +2842,6 @@ static int ravb_probe(struct platform_device *pdev) > } > for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++) > priv->desc_bat[q].die_dt = DT_EOS; > - ravb_write(ndev, priv->desc_bat_dma, DBAT); > > /* Initialise HW timestamp list */ > INIT_LIST_HEAD(&priv->ts_skb_list); > How about also removing the DBAT write from ravb_resume()? MBR, Sergey
On 12/15/23 12:03 AM, Sergey Shtylyov wrote: [...] >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> DBAT setup was 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 >> move the DBAT configuration in the driver's ndo_open API. >> >> This commit prepares the code for the addition of runtime PM. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > [...] >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index 04eaa1967651..6b8ca08be35e 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev) >> napi_enable(&priv->napi[RAVB_NC]); >> >> ravb_set_delay_mode(ndev); >> + ravb_write(ndev, priv->desc_bat_dma, DBAT); Looking at it again, I suspect this belong in ravb_dmac_init()... >> >> /* Device init */ >> error = ravb_dmac_init(ndev); [...] MBR, Sergey
On 15.12.2023 22:01, Sergey Shtylyov wrote: > On 12/15/23 12:03 AM, Sergey Shtylyov wrote: > [...] > >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> DBAT setup was 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 >>> move the DBAT configuration in the driver's ndo_open API. >>> >>> This commit prepares the code for the addition of runtime PM. >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> >> [...] >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>> index 04eaa1967651..6b8ca08be35e 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev) >>> napi_enable(&priv->napi[RAVB_NC]); >>> >>> ravb_set_delay_mode(ndev); >>> + ravb_write(ndev, priv->desc_bat_dma, DBAT); > > Looking at it again, I suspect this belong in ravb_dmac_init()... ravb_dmac_init() is called from multiple places in this driver, e.g., ravb_set_ringparam(), ravb_tx_timeout_work(). I'm afraid we may broke the behavior of these if DBAT setup is moved in ravb_dmac_init(). This is also valid for setting delay (see patch 10/12). > >>> >>> /* Device init */ >>> error = ravb_dmac_init(ndev); > [...] > > MBR, Sergey
On 12/17/23 3:54 PM, claudiu beznea wrote: [...] >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> DBAT setup was 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 >>>> move the DBAT configuration in the driver's ndo_open API. >>>> >>>> This commit prepares the code for the addition of runtime PM. >>>> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> >>> [...] >>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>>> index 04eaa1967651..6b8ca08be35e 100644 >>>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev) >>>> napi_enable(&priv->napi[RAVB_NC]); >>>> >>>> ravb_set_delay_mode(ndev); >>>> + ravb_write(ndev, priv->desc_bat_dma, DBAT); >> >> Looking at it again, I suspect this belong in ravb_dmac_init()... > > ravb_dmac_init() is called from multiple places in this driver, e.g., It's purpose is to configure AVB-DMAC and DBAT is the AVB-DMAC register, right? > ravb_set_ringparam(), ravb_tx_timeout_work(). I know. Its value is only calculated once, in ravb_probe(), right? > I'm afraid we may broke the behavior of these if DBAT setup is moved Do not be afraid! :-) > in ravb_dmac_init(). This is also > valid for setting delay (see patch 10/12). I don't think there will be a problem either... but maybe we should call it in ravb_emac_init() indeed. [...] MBR, Sergey
On 19.12.2023 20:54, Sergey Shtylyov wrote: > On 12/17/23 3:54 PM, claudiu beznea wrote: > > [...] > >>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>> >>>>> DBAT setup was 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 >>>>> move the DBAT configuration in the driver's ndo_open API. >>>>> >>>>> This commit prepares the code for the addition of runtime PM. >>>>> >>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>>> >>>> [...] >>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>>>> index 04eaa1967651..6b8ca08be35e 100644 >>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>>>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev) >>>>> napi_enable(&priv->napi[RAVB_NC]); >>>>> >>>>> ravb_set_delay_mode(ndev); >>>>> + ravb_write(ndev, priv->desc_bat_dma, DBAT); >>> >>> Looking at it again, I suspect this belong in ravb_dmac_init()... >> >> ravb_dmac_init() is called from multiple places in this driver, e.g., > > It's purpose is to configure AVB-DMAC and DBAT is the AVB-DMAC register, > right? It is. But it is pointless to configure it more than one time after ravb_open() has been called as the register content is not changed until IP enters reset mode (though ravb_close() now). > >> ravb_set_ringparam(), ravb_tx_timeout_work(). > > I know. Its value is only calculated once, in ravb_probe(), right? right > >> I'm afraid we may broke the behavior of these if DBAT setup is moved I was wrong here. DBAT is not changed by IP while TX/RX is working. > > Do not be afraid! :-) > >> in ravb_dmac_init(). This is also >> valid for setting delay (see patch 10/12). > > I don't think there will be a problem either... but maybe we > should call it in ravb_emac_init() indeed. > > [...] > > MBR, Sergey
On 12/20/23 2:41 PM, claudiu beznea wrote: [...] >>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>>> >>>>>> DBAT setup was 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 >>>>>> move the DBAT configuration in the driver's ndo_open API. >>>>>> >>>>>> This commit prepares the code for the addition of runtime PM. >>>>>> >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>>> >>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>>>> >>>>> [...] >>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>>>>> index 04eaa1967651..6b8ca08be35e 100644 >>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>>>>> @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev) >>>>>> napi_enable(&priv->napi[RAVB_NC]); >>>>>> >>>>>> ravb_set_delay_mode(ndev); >>>>>> + ravb_write(ndev, priv->desc_bat_dma, DBAT); >>>> >>>> Looking at it again, I suspect this belong in ravb_dmac_init()... >>> >>> ravb_dmac_init() is called from multiple places in this driver, e.g., >> >> It's purpose is to configure AVB-DMAC and DBAT is the AVB-DMAC register, >> right? > > It is. But it is pointless to configure it more than one time after > ravb_open() has been called as the register content is not changed until IP > enters reset mode (though ravb_close() now). The same is true for the most registers set by ravb_dmac_init()! [...] MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 04eaa1967651..6b8ca08be35e 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1822,6 +1822,7 @@ static int ravb_open(struct net_device *ndev) napi_enable(&priv->napi[RAVB_NC]); ravb_set_delay_mode(ndev); + ravb_write(ndev, priv->desc_bat_dma, DBAT); /* Device init */ error = ravb_dmac_init(ndev); @@ -2841,7 +2842,6 @@ static int ravb_probe(struct platform_device *pdev) } for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++) priv->desc_bat[q].die_dt = DT_EOS; - ravb_write(ndev, priv->desc_bat_dma, DBAT); /* Initialise HW timestamp list */ INIT_LIST_HEAD(&priv->ts_skb_list);