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

From: Maxime Ripard <maxime@cerno.tech>
To: Samuel Holland <samuel@sholland.org>
Cc: tiny.windzz@gmail.com, wens@csie.org, krzk@kernel.org,
	linux-kernel@vger.kernel.org, vkoul@kernel.org,
	Frank Lee <frank@allwinnertech.com>,
	colin.king@canonical.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/3] phy: sun4i-usb: Use power efficient workqueue for debounce and poll
Date: Thu, 12 Nov 2020 10:53:07 +0100	[thread overview]
Message-ID: <20201112095307.pblcmrn6smyecvqi@gilmour.lan> (raw)
In-Reply-To: <fba49549-9a78-32ee-a55c-97499f24fe62@sholland.org>

[-- Attachment #1.1: Type: text/plain, Size: 2293 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel