diff mbox series

[v3,4/8] usb: cdns3-ti: support reset-on-resume behavior

Message ID 20240223-j7200-usb-suspend-v3-4-b41c9893a130@bootlin.com
State New
Headers
Series usb: cdns: fix suspend on J7200 by assuming reset-on-resume |

Commit Message

Théo Lebrun Feb. 23, 2024, 4:05 p.m. UTC 
  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(+)

Comments

Sergei Shtylyov Feb. 24, 2024, 9:08 a.m. UTC | #1 
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
Théo Lebrun Feb. 26, 2024, 10:13 a.m. UTC | #2 
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
Sergei Shtylyov Feb. 27, 2024, 4:27 p.m. UTC | #3 
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 mbox series

Patch

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[] = {