diff mbox series

[3/3] phy: sun4i-usb: Use power efficient workqueue for debounce and poll

Message ID 20201109121214.19012-1-frank@allwinnertech.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Frank Lee Nov. 9, 2020, 12:12 p.m. UTC 
From: Yangtao Li <frank@allwinnertech.com>

The debounce and poll time is generally quite long and the work not
performance critical so allow the scheduler to run the work anywhere
rather than in the normal per-CPU workqueue.

Signed-off-by: Yangtao Li <frank@allwinnertech.com>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Maxime Ripard Nov. 10, 2020, 10:02 a.m. UTC | #1 
On Mon, Nov 09, 2020 at 08:12:14PM +0800, Frank Lee wrote:
> From: Yangtao Li <frank@allwinnertech.com>
> 
> The debounce and poll time is generally quite long and the work not
> performance critical so allow the scheduler to run the work anywhere
> rather than in the normal per-CPU workqueue.
> 
> Signed-off-by: Yangtao Li <frank@allwinnertech.com>

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime
Samuel Holland Nov. 11, 2020, 3:44 a.m. UTC | #2 
On 11/9/20 6:12 AM, Frank Lee wrote:
> From: Yangtao Li <frank@allwinnertech.com>
> 
> The debounce and poll time is generally quite long and the work not
> performance critical so allow the scheduler to run the work anywhere
> rather than in the normal per-CPU workqueue.
> 
> Signed-off-by: Yangtao Li <frank@allwinnertech.com>
> ---
>  drivers/phy/allwinner/phy-sun4i-usb.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index 651d5e2a25ce..4787ad13b255 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -326,7 +326,7 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>  		/* Force ISCR and cable state updates */
>  		data->id_det = -1;
>  		data->vbus_det = -1;
> -		queue_delayed_work(system_wq, &data->detect, 0);
> +		queue_delayed_work(system_power_efficient_wq, &data->detect, 0);
>  	}
>  
>  	return 0;
> @@ -444,7 +444,7 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
>  
>  	/* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */

This doesn't sound like "not performance critical" to me. My understanding is
the debouncing has a deadline from the USB spec. Maybe this is more flexible
than the comment makes it sound?

>  	if (phy->index == 0 && sun4i_usb_phy0_poll(data))
> -		mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
> +		mod_delayed_work(system_power_efficient_wq, &data->detect, DEBOUNCE_TIME);
>  
>  	return 0;
>  }
> @@ -465,7 +465,7 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
>  	 * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan.
>  	 */
>  	if (phy->index == 0 && !sun4i_usb_phy0_poll(data))
> -		mod_delayed_work(system_wq, &data->detect, POLL_TIME);
> +		mod_delayed_work(system_power_efficient_wq, &data->detect, POLL_TIME);
>  
>  	return 0;
>  }
> @@ -504,7 +504,7 @@ static int sun4i_usb_phy_set_mode(struct phy *_phy,
>  
>  	data->id_det = -1; /* Force reprocessing of id */
>  	data->force_session_end = true;
> -	queue_delayed_work(system_wq, &data->detect, 0);
> +	queue_delayed_work(system_power_efficient_wq, &data->detect, 0);
>  
>  	return 0;
>  }
> @@ -616,7 +616,7 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
>  		extcon_set_state_sync(data->extcon, EXTCON_USB, vbus_det);
>  
>  	if (sun4i_usb_phy0_poll(data))
> -		queue_delayed_work(system_wq, &data->detect, POLL_TIME);
> +		queue_delayed_work(system_power_efficient_wq, &data->detect, POLL_TIME);
>  }
>  
>  static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
> @@ -624,7 +624,7 @@ static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
>  	struct sun4i_usb_phy_data *data = dev_id;
>  
>  	/* vbus or id changed, let the pins settle and then scan them */
> -	mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
> +	mod_delayed_work(system_power_efficient_wq, &data->detect, DEBOUNCE_TIME);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -638,7 +638,7 @@ static int sun4i_usb_phy0_vbus_notify(struct notifier_block *nb,
>  
>  	/* Properties on the vbus_power_supply changed, scan vbus_det */
>  	if (val == PSY_EVENT_PROP_CHANGED && psy == data->vbus_power_supply)
> -		mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
> +		mod_delayed_work(system_power_efficient_wq, &data->detect, DEBOUNCE_TIME);
>  
>  	return NOTIFY_OK;
>  }
>
Yangtao Li Nov. 11, 2020, 5:54 a.m. UTC | #3 
HI,
On Wed, Nov 11, 2020 at 11:44 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 11/9/20 6:12 AM, Frank Lee wrote:
> > From: Yangtao Li <frank@allwinnertech.com>
> >
> > The debounce and poll time is generally quite long and the work not
> > performance critical so allow the scheduler to run the work anywhere
> > rather than in the normal per-CPU workqueue.
> >
> > Signed-off-by: Yangtao Li <frank@allwinnertech.com>
> > ---
> >  drivers/phy/allwinner/phy-sun4i-usb.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> > index 651d5e2a25ce..4787ad13b255 100644
> > --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> > +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> > @@ -326,7 +326,7 @@ static int sun4i_usb_phy_init(struct phy *_phy)
> >               /* Force ISCR and cable state updates */
> >               data->id_det = -1;
> >               data->vbus_det = -1;
> > -             queue_delayed_work(system_wq, &data->detect, 0);
> > +             queue_delayed_work(system_power_efficient_wq, &data->detect, 0);
> >       }
> >
> >       return 0;
> > @@ -444,7 +444,7 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
> >
> >       /* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
>
> This doesn't sound like "not performance critical" to me. My understanding is
> the debouncing has a deadline from the USB spec. Maybe this is more flexible
> than the comment makes it sound?

According to my understanding, the more meaning of performance here
comes from cache locality.
Maxime Ripard Nov. 12, 2020, 9:53 a.m. UTC | #4 
On Tue, Nov 10, 2020 at 09:44:37PM -0600, Samuel Holland wrote:
> On 11/9/20 6:12 AM, Frank Lee wrote:
> > From: Yangtao Li <frank@allwinnertech.com>
> > 
> > The debounce and poll time is generally quite long and the work not
> > performance critical so allow the scheduler to run the work anywhere
> > rather than in the normal per-CPU workqueue.
> > 
> > Signed-off-by: Yangtao Li <frank@allwinnertech.com>
> > ---
> >  drivers/phy/allwinner/phy-sun4i-usb.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> > index 651d5e2a25ce..4787ad13b255 100644
> > --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> > +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> > @@ -326,7 +326,7 @@ static int sun4i_usb_phy_init(struct phy *_phy)
> >  		/* Force ISCR and cable state updates */
> >  		data->id_det = -1;
> >  		data->vbus_det = -1;
> > -		queue_delayed_work(system_wq, &data->detect, 0);
> > +		queue_delayed_work(system_power_efficient_wq, &data->detect, 0);
> >  	}
> >  
> >  	return 0;
> > @@ -444,7 +444,7 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
> >  
> >  	/* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
> 
> This doesn't sound like "not performance critical" to me. My understanding is
> the debouncing has a deadline from the USB spec. Maybe this is more flexible
> than the comment makes it sound?

It's not really clear to me what the power_efficient workqueue brings to
the table exactly from the comments on WQ_POWER_EFFICIENT (and the
associated gmane link is long dead).

It's only effect seems to be that it sets WQ_UNBOUND when the proper
command line option is set, and WQ_UNBOUND allows for the scheduled work
to run on any CPU instead of the local one.

Given that we don't have any constraint on the CPU here, and the CPU
locality shouldn't really make any difference, I'm not sure we should
expect any meaningful difference.

This is also what the rest of the similar drivers seem to be using:
https://elixir.bootlin.com/linux/v5.10-rc3/source/drivers/usb/common/usb-conn-gpio.c#L119
https://elixir.bootlin.com/linux/v5.10-rc3/source/drivers/usb/core/hub.c#L1254

Maxime
Samuel Holland Nov. 17, 2020, 4:48 a.m. UTC | #5 
On 11/12/20 3:53 AM, Maxime Ripard wrote:
> On Tue, Nov 10, 2020 at 09:44:37PM -0600, Samuel Holland wrote:
>> On 11/9/20 6:12 AM, Frank Lee wrote:
>>> From: Yangtao Li <frank@allwinnertech.com>
>>>
>>> The debounce and poll time is generally quite long and the work not
>>> performance critical so allow the scheduler to run the work anywhere
>>> rather than in the normal per-CPU workqueue.
>>>
>>> Signed-off-by: Yangtao Li <frank@allwinnertech.com>
>>> ---
>>>  drivers/phy/allwinner/phy-sun4i-usb.c | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
>>> index 651d5e2a25ce..4787ad13b255 100644
>>> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
>>> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
>>> @@ -326,7 +326,7 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>>>  		/* Force ISCR and cable state updates */
>>>  		data->id_det = -1;
>>>  		data->vbus_det = -1;
>>> -		queue_delayed_work(system_wq, &data->detect, 0);
>>> +		queue_delayed_work(system_power_efficient_wq, &data->detect, 0);
>>>  	}
>>>  
>>>  	return 0;
>>> @@ -444,7 +444,7 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
>>>  
>>>  	/* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
>>
>> This doesn't sound like "not performance critical" to me. My understanding is
>> the debouncing has a deadline from the USB spec. Maybe this is more flexible
>> than the comment makes it sound?
> 
> It's not really clear to me what the power_efficient workqueue brings to
> the table exactly from the comments on WQ_POWER_EFFICIENT (and the
> associated gmane link is long dead).
> 
> It's only effect seems to be that it sets WQ_UNBOUND when the proper
> command line option is set, and WQ_UNBOUND allows for the scheduled work
> to run on any CPU instead of the local one.
> 
> Given that we don't have any constraint on the CPU here, and the CPU
> locality shouldn't really make any difference, I'm not sure we should
> expect any meaningful difference.
> 
> This is also what the rest of the similar drivers seem to be using:
> https://elixir.bootlin.com/linux/v5.10-rc3/source/drivers/usb/common/usb-conn-gpio.c#L119
> https://elixir.bootlin.com/linux/v5.10-rc3/source/drivers/usb/core/hub.c#L1254

Thanks for the explanation. Then the patch looks fine to me.

Reviewed-by: Samuel Holland <samuel@sholland.org>

Cheers,
Samuel
Vinod Koul Nov. 20, 2020, 10:08 a.m. UTC | #6 
On 09-11-20, 20:12, Frank Lee wrote:
> From: Yangtao Li <frank@allwinnertech.com>
> 
> The debounce and poll time is generally quite long and the work not
> performance critical so allow the scheduler to run the work anywhere
> rather than in the normal per-CPU workqueue.

Applied, thanks
diff mbox series

Patch

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index 651d5e2a25ce..4787ad13b255 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -326,7 +326,7 @@  static int sun4i_usb_phy_init(struct phy *_phy)
 		/* Force ISCR and cable state updates */
 		data->id_det = -1;
 		data->vbus_det = -1;
-		queue_delayed_work(system_wq, &data->detect, 0);
+		queue_delayed_work(system_power_efficient_wq, &data->detect, 0);
 	}
 
 	return 0;
@@ -444,7 +444,7 @@  static int sun4i_usb_phy_power_on(struct phy *_phy)
 
 	/* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
 	if (phy->index == 0 && sun4i_usb_phy0_poll(data))
-		mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+		mod_delayed_work(system_power_efficient_wq, &data->detect, DEBOUNCE_TIME);
 
 	return 0;
 }
@@ -465,7 +465,7 @@  static int sun4i_usb_phy_power_off(struct phy *_phy)
 	 * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan.
 	 */
 	if (phy->index == 0 && !sun4i_usb_phy0_poll(data))
-		mod_delayed_work(system_wq, &data->detect, POLL_TIME);
+		mod_delayed_work(system_power_efficient_wq, &data->detect, POLL_TIME);
 
 	return 0;
 }
@@ -504,7 +504,7 @@  static int sun4i_usb_phy_set_mode(struct phy *_phy,
 
 	data->id_det = -1; /* Force reprocessing of id */
 	data->force_session_end = true;
-	queue_delayed_work(system_wq, &data->detect, 0);
+	queue_delayed_work(system_power_efficient_wq, &data->detect, 0);
 
 	return 0;
 }
@@ -616,7 +616,7 @@  static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 		extcon_set_state_sync(data->extcon, EXTCON_USB, vbus_det);
 
 	if (sun4i_usb_phy0_poll(data))
-		queue_delayed_work(system_wq, &data->detect, POLL_TIME);
+		queue_delayed_work(system_power_efficient_wq, &data->detect, POLL_TIME);
 }
 
 static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
@@ -624,7 +624,7 @@  static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
 	struct sun4i_usb_phy_data *data = dev_id;
 
 	/* vbus or id changed, let the pins settle and then scan them */
-	mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+	mod_delayed_work(system_power_efficient_wq, &data->detect, DEBOUNCE_TIME);
 
 	return IRQ_HANDLED;
 }
@@ -638,7 +638,7 @@  static int sun4i_usb_phy0_vbus_notify(struct notifier_block *nb,
 
 	/* Properties on the vbus_power_supply changed, scan vbus_det */
 	if (val == PSY_EVENT_PROP_CHANGED && psy == data->vbus_power_supply)
-		mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+		mod_delayed_work(system_power_efficient_wq, &data->detect, DEBOUNCE_TIME);
 
 	return NOTIFY_OK;
 }