Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 12 Nov 2020 10:53:07 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     Samuel Holland <samuel@sholland.org>
Cc:     Frank Lee <frank@allwinnertech.com>, vkoul@kernel.org,
        wens@csie.org, krzk@kernel.org, colin.king@canonical.com,
        tiny.windzz@gmail.com, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/3] phy: sun4i-usb: Use power efficient workqueue for
 debounce and poll
Message-ID: <20201112095307.pblcmrn6smyecvqi@gilmour.lan>
References: <20201109121214.19012-1-frank@allwinnertech.com>
 <fba49549-9a78-32ee-a55c-97499f24fe62@sholland.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="thkg54eq3saapyx3"
Content-Disposition: inline
In-Reply-To: <fba49549-9a78-32ee-a55c-97499f24fe62@sholland.org>
Precedence: bulk


--thkg54eq3saapyx3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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>
> >=20
> > 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.
> >=20
> > Signed-off-by: Yangtao Li <frank@allwinnertech.com>
> > ---
> >  drivers/phy/allwinner/phy-sun4i-usb.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >=20
> > diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwin=
ner/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 =3D -1;
> >  		data->vbus_det =3D -1;
> > -		queue_delayed_work(system_wq, &data->detect, 0);
> > +		queue_delayed_work(system_power_efficient_wq, &data->detect, 0);
> >  	}
> > =20
> >  	return 0;
> > @@ -444,7 +444,7 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
> > =20
> >  	/* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
>=20
> This doesn't sound like "not performance critical" to me. My understandin=
g is
> the debouncing has a deadline from the USB spec. Maybe this is more flexi=
ble
> 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-co=
nn-gpio.c#L119
https://elixir.bootlin.com/linux/v5.10-rc3/source/drivers/usb/core/hub.c#L1=
254

Maxime

--thkg54eq3saapyx3
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCX60GAwAKCRDj7w1vZxhR
xWGHAQDmGJRbV80bZACkQg9QHK386dBawts71lgws8vTZO8fLQEAn43EQDv/hXjz
vgTtYcSCzPrZ4Zw1e5LbH9KJK71aegE=
=Wgu7
-----END PGP SIGNATURE-----

--thkg54eq3saapyx3--