| Message ID | 20240223-j7200-usb-suspend-v3-4-b41c9893a130@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: cdns: fix suspend on J7200 by assuming reset-on-resume | expand |
On 2/23/24 7:05 PM, Théo Lebrun wrote: > Add match data support, with one boolean to indicate whether the > hardware resets after a system-wide suspend. If hardware resets, we > force execute ->runtime_resume() at system-wide resume to run the > hardware init sequence. > > No compatible exploits this functionality, just yet. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index 4c8a557e6a6f..f76327566798 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c [...] > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) > return 0; > } > > +static int cdns_ti_suspend(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + > + if (data->match_data && data->match_data->reset_on_resume) > + return pm_runtime_force_suspend(dev); > + else Pointless *else* after *return*... > + return 0; > +} > + > +static int cdns_ti_resume(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + > + if (data->match_data && data->match_data->reset_on_resume) > + return pm_runtime_force_resume(dev); > + else Here as well... > + return 0; > +} > + > static const struct dev_pm_ops cdns_ti_pm_ops = { > RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) > + SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume) > }; > > static const struct of_device_id cdns_ti_of_match[] = { MBR, Sergey
Hello Sergey, On Sat Feb 24, 2024 at 10:08 AM CET, Sergei Shtylyov wrote: > On 2/23/24 7:05 PM, Théo Lebrun wrote: > > Add match data support, with one boolean to indicate whether the > > hardware resets after a system-wide suspend. If hardware resets, we > > force execute ->runtime_resume() at system-wide resume to run the > > hardware init sequence. > > > > No compatible exploits this functionality, just yet. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > > index 4c8a557e6a6f..f76327566798 100644 > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c > [...] > > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) > > return 0; > > } > > > > +static int cdns_ti_suspend(struct device *dev) > > +{ > > + struct cdns_ti *data = dev_get_drvdata(dev); > > + > > + if (data->match_data && data->match_data->reset_on_resume) > > + return pm_runtime_force_suspend(dev); > > + else > > Pointless *else* after *return*... Indeed! I used this form explicitely as it reads nicely: "if reset on reset, force suspend, else do nothing". It also prevents the error of adding behavior below the if-statement without seeing that it won't apply to both cases. If you do believe it would make the code better I'll happily change it for the next revision, I do not mind. Thanks for the review Sergey! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 2/26/24 1:13 PM, Théo Lebrun wrote: [...] >>> Add match data support, with one boolean to indicate whether the >>> hardware resets after a system-wide suspend. If hardware resets, we >>> force execute ->runtime_resume() at system-wide resume to run the >>> hardware init sequence. >>> >>> No compatible exploits this functionality, just yet. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >>> drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ >>> 1 file changed, 27 insertions(+) >>> >>> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c >>> index 4c8a557e6a6f..f76327566798 100644 >>> --- a/drivers/usb/cdns3/cdns3-ti.c >>> +++ b/drivers/usb/cdns3/cdns3-ti.c >> [...] >>> @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) >>> return 0; >>> } >>> >>> +static int cdns_ti_suspend(struct device *dev) >>> +{ >>> + struct cdns_ti *data = dev_get_drvdata(dev); >>> + >>> + if (data->match_data && data->match_data->reset_on_resume) >>> + return pm_runtime_force_suspend(dev); >>> + else >> >> Pointless *else* after *return*... > > Indeed! I used this form explicitely as it reads nicely: "if reset on > reset, force suspend, else do nothing". It also prevents the error of s/reset/resume/ here? :-) > adding behavior below the if-statement without seeing that it won't > apply to both cases. You were going to add stuff after the final *return*? :-) > If you do believe it would make the code better I'll happily change it > for the next revision, I do not mind. Up to you! This is a thing people usually complain about when reviewing patches. I even thought checkpatch.pl would complain as well, but it didn't... :-) > Thanks for the review Sergey! > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com MBR, Swrgey
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c index 4c8a557e6a6f..f76327566798 100644 --- a/drivers/usb/cdns3/cdns3-ti.c +++ b/drivers/usb/cdns3/cdns3-ti.c @@ -57,9 +57,14 @@ struct cdns_ti { unsigned vbus_divider:1; struct clk *usb2_refclk; struct clk *lpm_clk; + const struct cdns_ti_match_data *match_data; int usb2_refclk_rate_code; }; +struct cdns_ti_match_data { + bool reset_on_resume; +}; + static const int cdns_ti_rate_table[] = { /* in KHZ */ 9600, 10000, @@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev) platform_set_drvdata(pdev, data); data->dev = dev; + data->match_data = device_get_match_data(dev); data->usbss = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(data->usbss)) { @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) return 0; } +static int cdns_ti_suspend(struct device *dev) +{ + struct cdns_ti *data = dev_get_drvdata(dev); + + if (data->match_data && data->match_data->reset_on_resume) + return pm_runtime_force_suspend(dev); + else + return 0; +} + +static int cdns_ti_resume(struct device *dev) +{ + struct cdns_ti *data = dev_get_drvdata(dev); + + if (data->match_data && data->match_data->reset_on_resume) + return pm_runtime_force_resume(dev); + else + return 0; +} + static const struct dev_pm_ops cdns_ti_pm_ops = { RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) + SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume) }; static const struct of_device_id cdns_ti_of_match[] = {
Add match data support, with one boolean to indicate whether the hardware resets after a system-wide suspend. If hardware resets, we force execute ->runtime_resume() at system-wide resume to run the hardware init sequence. No compatible exploits this functionality, just yet. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)