linux-kernel - Re: [PATCH v4 1/4] phy: rockchip: inno-usb2: add soft vbusvalid control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7815489.EvYhyI6sBW@workhorse>
Date: Mon, 16 Jun 2025 14:48:01 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Kever Yang <kever.yang@...k-chips.com>,
 Frank Wang <frank.wang@...k-chips.com>,
 Neil Armstrong <neil.armstrong@...aro.org>
Cc: Alexey Charkov <alchark@...il.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>, kernel@...labora.com,
 linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject:
 Re: [PATCH v4 1/4] phy: rockchip: inno-usb2: add soft vbusvalid control

On Friday, 13 June 2025 11:02:40 Central European Summer Time neil.armstrong@...aro.org wrote:
> On 10/06/2025 16:07, Nicolas Frattaroli wrote:
> > With USB type C connectors, the vbus detect pin of the OTG controller
> > attached to it is pulled high by a USB Type C controller chip such as
> > the fusb302. This means USB enumeration on Type-C ports never works, as
> > the vbus is always seen as high.
> > 
> > Rockchip added some GRF register flags to deal with this situation. The
> > RK3576 TRM calls these "soft_vbusvalid_bvalid" (con0 bit index 15) and
> > "soft_vbusvalid_bvalid_sel" (con0 bit index 14).
> > 
> > Downstream introduces a new vendor property which tells the USB 2 PHY
> > that it's connected to a type C port, but we can do better. Since in
> > such an arrangement, we'll have an OF graph connection from the USB
> > controller to the USB connector anyway, we can walk said OF graph and
> > check the connector's compatible to determine this without adding any
> > further vendor properties.
> > 
> > Do keep in mind that the usbdp PHY driver seemingly fiddles with these
> > register fields as well, but what it does doesn't appear to be enough
> > for us to get working USB enumeration, presumably because the whole
> > vbus_attach logic needs to be adjusted as well either way.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> > ---
> >   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 113 +++++++++++++++++++++++++-
> >   1 file changed, 109 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index b0f23690ec3002202c0f33a6988f5509622fa10e..4f89bd6568cd3a7a1d2c10e9cddda9f3bd997ed0 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > [...]
> > @@ -666,8 +679,17 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> >   	unsigned long delay;
> >   	bool vbus_attach, sch_work, notify_charger;
> >   
> > -	vbus_attach = property_enabled(rphy->grf,
> > -				       &rport->port_cfg->utmi_bvalid);
> > +	if (rport->port_cfg->svbus_en.enable && rport->typec_vbus_det) {
> > +		if (property_enabled(rphy->grf, &rport->port_cfg->svbus_en) &&
> > +		    property_enabled(rphy->grf, &rport->port_cfg->svbus_sel)) {
> 
> Why do you check the registers since you always enable those bits on those conditions:
> 	rport->port_id == USB2PHY_PORT_OTG
> 	rport->typec_vbus_det
> 	rport->port_cfg->svbus_en.enable
> 	rport->typec_vbus_det
> Can't you us them instead ?

I did some more looking into this, and agree that I can drop the
property_enabled lines here. The other concern I had immediately (that
the bits never get turned off, which seemed fishy) isn't a concern after
all, because after sleeping on it some more, I realised that it's probably
not very likely that a USB-C connector is ever going to morph into anything
else spontaneously.

Thank you for spotting this!

> 
> Neil
> 
> [...]

Kind regards,
Nicolas Frattaroli