| Message ID | 20230113041132.4189268-1-quic_bjorande@quicinc.com |
|---|---|
| Headers | show |
| Series | soc: qcom: Introduce PMIC GLINK | expand |
On 13.01.2023 05:11, Bjorn Andersson wrote: > This implements the base PMIC GLINK driver, a power_supply driver and a > driver for the USB Type-C altmode protocol. This has been tested and > shown to provide battery information, USB Type-C switch and mux requests > and DisplayPort notifications on SC8180X, SC8280XP and SM8350. > For the series: Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> # SM8350 PDX215 Thanks a lot for working on this! One thing, /sys/class/power_supply/qcom-battmgr-usb/input_current_limit is stuck at zero and so is the current_now as a result (the voltage readout is 5V + some noise, so that looks good), but I don't see any SET paths for it in the driver, so I suppose that's what the firmware default is? Konrad > Bjorn Andersson (4): > dt-bindings: soc: qcom: Introduce PMIC GLINK binding > soc: qcom: pmic_glink: Introduce base PMIC GLINK driver > soc: qcom: pmic_glink: Introduce altmode support > power: supply: Introduce Qualcomm PMIC GLINK power supply > > .../bindings/soc/qcom/qcom,pmic-glink.yaml | 102 ++ > drivers/power/supply/Kconfig | 9 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/qcom_battmgr.c | 1421 +++++++++++++++++ > drivers/soc/qcom/Kconfig | 15 + > drivers/soc/qcom/Makefile | 2 + > drivers/soc/qcom/pmic_glink.c | 336 ++++ > drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++ > include/linux/soc/qcom/pmic_glink.h | 32 + > 9 files changed, 2395 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml > create mode 100644 drivers/power/supply/qcom_battmgr.c > create mode 100644 drivers/soc/qcom/pmic_glink.c > create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c > create mode 100644 include/linux/soc/qcom/pmic_glink.h >
On 13/01/2023 04:11, Bjorn Andersson wrote: > This implements the base PMIC GLINK driver, a power_supply driver and a > driver for the USB Type-C altmode protocol. This has been tested and > shown to provide battery information, USB Type-C switch and mux requests > and DisplayPort notifications on SC8180X, SC8280XP and SM8350. > > Bjorn Andersson (4): > dt-bindings: soc: qcom: Introduce PMIC GLINK binding > soc: qcom: pmic_glink: Introduce base PMIC GLINK driver > soc: qcom: pmic_glink: Introduce altmode support > power: supply: Introduce Qualcomm PMIC GLINK power supply > > .../bindings/soc/qcom/qcom,pmic-glink.yaml | 102 ++ > drivers/power/supply/Kconfig | 9 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/qcom_battmgr.c | 1421 +++++++++++++++++ > drivers/soc/qcom/Kconfig | 15 + > drivers/soc/qcom/Makefile | 2 + > drivers/soc/qcom/pmic_glink.c | 336 ++++ > drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++ > include/linux/soc/qcom/pmic_glink.h | 32 + > 9 files changed, 2395 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml > create mode 100644 drivers/power/supply/qcom_battmgr.c > create mode 100644 drivers/soc/qcom/pmic_glink.c > create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c > create mode 100644 include/linux/soc/qcom/pmic_glink.h > How does the USB PHY and a USB redriver fit into this ? Is the host supposed to manage both/neither ? Is the DSP responsible for configuring the PHY lanes and the turnaround on orientation switch ? --- bod
On Thu, Jan 12, 2023 at 10:13 PM Bjorn Andersson <quic_bjorande@quicinc.com> wrote: > > From: Bjorn Andersson <bjorn.andersson@linaro.org> > > With the PMIC GLINK service, the host OS subscribes to USB-C altmode > messages, which are sent by the firmware to notify the host OS about > state updates and HPD interrupts. > > The pmic_glink_altmode driver registers for these notifications and > propagates the notifications as typec_mux, typec_switch and DRM OOB > notifications as necessary to implement DisplayPort altmode support. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > > Changes since v1: > - None > > Johan reported a NULL pointer dereference in > drm_kms_helper_hotplug_event() for HPD event being reported while the > MSM DRM driver is still being initalized, a separate fix has been sent > in hope to remidy this race condition in the MSM driver. > > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++++++++++++++++++++++ > 2 files changed, 478 insertions(+) > create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c > > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index 29cccac472f3..f30552bf4da7 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o > obj-$(CONFIG_QCOM_OCMEM) += ocmem.o > obj-$(CONFIG_QCOM_PDR_HELPERS) += pdr_interface.o > obj-$(CONFIG_QCOM_PMIC_GLINK) += pmic_glink.o > +obj-$(CONFIG_QCOM_PMIC_GLINK) += pmic_glink_altmode.o > obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o > qmi_helpers-y += qmi_encdec.o qmi_interface.o > obj-$(CONFIG_QCOM_RAMP_CTRL) += ramp_controller.o > diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c > new file mode 100644 > index 000000000000..8d2d563cb756 > --- /dev/null > +++ b/drivers/soc/qcom/pmic_glink_altmode.c > @@ -0,0 +1,477 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved. > + * Copyright (c) 2022, Linaro Ltd > + */ > +#include <linux/auxiliary_bus.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/mutex.h> > +#include <linux/property.h> > +#include <linux/soc/qcom/pdr.h> > +#include <drm/drm_bridge.h> > + > +#include <linux/usb/typec_altmode.h> > +#include <linux/usb/typec_dp.h> > +#include <linux/usb/typec_mux.h> > + > +#include <linux/soc/qcom/pmic_glink.h> > + > +#define PMIC_GLINK_MAX_PORTS 2 > + > +#define USBC_SC8180X_NOTIFY_IND 0x13 > +#define USBC_CMD_WRITE_REQ 0x15 > +#define USBC_NOTIFY_IND 0x16 > + > +#define ALTMODE_PAN_EN 0x10 > +#define ALTMODE_PAN_ACK 0x11 > + > +struct usbc_write_req { > + struct pmic_glink_hdr hdr; > + __le32 cmd; > + __le32 arg; > + __le32 reserved; > +}; > + > +#define NOTIFY_PAYLOAD_SIZE 16 > +struct usbc_notify { > + struct pmic_glink_hdr hdr; > + char payload[NOTIFY_PAYLOAD_SIZE]; > + u32 reserved; > +}; > + > +struct usbc_sc8180x_notify { > + struct pmic_glink_hdr hdr; > + __le32 notification; > + __le32 reserved[2]; > +}; > + > +enum pmic_glink_altmode_pin_assignment { > + DPAM_HPD_OUT, > + DPAM_HPD_A, > + DPAM_HPD_B, > + DPAM_HPD_C, > + DPAM_HPD_D, > + DPAM_HPD_E, > + DPAM_HPD_F, > +}; > + > +struct pmic_glink_altmode; > + > +#define work_to_altmode_port(w) container_of((w), struct pmic_glink_altmode_port, work) > + > +struct pmic_glink_altmode_port { > + struct pmic_glink_altmode *altmode; > + unsigned int index; > + > + struct typec_switch *typec_switch; > + struct typec_mux *typec_mux; > + struct typec_mux_state state; > + struct typec_altmode dp_alt; > + > + struct work_struct work; > + > + struct drm_bridge bridge; > + > + enum typec_orientation orientation; > + u16 svid; > + u8 dp_data; > + u8 mode; > + u8 hpd_state; > + u8 hpd_irq; > +}; > + > +#define work_to_altmode(w) container_of((w), struct pmic_glink_altmode, enable_work) > + > +struct pmic_glink_altmode { > + struct device *dev; > + > + unsigned int owner_id; > + > + /* To synchronize WRITE_REQ acks */ > + struct mutex lock; > + > + struct completion pan_ack; > + struct pmic_glink_client *client; > + > + struct work_struct enable_work; > + > + struct pmic_glink_altmode_port ports[PMIC_GLINK_MAX_PORTS]; > +}; > + > +static int pmic_glink_altmode_request(struct pmic_glink_altmode *altmode, u32 cmd, u32 arg) > +{ > + struct usbc_write_req req = {}; > + unsigned long left; > + int ret; > + > + /* > + * The USBC_CMD_WRITE_REQ ack doesn't identify the request, so wait for > + * one ack at a time. > + */ > + mutex_lock(&altmode->lock); > + > + req.hdr.owner = cpu_to_le32(altmode->owner_id); > + req.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP); > + req.hdr.opcode = cpu_to_le32(USBC_CMD_WRITE_REQ); > + req.cmd = cpu_to_le32(cmd); > + req.arg = cpu_to_le32(arg); > + > + ret = pmic_glink_send(altmode->client, &req, sizeof(req)); > + if (ret) { > + dev_err(altmode->dev, "failed to send altmode request: %#x (%d)\n", cmd, ret); > + goto out_unlock; > + } > + > + left = wait_for_completion_timeout(&altmode->pan_ack, 5 * HZ); > + if (!left) { > + dev_err(altmode->dev, "timeout waiting for altmode request ack for: %#x\n", cmd); > + ret = -ETIMEDOUT; > + } > + > +out_unlock: > + mutex_unlock(&altmode->lock); > + return ret; > +} > + > +static void pmic_glink_altmode_enable_dp(struct pmic_glink_altmode *altmode, > + struct pmic_glink_altmode_port *port, > + u8 mode, bool hpd_state, > + bool hpd_irq) > +{ > + struct typec_displayport_data dp_data = {}; > + int ret; > + > + dp_data.status = DP_STATUS_ENABLED; > + if (hpd_state) > + dp_data.status |= DP_STATUS_HPD_STATE; > + if (hpd_irq) > + dp_data.status |= DP_STATUS_IRQ_HPD; > + dp_data.conf = DP_CONF_SET_PIN_ASSIGN(mode); > + > + port->state.alt = &port->dp_alt; > + port->state.data = &dp_data; > + port->state.mode = TYPEC_MODAL_STATE(mode); > + > + ret = typec_mux_set(port->typec_mux, &port->state); > + if (ret) > + dev_err(altmode->dev, "failed to switch mux to DP\n"); > +} > + > +static void pmic_glink_altmode_enable_usb(struct pmic_glink_altmode *altmode, > + struct pmic_glink_altmode_port *port) > +{ > + int ret; > + > + port->state.alt = NULL; > + port->state.data = NULL; > + port->state.mode = TYPEC_STATE_USB; > + > + ret = typec_mux_set(port->typec_mux, &port->state); > + if (ret) > + dev_err(altmode->dev, "failed to switch mux to USB\n"); > +} > + > +static void pmic_glink_altmode_worker(struct work_struct *work) > +{ > + struct pmic_glink_altmode_port *alt_port = work_to_altmode_port(work); > + struct pmic_glink_altmode *altmode = alt_port->altmode; > + > + typec_switch_set(alt_port->typec_switch, alt_port->orientation); > + > + if (alt_port->svid == USB_TYPEC_DP_SID) > + pmic_glink_altmode_enable_dp(altmode, alt_port, alt_port->mode, > + alt_port->hpd_state, alt_port->hpd_irq); > + else > + pmic_glink_altmode_enable_usb(altmode, alt_port); > + > + if (alt_port->hpd_state) > + drm_bridge_hpd_notify(&alt_port->bridge, connector_status_connected); > + else > + drm_bridge_hpd_notify(&alt_port->bridge, connector_status_disconnected); > + > + pmic_glink_altmode_request(altmode, ALTMODE_PAN_ACK, alt_port->index); > +}; > + > +static enum typec_orientation pmic_glink_altmode_orientation(unsigned int orientation) > +{ > + if (orientation == 0) > + return TYPEC_ORIENTATION_NORMAL; > + else if (orientation == 1) > + return TYPEC_ORIENTATION_REVERSE; > + else > + return TYPEC_ORIENTATION_NONE; > +} > + > +#define SC8180X_PORT_MASK 0x000000ff > +#define SC8180X_ORIENTATION_MASK 0x0000ff00 > +#define SC8180X_MUX_MASK 0x00ff0000 > +#define SC8180X_MODE_MASK 0x3f000000 > +#define SC8180X_HPD_STATE_MASK 0x40000000 > +#define SC8180X_HPD_IRQ_MASK 0x80000000 > + > +static void pmic_glink_altmode_sc8180xp_notify(struct pmic_glink_altmode *altmode, > + const void *data, size_t len) > +{ > + struct pmic_glink_altmode_port *alt_port; > + const struct usbc_sc8180x_notify *msg; > + u32 notification; > + u8 orientation; > + u8 hpd_state; > + u8 hpd_irq; > + u16 svid; > + u8 port; > + u8 mode; > + u8 mux; > + > + if (len != sizeof(*msg)) { > + dev_warn(altmode->dev, "invalid length of USBC_NOTIFY indication: %zd\n", len); > + return; > + } > + > + msg = data; > + notification = le32_to_cpu(msg->notification); > + port = FIELD_GET(SC8180X_PORT_MASK, notification); > + orientation = FIELD_GET(SC8180X_ORIENTATION_MASK, notification); > + mux = FIELD_GET(SC8180X_MUX_MASK, notification); > + mode = FIELD_GET(SC8180X_MODE_MASK, notification); > + hpd_state = FIELD_GET(SC8180X_HPD_STATE_MASK, notification); > + hpd_irq = FIELD_GET(SC8180X_HPD_IRQ_MASK, notification); > + The kernel test robot keeps complaining about these FIELD_GET because there is no #include <linux/bitfield.h> > + svid = mux == 2 ? USB_TYPEC_DP_SID : 0; > + > + if (!altmode->ports[port].altmode) { > + dev_dbg(altmode->dev, "notification on undefined port %d\n", port); > + return; > + } > + > + alt_port = &altmode->ports[port]; > + alt_port->orientation = pmic_glink_altmode_orientation(orientation); > + alt_port->svid = mux == 2 ? USB_TYPEC_DP_SID : 0; > + alt_port->mode = mode; > + alt_port->hpd_state = hpd_state; > + alt_port->hpd_irq = hpd_irq; > + schedule_work(&alt_port->work); > +} > + > +#define SC8280XP_DPAM_MASK 0x3f > +#define SC8280XP_HPD_STATE_MASK BIT(6) > +#define SC8280XP_HPD_IRQ_MASK BIT(7) > + > +static void pmic_glink_altmode_sc8280xp_notify(struct pmic_glink_altmode *altmode, > + u16 svid, const void *data, size_t len) > +{ > + struct pmic_glink_altmode_port *alt_port; > + const struct usbc_notify *notify; > + u8 orientation; > + u8 hpd_state; > + u8 hpd_irq; > + u8 mode; > + u8 port; > + > + if (len != sizeof(*notify)) { > + dev_warn(altmode->dev, "invalid length USBC_NOTIFY_IND: %zd\n", > + len); > + return; > + } > + > + notify = data; > + > + port = notify->payload[0]; > + orientation = notify->payload[1]; > + mode = FIELD_GET(SC8280XP_DPAM_MASK, notify->payload[8]) - DPAM_HPD_A; > + hpd_state = FIELD_GET(SC8280XP_HPD_STATE_MASK, notify->payload[8]); > + hpd_irq = FIELD_GET(SC8280XP_HPD_IRQ_MASK, notify->payload[8]); > + > + if (!altmode->ports[port].altmode) { > + dev_dbg(altmode->dev, "notification on undefined port %d\n", port); > + return; > + } > + > + alt_port = &altmode->ports[port]; > + alt_port->orientation = pmic_glink_altmode_orientation(orientation); > + alt_port->svid = svid; > + alt_port->mode = mode; > + alt_port->hpd_state = hpd_state; > + alt_port->hpd_irq = hpd_irq; > + schedule_work(&alt_port->work); > +} > + > +static void pmic_glink_altmode_callback(const void *data, size_t len, void *priv) > +{ > + struct pmic_glink_altmode *altmode = priv; > + const struct pmic_glink_hdr *hdr = data; > + u16 opcode; > + u16 svid; > + > + opcode = le32_to_cpu(hdr->opcode) & 0xff; > + svid = le32_to_cpu(hdr->opcode) >> 16; > + > + switch (opcode) { > + case USBC_CMD_WRITE_REQ: > + complete(&altmode->pan_ack); > + break; > + case USBC_NOTIFY_IND: > + pmic_glink_altmode_sc8280xp_notify(altmode, svid, data, len); > + break; > + case USBC_SC8180X_NOTIFY_IND: > + pmic_glink_altmode_sc8180xp_notify(altmode, data, len); > + break; > + } > +} > + > +static int pmic_glink_altmode_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; > +} > + > +static const struct drm_bridge_funcs pmic_glink_altmode_bridge_funcs = { > + .attach = pmic_glink_altmode_attach, > +}; > + > +static void pmic_glink_altmode_put_mux(void *data) > +{ > + typec_mux_put(data); > +} > + > +static void pmic_glink_altmode_put_switch(void *data) > +{ > + typec_switch_put(data); > +} > + > +static void pmic_glink_altmode_enable_worker(struct work_struct *work) > +{ > + struct pmic_glink_altmode *altmode = work_to_altmode(work); > + int ret; > + > + ret = pmic_glink_altmode_request(altmode, ALTMODE_PAN_EN, 0); > + if (ret) > + dev_err(altmode->dev, "failed to request altmode notifications\n"); > +} > + > +static void pmic_glink_altmode_pdr_notify(void *priv, int state) > +{ > + struct pmic_glink_altmode *altmode = priv; > + > + if (state == SERVREG_SERVICE_STATE_UP) > + schedule_work(&altmode->enable_work); > +} > + > +static const struct of_device_id pmic_glink_altmode_of_quirks[] = { > + { .compatible = "qcom,sc8180x-pmic-glink", .data = (void *)PMIC_GLINK_OWNER_USBC }, > + {} > +}; > + > +static int pmic_glink_altmode_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct pmic_glink_altmode_port *alt_port; > + struct pmic_glink_altmode *altmode; > + struct typec_altmode_desc mux_desc = {}; > + const struct of_device_id *match; > + struct fwnode_handle *fwnode; > + struct device *dev = &adev->dev; > + u32 port; > + int ret; > + > + altmode = devm_kzalloc(dev, sizeof(*altmode), GFP_KERNEL); > + if (!altmode) > + return -ENOMEM; > + > + altmode->dev = dev; > + > + match = of_match_device(pmic_glink_altmode_of_quirks, dev->parent); > + if (match) > + altmode->owner_id = (unsigned long)match->data; > + else > + altmode->owner_id = PMIC_GLINK_OWNER_USBC_PAN; > + > + INIT_WORK(&altmode->enable_work, pmic_glink_altmode_enable_worker); > + init_completion(&altmode->pan_ack); > + mutex_init(&altmode->lock); > + > + device_for_each_child_node(dev, fwnode) { > + ret = fwnode_property_read_u32(fwnode, "reg", &port); > + if (ret < 0) { > + dev_err(dev, "missing reg property of %pOFn\n", fwnode); > + return ret; > + } > + > + if (port >= ARRAY_SIZE(altmode->ports)) { > + dev_warn(dev, "invalid connector number, ignoring\n"); > + continue; > + } > + > + if (altmode->ports[port].altmode) { > + dev_err(dev, "multiple connector definition for port %u\n", port); > + return -EINVAL; > + } > + > + alt_port = &altmode->ports[port]; > + alt_port->altmode = altmode; > + alt_port->index = port; > + INIT_WORK(&alt_port->work, pmic_glink_altmode_worker); > + > + alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs; > + alt_port->bridge.of_node = to_of_node(fwnode); > + alt_port->bridge.ops = DRM_BRIDGE_OP_HPD; > + alt_port->bridge.type = DRM_MODE_CONNECTOR_USB; > + > + ret = devm_drm_bridge_add(dev, &alt_port->bridge); > + if (ret) > + return ret; > + > + alt_port->dp_alt.svid = USB_TYPEC_DP_SID; > + alt_port->dp_alt.mode = USB_TYPEC_DP_MODE; > + alt_port->dp_alt.active = 1; > + > + mux_desc.svid = USB_TYPEC_DP_SID; > + mux_desc.mode = USB_TYPEC_DP_MODE; > + alt_port->typec_mux = fwnode_typec_mux_get(fwnode, &mux_desc); > + if (IS_ERR(alt_port->typec_mux)) > + return dev_err_probe(dev, PTR_ERR(alt_port->typec_mux), > + "failed to acquire mode-switch for port: %d\n", > + port); > + > + ret = devm_add_action_or_reset(dev, pmic_glink_altmode_put_mux, > + alt_port->typec_mux); > + if (ret) > + return ret; > + > + alt_port->typec_switch = fwnode_typec_switch_get(fwnode); > + if (IS_ERR(alt_port->typec_switch)) > + return dev_err_probe(dev, PTR_ERR(alt_port->typec_switch), > + "failed to acquire orientation-switch for port: %d\n", > + port); > + > + ret = devm_add_action_or_reset(dev, pmic_glink_altmode_put_switch, > + alt_port->typec_switch); > + if (ret) > + return ret; > + } > + > + altmode->client = devm_pmic_glink_register_client(dev, > + altmode->owner_id, > + pmic_glink_altmode_callback, > + pmic_glink_altmode_pdr_notify, > + altmode); > + return PTR_ERR_OR_ZERO(altmode->client); > +} > + > +static const struct auxiliary_device_id pmic_glink_altmode_id_table[] = { > + { .name = "pmic_glink.altmode", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(auxiliary, pmic_glink_altmode_id_table); > + > +static struct auxiliary_driver pmic_glink_altmode_driver = { > + .name = "pmic_glink_altmode", > + .probe = pmic_glink_altmode_probe, > + .id_table = pmic_glink_altmode_id_table, > +}; > + > +module_auxiliary_driver(pmic_glink_altmode_driver); > + > +MODULE_DESCRIPTION("Qualcomm PMIC GLINK Altmode driver"); > +MODULE_LICENSE("GPL"); > -- > 2.37.3 >
On Sun, Jan 15, 2023 at 01:10:14PM -0600, Steev Klimaszewski wrote: > > diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c [..] > > + msg = data; > > + notification = le32_to_cpu(msg->notification); > > + port = FIELD_GET(SC8180X_PORT_MASK, notification); > > + orientation = FIELD_GET(SC8180X_ORIENTATION_MASK, notification); > > + mux = FIELD_GET(SC8180X_MUX_MASK, notification); > > + mode = FIELD_GET(SC8180X_MODE_MASK, notification); > > + hpd_state = FIELD_GET(SC8180X_HPD_STATE_MASK, notification); > > + hpd_irq = FIELD_GET(SC8180X_HPD_IRQ_MASK, notification); > > + > The kernel test robot keeps complaining about these FIELD_GET because > there is no #include <linux/bitfield.h> > I must have missed those complains before, thanks for pointing it out! Regards, Bjorn
On Fri, Jan 13, 2023 at 03:56:59PM +0100, Konrad Dybcio wrote: > > > On 13.01.2023 05:11, Bjorn Andersson wrote: > > This implements the base PMIC GLINK driver, a power_supply driver and a > > driver for the USB Type-C altmode protocol. This has been tested and > > shown to provide battery information, USB Type-C switch and mux requests > > and DisplayPort notifications on SC8180X, SC8280XP and SM8350. > > > For the series: > > Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> # SM8350 PDX215 > > Thanks a lot for working on this! > Thank you for testing it :) > One thing, /sys/class/power_supply/qcom-battmgr-usb/input_current_limit > is stuck at zero and so is the current_now as a result (the voltage > readout is 5V + some noise, so that looks good), but I don't see any > SET paths for it in the driver, so I suppose that's what the firmware > default is? > I have not experimented with adjusting any configuration in this initial set, but there are a few knobs that could/should be introduced on top of this. That said, I believe input_current_limit should somehow come from the USB stack, rather than user space? Regards, Bjorn
On Fri, Jan 13, 2023 at 05:10:17PM +0000, Bryan O'Donoghue wrote: > On 13/01/2023 04:11, Bjorn Andersson wrote: > > This implements the base PMIC GLINK driver, a power_supply driver and a > > driver for the USB Type-C altmode protocol. This has been tested and > > shown to provide battery information, USB Type-C switch and mux requests > > and DisplayPort notifications on SC8180X, SC8280XP and SM8350. > > > > Bjorn Andersson (4): > > dt-bindings: soc: qcom: Introduce PMIC GLINK binding > > soc: qcom: pmic_glink: Introduce base PMIC GLINK driver > > soc: qcom: pmic_glink: Introduce altmode support > > power: supply: Introduce Qualcomm PMIC GLINK power supply > > > > .../bindings/soc/qcom/qcom,pmic-glink.yaml | 102 ++ > > drivers/power/supply/Kconfig | 9 + > > drivers/power/supply/Makefile | 1 + > > drivers/power/supply/qcom_battmgr.c | 1421 +++++++++++++++++ > > drivers/soc/qcom/Kconfig | 15 + > > drivers/soc/qcom/Makefile | 2 + > > drivers/soc/qcom/pmic_glink.c | 336 ++++ > > drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++ > > include/linux/soc/qcom/pmic_glink.h | 32 + > > 9 files changed, 2395 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml > > create mode 100644 drivers/power/supply/qcom_battmgr.c > > create mode 100644 drivers/soc/qcom/pmic_glink.c > > create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c > > create mode 100644 include/linux/soc/qcom/pmic_glink.h > > > > How does the USB PHY and a USB redriver fit into this ? > > Is the host supposed to manage both/neither ? Is the DSP responsible for > configuring the PHY lanes and the turnaround on orientation switch ? > As indicated above, the firmware deals with battery management and USB Type-C handling. The battery/power management is handled by the battmgr implementation, exposing the various properties through a set of power_supply objects. The USB Type-C handling comes in two forms. The "altmode" protocol handles DisplayPort notifications - plug detect, orientation and mode switches. The other part of the USB implementation exposes UCSI. The altmode implementation provides two things: - A drm_bridge, per connector, which can be tied (of_graph) to a DisplayPort instance, and will invoke HPD notifications on the drm_bridge, based on notification messages thereof. - Acquire typec_switch and typec_mux handles through the of_graph and signal the remotes when notifications of state changes occur. Linking this to the FSA4480, is sufficient to get USB/DP combo (2+2 lanes) working on e.g. SM8350 HDK. Work in progress patches also exists for teaching QMP about orientation switching of the SS lines, but it seems this needs to be rebased onto the refactored QMP driver. I also have patches for QMP to make it switch USB/DP combo -> 4-lane DP, which allow 4k support without DSC, unfortunately switch back to USB has not been fully reliable, so this requires some more work (downstream involves DWC3 here as well, to reprogram the PHY). I have been experimenting with UCSI in the past, but my goal for this series was to support external displays on my desktop (laptop...), but through some experiments I've wired the connectors to dwc3 in order to get usb_role_switch working. Neil has been looking at this in more detail lately though. Regards, Bjorn
On 17/01/2023 02:32, Bjorn Andersson wrote: > On Fri, Jan 13, 2023 at 05:10:17PM +0000, Bryan O'Donoghue wrote: >> On 13/01/2023 04:11, Bjorn Andersson wrote: >>> This implements the base PMIC GLINK driver, a power_supply driver and a >>> driver for the USB Type-C altmode protocol. This has been tested and >>> shown to provide battery information, USB Type-C switch and mux requests >>> and DisplayPort notifications on SC8180X, SC8280XP and SM8350. >>> >>> Bjorn Andersson (4): >>> dt-bindings: soc: qcom: Introduce PMIC GLINK binding >>> soc: qcom: pmic_glink: Introduce base PMIC GLINK driver >>> soc: qcom: pmic_glink: Introduce altmode support >>> power: supply: Introduce Qualcomm PMIC GLINK power supply >>> >>> .../bindings/soc/qcom/qcom,pmic-glink.yaml | 102 ++ >>> drivers/power/supply/Kconfig | 9 + >>> drivers/power/supply/Makefile | 1 + >>> drivers/power/supply/qcom_battmgr.c | 1421 +++++++++++++++++ >>> drivers/soc/qcom/Kconfig | 15 + >>> drivers/soc/qcom/Makefile | 2 + >>> drivers/soc/qcom/pmic_glink.c | 336 ++++ >>> drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++ >>> include/linux/soc/qcom/pmic_glink.h | 32 + >>> 9 files changed, 2395 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml >>> create mode 100644 drivers/power/supply/qcom_battmgr.c >>> create mode 100644 drivers/soc/qcom/pmic_glink.c >>> create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c >>> create mode 100644 include/linux/soc/qcom/pmic_glink.h >>> >> >> How does the USB PHY and a USB redriver fit into this ? >> >> Is the host supposed to manage both/neither ? Is the DSP responsible for >> configuring the PHY lanes and the turnaround on orientation switch ? >> > > As indicated above, the firmware deals with battery management and USB > Type-C handling. > > The battery/power management is handled by the battmgr implementation, > exposing the various properties through a set of power_supply objects. > > The USB Type-C handling comes in two forms. The "altmode" protocol > handles DisplayPort notifications - plug detect, orientation and mode > switches. The other part of the USB implementation exposes UCSI. > > The altmode implementation provides two things: > - A drm_bridge, per connector, which can be tied (of_graph) to a > DisplayPort instance, and will invoke HPD notifications on the > drm_bridge, based on notification messages thereof. > > - Acquire typec_switch and typec_mux handles through the of_graph and > signal the remotes when notifications of state changes occur. Linking > this to the FSA4480, is sufficient to get USB/DP combo (2+2 lanes) > working on e.g. SM8350 HDK. > Work in progress patches also exists for teaching QMP about > orientation switching of the SS lines, but it seems this needs to be > rebased onto the refactored QMP driver. > I also have patches for QMP to make it switch USB/DP combo -> 4-lane > DP, which allow 4k support without DSC, unfortunately switch back to > USB has not been fully reliable, so this requires some more work > (downstream involves DWC3 here as well, to reprogram the PHY). Oki doki that makes sense and is pretty much in-line with what I thought. We still have a bunch of typec-mux and phy work to do even with adsp/glink doing the TCPM. Thanks for the explanation. --- bod
On Tue, Jan 17, 2023 at 02:37:27AM +0000, Bryan O'Donoghue wrote: > On 17/01/2023 02:32, Bjorn Andersson wrote: > > On Fri, Jan 13, 2023 at 05:10:17PM +0000, Bryan O'Donoghue wrote: > > > On 13/01/2023 04:11, Bjorn Andersson wrote: > > > > This implements the base PMIC GLINK driver, a power_supply driver and a > > > > driver for the USB Type-C altmode protocol. This has been tested and > > > > shown to provide battery information, USB Type-C switch and mux requests > > > > and DisplayPort notifications on SC8180X, SC8280XP and SM8350. > > > > > > > > Bjorn Andersson (4): > > > > dt-bindings: soc: qcom: Introduce PMIC GLINK binding > > > > soc: qcom: pmic_glink: Introduce base PMIC GLINK driver > > > > soc: qcom: pmic_glink: Introduce altmode support > > > > power: supply: Introduce Qualcomm PMIC GLINK power supply > > > > > > > > .../bindings/soc/qcom/qcom,pmic-glink.yaml | 102 ++ > > > > drivers/power/supply/Kconfig | 9 + > > > > drivers/power/supply/Makefile | 1 + > > > > drivers/power/supply/qcom_battmgr.c | 1421 +++++++++++++++++ > > > > drivers/soc/qcom/Kconfig | 15 + > > > > drivers/soc/qcom/Makefile | 2 + > > > > drivers/soc/qcom/pmic_glink.c | 336 ++++ > > > > drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++ > > > > include/linux/soc/qcom/pmic_glink.h | 32 + > > > > 9 files changed, 2395 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml > > > > create mode 100644 drivers/power/supply/qcom_battmgr.c > > > > create mode 100644 drivers/soc/qcom/pmic_glink.c > > > > create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c > > > > create mode 100644 include/linux/soc/qcom/pmic_glink.h > > > > > > > > > > How does the USB PHY and a USB redriver fit into this ? > > > > > > Is the host supposed to manage both/neither ? Is the DSP responsible for > > > configuring the PHY lanes and the turnaround on orientation switch ? > > > > > > > As indicated above, the firmware deals with battery management and USB > > Type-C handling. > > > > The battery/power management is handled by the battmgr implementation, > > exposing the various properties through a set of power_supply objects. > > > > The USB Type-C handling comes in two forms. The "altmode" protocol > > handles DisplayPort notifications - plug detect, orientation and mode > > switches. The other part of the USB implementation exposes UCSI. > > > > The altmode implementation provides two things: > > - A drm_bridge, per connector, which can be tied (of_graph) to a > > DisplayPort instance, and will invoke HPD notifications on the > > drm_bridge, based on notification messages thereof. > > > > - Acquire typec_switch and typec_mux handles through the of_graph and > > signal the remotes when notifications of state changes occur. Linking > > this to the FSA4480, is sufficient to get USB/DP combo (2+2 lanes) > > working on e.g. SM8350 HDK. > > Work in progress patches also exists for teaching QMP about > > orientation switching of the SS lines, but it seems this needs to be > > rebased onto the refactored QMP driver. > > I also have patches for QMP to make it switch USB/DP combo -> 4-lane > > DP, which allow 4k support without DSC, unfortunately switch back to > > USB has not been fully reliable, so this requires some more work > > (downstream involves DWC3 here as well, to reprogram the PHY). > > Oki doki that makes sense and is pretty much in-line with what I thought. > > We still have a bunch of typec-mux and phy work to do even with adsp/glink > doing the TCPM. > Correct, the registration of QMP as a typec_switch and typec_mux and handling of respective notification remains open and should (by design) be independent of the TCPM implementation. In particular the orientation switching is an itch worth scratching at this time. But when the DPU becomes capable of producing 4k@60 output it would obviously be nice to have the whole shebang :) Regards, Bjorn > Thanks for the explanation. > > --- > bod >
On 17/01/2023 04:58, Bjorn Andersson wrote: > On Tue, Jan 17, 2023 at 02:37:27AM +0000, Bryan O'Donoghue wrote: >> On 17/01/2023 02:32, Bjorn Andersson wrote: >>> On Fri, Jan 13, 2023 at 05:10:17PM +0000, Bryan O'Donoghue wrote: >>>> On 13/01/2023 04:11, Bjorn Andersson wrote: >>>>> This implements the base PMIC GLINK driver, a power_supply driver and a >>>>> driver for the USB Type-C altmode protocol. This has been tested and >>>>> shown to provide battery information, USB Type-C switch and mux requests >>>>> and DisplayPort notifications on SC8180X, SC8280XP and SM8350. >>>>> >>>>> Bjorn Andersson (4): >>>>> dt-bindings: soc: qcom: Introduce PMIC GLINK binding >>>>> soc: qcom: pmic_glink: Introduce base PMIC GLINK driver >>>>> soc: qcom: pmic_glink: Introduce altmode support >>>>> power: supply: Introduce Qualcomm PMIC GLINK power supply >>>>> >>>>> .../bindings/soc/qcom/qcom,pmic-glink.yaml | 102 ++ >>>>> drivers/power/supply/Kconfig | 9 + >>>>> drivers/power/supply/Makefile | 1 + >>>>> drivers/power/supply/qcom_battmgr.c | 1421 +++++++++++++++++ >>>>> drivers/soc/qcom/Kconfig | 15 + >>>>> drivers/soc/qcom/Makefile | 2 + >>>>> drivers/soc/qcom/pmic_glink.c | 336 ++++ >>>>> drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++ >>>>> include/linux/soc/qcom/pmic_glink.h | 32 + >>>>> 9 files changed, 2395 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml >>>>> create mode 100644 drivers/power/supply/qcom_battmgr.c >>>>> create mode 100644 drivers/soc/qcom/pmic_glink.c >>>>> create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c >>>>> create mode 100644 include/linux/soc/qcom/pmic_glink.h >>>>> >>>> >>>> How does the USB PHY and a USB redriver fit into this ? >>>> >>>> Is the host supposed to manage both/neither ? Is the DSP responsible for >>>> configuring the PHY lanes and the turnaround on orientation switch ? >>>> >>> >>> As indicated above, the firmware deals with battery management and USB >>> Type-C handling. >>> >>> The battery/power management is handled by the battmgr implementation, >>> exposing the various properties through a set of power_supply objects. >>> >>> The USB Type-C handling comes in two forms. The "altmode" protocol >>> handles DisplayPort notifications - plug detect, orientation and mode >>> switches. The other part of the USB implementation exposes UCSI. >>> >>> The altmode implementation provides two things: >>> - A drm_bridge, per connector, which can be tied (of_graph) to a >>> DisplayPort instance, and will invoke HPD notifications on the >>> drm_bridge, based on notification messages thereof. >>> >>> - Acquire typec_switch and typec_mux handles through the of_graph and >>> signal the remotes when notifications of state changes occur. Linking >>> this to the FSA4480, is sufficient to get USB/DP combo (2+2 lanes) >>> working on e.g. SM8350 HDK. >>> Work in progress patches also exists for teaching QMP about >>> orientation switching of the SS lines, but it seems this needs to be >>> rebased onto the refactored QMP driver. >>> I also have patches for QMP to make it switch USB/DP combo -> 4-lane >>> DP, which allow 4k support without DSC, unfortunately switch back to >>> USB has not been fully reliable, so this requires some more work >>> (downstream involves DWC3 here as well, to reprogram the PHY). >> >> Oki doki that makes sense and is pretty much in-line with what I thought. >> >> We still have a bunch of typec-mux and phy work to do even with adsp/glink >> doing the TCPM. >> > > Correct, the registration of QMP as a typec_switch and typec_mux and > handling of respective notification remains open and should (by design) > be independent of the TCPM implementation. > > In particular the orientation switching is an itch worth scratching at > this time. But when the DPU becomes capable of producing 4k@60 output it > would obviously be nice to have the whole shebang :) Did you try it with the wide planes patchset at [1]? I was able to get stable 4k@30 on RB3 (being limited only by the DSI-HDMI bridge). [1] https://lore.kernel.org/linux-arm-msm/20221229191856.3508092-1-dmitry.baryshkov@linaro.org/
On Tue, Jan 17, 2023 at 11:26:58AM +0200, Dmitry Baryshkov wrote: > On 17/01/2023 04:58, Bjorn Andersson wrote: > > On Tue, Jan 17, 2023 at 02:37:27AM +0000, Bryan O'Donoghue wrote: > > > On 17/01/2023 02:32, Bjorn Andersson wrote: > > > > On Fri, Jan 13, 2023 at 05:10:17PM +0000, Bryan O'Donoghue wrote: > > > > > On 13/01/2023 04:11, Bjorn Andersson wrote: > > > > > > This implements the base PMIC GLINK driver, a power_supply driver and a > > > > > > driver for the USB Type-C altmode protocol. This has been tested and > > > > > > shown to provide battery information, USB Type-C switch and mux requests > > > > > > and DisplayPort notifications on SC8180X, SC8280XP and SM8350. > > > > > > > > > > > > Bjorn Andersson (4): > > > > > > dt-bindings: soc: qcom: Introduce PMIC GLINK binding > > > > > > soc: qcom: pmic_glink: Introduce base PMIC GLINK driver > > > > > > soc: qcom: pmic_glink: Introduce altmode support > > > > > > power: supply: Introduce Qualcomm PMIC GLINK power supply > > > > > > > > > > > > .../bindings/soc/qcom/qcom,pmic-glink.yaml | 102 ++ > > > > > > drivers/power/supply/Kconfig | 9 + > > > > > > drivers/power/supply/Makefile | 1 + > > > > > > drivers/power/supply/qcom_battmgr.c | 1421 +++++++++++++++++ > > > > > > drivers/soc/qcom/Kconfig | 15 + > > > > > > drivers/soc/qcom/Makefile | 2 + > > > > > > drivers/soc/qcom/pmic_glink.c | 336 ++++ > > > > > > drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++ > > > > > > include/linux/soc/qcom/pmic_glink.h | 32 + > > > > > > 9 files changed, 2395 insertions(+) > > > > > > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml > > > > > > create mode 100644 drivers/power/supply/qcom_battmgr.c > > > > > > create mode 100644 drivers/soc/qcom/pmic_glink.c > > > > > > create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c > > > > > > create mode 100644 include/linux/soc/qcom/pmic_glink.h > > > > > > > > > > > > > > > > How does the USB PHY and a USB redriver fit into this ? > > > > > > > > > > Is the host supposed to manage both/neither ? Is the DSP responsible for > > > > > configuring the PHY lanes and the turnaround on orientation switch ? > > > > > > > > > > > > > As indicated above, the firmware deals with battery management and USB > > > > Type-C handling. > > > > > > > > The battery/power management is handled by the battmgr implementation, > > > > exposing the various properties through a set of power_supply objects. > > > > > > > > The USB Type-C handling comes in two forms. The "altmode" protocol > > > > handles DisplayPort notifications - plug detect, orientation and mode > > > > switches. The other part of the USB implementation exposes UCSI. > > > > > > > > The altmode implementation provides two things: > > > > - A drm_bridge, per connector, which can be tied (of_graph) to a > > > > DisplayPort instance, and will invoke HPD notifications on the > > > > drm_bridge, based on notification messages thereof. > > > > > > > > - Acquire typec_switch and typec_mux handles through the of_graph and > > > > signal the remotes when notifications of state changes occur. Linking > > > > this to the FSA4480, is sufficient to get USB/DP combo (2+2 lanes) > > > > working on e.g. SM8350 HDK. > > > > Work in progress patches also exists for teaching QMP about > > > > orientation switching of the SS lines, but it seems this needs to be > > > > rebased onto the refactored QMP driver. > > > > I also have patches for QMP to make it switch USB/DP combo -> 4-lane > > > > DP, which allow 4k support without DSC, unfortunately switch back to > > > > USB has not been fully reliable, so this requires some more work > > > > (downstream involves DWC3 here as well, to reprogram the PHY). > > > > > > Oki doki that makes sense and is pretty much in-line with what I thought. > > > > > > We still have a bunch of typec-mux and phy work to do even with adsp/glink > > > doing the TCPM. > > > > > > > Correct, the registration of QMP as a typec_switch and typec_mux and > > handling of respective notification remains open and should (by design) > > be independent of the TCPM implementation. > > > > In particular the orientation switching is an itch worth scratching at > > this time. But when the DPU becomes capable of producing 4k@60 output it > > would obviously be nice to have the whole shebang :) > > Did you try it with the wide planes patchset at [1]? I was able to get > stable 4k@30 on RB3 (being limited only by the DSI-HDMI bridge). > > [1] https://lore.kernel.org/linux-arm-msm/20221229191856.3508092-1-dmitry.baryshkov@linaro.org/ > I have not done so, as the patches I had for switching to 4-lane DP output needs to be rewritten since the refactoring. I had no problem doing 4k@30 prior to your efforts, so I'm interested in validating the changes you've made. Thanks, Bjorn > -- > With best wishes > Dmitry >
On 13/01/2023 05:11, Bjorn Andersson wrote: > From: Bjorn Andersson <bjorn.andersson@linaro.org> > > With the PMIC GLINK service, the host OS subscribes to USB-C altmode > messages, which are sent by the firmware to notify the host OS about > state updates and HPD interrupts. > > The pmic_glink_altmode driver registers for these notifications and > propagates the notifications as typec_mux, typec_switch and DRM OOB > notifications as necessary to implement DisplayPort altmode support. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > > Changes since v1: > - None > > Johan reported a NULL pointer dereference in > drm_kms_helper_hotplug_event() for HPD event being reported while the > MSM DRM driver is still being initalized, a separate fix has been sent > in hope to remidy this race condition in the MSM driver. > > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++++++++++++++++++++++ > 2 files changed, 478 insertions(+) > create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c > > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index 29cccac472f3..f30552bf4da7 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o > obj-$(CONFIG_QCOM_OCMEM) += ocmem.o > obj-$(CONFIG_QCOM_PDR_HELPERS) += pdr_interface.o > obj-$(CONFIG_QCOM_PMIC_GLINK) += pmic_glink.o > +obj-$(CONFIG_QCOM_PMIC_GLINK) += pmic_glink_altmode.o > obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o > qmi_helpers-y += qmi_encdec.o qmi_interface.o > obj-$(CONFIG_QCOM_RAMP_CTRL) += ramp_controller.o > diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c > new file mode 100644 > index 000000000000..8d2d563cb756 > --- /dev/null > +++ b/drivers/soc/qcom/pmic_glink_altmode.c > @@ -0,0 +1,477 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved. > + * Copyright (c) 2022, Linaro Ltd > + */ > +#include <linux/auxiliary_bus.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/mutex.h> > +#include <linux/property.h> > +#include <linux/soc/qcom/pdr.h> > +#include <drm/drm_bridge.h> > + > +#include <linux/usb/typec_altmode.h> > +#include <linux/usb/typec_dp.h> > +#include <linux/usb/typec_mux.h> > + > +#include <linux/soc/qcom/pmic_glink.h> > + > +#define PMIC_GLINK_MAX_PORTS 2 > + > +#define USBC_SC8180X_NOTIFY_IND 0x13 > +#define USBC_CMD_WRITE_REQ 0x15 > +#define USBC_NOTIFY_IND 0x16 > + > +#define ALTMODE_PAN_EN 0x10 > +#define ALTMODE_PAN_ACK 0x11 > + > +struct usbc_write_req { > + struct pmic_glink_hdr hdr; > + __le32 cmd; > + __le32 arg; > + __le32 reserved; > +}; > + > +#define NOTIFY_PAYLOAD_SIZE 16 > +struct usbc_notify { > + struct pmic_glink_hdr hdr; > + char payload[NOTIFY_PAYLOAD_SIZE]; > + u32 reserved; > +}; > + > +struct usbc_sc8180x_notify { > + struct pmic_glink_hdr hdr; > + __le32 notification; > + __le32 reserved[2]; > +}; > + > +enum pmic_glink_altmode_pin_assignment { > + DPAM_HPD_OUT, > + DPAM_HPD_A, > + DPAM_HPD_B, > + DPAM_HPD_C, > + DPAM_HPD_D, > + DPAM_HPD_E, > + DPAM_HPD_F, > +}; > + > +struct pmic_glink_altmode; > + > +#define work_to_altmode_port(w) container_of((w), struct pmic_glink_altmode_port, work) > + > +struct pmic_glink_altmode_port { > + struct pmic_glink_altmode *altmode; > + unsigned int index; > + > + struct typec_switch *typec_switch; > + struct typec_mux *typec_mux; > + struct typec_mux_state state; > + struct typec_altmode dp_alt; > + > + struct work_struct work; > + > + struct drm_bridge bridge; > + > + enum typec_orientation orientation; > + u16 svid; > + u8 dp_data; > + u8 mode; > + u8 hpd_state; > + u8 hpd_irq; > +}; > + > +#define work_to_altmode(w) container_of((w), struct pmic_glink_altmode, enable_work) > + > +struct pmic_glink_altmode { > + struct device *dev; > + > + unsigned int owner_id; > + > + /* To synchronize WRITE_REQ acks */ > + struct mutex lock; > + > + struct completion pan_ack; > + struct pmic_glink_client *client; > + > + struct work_struct enable_work; > + > + struct pmic_glink_altmode_port ports[PMIC_GLINK_MAX_PORTS]; > +}; > + > +static int pmic_glink_altmode_request(struct pmic_glink_altmode *altmode, u32 cmd, u32 arg) > +{ > + struct usbc_write_req req = {}; > + unsigned long left; > + int ret; > + > + /* > + * The USBC_CMD_WRITE_REQ ack doesn't identify the request, so wait for > + * one ack at a time. > + */ > + mutex_lock(&altmode->lock); > + > + req.hdr.owner = cpu_to_le32(altmode->owner_id); > + req.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP); > + req.hdr.opcode = cpu_to_le32(USBC_CMD_WRITE_REQ); > + req.cmd = cpu_to_le32(cmd); > + req.arg = cpu_to_le32(arg); > + > + ret = pmic_glink_send(altmode->client, &req, sizeof(req)); > + if (ret) { > + dev_err(altmode->dev, "failed to send altmode request: %#x (%d)\n", cmd, ret); > + goto out_unlock; > + } > + > + left = wait_for_completion_timeout(&altmode->pan_ack, 5 * HZ); > + if (!left) { > + dev_err(altmode->dev, "timeout waiting for altmode request ack for: %#x\n", cmd); > + ret = -ETIMEDOUT; > + } > + > +out_unlock: > + mutex_unlock(&altmode->lock); > + return ret; > +} > + > +static void pmic_glink_altmode_enable_dp(struct pmic_glink_altmode *altmode, > + struct pmic_glink_altmode_port *port, > + u8 mode, bool hpd_state, > + bool hpd_irq) > +{ > + struct typec_displayport_data dp_data = {}; > + int ret; > + > + dp_data.status = DP_STATUS_ENABLED; > + if (hpd_state) > + dp_data.status |= DP_STATUS_HPD_STATE; > + if (hpd_irq) > + dp_data.status |= DP_STATUS_IRQ_HPD; > + dp_data.conf = DP_CONF_SET_PIN_ASSIGN(mode); > + > + port->state.alt = &port->dp_alt; > + port->state.data = &dp_data; > + port->state.mode = TYPEC_MODAL_STATE(mode); > + > + ret = typec_mux_set(port->typec_mux, &port->state); > + if (ret) > + dev_err(altmode->dev, "failed to switch mux to DP\n"); > +} > + > +static void pmic_glink_altmode_enable_usb(struct pmic_glink_altmode *altmode, > + struct pmic_glink_altmode_port *port) > +{ > + int ret; > + > + port->state.alt = NULL; > + port->state.data = NULL; > + port->state.mode = TYPEC_STATE_USB; > + > + ret = typec_mux_set(port->typec_mux, &port->state); > + if (ret) > + dev_err(altmode->dev, "failed to switch mux to USB\n"); > +} > + > +static void pmic_glink_altmode_worker(struct work_struct *work) > +{ > + struct pmic_glink_altmode_port *alt_port = work_to_altmode_port(work); > + struct pmic_glink_altmode *altmode = alt_port->altmode; > + > + typec_switch_set(alt_port->typec_switch, alt_port->orientation); > + > + if (alt_port->svid == USB_TYPEC_DP_SID) > + pmic_glink_altmode_enable_dp(altmode, alt_port, alt_port->mode, > + alt_port->hpd_state, alt_port->hpd_irq); > + else > + pmic_glink_altmode_enable_usb(altmode, alt_port); > + > + if (alt_port->hpd_state) > + drm_bridge_hpd_notify(&alt_port->bridge, connector_status_connected); > + else > + drm_bridge_hpd_notify(&alt_port->bridge, connector_status_disconnected); > + > + pmic_glink_altmode_request(altmode, ALTMODE_PAN_ACK, alt_port->index); > +}; > + > +static enum typec_orientation pmic_glink_altmode_orientation(unsigned int orientation) > +{ > + if (orientation == 0) > + return TYPEC_ORIENTATION_NORMAL; > + else if (orientation == 1) > + return TYPEC_ORIENTATION_REVERSE; > + else > + return TYPEC_ORIENTATION_NONE; > +} > + > +#define SC8180X_PORT_MASK 0x000000ff > +#define SC8180X_ORIENTATION_MASK 0x0000ff00 > +#define SC8180X_MUX_MASK 0x00ff0000 > +#define SC8180X_MODE_MASK 0x3f000000 > +#define SC8180X_HPD_STATE_MASK 0x40000000 > +#define SC8180X_HPD_IRQ_MASK 0x80000000 > + > +static void pmic_glink_altmode_sc8180xp_notify(struct pmic_glink_altmode *altmode, > + const void *data, size_t len) > +{ > + struct pmic_glink_altmode_port *alt_port; > + const struct usbc_sc8180x_notify *msg; > + u32 notification; > + u8 orientation; > + u8 hpd_state; > + u8 hpd_irq; > + u16 svid; > + u8 port; > + u8 mode; > + u8 mux; > + > + if (len != sizeof(*msg)) { > + dev_warn(altmode->dev, "invalid length of USBC_NOTIFY indication: %zd\n", len); > + return; > + } > + > + msg = data; > + notification = le32_to_cpu(msg->notification); > + port = FIELD_GET(SC8180X_PORT_MASK, notification); > + orientation = FIELD_GET(SC8180X_ORIENTATION_MASK, notification); > + mux = FIELD_GET(SC8180X_MUX_MASK, notification); > + mode = FIELD_GET(SC8180X_MODE_MASK, notification); > + hpd_state = FIELD_GET(SC8180X_HPD_STATE_MASK, notification); > + hpd_irq = FIELD_GET(SC8180X_HPD_IRQ_MASK, notification); > + > + svid = mux == 2 ? USB_TYPEC_DP_SID : 0; > + > + if (!altmode->ports[port].altmode) { > + dev_dbg(altmode->dev, "notification on undefined port %d\n", port); > + return; > + } > + > + alt_port = &altmode->ports[port]; > + alt_port->orientation = pmic_glink_altmode_orientation(orientation); > + alt_port->svid = mux == 2 ? USB_TYPEC_DP_SID : 0; > + alt_port->mode = mode; > + alt_port->hpd_state = hpd_state; > + alt_port->hpd_irq = hpd_irq; > + schedule_work(&alt_port->work); > +} > + > +#define SC8280XP_DPAM_MASK 0x3f > +#define SC8280XP_HPD_STATE_MASK BIT(6) > +#define SC8280XP_HPD_IRQ_MASK BIT(7) > + > +static void pmic_glink_altmode_sc8280xp_notify(struct pmic_glink_altmode *altmode, > + u16 svid, const void *data, size_t len) > +{ > + struct pmic_glink_altmode_port *alt_port; > + const struct usbc_notify *notify; > + u8 orientation; > + u8 hpd_state; > + u8 hpd_irq; > + u8 mode; > + u8 port; > + > + if (len != sizeof(*notify)) { > + dev_warn(altmode->dev, "invalid length USBC_NOTIFY_IND: %zd\n", > + len); > + return; > + } > + > + notify = data; > + > + port = notify->payload[0]; > + orientation = notify->payload[1]; > + mode = FIELD_GET(SC8280XP_DPAM_MASK, notify->payload[8]) - DPAM_HPD_A; > + hpd_state = FIELD_GET(SC8280XP_HPD_STATE_MASK, notify->payload[8]); > + hpd_irq = FIELD_GET(SC8280XP_HPD_IRQ_MASK, notify->payload[8]); > + > + if (!altmode->ports[port].altmode) { > + dev_dbg(altmode->dev, "notification on undefined port %d\n", port); > + return; > + } > + > + alt_port = &altmode->ports[port]; > + alt_port->orientation = pmic_glink_altmode_orientation(orientation); > + alt_port->svid = svid; > + alt_port->mode = mode; > + alt_port->hpd_state = hpd_state; > + alt_port->hpd_irq = hpd_irq; > + schedule_work(&alt_port->work); > +} > + > +static void pmic_glink_altmode_callback(const void *data, size_t len, void *priv) > +{ > + struct pmic_glink_altmode *altmode = priv; > + const struct pmic_glink_hdr *hdr = data; > + u16 opcode; > + u16 svid; > + > + opcode = le32_to_cpu(hdr->opcode) & 0xff; > + svid = le32_to_cpu(hdr->opcode) >> 16; > + > + switch (opcode) { > + case USBC_CMD_WRITE_REQ: > + complete(&altmode->pan_ack); > + break; > + case USBC_NOTIFY_IND: > + pmic_glink_altmode_sc8280xp_notify(altmode, svid, data, len); > + break; > + case USBC_SC8180X_NOTIFY_IND: > + pmic_glink_altmode_sc8180xp_notify(altmode, data, len); > + break; > + } > +} > + > +static int pmic_glink_altmode_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; > +} > + > +static const struct drm_bridge_funcs pmic_glink_altmode_bridge_funcs = { > + .attach = pmic_glink_altmode_attach, > +}; > + > +static void pmic_glink_altmode_put_mux(void *data) > +{ > + typec_mux_put(data); > +} > + > +static void pmic_glink_altmode_put_switch(void *data) > +{ > + typec_switch_put(data); > +} > + > +static void pmic_glink_altmode_enable_worker(struct work_struct *work) > +{ > + struct pmic_glink_altmode *altmode = work_to_altmode(work); > + int ret; > + > + ret = pmic_glink_altmode_request(altmode, ALTMODE_PAN_EN, 0); > + if (ret) > + dev_err(altmode->dev, "failed to request altmode notifications\n"); > +} > + > +static void pmic_glink_altmode_pdr_notify(void *priv, int state) > +{ > + struct pmic_glink_altmode *altmode = priv; > + > + if (state == SERVREG_SERVICE_STATE_UP) > + schedule_work(&altmode->enable_work); > +} > + > +static const struct of_device_id pmic_glink_altmode_of_quirks[] = { > + { .compatible = "qcom,sc8180x-pmic-glink", .data = (void *)PMIC_GLINK_OWNER_USBC }, > + {} > +}; > + > +static int pmic_glink_altmode_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct pmic_glink_altmode_port *alt_port; > + struct pmic_glink_altmode *altmode; > + struct typec_altmode_desc mux_desc = {}; > + const struct of_device_id *match; > + struct fwnode_handle *fwnode; > + struct device *dev = &adev->dev; > + u32 port; > + int ret; > + > + altmode = devm_kzalloc(dev, sizeof(*altmode), GFP_KERNEL); > + if (!altmode) > + return -ENOMEM; > + > + altmode->dev = dev; > + > + match = of_match_device(pmic_glink_altmode_of_quirks, dev->parent); > + if (match) > + altmode->owner_id = (unsigned long)match->data; > + else > + altmode->owner_id = PMIC_GLINK_OWNER_USBC_PAN; > + > + INIT_WORK(&altmode->enable_work, pmic_glink_altmode_enable_worker); > + init_completion(&altmode->pan_ack); > + mutex_init(&altmode->lock); > + > + device_for_each_child_node(dev, fwnode) { > + ret = fwnode_property_read_u32(fwnode, "reg", &port); > + if (ret < 0) { > + dev_err(dev, "missing reg property of %pOFn\n", fwnode); > + return ret; > + } > + > + if (port >= ARRAY_SIZE(altmode->ports)) { > + dev_warn(dev, "invalid connector number, ignoring\n"); > + continue; > + } > + > + if (altmode->ports[port].altmode) { > + dev_err(dev, "multiple connector definition for port %u\n", port); > + return -EINVAL; > + } > + > + alt_port = &altmode->ports[port]; > + alt_port->altmode = altmode; > + alt_port->index = port; > + INIT_WORK(&alt_port->work, pmic_glink_altmode_worker); > + > + alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs; > + alt_port->bridge.of_node = to_of_node(fwnode); > + alt_port->bridge.ops = DRM_BRIDGE_OP_HPD; > + alt_port->bridge.type = DRM_MODE_CONNECTOR_USB; > + > + ret = devm_drm_bridge_add(dev, &alt_port->bridge); > + if (ret) > + return ret; In my testing, the design of a bridge in the altmode driver made all the probe very fragile, meaning that any device that won't probe in the full usb--pmic-glink--display driver would prevent the whole to actually probe. This is why drm_connector_oob_hotplug_event() was used in drivers/usb/typec/altmodes/displayport.c and a similar design for cec where both attach to a device so there's no probe dependency. I think there's a possible simplification here by using : of_drm_find_bridge() instead on the endpoint target to get the last bridge, or we could probably use the same drm_connector_oob_hotplug_event() but in any way we should add missing pieces in drm_bridge_connector and drm/msm/dp/dp_drm.c First in drm/msm/dp/dp_drm.c, the driver should add the of_node to the DP bridge so it can be found from the dp controller node. Secondly, we could add a fwnode in drm_bridge like in drm_connector and in drm_bridge_connector_init() we could set the connector fwnode to the last bridge fwnode + the connector oob event handler. In this case the drm_connector_oob_hotplug_event() design should work. Anyway, I think the bindings is correct, so it's a matter of implementation to avoid delaying the display driver probe until the pmic_glink probes entirely. Neil > + > + alt_port->dp_alt.svid = USB_TYPEC_DP_SID; > + alt_port->dp_alt.mode = USB_TYPEC_DP_MODE; > + alt_port->dp_alt.active = 1; > + > + mux_desc.svid = USB_TYPEC_DP_SID; > + mux_desc.mode = USB_TYPEC_DP_MODE; > + alt_port->typec_mux = fwnode_typec_mux_get(fwnode, &mux_desc); > + if (IS_ERR(alt_port->typec_mux)) > + return dev_err_probe(dev, PTR_ERR(alt_port->typec_mux), > + "failed to acquire mode-switch for port: %d\n", > + port); > + > + ret = devm_add_action_or_reset(dev, pmic_glink_altmode_put_mux, > + alt_port->typec_mux); > + if (ret) > + return ret; > + > + alt_port->typec_switch = fwnode_typec_switch_get(fwnode); > + if (IS_ERR(alt_port->typec_switch)) > + return dev_err_probe(dev, PTR_ERR(alt_port->typec_switch), > + "failed to acquire orientation-switch for port: %d\n", > + port); > + > + ret = devm_add_action_or_reset(dev, pmic_glink_altmode_put_switch, > + alt_port->typec_switch); > + if (ret) > + return ret; > + } > + > + altmode->client = devm_pmic_glink_register_client(dev, > + altmode->owner_id, > + pmic_glink_altmode_callback, > + pmic_glink_altmode_pdr_notify, > + altmode); > + return PTR_ERR_OR_ZERO(altmode->client); > +} > + > +static const struct auxiliary_device_id pmic_glink_altmode_id_table[] = { > + { .name = "pmic_glink.altmode", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(auxiliary, pmic_glink_altmode_id_table); > + > +static struct auxiliary_driver pmic_glink_altmode_driver = { > + .name = "pmic_glink_altmode", > + .probe = pmic_glink_altmode_probe, > + .id_table = pmic_glink_altmode_id_table, > +}; > + > +module_auxiliary_driver(pmic_glink_altmode_driver); > + > +MODULE_DESCRIPTION("Qualcomm PMIC GLINK Altmode driver"); > +MODULE_LICENSE("GPL");
On 20/01/2023 11:06, Neil Armstrong wrote: > On 13/01/2023 05:11, Bjorn Andersson wrote: >> From: Bjorn Andersson <bjorn.andersson@linaro.org> >> >> With the PMIC GLINK service, the host OS subscribes to USB-C altmode >> messages, which are sent by the firmware to notify the host OS about >> state updates and HPD interrupts. >> >> The pmic_glink_altmode driver registers for these notifications and >> propagates the notifications as typec_mux, typec_switch and DRM OOB >> notifications as necessary to implement DisplayPort altmode support. >> >> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> >> --- >> >> Changes since v1: >> - None >> >> Johan reported a NULL pointer dereference in >> drm_kms_helper_hotplug_event() for HPD event being reported while the >> MSM DRM driver is still being initalized, a separate fix has been sent >> in hope to remidy this race condition in the MSM driver. >> >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/pmic_glink_altmode.c | 477 ++++++++++++++++++++++++++ >> 2 files changed, 478 insertions(+) >> create mode 100644 drivers/soc/qcom/pmic_glink_altmode.c >> >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile >> index 29cccac472f3..f30552bf4da7 100644 >> --- a/drivers/soc/qcom/Makefile >> +++ b/drivers/soc/qcom/Makefile >> @@ -10,6 +10,7 @@ obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o >> obj-$(CONFIG_QCOM_OCMEM) += ocmem.o >> obj-$(CONFIG_QCOM_PDR_HELPERS) += pdr_interface.o >> obj-$(CONFIG_QCOM_PMIC_GLINK) += pmic_glink.o >> +obj-$(CONFIG_QCOM_PMIC_GLINK) += pmic_glink_altmode.o >> obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o >> qmi_helpers-y += qmi_encdec.o qmi_interface.o >> obj-$(CONFIG_QCOM_RAMP_CTRL) += ramp_controller.o >> diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c >> new file mode 100644 >> index 000000000000..8d2d563cb756 >> --- /dev/null >> +++ b/drivers/soc/qcom/pmic_glink_altmode.c >> @@ -0,0 +1,477 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved. >> + * Copyright (c) 2022, Linaro Ltd >> + */ >> +#include <linux/auxiliary_bus.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/mutex.h> >> +#include <linux/property.h> >> +#include <linux/soc/qcom/pdr.h> >> +#include <drm/drm_bridge.h> >> + >> +#include <linux/usb/typec_altmode.h> >> +#include <linux/usb/typec_dp.h> >> +#include <linux/usb/typec_mux.h> >> + >> +#include <linux/soc/qcom/pmic_glink.h> >> + >> +#define PMIC_GLINK_MAX_PORTS 2 >> + >> +#define USBC_SC8180X_NOTIFY_IND 0x13 >> +#define USBC_CMD_WRITE_REQ 0x15 >> +#define USBC_NOTIFY_IND 0x16 >> + >> +#define ALTMODE_PAN_EN 0x10 >> +#define ALTMODE_PAN_ACK 0x11 >> + >> +struct usbc_write_req { >> + struct pmic_glink_hdr hdr; >> + __le32 cmd; >> + __le32 arg; >> + __le32 reserved; >> +}; >> + >> +#define NOTIFY_PAYLOAD_SIZE 16 >> +struct usbc_notify { >> + struct pmic_glink_hdr hdr; >> + char payload[NOTIFY_PAYLOAD_SIZE]; >> + u32 reserved; >> +}; >> + >> +struct usbc_sc8180x_notify { >> + struct pmic_glink_hdr hdr; >> + __le32 notification; >> + __le32 reserved[2]; >> +}; >> + >> +enum pmic_glink_altmode_pin_assignment { >> + DPAM_HPD_OUT, >> + DPAM_HPD_A, >> + DPAM_HPD_B, >> + DPAM_HPD_C, >> + DPAM_HPD_D, >> + DPAM_HPD_E, >> + DPAM_HPD_F, >> +}; >> + >> +struct pmic_glink_altmode; >> + >> +#define work_to_altmode_port(w) container_of((w), struct pmic_glink_altmode_port, work) >> + >> +struct pmic_glink_altmode_port { >> + struct pmic_glink_altmode *altmode; >> + unsigned int index; >> + >> + struct typec_switch *typec_switch; >> + struct typec_mux *typec_mux; >> + struct typec_mux_state state; >> + struct typec_altmode dp_alt; >> + >> + struct work_struct work; >> + >> + struct drm_bridge bridge; >> + >> + enum typec_orientation orientation; >> + u16 svid; >> + u8 dp_data; >> + u8 mode; >> + u8 hpd_state; >> + u8 hpd_irq; >> +}; >> + >> +#define work_to_altmode(w) container_of((w), struct pmic_glink_altmode, enable_work) >> + >> +struct pmic_glink_altmode { >> + struct device *dev; >> + >> + unsigned int owner_id; >> + >> + /* To synchronize WRITE_REQ acks */ >> + struct mutex lock; >> + >> + struct completion pan_ack; >> + struct pmic_glink_client *client; >> + >> + struct work_struct enable_work; >> + >> + struct pmic_glink_altmode_port ports[PMIC_GLINK_MAX_PORTS]; >> +}; >> + >> +static int pmic_glink_altmode_request(struct pmic_glink_altmode *altmode, u32 cmd, u32 arg) >> +{ >> + struct usbc_write_req req = {}; >> + unsigned long left; >> + int ret; >> + >> + /* >> + * The USBC_CMD_WRITE_REQ ack doesn't identify the request, so wait for >> + * one ack at a time. >> + */ >> + mutex_lock(&altmode->lock); >> + >> + req.hdr.owner = cpu_to_le32(altmode->owner_id); >> + req.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP); >> + req.hdr.opcode = cpu_to_le32(USBC_CMD_WRITE_REQ); >> + req.cmd = cpu_to_le32(cmd); >> + req.arg = cpu_to_le32(arg); >> + >> + ret = pmic_glink_send(altmode->client, &req, sizeof(req)); >> + if (ret) { >> + dev_err(altmode->dev, "failed to send altmode request: %#x (%d)\n", cmd, ret); >> + goto out_unlock; >> + } >> + >> + left = wait_for_completion_timeout(&altmode->pan_ack, 5 * HZ); >> + if (!left) { >> + dev_err(altmode->dev, "timeout waiting for altmode request ack for: %#x\n", cmd); >> + ret = -ETIMEDOUT; >> + } >> + >> +out_unlock: >> + mutex_unlock(&altmode->lock); >> + return ret; >> +} >> + >> +static void pmic_glink_altmode_enable_dp(struct pmic_glink_altmode *altmode, >> + struct pmic_glink_altmode_port *port, >> + u8 mode, bool hpd_state, >> + bool hpd_irq) >> +{ >> + struct typec_displayport_data dp_data = {}; >> + int ret; >> + >> + dp_data.status = DP_STATUS_ENABLED; >> + if (hpd_state) >> + dp_data.status |= DP_STATUS_HPD_STATE; >> + if (hpd_irq) >> + dp_data.status |= DP_STATUS_IRQ_HPD; >> + dp_data.conf = DP_CONF_SET_PIN_ASSIGN(mode); >> + >> + port->state.alt = &port->dp_alt; >> + port->state.data = &dp_data; >> + port->state.mode = TYPEC_MODAL_STATE(mode); >> + >> + ret = typec_mux_set(port->typec_mux, &port->state); >> + if (ret) >> + dev_err(altmode->dev, "failed to switch mux to DP\n"); >> +} >> + >> +static void pmic_glink_altmode_enable_usb(struct pmic_glink_altmode *altmode, >> + struct pmic_glink_altmode_port *port) >> +{ >> + int ret; >> + >> + port->state.alt = NULL; >> + port->state.data = NULL; >> + port->state.mode = TYPEC_STATE_USB; >> + >> + ret = typec_mux_set(port->typec_mux, &port->state); >> + if (ret) >> + dev_err(altmode->dev, "failed to switch mux to USB\n"); >> +} >> + >> +static void pmic_glink_altmode_worker(struct work_struct *work) >> +{ >> + struct pmic_glink_altmode_port *alt_port = work_to_altmode_port(work); >> + struct pmic_glink_altmode *altmode = alt_port->altmode; >> + >> + typec_switch_set(alt_port->typec_switch, alt_port->orientation); >> + >> + if (alt_port->svid == USB_TYPEC_DP_SID) >> + pmic_glink_altmode_enable_dp(altmode, alt_port, alt_port->mode, >> + alt_port->hpd_state, alt_port->hpd_irq); >> + else >> + pmic_glink_altmode_enable_usb(altmode, alt_port); >> + >> + if (alt_port->hpd_state) >> + drm_bridge_hpd_notify(&alt_port->bridge, connector_status_connected); >> + else >> + drm_bridge_hpd_notify(&alt_port->bridge, connector_status_disconnected); >> + >> + pmic_glink_altmode_request(altmode, ALTMODE_PAN_ACK, alt_port->index); >> +}; >> + >> +static enum typec_orientation pmic_glink_altmode_orientation(unsigned int orientation) >> +{ >> + if (orientation == 0) >> + return TYPEC_ORIENTATION_NORMAL; >> + else if (orientation == 1) >> + return TYPEC_ORIENTATION_REVERSE; >> + else >> + return TYPEC_ORIENTATION_NONE; >> +} >> + >> +#define SC8180X_PORT_MASK 0x000000ff >> +#define SC8180X_ORIENTATION_MASK 0x0000ff00 >> +#define SC8180X_MUX_MASK 0x00ff0000 >> +#define SC8180X_MODE_MASK 0x3f000000 >> +#define SC8180X_HPD_STATE_MASK 0x40000000 >> +#define SC8180X_HPD_IRQ_MASK 0x80000000 >> + >> +static void pmic_glink_altmode_sc8180xp_notify(struct pmic_glink_altmode *altmode, >> + const void *data, size_t len) >> +{ >> + struct pmic_glink_altmode_port *alt_port; >> + const struct usbc_sc8180x_notify *msg; >> + u32 notification; >> + u8 orientation; >> + u8 hpd_state; >> + u8 hpd_irq; >> + u16 svid; >> + u8 port; >> + u8 mode; >> + u8 mux; >> + >> + if (len != sizeof(*msg)) { >> + dev_warn(altmode->dev, "invalid length of USBC_NOTIFY indication: %zd\n", len); >> + return; >> + } >> + >> + msg = data; >> + notification = le32_to_cpu(msg->notification); >> + port = FIELD_GET(SC8180X_PORT_MASK, notification); >> + orientation = FIELD_GET(SC8180X_ORIENTATION_MASK, notification); >> + mux = FIELD_GET(SC8180X_MUX_MASK, notification); >> + mode = FIELD_GET(SC8180X_MODE_MASK, notification); >> + hpd_state = FIELD_GET(SC8180X_HPD_STATE_MASK, notification); >> + hpd_irq = FIELD_GET(SC8180X_HPD_IRQ_MASK, notification); >> + >> + svid = mux == 2 ? USB_TYPEC_DP_SID : 0; >> + >> + if (!altmode->ports[port].altmode) { >> + dev_dbg(altmode->dev, "notification on undefined port %d\n", port); >> + return; >> + } >> + >> + alt_port = &altmode->ports[port]; >> + alt_port->orientation = pmic_glink_altmode_orientation(orientation); >> + alt_port->svid = mux == 2 ? USB_TYPEC_DP_SID : 0; >> + alt_port->mode = mode; >> + alt_port->hpd_state = hpd_state; >> + alt_port->hpd_irq = hpd_irq; >> + schedule_work(&alt_port->work); >> +} >> + >> +#define SC8280XP_DPAM_MASK 0x3f >> +#define SC8280XP_HPD_STATE_MASK BIT(6) >> +#define SC8280XP_HPD_IRQ_MASK BIT(7) >> + >> +static void pmic_glink_altmode_sc8280xp_notify(struct pmic_glink_altmode *altmode, >> + u16 svid, const void *data, size_t len) >> +{ >> + struct pmic_glink_altmode_port *alt_port; >> + const struct usbc_notify *notify; >> + u8 orientation; >> + u8 hpd_state; >> + u8 hpd_irq; >> + u8 mode; >> + u8 port; >> + >> + if (len != sizeof(*notify)) { >> + dev_warn(altmode->dev, "invalid length USBC_NOTIFY_IND: %zd\n", >> + len); >> + return; >> + } >> + >> + notify = data; >> + >> + port = notify->payload[0]; >> + orientation = notify->payload[1]; >> + mode = FIELD_GET(SC8280XP_DPAM_MASK, notify->payload[8]) - DPAM_HPD_A; >> + hpd_state = FIELD_GET(SC8280XP_HPD_STATE_MASK, notify->payload[8]); >> + hpd_irq = FIELD_GET(SC8280XP_HPD_IRQ_MASK, notify->payload[8]); >> + >> + if (!altmode->ports[port].altmode) { >> + dev_dbg(altmode->dev, "notification on undefined port %d\n", port); >> + return; >> + } >> + >> + alt_port = &altmode->ports[port]; >> + alt_port->orientation = pmic_glink_altmode_orientation(orientation); >> + alt_port->svid = svid; >> + alt_port->mode = mode; >> + alt_port->hpd_state = hpd_state; >> + alt_port->hpd_irq = hpd_irq; >> + schedule_work(&alt_port->work); >> +} >> + >> +static void pmic_glink_altmode_callback(const void *data, size_t len, void *priv) >> +{ >> + struct pmic_glink_altmode *altmode = priv; >> + const struct pmic_glink_hdr *hdr = data; >> + u16 opcode; >> + u16 svid; >> + >> + opcode = le32_to_cpu(hdr->opcode) & 0xff; >> + svid = le32_to_cpu(hdr->opcode) >> 16; >> + >> + switch (opcode) { >> + case USBC_CMD_WRITE_REQ: >> + complete(&altmode->pan_ack); >> + break; >> + case USBC_NOTIFY_IND: >> + pmic_glink_altmode_sc8280xp_notify(altmode, svid, data, len); >> + break; >> + case USBC_SC8180X_NOTIFY_IND: >> + pmic_glink_altmode_sc8180xp_notify(altmode, data, len); >> + break; >> + } >> +} >> + >> +static int pmic_glink_altmode_attach(struct drm_bridge *bridge, >> + enum drm_bridge_attach_flags flags) >> +{ >> + return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL; >> +} >> + >> +static const struct drm_bridge_funcs pmic_glink_altmode_bridge_funcs = { >> + .attach = pmic_glink_altmode_attach, >> +}; >> + >> +static void pmic_glink_altmode_put_mux(void *data) >> +{ >> + typec_mux_put(data); >> +} >> + >> +static void pmic_glink_altmode_put_switch(void *data) >> +{ >> + typec_switch_put(data); >> +} >> + >> +static void pmic_glink_altmode_enable_worker(struct work_struct *work) >> +{ >> + struct pmic_glink_altmode *altmode = work_to_altmode(work); >> + int ret; >> + >> + ret = pmic_glink_altmode_request(altmode, ALTMODE_PAN_EN, 0); >> + if (ret) >> + dev_err(altmode->dev, "failed to request altmode notifications\n"); >> +} >> + >> +static void pmic_glink_altmode_pdr_notify(void *priv, int state) >> +{ >> + struct pmic_glink_altmode *altmode = priv; >> + >> + if (state == SERVREG_SERVICE_STATE_UP) >> + schedule_work(&altmode->enable_work); >> +} >> + >> +static const struct of_device_id pmic_glink_altmode_of_quirks[] = { >> + { .compatible = "qcom,sc8180x-pmic-glink", .data = (void *)PMIC_GLINK_OWNER_USBC }, >> + {} >> +}; >> + >> +static int pmic_glink_altmode_probe(struct auxiliary_device *adev, >> + const struct auxiliary_device_id *id) >> +{ >> + struct pmic_glink_altmode_port *alt_port; >> + struct pmic_glink_altmode *altmode; >> + struct typec_altmode_desc mux_desc = {}; >> + const struct of_device_id *match; >> + struct fwnode_handle *fwnode; >> + struct device *dev = &adev->dev; >> + u32 port; >> + int ret; >> + >> + altmode = devm_kzalloc(dev, sizeof(*altmode), GFP_KERNEL); >> + if (!altmode) >> + return -ENOMEM; >> + >> + altmode->dev = dev; >> + >> + match = of_match_device(pmic_glink_altmode_of_quirks, dev->parent); >> + if (match) >> + altmode->owner_id = (unsigned long)match->data; >> + else >> + altmode->owner_id = PMIC_GLINK_OWNER_USBC_PAN; >> + >> + INIT_WORK(&altmode->enable_work, pmic_glink_altmode_enable_worker); >> + init_completion(&altmode->pan_ack); >> + mutex_init(&altmode->lock); >> + >> + device_for_each_child_node(dev, fwnode) { >> + ret = fwnode_property_read_u32(fwnode, "reg", &port); >> + if (ret < 0) { >> + dev_err(dev, "missing reg property of %pOFn\n", fwnode); >> + return ret; >> + } >> + >> + if (port >= ARRAY_SIZE(altmode->ports)) { >> + dev_warn(dev, "invalid connector number, ignoring\n"); >> + continue; >> + } >> + >> + if (altmode->ports[port].altmode) { >> + dev_err(dev, "multiple connector definition for port %u\n", port); >> + return -EINVAL; >> + } >> + >> + alt_port = &altmode->ports[port]; >> + alt_port->altmode = altmode; >> + alt_port->index = port; >> + INIT_WORK(&alt_port->work, pmic_glink_altmode_worker); >> + >> + alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs; >> + alt_port->bridge.of_node = to_of_node(fwnode); >> + alt_port->bridge.ops = DRM_BRIDGE_OP_HPD; >> + alt_port->bridge.type = DRM_MODE_CONNECTOR_USB; >> + >> + ret = devm_drm_bridge_add(dev, &alt_port->bridge); >> + if (ret) >> + return ret; > > In my testing, the design of a bridge in the altmode driver made all the probe very fragile, > meaning that any device that won't probe in the full usb--pmic-glink--display driver would > prevent the whole to actually probe. > > This is why drm_connector_oob_hotplug_event() was used in drivers/usb/typec/altmodes/displayport.c > and a similar design for cec where both attach to a device so there's no probe dependency. > > I think there's a possible simplification here by using : > of_drm_find_bridge() instead on the endpoint target to get the last bridge, > or we could probably use the same drm_connector_oob_hotplug_event() but in any way we should > add missing pieces in drm_bridge_connector and drm/msm/dp/dp_drm.c > > First in drm/msm/dp/dp_drm.c, the driver should add the of_node to the DP bridge so it can > be found from the dp controller node. > > Secondly, we could add a fwnode in drm_bridge like in drm_connector and in drm_bridge_connector_init() > we could set the connector fwnode to the last bridge fwnode + the connector oob event handler. > In this case the drm_connector_oob_hotplug_event() design should work. > > Anyway, I think the bindings is correct, so it's a matter of implementation to avoid delaying the > display driver probe until the pmic_glink probes entirely. While looking closely, I don't think we can design differently except moving the bridge code into a dummy bridge into gpu/drm/bridge, but this won't solve anything but make things even more complex. This design follows how generic display connector bridge is designed like in gpu/drm/bridge/display-connector.c so it's valid. So for the DRM Bridge part: Acked-by: Neil Armstrong <neil.armstrong@linaro.org> Neil > > Neil > >> + >> + alt_port->dp_alt.svid = USB_TYPEC_DP_SID; >> + alt_port->dp_alt.mode = USB_TYPEC_DP_MODE; >> + alt_port->dp_alt.active = 1; >> + >> + mux_desc.svid = USB_TYPEC_DP_SID; >> + mux_desc.mode = USB_TYPEC_DP_MODE; >> + alt_port->typec_mux = fwnode_typec_mux_get(fwnode, &mux_desc); >> + if (IS_ERR(alt_port->typec_mux)) >> + return dev_err_probe(dev, PTR_ERR(alt_port->typec_mux), >> + "failed to acquire mode-switch for port: %d\n", >> + port); >> + >> + ret = devm_add_action_or_reset(dev, pmic_glink_altmode_put_mux, >> + alt_port->typec_mux); >> + if (ret) >> + return ret; >> + >> + alt_port->typec_switch = fwnode_typec_switch_get(fwnode); >> + if (IS_ERR(alt_port->typec_switch)) >> + return dev_err_probe(dev, PTR_ERR(alt_port->typec_switch), >> + "failed to acquire orientation-switch for port: %d\n", >> + port); >> + >> + ret = devm_add_action_or_reset(dev, pmic_glink_altmode_put_switch, >> + alt_port->typec_switch); >> + if (ret) >> + return ret; >> + } >> + >> + altmode->client = devm_pmic_glink_register_client(dev, >> + altmode->owner_id, >> + pmic_glink_altmode_callback, >> + pmic_glink_altmode_pdr_notify, >> + altmode); >> + return PTR_ERR_OR_ZERO(altmode->client); >> +} >> + >> +static const struct auxiliary_device_id pmic_glink_altmode_id_table[] = { >> + { .name = "pmic_glink.altmode", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(auxiliary, pmic_glink_altmode_id_table); >> + >> +static struct auxiliary_driver pmic_glink_altmode_driver = { >> + .name = "pmic_glink_altmode", >> + .probe = pmic_glink_altmode_probe, >> + .id_table = pmic_glink_altmode_id_table, >> +}; >> + >> +module_auxiliary_driver(pmic_glink_altmode_driver); >> + >> +MODULE_DESCRIPTION("Qualcomm PMIC GLINK Altmode driver"); >> +MODULE_LICENSE("GPL"); >