Message-ID: <3284807.VLH7GnMWUR@workhorse>
Date: Fri, 13 Jun 2025 15:08:42 +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

Hi Neil,

thank you for the review! Responses are inline.

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
> > @@ -17,6 +17,7 @@
> >   #include <linux/module.h>
> >   #include <linux/mutex.h>
> >   #include <linux/of.h>
> > +#include <linux/of_graph.h>
> >   #include <linux/of_irq.h>
> >   #include <linux/phy/phy.h>
> >   #include <linux/platform_device.h>
> > @@ -114,6 +115,8 @@ struct rockchip_chg_det_reg {
> >   /**
> >    * struct rockchip_usb2phy_port_cfg - usb-phy port configuration.
> >    * @phy_sus: phy suspend register.
> > + * @svbus_en: soft vbus bvalid enable register.
> > + * @svbus_sel: soft vbus bvalid selection register.
> >    * @bvalid_det_en: vbus valid rise detection enable register.
> >    * @bvalid_det_st: vbus valid rise detection status register.
> >    * @bvalid_det_clr: vbus valid rise detection clear register.
> > @@ -140,6 +143,8 @@ struct rockchip_chg_det_reg {
> >    */
> >   struct rockchip_usb2phy_port_cfg {
> >   	struct usb2phy_reg	phy_sus;
> > +	struct usb2phy_reg	svbus_en;
> > +	struct usb2phy_reg	svbus_sel;
> >   	struct usb2phy_reg	bvalid_det_en;
> >   	struct usb2phy_reg	bvalid_det_st;
> >   	struct usb2phy_reg	bvalid_det_clr;
> > @@ -203,6 +208,7 @@ struct rockchip_usb2phy_cfg {
> >    * @event_nb: hold event notification callback.
> >    * @state: define OTG enumeration states before device reset.
> >    * @mode: the dr_mode of the controller.
> > + * @typec_vbus_det: whether to apply Type C logic to OTG vbus detection.
> >    */
> >   struct rockchip_usb2phy_port {
> >   	struct phy	*phy;
> > @@ -222,6 +228,7 @@ struct rockchip_usb2phy_port {
> >   	struct notifier_block	event_nb;
> >   	enum usb_otg_state	state;
> >   	enum usb_dr_mode	mode;
> > +	bool typec_vbus_det;
> >   };
> >   
> >   /**
> > @@ -495,6 +502,13 @@ static int rockchip_usb2phy_init(struct phy *phy)
> >   	mutex_lock(&rport->mutex);
> >   
> >   	if (rport->port_id == USB2PHY_PORT_OTG) {
> > +		if (rport->typec_vbus_det) {
> > +			if (rport->port_cfg->svbus_en.enable &&
> > +					rport->port_cfg->svbus_sel.enable) {
> > +				property_enable(rphy->grf, &rport->port_cfg->svbus_en, true);
> > +				property_enable(rphy->grf, &rport->port_cfg->svbus_sel, true);
> > +			}
> > +		}
> >   		if (rport->mode != USB_DR_MODE_HOST &&
> >   		    rport->mode != USB_DR_MODE_UNKNOWN) {
> >   			/* clear bvalid status and enable bvalid detect irq */
> > @@ -535,8 +549,7 @@ static int rockchip_usb2phy_init(struct phy *phy)
> >   			if (ret)
> >   				goto out;
> >   
> > -			schedule_delayed_work(&rport->otg_sm_work,
> > -					      OTG_SCHEDULE_DELAY * 3);
> > +			schedule_delayed_work(&rport->otg_sm_work, 0);
> >   		} else {
> >   			/* If OTG works in host only mode, do nothing. */
> >   			dev_dbg(&rport->phy->dev, "mode %d\n", rport->mode);
> > @@ -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 ?

That is an excellent question. I think I was imitating downstream code
here without thinking too much. This whole construct seems a little fishy,
as these GRF registers are then never cleared again if it's not in OTG
mode anymore. I'll look into it further, including cross-checking what
those bits actually do based on the TRMs of various SoCs that use this
hardware.

> 
> Neil
> 
> > +			vbus_attach = true;
> > +		} else {
> > +			vbus_attach = false;
> > +		}
> > +	} else {
> > +		vbus_attach = property_enabled(rphy->grf,
> > +					       &rport->port_cfg->utmi_bvalid);
> > +	}
> >   
> >   	sch_work = false;
> >   	notify_charger = false;
> > @@ -1276,6 +1298,83 @@ static int rockchip_otg_event(struct notifier_block *nb,
> >   	return NOTIFY_DONE;
> >   }
> >   
> > +static const char *const rockchip_usb2phy_typec_cons[] = {
> > +	"usb-c-connector",
> > +	NULL,
> > +};
> > +
> > +static struct device_node *rockchip_usb2phy_to_controller(struct rockchip_usb2phy *rphy)
> > +{
> > +	struct device_node *np;
> > +	struct device_node *parent;
> > +
> > +	for_each_node_with_property(np, "phys") {
> > +		struct of_phandle_iterator it;
> > +		int ret;
> > +
> > +		of_for_each_phandle(&it, ret, np, "phys", NULL, 0) {
> > +			parent = of_get_parent(it.node);
> > +			if (it.node != rphy->dev->of_node && rphy->dev->of_node != parent) {
> > +				if (parent)
> > +					of_node_put(parent);
> > +				continue;
> > +			}
> > +
> > +			/*
> > +			 * Either the PHY phandle we're iterating or its parent
> > +			 * matched, we don't care about which out of the two in
> > +			 * particular as we just need to know it's the right
> > +			 * USB controller for this PHY.
> > +			 */
> > +			of_node_put(it.node);
> > +			of_node_put(parent);
> > +			return np;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static bool rockchip_usb2phy_otg_is_type_c(struct rockchip_usb2phy *rphy)
> > +{
> > +	struct device_node *controller = rockchip_usb2phy_to_controller(rphy);
> > +	struct device_node *ports;
> > +	struct device_node *ep = NULL;
> > +	struct device_node *parent;
> > +
> > +	if (!controller)
> > +		return false;
> > +
> > +	ports = of_get_child_by_name(controller, "ports");
> > +	if (ports) {
> > +		of_node_put(controller);
> > +		controller = ports;
> > +	}
> > +
> > +	for_each_of_graph_port(controller, port) {
> > +		ep = of_get_child_by_name(port, "endpoint");
> > +		if (!ep)
> > +			continue;
> > +
> > +		parent = of_graph_get_remote_port_parent(ep);
> > +		of_node_put(ep);
> > +		if (!parent)
> > +			continue;
> > +
> > +		if (of_device_compatible_match(parent, rockchip_usb2phy_typec_cons)) {
> > +			of_node_put(parent);
> > +			of_node_put(controller);
> > +			return true;
> > +		}
> > +
> > +		of_node_put(parent);
> > +	}
> > +
> > +	of_node_put(controller);
> > +
> > +	return false;
> > +}
> 
> This DT parsing is quite complex, but seems it's the only way to detect such features
> without bindings change, not sure it's really beneficial.

Yeah, v1 abused the existing connector binding by having the HS graph
port point to the PHY instead, and Rob wasn't happy with that. He
suggested this can actually be done just by walking the graph.

FWIW, even if the PHY knew its controller, that would still only get rid
of rockchip_usb2phy_to_controller, and leave the graph walking to get to
the connector in place.

So a bindings change would only be of measurable benefit if it added a
phandle property in USB PHYs to the connector or something, and I'm not
sure if that's the right solution either. It'd once again shift the
burden of remembering to let PHYs know about their connectors onto board
DTs, which forget such things all the time.

The benefit of this solution is that board DTs don't need to think about
this at all, and the search is unambiguous, as we really don't lack any
information, we just lack a direct view of that information in the node
our driver receives.

> 
> > +
> >   static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> >   					  struct rockchip_usb2phy_port *rport,
> >   					  struct device_node *child_np)
> > @@ -1297,6 +1396,8 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> >   
> >   	mutex_init(&rport->mutex);
> >   
> > +	rport->typec_vbus_det = rockchip_usb2phy_otg_is_type_c(rphy);
> > +
> >   	rport->mode = of_usb_get_dr_mode_by_phy(child_np, -1);
> >   	if (rport->mode == USB_DR_MODE_HOST ||
> >   	    rport->mode == USB_DR_MODE_UNKNOWN) {
> > @@ -2050,6 +2151,8 @@ static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
> >   		.port_cfgs	= {
> >   			[USB2PHY_PORT_OTG] = {
> >   				.phy_sus	= { 0x0000, 8, 0, 0, 0x1d1 },
> > +				.svbus_en	= { 0x0000, 15, 15, 0, 1 },
> > +				.svbus_sel	= { 0x0000, 14, 14, 0, 1 },
> >   				.bvalid_det_en	= { 0x00c0, 1, 1, 0, 1 },
> >   				.bvalid_det_st	= { 0x00c4, 1, 1, 0, 1 },
> >   				.bvalid_det_clr = { 0x00c8, 1, 1, 0, 1 },
> > @@ -2087,6 +2190,8 @@ static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
> >   		.port_cfgs	= {
> >   			[USB2PHY_PORT_OTG] = {
> >   				.phy_sus	= { 0x2000, 8, 0, 0, 0x1d1 },
> > +				.svbus_en	= { 0x2000, 15, 15, 0, 1 },
> > +				.svbus_sel	= { 0x2000, 14, 14, 0, 1 },
> >   				.bvalid_det_en	= { 0x20c0, 1, 1, 0, 1 },
> >   				.bvalid_det_st	= { 0x20c4, 1, 1, 0, 1 },
> >   				.bvalid_det_clr = { 0x20c8, 1, 1, 0, 1 },
> > 
> 
> Thanks,
> Neil
> 

Kind regards,
Nicolas Frattaroli

Powered by blists - more mailing lists