Message ID | cover.1641288286.git.quic_schowdhu@quicinc.com |
---|---|
Headers | show |
Series | Add Embedded USB Debugger (EUD) driver | expand |
On Tue, 4 Jan 2022 at 15:00, Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote: > > Add the Embedded USB Debugger(EUD) device tree node. The > node contains EUD base register region and EUD mode > manager register regions along with the interrupt entry. > Also add the connector to EUD which is mapped as the child > of dwc3. The connector is attached to EUD via port. Also add > the role-switch property to dwc3 node. > > Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 53a21d0..2d14e5c 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -1315,6 +1315,18 @@ > phys = <&usb_2_hsphy>; > phy-names = "usb2-phy"; > maximum-speed = "high-speed"; > + usb-role-switch; > + usb_con: eud_usb_connector { Is this node meant to be named connector instead? > + compatible = "qcom,usb-connector-eud", > + "usb-c-connector"; > + ports { > + port@0 { > + usb2_role_switch: endpoint { > + remote-endpoint = <&eud_ep>; > + }; > + }; > + }; > + }; > }; > }; > > @@ -1339,6 +1351,19 @@ > interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>; > }; > > + eud: eud@88e0000 { > + compatible = "qcom,sc7280-eud","qcom,eud"; > + reg = <0 0x88e0000 0 0x2000>, > + <0 0x88e2000 0 0x1000>; > + interrupt-parent = <&pdc>; > + interrupts = <11 IRQ_TYPE_LEVEL_HIGH>; > + port { > + eud_ep: endpoint { > + remote-endpoint = <&usb2_role_switch>; > + }; > + }; > + }; > + > nsp_noc: interconnect@a0c0000 { > reg = <0 0x0a0c0000 0 0x10000>; > compatible = "qcom,sc7280-nsp-noc"; > -- > 2.7.4 >
On Tue 2022-01-04 17:28:20, Souradeep Chowdhury wrote: > Add the entry for maintainer for EUD driver > and other associated files. > > Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > --- > MAINTAINERS | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0227e15..54f616a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7018,6 +7018,14 @@ F: include/trace/events/mdio.h > F: include/uapi/linux/mdio.h > F: include/uapi/linux/mii.h > > +EUD-QCOM This should really explain the "EUD" and "QCOM". Something like EMBEDDED USB DEBUGGER (EUD) ? Is it Qualcomm specific? Best regards, Pavel
On Tue 04 Jan 03:58 PST 2022, Souradeep Chowdhury wrote: > Add the Embedded USB Debugger(EUD) device tree node. The > node contains EUD base register region and EUD mode > manager register regions along with the interrupt entry. > Also add the connector to EUD which is mapped as the child > of dwc3. The connector is attached to EUD via port. Also add > the role-switch property to dwc3 node. > > Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 53a21d0..2d14e5c 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -1315,6 +1315,18 @@ > phys = <&usb_2_hsphy>; > phy-names = "usb2-phy"; > maximum-speed = "high-speed"; > + usb-role-switch; > + usb_con: eud_usb_connector { I think this is backwards. Iiuc you have dwc3 <-> EUD <-> connector, where the connector typically would be represented in relation to a Type-C controller. So I think this graph should be looking something like: dwc3 { ports { port { dwc3_eud: endpoint { remote-endpoint = <&eud_ep>; }; }; }; }; eud { ports { port { eud_dwc3: endpoint { remote-endpoint = <&dwc3_eud>; }; }; port { eud_con: endpoint { remote-endpoint = <&con_eud>; }; }; }; }; connector { compatible = "usb-c-connector"; ports { port { con_eud: endpoint { remote-endpoint = <&eud_con>; }; }; }; }; With EUD "forwarding" the role switching requests from the connector, or overriding them if the EUD is enabled. Alternatively the dwc3 would be in a graph with both the eud and the connector, but I don't know how to ensure that the EUD decision takes precedence over the type-c controllers role request... In the case of us having a type-c controller, such as the pmic_glink, the connector node would then be a child of the controller. Regards, Bjorn > + compatible = "qcom,usb-connector-eud", > + "usb-c-connector"; > + ports { > + port@0 { > + usb2_role_switch: endpoint { > + remote-endpoint = <&eud_ep>; > + }; > + }; > + }; > + }; > }; > }; > > @@ -1339,6 +1351,19 @@ > interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>; > }; > > + eud: eud@88e0000 { > + compatible = "qcom,sc7280-eud","qcom,eud"; > + reg = <0 0x88e0000 0 0x2000>, > + <0 0x88e2000 0 0x1000>; > + interrupt-parent = <&pdc>; > + interrupts = <11 IRQ_TYPE_LEVEL_HIGH>; > + port { > + eud_ep: endpoint { > + remote-endpoint = <&usb2_role_switch>; > + }; > + }; > + }; > + > nsp_noc: interconnect@a0c0000 { > reg = <0 0x0a0c0000 0 0x10000>; > compatible = "qcom,sc7280-nsp-noc"; > -- > 2.7.4 >
On Tue 04 Jan 03:58 PST 2022, Souradeep Chowdhury wrote: > Add support for control peripheral of EUD (Embedded USB Debugger) to > listen to events such as USB attach/detach, pet EUD to indicate software > is functional.Reusing the platform device kobj, sysfs entry 'enable' is > created to enable or disable EUD. > > To enable the eud the following needs to be done > echo 1 > /sys/bus/platform/.../enable > > To disable eud, following is the command > echo 0 > /sys/bus/platform/.../enable > > Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> > --- > Documentation/ABI/testing/sysfs-driver-eud | 9 + > drivers/soc/qcom/Kconfig | 10 ++ > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/qcom_eud.c | 260 +++++++++++++++++++++++++++++ > 4 files changed, 280 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-driver-eud > create mode 100644 drivers/soc/qcom/qcom_eud.c > > diff --git a/Documentation/ABI/testing/sysfs-driver-eud b/Documentation/ABI/testing/sysfs-driver-eud > new file mode 100644 > index 0000000..2381552 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-driver-eud > @@ -0,0 +1,9 @@ > +What: /sys/bus/platform/drivers/eud/.../enable > +Date: January 2022 > +Contact: Souradeep Chowdhury <schowdhu@codeaurora.org> > +Description: > + The Enable/Disable sysfs interface for Embedded > + USB Debugger(EUD). This enables and disables the > + EUD based on a 1 or a 0 value. By enabling EUD, > + the user is able to activate the mini-usb hub of > + EUD for debug and trace capabilities. > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index 79b568f..a4db41b 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -42,6 +42,16 @@ config QCOM_CPR > To compile this driver as a module, choose M here: the module will > be called qcom-cpr > > +config QCOM_EUD > + tristate "QCOM Embedded USB Debugger(EUD) Driver" > + select USB_ROLE_SWITCH > + help > + This module enables support for Qualcomm Technologies, Inc. > + Embedded USB Debugger (EUD). The EUD is a control peripheral > + which reports VBUS attach/detach events and has USB-based > + debug and trace capabilities. On selecting m, the module name > + that is built is qcom_eud.ko > + > config QCOM_GENI_SE > tristate "QCOM GENI Serial Engine Driver" > depends on ARCH_QCOM || COMPILE_TEST > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index ad675a6..3331a40 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o > obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o > obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o > obj-$(CONFIG_QCOM_CPR) += cpr.o > +obj-$(CONFIG_QCOM_EUD) += qcom_eud.o > obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o > obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o > obj-$(CONFIG_QCOM_OCMEM) += ocmem.o > diff --git a/drivers/soc/qcom/qcom_eud.c b/drivers/soc/qcom/qcom_eud.c > new file mode 100644 > index 0000000..7c4cc7d > --- /dev/null > +++ b/drivers/soc/qcom/qcom_eud.c > @@ -0,0 +1,260 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/bitops.h> > +#include <linux/delay.h> Is this needed? > +#include <linux/err.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/iopoll.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/sysfs.h> > +#include <linux/usb/role.h> > + > +#define EUD_REG_INT1_EN_MASK 0x0024 > +#define EUD_REG_INT_STATUS_1 0x0044 > +#define EUD_REG_CTL_OUT_1 0x0074 > +#define EUD_REG_VBUS_INT_CLR 0x0080 > +#define EUD_REG_CSR_EUD_EN 0x1014 > +#define EUD_REG_SW_ATTACH_DET 0x1018 > +#define EUD_REG_EUD_EN2 0x0000 > + > +#define EUD_ENABLE BIT(0) > +#define EUD_INT_PET_EUD BIT(0) > +#define EUD_INT_VBUS BIT(2) > +#define EUD_INT_SAFE_MODE BIT(4) > +#define EUD_INT_ALL (EUD_INT_VBUS|EUD_INT_SAFE_MODE) > + > +struct eud_chip { > + struct device *dev; > + struct usb_role_switch *role_sw; > + void __iomem *eud_reg_base; This is unnecessarily long, "base" or "eud" seems sufficient. > + void __iomem *eud_mode_mgr2_phys_base; "mode_mgr" > + unsigned int int_status; > + int eud_irq; "irq" > + bool enable; "enable" is an action, but you use it to track if the state of the EUD, i.e if it's "enabled". > + bool usb_attach; "attach" vs "attached" > + Empty line. > +}; > + > +static int enable_eud(struct eud_chip *priv) > +{ > + writel(EUD_ENABLE, priv->eud_reg_base + EUD_REG_CSR_EUD_EN); > + writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE, > + priv->eud_reg_base + EUD_REG_INT1_EN_MASK); > + writel(1, priv->eud_mode_mgr2_phys_base + EUD_REG_EUD_EN2); > + > + return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); When this fails, don't you want to disable the EUD again? > +} > + > +static void disable_eud(struct eud_chip *priv) > +{ > + writel(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN); > + writel(0, priv->eud_mode_mgr2_phys_base + EUD_REG_EUD_EN2); > +} > + > +static ssize_t enable_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct eud_chip *chip = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%d\n", chip->enable); > +} > + > +static ssize_t enable_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct eud_chip *chip = dev_get_drvdata(dev); > + bool enable; > + int ret; > + > + if (kstrtobool(buf, &enable)) > + return -EINVAL; > + > + if (enable) { > + ret = enable_eud(chip); > + if (!ret) > + chip->enable = enable; > + } else { > + disable_eud(chip); > + } > + > + return count; > +} > + > +static DEVICE_ATTR_RW(enable); > + > +static struct attribute *eud_attrs[] = { > + &dev_attr_enable.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(eud); > + > +static void usb_attach_detach(struct eud_chip *chip) > +{ > + u32 reg; > + > + /* read ctl_out_1[4] to find USB attach or detach event */ > + reg = readl(chip->eud_reg_base + EUD_REG_CTL_OUT_1); > + if (reg & EUD_INT_SAFE_MODE) > + chip->usb_attach = true; > + else > + chip->usb_attach = false; chip->usb_attach = reg & EUD_INT_SAFE_MODE; > + > + /* set and clear vbus_int_clr[0] to clear interrupt */ > + writel(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR); > + writel(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR); How come you do this before waking up the threaded handler? > +} > + > +static void pet_eud(struct eud_chip *chip) > +{ > + u32 reg; > + int ret; > + > + /* read sw_attach_det[0] to find attach/detach event */ Please instead describe what this means, like: /* * When the EUD_INT_PET_EUD in SW_ATTACH_DET is set, the cable has been * disconnected and we need to detach the something because something. */ > + reg = readl(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET); Please drop the double space. > + if (reg & EUD_INT_PET_EUD) { > + /* Detach & Attach pet for EUD */ > + writel(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET); > + /* Delay to make sure detach pet is done before attach pet */ > + ret = readl_poll_timeout(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET, > + reg, (reg == 0), 1, 100); > + if (ret) { > + dev_err(chip->dev, "Detach pet failed\n"); > + return; > + } > + > + writel(EUD_INT_PET_EUD, chip->eud_reg_base + > + EUD_REG_SW_ATTACH_DET); Can you please help me see what differs between this and the write in the else statement? > + } else { > + /* Attach pet for EUD */ > + writel(EUD_INT_PET_EUD, chip->eud_reg_base + > + EUD_REG_SW_ATTACH_DET); > + } > +} > + > +static irqreturn_t handle_eud_irq(int irq, void *data) > +{ > + struct eud_chip *chip = data; > + u32 reg; > + > + /* read status register and find out which interrupt triggered */ That's what the next line says as well, please drop this comment. > + reg = readl(chip->eud_reg_base + EUD_REG_INT_STATUS_1); Please drop the double space. > + switch (reg & EUD_INT_ALL) { What if both of these bits are set? Won't you just end up with an interrupt storm until someone disables the EUD again? > + case EUD_INT_VBUS: > + chip->int_status = EUD_INT_VBUS; > + usb_attach_detach(chip); > + return IRQ_WAKE_THREAD; > + case EUD_INT_SAFE_MODE: > + pet_eud(chip); > + break; This is the only case where you break to return IRQ_HANDLED. Please be consistent. > + default: > + return IRQ_NONE; > + } > + return IRQ_HANDLED; > +} > + > +static irqreturn_t handle_eud_irq_thread(int irq, void *data) > +{ > + struct eud_chip *chip = data; > + int ret; > + > + if (chip->int_status == EUD_INT_VBUS) { I presume you have this here to signal that EUD_INT_VBUS was set in EUD_REG_INT_STATUS_1 in handle_eud_irq(). But 1) unless this was true you wouldn't have woken up the threaded handler and 2) you never clear int_status, so once you've received the EUD_INT_VBUS it seems to forever be true. > + if (chip->usb_attach) > + ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE); > + else > + ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST); > + if (ret) > + dev_err(chip->dev, "failed to set role switch\n"); > + } > + > + return IRQ_HANDLED; > +} > + > +static int eud_probe(struct platform_device *pdev) > +{ > + struct eud_chip *chip; > + struct fwnode_handle *fwnode = pdev->dev.fwnode, *ep, *connector; > + int ret; > + > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->dev = &pdev->dev; > + > + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > + if (!ep) > + return -ENODEV; > + > + connector = fwnode_graph_get_remote_port_parent(ep); > + fwnode_handle_put(ep); > + if (!connector) > + return -ENODEV; > + > + chip->role_sw = fwnode_usb_role_switch_get(connector); Can't you just call usb_role_switch_get(&pdev->dev) to find the "usb-role-switch", without having to traverse the fwnode_graph manually? > + if (IS_ERR(chip->role_sw)) { > + return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw), > + "failed to get role switch\n"); > + } Note that from now on you must usb_role_switch_put(chip->role_sw); > + > + chip->eud_reg_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(chip->eud_reg_base)) > + return PTR_ERR(chip->eud_reg_base); > + > + chip->eud_mode_mgr2_phys_base = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(chip->eud_mode_mgr2_phys_base)) > + return PTR_ERR(chip->eud_mode_mgr2_phys_base); > + > + chip->eud_irq = platform_get_irq(pdev, 0); > + ret = devm_request_threaded_irq(&pdev->dev, chip->eud_irq, handle_eud_irq, > + handle_eud_irq_thread, IRQF_ONESHOT, NULL, chip); > + if (ret) > + return dev_err_probe(chip->dev, ret, "failed to allocate irq\n"); > + > + enable_irq_wake(chip->eud_irq); > + > + platform_set_drvdata(pdev, chip); > + > + return 0; > +} > + > +static int eud_remove(struct platform_device *pdev) > +{ > + struct eud_chip *chip = platform_get_drvdata(pdev); > + > + if (chip->enable) > + disable_eud(chip); > + > + device_init_wakeup(&pdev->dev, false); > + disable_irq_wake(chip->eud_irq); > + > + return 0; > +} > + > +static const struct of_device_id eud_dt_match[] = { > + { .compatible = "qcom,sc7280-eud" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, eud_dt_match); > + > +static struct platform_driver eud_driver = { > + .probe = eud_probe, I would prefer if you didn't indent the assignments. > + .remove = eud_remove, > + .driver = { > + .name = "eud", "qcom_eud" please. Regards, Bjorn > + .dev_groups = eud_groups, > + .of_match_table = eud_dt_match, > + }, > +}; > +module_platform_driver(eud_driver); > + > +MODULE_DESCRIPTION("QTI EUD driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.7.4 >
On 1/7/2022 6:49 AM, Bjorn Andersson wrote: > On Tue 04 Jan 03:58 PST 2022, Souradeep Chowdhury wrote: > >> Add support for control peripheral of EUD (Embedded USB Debugger) to >> listen to events such as USB attach/detach, pet EUD to indicate software >> is functional.Reusing the platform device kobj, sysfs entry 'enable' is >> created to enable or disable EUD. >> >> To enable the eud the following needs to be done >> echo 1 > /sys/bus/platform/.../enable >> >> To disable eud, following is the command >> echo 0 > /sys/bus/platform/.../enable >> >> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> --- >> Documentation/ABI/testing/sysfs-driver-eud | 9 + >> drivers/soc/qcom/Kconfig | 10 ++ >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/qcom_eud.c | 260 +++++++++++++++++++++++++++++ >> 4 files changed, 280 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-driver-eud >> create mode 100644 drivers/soc/qcom/qcom_eud.c >> >> diff --git a/Documentation/ABI/testing/sysfs-driver-eud b/Documentation/ABI/testing/sysfs-driver-eud >> new file mode 100644 >> index 0000000..2381552 >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-driver-eud >> @@ -0,0 +1,9 @@ >> +What: /sys/bus/platform/drivers/eud/.../enable >> +Date: January 2022 >> +Contact: Souradeep Chowdhury <schowdhu@codeaurora.org> >> +Description: >> + The Enable/Disable sysfs interface for Embedded >> + USB Debugger(EUD). This enables and disables the >> + EUD based on a 1 or a 0 value. By enabling EUD, >> + the user is able to activate the mini-usb hub of >> + EUD for debug and trace capabilities. >> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig >> index 79b568f..a4db41b 100644 >> --- a/drivers/soc/qcom/Kconfig >> +++ b/drivers/soc/qcom/Kconfig >> @@ -42,6 +42,16 @@ config QCOM_CPR >> To compile this driver as a module, choose M here: the module will >> be called qcom-cpr >> >> +config QCOM_EUD >> + tristate "QCOM Embedded USB Debugger(EUD) Driver" >> + select USB_ROLE_SWITCH >> + help >> + This module enables support for Qualcomm Technologies, Inc. >> + Embedded USB Debugger (EUD). The EUD is a control peripheral >> + which reports VBUS attach/detach events and has USB-based >> + debug and trace capabilities. On selecting m, the module name >> + that is built is qcom_eud.ko >> + >> config QCOM_GENI_SE >> tristate "QCOM GENI Serial Engine Driver" >> depends on ARCH_QCOM || COMPILE_TEST >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile >> index ad675a6..3331a40 100644 >> --- a/drivers/soc/qcom/Makefile >> +++ b/drivers/soc/qcom/Makefile >> @@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o >> obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o >> obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o >> obj-$(CONFIG_QCOM_CPR) += cpr.o >> +obj-$(CONFIG_QCOM_EUD) += qcom_eud.o >> obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o >> obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o >> obj-$(CONFIG_QCOM_OCMEM) += ocmem.o >> diff --git a/drivers/soc/qcom/qcom_eud.c b/drivers/soc/qcom/qcom_eud.c >> new file mode 100644 >> index 0000000..7c4cc7d >> --- /dev/null >> +++ b/drivers/soc/qcom/qcom_eud.c >> @@ -0,0 +1,260 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <linux/bitops.h> >> +#include <linux/delay.h> > Is this needed? Ack. > >> +#include <linux/err.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/iopoll.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/sysfs.h> >> +#include <linux/usb/role.h> >> + >> +#define EUD_REG_INT1_EN_MASK 0x0024 >> +#define EUD_REG_INT_STATUS_1 0x0044 >> +#define EUD_REG_CTL_OUT_1 0x0074 >> +#define EUD_REG_VBUS_INT_CLR 0x0080 >> +#define EUD_REG_CSR_EUD_EN 0x1014 >> +#define EUD_REG_SW_ATTACH_DET 0x1018 >> +#define EUD_REG_EUD_EN2 0x0000 >> + >> +#define EUD_ENABLE BIT(0) >> +#define EUD_INT_PET_EUD BIT(0) >> +#define EUD_INT_VBUS BIT(2) >> +#define EUD_INT_SAFE_MODE BIT(4) >> +#define EUD_INT_ALL (EUD_INT_VBUS|EUD_INT_SAFE_MODE) >> + >> +struct eud_chip { >> + struct device *dev; >> + struct usb_role_switch *role_sw; >> + void __iomem *eud_reg_base; > This is unnecessarily long, "base" or "eud" seems sufficient. Ack > >> + void __iomem *eud_mode_mgr2_phys_base; > "mode_mgr" Ack > >> + unsigned int int_status; >> + int eud_irq; > "irq" Ack > >> + bool enable; > "enable" is an action, but you use it to track if the state of the EUD, > i.e if it's "enabled". Ack > >> + bool usb_attach; > "attach" vs "attached" Ack > >> + > Empty line. Ack > >> +}; >> + >> +static int enable_eud(struct eud_chip *priv) >> +{ >> + writel(EUD_ENABLE, priv->eud_reg_base + EUD_REG_CSR_EUD_EN); >> + writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE, >> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK); >> + writel(1, priv->eud_mode_mgr2_phys_base + EUD_REG_EUD_EN2); >> + >> + return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); > When this fails, don't you want to disable the EUD again? Ack > >> +} >> + >> +static void disable_eud(struct eud_chip *priv) >> +{ >> + writel(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN); >> + writel(0, priv->eud_mode_mgr2_phys_base + EUD_REG_EUD_EN2); >> +} >> + >> +static ssize_t enable_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct eud_chip *chip = dev_get_drvdata(dev); >> + >> + return sysfs_emit(buf, "%d\n", chip->enable); >> +} >> + >> +static ssize_t enable_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct eud_chip *chip = dev_get_drvdata(dev); >> + bool enable; >> + int ret; >> + >> + if (kstrtobool(buf, &enable)) >> + return -EINVAL; >> + >> + if (enable) { >> + ret = enable_eud(chip); >> + if (!ret) >> + chip->enable = enable; >> + } else { >> + disable_eud(chip); >> + } >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR_RW(enable); >> + >> +static struct attribute *eud_attrs[] = { >> + &dev_attr_enable.attr, >> + NULL, >> +}; >> +ATTRIBUTE_GROUPS(eud); >> + >> +static void usb_attach_detach(struct eud_chip *chip) >> +{ >> + u32 reg; >> + >> + /* read ctl_out_1[4] to find USB attach or detach event */ >> + reg = readl(chip->eud_reg_base + EUD_REG_CTL_OUT_1); >> + if (reg & EUD_INT_SAFE_MODE) >> + chip->usb_attach = true; >> + else >> + chip->usb_attach = false; > chip->usb_attach = reg & EUD_INT_SAFE_MODE; Ack >> + >> + /* set and clear vbus_int_clr[0] to clear interrupt */ >> + writel(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR); >> + writel(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR); > How come you do this before waking up the threaded handler? Ack > >> +} >> + >> +static void pet_eud(struct eud_chip *chip) >> +{ >> + u32 reg; >> + int ret; >> + >> + /* read sw_attach_det[0] to find attach/detach event */ > Please instead describe what this means, like: > > /* > * When the EUD_INT_PET_EUD in SW_ATTACH_DET is set, the cable has been > * disconnected and we need to detach the something because something. > */ Ack > >> + reg = readl(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET); > Please drop the double space. Ack > >> + if (reg & EUD_INT_PET_EUD) { >> + /* Detach & Attach pet for EUD */ >> + writel(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET); >> + /* Delay to make sure detach pet is done before attach pet */ >> + ret = readl_poll_timeout(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET, >> + reg, (reg == 0), 1, 100); >> + if (ret) { >> + dev_err(chip->dev, "Detach pet failed\n"); >> + return; >> + } >> + >> + writel(EUD_INT_PET_EUD, chip->eud_reg_base + >> + EUD_REG_SW_ATTACH_DET); > Can you please help me see what differs between this and the write in > the else statement? Ack. Will move this out of the if else statement. > >> + } else { >> + /* Attach pet for EUD */ >> + writel(EUD_INT_PET_EUD, chip->eud_reg_base + >> + EUD_REG_SW_ATTACH_DET); >> + } >> +} >> + >> +static irqreturn_t handle_eud_irq(int irq, void *data) >> +{ >> + struct eud_chip *chip = data; >> + u32 reg; >> + >> + /* read status register and find out which interrupt triggered */ > That's what the next line says as well, please drop this comment. Ack > >> + reg = readl(chip->eud_reg_base + EUD_REG_INT_STATUS_1); > Please drop the double space. Ack > >> + switch (reg & EUD_INT_ALL) { > What if both of these bits are set? Won't you just end up with an > interrupt storm until someone disables the EUD again? So EUD can either be in safe mode or vbus mode , both bits will not be set together. > >> + case EUD_INT_VBUS: >> + chip->int_status = EUD_INT_VBUS; >> + usb_attach_detach(chip); >> + return IRQ_WAKE_THREAD; >> + case EUD_INT_SAFE_MODE: >> + pet_eud(chip); >> + break; > This is the only case where you break to return IRQ_HANDLED. Please be > consistent. Ack > >> + default: >> + return IRQ_NONE; >> + } >> + return IRQ_HANDLED; >> +} >> + >> +static irqreturn_t handle_eud_irq_thread(int irq, void *data) >> +{ >> + struct eud_chip *chip = data; >> + int ret; >> + >> + if (chip->int_status == EUD_INT_VBUS) { > I presume you have this here to signal that EUD_INT_VBUS was set in > EUD_REG_INT_STATUS_1 in handle_eud_irq(). > > But 1) unless this was true you wouldn't have woken up the threaded > handler and 2) you never clear int_status, so once you've received the > EUD_INT_VBUS it seems to forever be true. Ack. Will be clearing the int bits once this is done. > >> + if (chip->usb_attach) >> + ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE); >> + else >> + ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST); >> + if (ret) >> + dev_err(chip->dev, "failed to set role switch\n"); >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int eud_probe(struct platform_device *pdev) >> +{ >> + struct eud_chip *chip; >> + struct fwnode_handle *fwnode = pdev->dev.fwnode, *ep, *connector; >> + int ret; >> + >> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); >> + if (!chip) >> + return -ENOMEM; >> + >> + chip->dev = &pdev->dev; >> + >> + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); >> + if (!ep) >> + return -ENODEV; >> + >> + connector = fwnode_graph_get_remote_port_parent(ep); >> + fwnode_handle_put(ep); >> + if (!connector) >> + return -ENODEV; >> + >> + chip->role_sw = fwnode_usb_role_switch_get(connector); > Can't you just call usb_role_switch_get(&pdev->dev) to find the > "usb-role-switch", without having to traverse the fwnode_graph manually? "usb_role_switch_get" finds the role_switch from the current or the parent node, however in this case the usb-role-switch property is present at the parent of the connector node which is attached to the eud node via port end point. > >> + if (IS_ERR(chip->role_sw)) { >> + return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw), >> + "failed to get role switch\n"); >> + } > Note that from now on you must usb_role_switch_put(chip->role_sw); Ack > >> + >> + chip->eud_reg_base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(chip->eud_reg_base)) >> + return PTR_ERR(chip->eud_reg_base); >> + >> + chip->eud_mode_mgr2_phys_base = devm_platform_ioremap_resource(pdev, 1); >> + if (IS_ERR(chip->eud_mode_mgr2_phys_base)) >> + return PTR_ERR(chip->eud_mode_mgr2_phys_base); >> + >> + chip->eud_irq = platform_get_irq(pdev, 0); >> + ret = devm_request_threaded_irq(&pdev->dev, chip->eud_irq, handle_eud_irq, >> + handle_eud_irq_thread, IRQF_ONESHOT, NULL, chip); >> + if (ret) >> + return dev_err_probe(chip->dev, ret, "failed to allocate irq\n"); >> + >> + enable_irq_wake(chip->eud_irq); >> + >> + platform_set_drvdata(pdev, chip); >> + >> + return 0; >> +} >> + >> +static int eud_remove(struct platform_device *pdev) >> +{ >> + struct eud_chip *chip = platform_get_drvdata(pdev); >> + >> + if (chip->enable) >> + disable_eud(chip); >> + >> + device_init_wakeup(&pdev->dev, false); >> + disable_irq_wake(chip->eud_irq); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id eud_dt_match[] = { >> + { .compatible = "qcom,sc7280-eud" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, eud_dt_match); >> + >> +static struct platform_driver eud_driver = { >> + .probe = eud_probe, > I would prefer if you didn't indent the assignments. Ack > >> + .remove = eud_remove, >> + .driver = { >> + .name = "eud", > "qcom_eud" please. Ack > > Regards, > Bjorn > >> + .dev_groups = eud_groups, >> + .of_match_table = eud_dt_match, >> + }, >> +}; >> +module_platform_driver(eud_driver); >> + >> +MODULE_DESCRIPTION("QTI EUD driver"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.7.4 >>
On 1/7/2022 6:23 AM, Bjorn Andersson wrote: > On Tue 04 Jan 03:58 PST 2022, Souradeep Chowdhury wrote: > >> Add the Embedded USB Debugger(EUD) device tree node. The >> node contains EUD base register region and EUD mode >> manager register regions along with the interrupt entry. >> Also add the connector to EUD which is mapped as the child >> of dwc3. The connector is attached to EUD via port. Also add >> the role-switch property to dwc3 node. >> >> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sc7280.dtsi | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> index 53a21d0..2d14e5c 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> @@ -1315,6 +1315,18 @@ >> phys = <&usb_2_hsphy>; >> phy-names = "usb2-phy"; >> maximum-speed = "high-speed"; >> + usb-role-switch; >> + usb_con: eud_usb_connector { > I think this is backwards. > > Iiuc you have dwc3 <-> EUD <-> connector, where the connector typically > would be represented in relation to a Type-C controller. > > So I think this graph should be looking something like: > > > dwc3 { > ports { > port { > dwc3_eud: endpoint { > remote-endpoint = <&eud_ep>; > }; > }; > }; > }; > > eud { > ports { > port { > eud_dwc3: endpoint { > remote-endpoint = <&dwc3_eud>; > }; > }; > > port { > eud_con: endpoint { > remote-endpoint = <&con_eud>; > }; > }; > }; > }; > > connector { > compatible = "usb-c-connector"; > > ports { > port { > con_eud: endpoint { > remote-endpoint = <&eud_con>; > }; > }; > }; > }; > > With EUD "forwarding" the role switching requests from the connector, or > overriding them if the EUD is enabled. > > Alternatively the dwc3 would be in a graph with both the eud and the > connector, but I don't know how to ensure that the EUD decision takes > precedence over the type-c controllers role request... > > > In the case of us having a type-c controller, such as the pmic_glink, > the connector node would then be a child of the controller. Ack. Will align the device tree with this structure. > > Regards, > Bjorn > >> + compatible = "qcom,usb-connector-eud", >> + "usb-c-connector"; >> + ports { >> + port@0 { >> + usb2_role_switch: endpoint { >> + remote-endpoint = <&eud_ep>; >> + }; >> + }; >> + }; >> + }; >> }; >> }; >> >> @@ -1339,6 +1351,19 @@ >> interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>; >> }; >> >> + eud: eud@88e0000 { >> + compatible = "qcom,sc7280-eud","qcom,eud"; >> + reg = <0 0x88e0000 0 0x2000>, >> + <0 0x88e2000 0 0x1000>; >> + interrupt-parent = <&pdc>; >> + interrupts = <11 IRQ_TYPE_LEVEL_HIGH>; >> + port { >> + eud_ep: endpoint { >> + remote-endpoint = <&usb2_role_switch>; >> + }; >> + }; >> + }; >> + >> nsp_noc: interconnect@a0c0000 { >> reg = <0 0x0a0c0000 0 0x10000>; >> compatible = "qcom,sc7280-nsp-noc"; >> -- >> 2.7.4 >>