| 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
| ||
|
Message-ID: <5c531d53-3573-4c25-a32b-79dfd5abd4cd@linaro.org>
Date: Fri, 13 Jun 2025 11:02:40 +0200
From: neil.armstrong@...aro.org
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>,
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>
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 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 ?
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.
> +
> 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
Powered by blists - more mailing lists