| Message ID | 20231214113137.2450292-3-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ravb: fixes for the ravb driver | expand |
Hi Claudiu, Thanks for your work. On 2023-12-14 13:31:37 +0200, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Hardware manual specifies the following for GCCR.LTI bit: > 0: Setting completed > 1: When written: Issue a configuration request. > When read: Completion of settings is pending This is hard to parse at first glance, the last row have odd indentation and the mixing of read and write info is odd. I know this reflects how it's written in the datasheet, but at least there the indentation is correct. Also missing here is the fact that only 1 can be written to the bit. > > Thus, check the completion status when setting 1 to GCCR.LTI. Can you describe in the commit why this fix is needed. I agree it is, but would be nice to record why. As this have a fixes tags have you hit an issue? Or are you correcting the driver based on the datasheet? Maybe a more informative commit message could be to describe the change and why it's needed instead of the register layout? The driver do not wait for the confirmation of the configuring request of the gPTP timer increment before moving on. Add a check to make sure the request completes successfully. > > Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") > Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code") > Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index ce95eb5af354..1c253403a297 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2819,6 +2819,10 @@ static int ravb_probe(struct platform_device *pdev) > > /* Request GTI loading */ > ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); > + /* Check completion status. */ > + error = ravb_wait(ndev, GCCR, GCCR_LTI, 0); > + if (error) > + goto out_disable_refclk; nit: Maybe create a helper for this so future fixes only need to be addressed in one location? > } > > if (info->internal_delay) { > @@ -3041,6 +3045,10 @@ static int __maybe_unused ravb_resume(struct device *dev) > > /* Request GTI loading */ > ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); > + /* Check completion status. */ > + ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0); > + if (ret) > + return ret; > } > > if (info->internal_delay) > -- > 2.39.2 >
On 14.12.2023 14:33, Niklas Söderlund wrote: > Hi Claudiu, > > Thanks for your work. > > On 2023-12-14 13:31:37 +0200, Claudiu wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Hardware manual specifies the following for GCCR.LTI bit: >> 0: Setting completed >> 1: When written: Issue a configuration request. >> When read: Completion of settings is pending > > This is hard to parse at first glance, the last row have odd indentation > and the mixing of read and write info is odd. I know this reflects how > it's written in the datasheet, but at least there the indentation is > correct. Also missing here is the fact that only 1 can be written to the > bit. > >> >> Thus, check the completion status when setting 1 to GCCR.LTI. > > Can you describe in the commit why this fix is needed. I agree it is, > but would be nice to record why. As this have a fixes tags have you hit > an issue? Or are you correcting the driver based on the datasheet? Yes, this is a issue I faced while working on runtime PM support for this driver. > > Maybe a more informative commit message could be to describe the change > and why it's needed instead of the register layout? ok > > The driver do not wait for the confirmation of the configuring request > of the gPTP timer increment before moving on. Add a check to make sure > the request completes successfully. > >> >> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") >> Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code") >> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support") >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> drivers/net/ethernet/renesas/ravb_main.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index ce95eb5af354..1c253403a297 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -2819,6 +2819,10 @@ static int ravb_probe(struct platform_device *pdev) >> >> /* Request GTI loading */ >> ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); >> + /* Check completion status. */ >> + error = ravb_wait(ndev, GCCR, GCCR_LTI, 0); >> + if (error) >> + goto out_disable_refclk; > > nit: Maybe create a helper for this so future fixes only need to be > addressed in one location? The code intended for runtime PM support addresses this. > >> } >> >> if (info->internal_delay) { >> @@ -3041,6 +3045,10 @@ static int __maybe_unused ravb_resume(struct device *dev) >> >> /* Request GTI loading */ >> ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); >> + /* Check completion status. */ >> + ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0); >> + if (ret) >> + return ret; >> } >> >> if (info->internal_delay) >> -- >> 2.39.2 >> >
On 12/14/23 2:31 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Hardware manual specifies the following for GCCR.LTI bit: > 0: Setting completed > 1: When written: Issue a configuration request. > When read: Completion of settings is pending > > Thus, check the completion status when setting 1 to GCCR.LTI. But do we really need to? Seems quite dubious... currently we just let it the loading complete asynchronously... > Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") > Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code") > Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index ce95eb5af354..1c253403a297 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2819,6 +2819,10 @@ static int ravb_probe(struct platform_device *pdev) > > /* Request GTI loading */ > ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); > + /* Check completion status. */ > + error = ravb_wait(ndev, GCCR, GCCR_LTI, 0); > + if (error) > + goto out_disable_refclk; > } > > if (info->internal_delay) { > @@ -3041,6 +3045,10 @@ static int __maybe_unused ravb_resume(struct device *dev) > > /* Request GTI loading */ > ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); > + /* Check completion status. */ > + ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0); > + if (ret) > + return ret; > } > > if (info->internal_delay) > BTW, seems worth factoring out into a separate function... MBR, Sergey
On 14.12.2023 22:22, Sergey Shtylyov wrote: > On 12/14/23 2:31 PM, Claudiu wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Hardware manual specifies the following for GCCR.LTI bit: >> 0: Setting completed >> 1: When written: Issue a configuration request. >> When read: Completion of settings is pending >> >> Thus, check the completion status when setting 1 to GCCR.LTI. > > But do we really need to? Seems quite dubious... currently we > just let it the loading complete asynchronously... Now, thinking again at it... we should be safe w/o it (even though I said we need it in a previous thread, I think I was wrong). > >> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") >> Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code") >> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support") >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> drivers/net/ethernet/renesas/ravb_main.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index ce95eb5af354..1c253403a297 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -2819,6 +2819,10 @@ static int ravb_probe(struct platform_device *pdev) >> >> /* Request GTI loading */ >> ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); >> + /* Check completion status. */ >> + error = ravb_wait(ndev, GCCR, GCCR_LTI, 0); >> + if (error) >> + goto out_disable_refclk; >> } >> >> if (info->internal_delay) { >> @@ -3041,6 +3045,10 @@ static int __maybe_unused ravb_resume(struct device *dev) >> >> /* Request GTI loading */ >> ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); >> + /* Check completion status. */ >> + ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0); >> + if (ret) >> + return ret; >> } >> >> if (info->internal_delay) >> > > BTW, seems worth factoring out into a separate function... > > MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index ce95eb5af354..1c253403a297 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2819,6 +2819,10 @@ static int ravb_probe(struct platform_device *pdev) /* Request GTI loading */ ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); + /* Check completion status. */ + error = ravb_wait(ndev, GCCR, GCCR_LTI, 0); + if (error) + goto out_disable_refclk; } if (info->internal_delay) { @@ -3041,6 +3045,10 @@ static int __maybe_unused ravb_resume(struct device *dev) /* Request GTI loading */ ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); + /* Check completion status. */ + ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0); + if (ret) + return ret; } if (info->internal_delay)