This is a series of patches that implements a driver for the control
peripheral, EUD (Embedded USB Debugger). The EUD is a mini-USB hub
implemented on chip to support the USB-based debug and trace capabilities.
Apart from debug capabilities, EUD has a control peripheral. Control
Peripheral is on when EUD is on and gets signals like USB attach, pet EUD,
charge phone etc. EUD driver listens to events like USB attach or detach
and charger enable or disable and then notifies the USB driver or PMIC
driver respectively about these events via EXTCON. At regular intervals,
the EUD driver receives an interrupt to pet the driver indicating that
the software is functional.
Changes since v3:
- Remove power supply type check in the enable path of EUD
- Use default attribute group to create sysfs attribute
- Updated the dt-binding
Changes since v2:
- Remove module_param and add sysfs support instead
- Simplify if-else condition
- Change if-elseif to switch case
- Return -ENOMEM
- Got rid of unnecessary checks in sysfs store function
- Updated the dt-binding
Changes since v1:
- Remove EUD_NR as it is an unused macro
Changes since v0:
- Remove select SERIAL_CORE from Kconfig as this patch doesn't involve
anything related to serial console
- Changed the dt-bindings to remove extcon and replace it with graphs
to represent a connection with client
Avaneesh Kumar Dwivedi (2):
dt-bindings: Documentation for qcom,eud
Embedded USB Debugger (EUD) driver
Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
.../devicetree/bindings/soc/qcom/qcom,msm-eud.txt | 43 +++
drivers/soc/qcom/Kconfig | 12 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/eud.c | 329 +++++++++++++++++++++
5 files changed, 390 insertions(+)
create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
create mode 100644 drivers/soc/qcom/eud.c
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
Documentation for Embedded USB Debugger (EUD) device tree bindings.
Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
---
.../devicetree/bindings/soc/qcom/qcom,msm-eud.txt | 43 ++++++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
new file mode 100644
index 0000000..57476ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
@@ -0,0 +1,43 @@
+* Qualcomm Technologies Inc Embedded USB Debugger (EUD)
+
+The EUD (Embedded USB Debugger) is a mini-USB hub implemented
+on chip to support the USB-based debug and trace capabilities.
+
+Required properties:
+
+ - compatible: Should be "qcom,msm-eud"
+ - interrupts: Interrupt number
+ - reg: Should be address and size of EUD register space
+
+EUD notifies clients for VBUS attach/detach and charger enable/disable
+events. The link between event consumer(i.e.USB controller for vbus
+attach/detach event) and EUD is established via a directed graph. EUD
+act as an output link and clients of EUD as input link of this directed
+graph. Events flows through the directed graph only during debug mode.
+
+An example for EUD device node:
+
+ eud: qcom,msm-eud@88e0000 {
+ compatible = "qcom,msm-eud";
+ interrupts = <GIC_SPI 492 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x88e0000 0x4000>;
+ usb_con: connector {
+ compatible = "usb-c-connector";
+ label = "USB-C";
+ port {
+ eud_usb_output: endpoint {
+ remote-endpoint = <&eud_usb3_input>;
+ };
+ };
+ };
+ };
+
+An example for EUD client:
+
+ usb3 {
+ port {
+ eud_usb3_input: endpoint {
+ remote-endpoint = <&eud_usb_output>;
+ };
+ };
+ };
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
Add support for control peripheral of EUD (Embedded USB Debugger) to
listen to events such as USB attach/detach, charger enable/disable, pet
EUD to indicate software is functional. Reusing the platform device kobj,
sysfs entry 'enable' is created to enable or disable EUD.
Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
---
Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
drivers/soc/qcom/Kconfig | 12 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/eud.c | 329 ++++++++++++++++++++++++++
4 files changed, 347 insertions(+)
create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
create mode 100644 drivers/soc/qcom/eud.c
diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
new file mode 100644
index 0000000..d96ae05
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
@@ -0,0 +1,5 @@
+What: /sys/bus/platform/drivers/msm-eud/enable
+Date: Jan 2020
+Contact: Avaneesh Kumar Dwivedi <[email protected]>
+Description: Enable/Disable use of eud device.
+Users: User space debug application which intend to use EUD h/w block.
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index d0a73e7..6b7c9d0 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -202,4 +202,16 @@ config QCOM_APR
application processor and QDSP6. APR is
used by audio driver to configure QDSP6
ASM, ADM and AFE modules.
+
+config QCOM_EUD
+ tristate "QTI Embedded USB Debugger (EUD)"
+ depends on ARCH_QCOM
+ help
+ The Embedded USB Debugger (EUD) driver is a driver for the
+ control peripheral which waits on events like USB attach/detach
+ and charger enable/disable. The control peripheral further helps
+ support the USB-based debug and trace capabilities.
+ This module enables support for Qualcomm Technologies, Inc.
+ Embedded USB Debugger (EUD).
+ If unsure, say N.
endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 9fb35c8..c15be68 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -25,3 +25,4 @@ obj-$(CONFIG_QCOM_APR) += apr.o
obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
+obj-$(CONFIG_QCOM_EUD) += eud.o
diff --git a/drivers/soc/qcom/eud.c b/drivers/soc/qcom/eud.c
new file mode 100644
index 0000000..e6c3604
--- /dev/null
+++ b/drivers/soc/qcom/eud.c
@@ -0,0 +1,329 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/extcon.h>
+#include <linux/extcon-provider.h>
+#include <linux/delay.h>
+#include <linux/sysfs.h>
+#include <linux/io.h>
+#include <linux/bitops.h>
+#include <linux/workqueue.h>
+#include <linux/power_supply.h>
+
+#define EUD_ENABLE_CMD 1
+#define EUD_DISABLE_CMD 0
+
+#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_CHGR_INT_CLR 0x0084
+#define EUD_REG_CSR_EUD_EN 0x1014
+#define EUD_REG_SW_ATTACH_DET 0x1018
+
+#define EUD_INT_VBUS BIT(2)
+#define EUD_INT_CHGR BIT(3)
+#define EUD_INT_SAFE_MODE BIT(4)
+#define EUD_INT_ALL (EUD_INT_VBUS|EUD_INT_CHGR|\
+ EUD_INT_SAFE_MODE)
+
+struct eud_chip {
+ struct device *dev;
+ int eud_irq;
+ unsigned int extcon_id;
+ unsigned int int_status;
+ bool usb_attach;
+ bool chgr_enable;
+ void __iomem *eud_reg_base;
+ struct extcon_dev *extcon;
+ int enable;
+ struct work_struct eud_work;
+};
+
+static const unsigned int eud_extcon_cable[] = {
+ EXTCON_USB,
+ EXTCON_CHG_USB_SDP,
+ EXTCON_NONE,
+};
+
+static int enable_eud(struct eud_chip *priv)
+{
+ int ret;
+
+ /* write into CSR to enable EUD */
+ writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
+ /* Enable vbus, chgr & safe mode warning interrupts */
+ writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
+ priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
+
+ /* Ensure Register Writes Complete */
+ wmb();
+
+ /*
+ * Set the default cable state to usb connect and charger
+ * enable
+ */
+ ret = extcon_set_state_sync(priv->extcon, EXTCON_USB, true);
+ if (ret)
+ return ret;
+ ret = extcon_set_state_sync(priv->extcon,
+ EXTCON_CHG_USB_SDP, true);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void disable_eud(struct eud_chip *priv)
+{
+ /* write into CSR to disable EUD */
+ writel_relaxed(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
+}
+
+static ssize_t enable_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct eud_chip *chip = dev_get_drvdata(dev);
+
+ return snprintf(buf, sizeof(int), "%d", 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);
+ int enable = 0;
+ int ret = 0;
+
+ if (sscanf(buf, "%du", &enable) != 1)
+ return -EINVAL;
+
+ if (enable == EUD_ENABLE_CMD)
+ ret = enable_eud(chip);
+ else if (enable == EUD_DISABLE_CMD)
+ disable_eud(chip);
+ if (!ret)
+ chip->enable = enable;
+ return count;
+}
+
+static DEVICE_ATTR_RW(enable);
+
+static struct attribute *attrs[] = {
+ &dev_attr_enable.attr,
+ NULL
+};
+
+static struct attribute_group attr_group = {
+ .attrs = attrs,
+};
+
+static const struct attribute_group *attr_groups[] = {
+ &attr_group,
+ NULL
+};
+
+static void eud_event_notifier(struct work_struct *eud_work)
+{
+ struct eud_chip *chip = container_of(eud_work, struct eud_chip,
+ eud_work);
+ int ret;
+
+ if (chip->int_status == EUD_INT_VBUS) {
+ ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
+ chip->usb_attach);
+ if (ret)
+ return;
+ } else if (chip->int_status == EUD_INT_CHGR) {
+ ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
+ chip->chgr_enable);
+ if (ret)
+ return;
+ }
+}
+
+static void usb_attach_detach(struct eud_chip *chip)
+{
+ u32 reg;
+
+ chip->extcon_id = EXTCON_USB;
+ /* read ctl_out_1[4] to find USB attach or detach event */
+ reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
+ if (reg & BIT(4))
+ chip->usb_attach = true;
+ else
+ chip->usb_attach = false;
+
+ schedule_work(&chip->eud_work);
+
+ /* set and clear vbus_int_clr[0] to clear interrupt */
+ writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
+ /* Ensure Register Writes Complete */
+ wmb();
+ writel_relaxed(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
+}
+
+static void chgr_enable_disable(struct eud_chip *chip)
+{
+ u32 reg;
+
+ chip->extcon_id = EXTCON_CHG_USB_SDP;
+ /* read ctl_out_1[6] to find charger enable or disable event */
+ reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
+ if (reg & BIT(6))
+ chip->chgr_enable = true;
+ else
+ chip->chgr_enable = false;
+
+ schedule_work(&chip->eud_work);
+
+ /* set and clear chgr_int_clr[0] to clear interrupt */
+ writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
+ /* Ensure Register Writes Complete */
+ wmb();
+ writel_relaxed(0, chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
+}
+
+static void pet_eud(struct eud_chip *chip)
+{
+ u32 reg;
+
+ /* read sw_attach_det[0] to find attach/detach event */
+ reg = readl_relaxed(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
+ if (reg & BIT(0)) {
+ /* Detach & Attach pet for EUD */
+ writel_relaxed(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
+ /* Ensure Register Writes Complete */
+ wmb();
+ /* Delay to make sure detach pet is done before attach pet */
+ udelay(100);
+ writel_relaxed(BIT(0), chip->eud_reg_base +
+ EUD_REG_SW_ATTACH_DET);
+ /* Ensure Register Writes Complete */
+ wmb();
+ } else {
+ /* Attach pet for EUD */
+ writel_relaxed(BIT(0), chip->eud_reg_base +
+ EUD_REG_SW_ATTACH_DET);
+ /* Ensure Register Writes Complete */
+ wmb();
+ }
+}
+
+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 */
+ reg = readl_relaxed(chip->eud_reg_base + EUD_REG_INT_STATUS_1);
+ switch (reg & EUD_INT_ALL) {
+ case EUD_INT_VBUS:
+ chip->int_status = EUD_INT_VBUS;
+ usb_attach_detach(chip);
+ break;
+ case EUD_INT_CHGR:
+ chip->int_status = EUD_INT_CHGR;
+ chgr_enable_disable(chip);
+ break;
+ case EUD_INT_SAFE_MODE:
+ pet_eud(chip);
+ break;
+ default:
+ return IRQ_NONE;
+ }
+ return IRQ_HANDLED;
+}
+
+static int msm_eud_probe(struct platform_device *pdev)
+{
+ struct eud_chip *chip;
+ struct resource *res;
+ int ret;
+
+ chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->dev = &pdev->dev;
+ platform_set_drvdata(pdev, chip);
+
+ chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable);
+ if (IS_ERR(chip->extcon))
+ return PTR_ERR(chip->extcon);
+
+ ret = devm_extcon_dev_register(&pdev->dev, chip->extcon);
+ if (ret)
+ return ret;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENOMEM;
+
+ chip->eud_reg_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(chip->eud_reg_base))
+ return PTR_ERR(chip->eud_reg_base);
+
+ chip->eud_irq = platform_get_irq(pdev, 0);
+
+ ret = devm_request_irq(&pdev->dev, chip->eud_irq, handle_eud_irq,
+ IRQF_TRIGGER_HIGH, NULL, chip);
+ if (ret)
+ return ret;
+
+ device_init_wakeup(&pdev->dev, true);
+ enable_irq_wake(chip->eud_irq);
+
+ INIT_WORK(&chip->eud_work, eud_event_notifier);
+
+ if (ret)
+ return ret;
+
+ /* Enable EUD */
+ if (chip->enable)
+ enable_eud(chip);
+
+ return 0;
+}
+
+static int msm_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 msm_eud_dt_match[] = {
+ {.compatible = "qcom,msm-eud"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, msm_eud_dt_match);
+
+static struct platform_driver msm_eud_driver = {
+ .probe = msm_eud_probe,
+ .remove = msm_eud_remove,
+ .driver = {
+ .name = "msm-eud",
+ .groups = attr_groups,
+ .of_match_table = msm_eud_dt_match,
+ },
+};
+module_platform_driver(msm_eud_driver);
+
+MODULE_DESCRIPTION("QTI EUD driver");
+MODULE_LICENSE("GPL v2");
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
> Add support for control peripheral of EUD (Embedded USB Debugger) to
> listen to events such as USB attach/detach, charger enable/disable, pet
> EUD to indicate software is functional. Reusing the platform device kobj,
> sysfs entry 'enable' is created to enable or disable EUD.
>
> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
Either ensure Satya is the author, or add some Co-developed-by to
indicate that all three of you have authored the patch.
> ---
> Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
> drivers/soc/qcom/Kconfig | 12 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/eud.c | 329 ++++++++++++++++++++++++++
> 4 files changed, 347 insertions(+)
> create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
> create mode 100644 drivers/soc/qcom/eud.c
>
> diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
> new file mode 100644
> index 0000000..d96ae05
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
> @@ -0,0 +1,5 @@
> +What: /sys/bus/platform/drivers/msm-eud/enable
> +Date: Jan 2020
> +Contact: Avaneesh Kumar Dwivedi <[email protected]>
> +Description: Enable/Disable use of eud device.
> +Users: User space debug application which intend to use EUD h/w block.
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index d0a73e7..6b7c9d0 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -202,4 +202,16 @@ config QCOM_APR
> application processor and QDSP6. APR is
> used by audio driver to configure QDSP6
> ASM, ADM and AFE modules.
> +
> +config QCOM_EUD
Please aim for keeping the sort order in this file (ignore QCOM_APR
which obviously is in the wrong place)
> + tristate "QTI Embedded USB Debugger (EUD)"
> + depends on ARCH_QCOM
> + help
> + The Embedded USB Debugger (EUD) driver is a driver for the
> + control peripheral which waits on events like USB attach/detach
> + and charger enable/disable. The control peripheral further helps
> + support the USB-based debug and trace capabilities.
> + This module enables support for Qualcomm Technologies, Inc.
> + Embedded USB Debugger (EUD).
> + If unsure, say N.
> endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 9fb35c8..c15be68 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_QCOM_APR) += apr.o
> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> +obj-$(CONFIG_QCOM_EUD) += eud.o
> diff --git a/drivers/soc/qcom/eud.c b/drivers/soc/qcom/eud.c
> new file mode 100644
> index 0000000..e6c3604
> --- /dev/null
> +++ b/drivers/soc/qcom/eud.c
> @@ -0,0 +1,329 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/extcon.h>
> +#include <linux/extcon-provider.h>
> +#include <linux/delay.h>
> +#include <linux/sysfs.h>
> +#include <linux/io.h>
> +#include <linux/bitops.h>
> +#include <linux/workqueue.h>
> +#include <linux/power_supply.h>
Please sort these.
> +
> +#define EUD_ENABLE_CMD 1
> +#define EUD_DISABLE_CMD 0
These defines doesn't add much value.
> +
> +#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_CHGR_INT_CLR 0x0084
> +#define EUD_REG_CSR_EUD_EN 0x1014
> +#define EUD_REG_SW_ATTACH_DET 0x1018
> +
> +#define EUD_INT_VBUS BIT(2)
> +#define EUD_INT_CHGR BIT(3)
> +#define EUD_INT_SAFE_MODE BIT(4)
> +#define EUD_INT_ALL (EUD_INT_VBUS|EUD_INT_CHGR|\
> + EUD_INT_SAFE_MODE)
> +
> +struct eud_chip {
> + struct device *dev;
> + int eud_irq;
> + unsigned int extcon_id;
> + unsigned int int_status;
> + bool usb_attach;
> + bool chgr_enable;
> + void __iomem *eud_reg_base;
> + struct extcon_dev *extcon;
> + int enable;
> + struct work_struct eud_work;
> +};
> +
> +static const unsigned int eud_extcon_cable[] = {
> + EXTCON_USB,
> + EXTCON_CHG_USB_SDP,
> + EXTCON_NONE,
> +};
> +
> +static int enable_eud(struct eud_chip *priv)
> +{
> + int ret;
> +
> + /* write into CSR to enable EUD */
Make up a define for BIT(0) and the next line is self explanatory - i.e.
drop the comment..
> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
Don't use _relaxed version of writel/readl unless you have a really good
reason - and if so provide a comment to why this is.
> + /* Enable vbus, chgr & safe mode warning interrupts */
This just repeats exactly what can be read from the next line.
> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
> +
> + /* Ensure Register Writes Complete */
wmb() ensures ordering, it deosn't wait for the operation to complete,
if you need that readl() the register.
> + wmb();
> +
> + /*
> + * Set the default cable state to usb connect and charger
> + * enable
> + */
> + ret = extcon_set_state_sync(priv->extcon, EXTCON_USB, true);
> + if (ret)
> + return ret;
> + ret = extcon_set_state_sync(priv->extcon,
> + EXTCON_CHG_USB_SDP, true);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void disable_eud(struct eud_chip *priv)
> +{
> + /* write into CSR to disable EUD */
> + writel_relaxed(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
Use writel() and drop the comment.
> +}
> +
> +static ssize_t enable_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct eud_chip *chip = dev_get_drvdata(dev);
> +
> + return snprintf(buf, sizeof(int), "%d", chip->enable);
buf is not sizeof(int) big...Just do sprintf()...
> +}
> +
> +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);
> + int enable = 0;
You shouldn't need to initialize this as you're checking the return
value of sscanf().
> + int ret = 0;
> +
> + if (sscanf(buf, "%du", &enable) != 1)
> + return -EINVAL;
> +
> + if (enable == EUD_ENABLE_CMD)
> + ret = enable_eud(chip);
If ret is !0 you should probably return that, rather than count...
> + else if (enable == EUD_DISABLE_CMD)
> + disable_eud(chip);
> + if (!ret)
...and then you don't need this check, or initialize ret to 0 above.
> + chip->enable = enable;
So if I write 42 to "enable" nothing will change in the hardware, but
chip->enable will be 42...
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(enable);
> +
> +static struct attribute *attrs[] = {
> + &dev_attr_enable.attr,
> + NULL
> +};
> +
> +static struct attribute_group attr_group = {
> + .attrs = attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> + &attr_group,
> + NULL
> +};
> +
> +static void eud_event_notifier(struct work_struct *eud_work)
Why do you need a worker for this? Why not just use a threaded handler
and execute this directly in that context?
> +{
> + struct eud_chip *chip = container_of(eud_work, struct eud_chip,
> + eud_work);
> + int ret;
> +
> + if (chip->int_status == EUD_INT_VBUS) {
And if you just call this function from the handler, you don't need
chip->int_status to pass parameters between the handler and the worker.
> + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
> + chip->usb_attach);
> + if (ret)
> + return;
> + } else if (chip->int_status == EUD_INT_CHGR) {
> + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
> + chip->chgr_enable);
> + if (ret)
> + return;
> + }
> +}
> +
> +static void usb_attach_detach(struct eud_chip *chip)
> +{
> + u32 reg;
> +
> + chip->extcon_id = EXTCON_USB;
> + /* read ctl_out_1[4] to find USB attach or detach event */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
> + if (reg & BIT(4))
Give this bit a define
> + chip->usb_attach = true;
> + else
> + chip->usb_attach = false;
> +
> + schedule_work(&chip->eud_work);
> +
> + /* set and clear vbus_int_clr[0] to clear interrupt */
> + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
> + /* Ensure Register Writes Complete */
> + wmb();
Use writel() and you probably don't need the wmb() here.
> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
> +}
> +
> +static void chgr_enable_disable(struct eud_chip *chip)
> +{
> + u32 reg;
> +
> + chip->extcon_id = EXTCON_CHG_USB_SDP;
> + /* read ctl_out_1[6] to find charger enable or disable event */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
> + if (reg & BIT(6))
Again, this deserves a define
> + chip->chgr_enable = true;
> + else
> + chip->chgr_enable = false;
> +
> + schedule_work(&chip->eud_work);
> +
> + /* set and clear chgr_int_clr[0] to clear interrupt */
> + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
> + /* Ensure Register Writes Complete */
> + wmb();
> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
> +}
> +
> +static void pet_eud(struct eud_chip *chip)
> +{
> + u32 reg;
> +
> + /* read sw_attach_det[0] to find attach/detach event */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
> + if (reg & BIT(0)) {
define
> + /* Detach & Attach pet for EUD */
All comments in this driver relates to the very next line, but this
seems to document the next two writes - i.e. this seems to be a proper
comment.
> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
> + /* Ensure Register Writes Complete */
> + wmb();
> + /* Delay to make sure detach pet is done before attach pet */
> + udelay(100);
Better read back the value if you want to ensure the length of the delay
between the two writes.
> + writel_relaxed(BIT(0), chip->eud_reg_base +
> + EUD_REG_SW_ATTACH_DET);
> + /* Ensure Register Writes Complete */
> + wmb();
> + } else {
> + /* Attach pet for EUD */
> + writel_relaxed(BIT(0), chip->eud_reg_base +
> + EUD_REG_SW_ATTACH_DET);
> + /* Ensure Register Writes Complete */
> + wmb();
It will complete, if you need to wait for it to have completed read back
the value.
> + }
> +}
> +
> +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 */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_INT_STATUS_1);
> + switch (reg & EUD_INT_ALL) {
What is the expected outcome if for some reason more than one of these
bits are set?
> + case EUD_INT_VBUS:
> + chip->int_status = EUD_INT_VBUS;
> + usb_attach_detach(chip);
> + break;
> + case EUD_INT_CHGR:
> + chip->int_status = EUD_INT_CHGR;
> + chgr_enable_disable(chip);
> + break;
> + case EUD_INT_SAFE_MODE:
> + pet_eud(chip);
> + break;
> + default:
> + return IRQ_NONE;
> + }
> + return IRQ_HANDLED;
> +}
> +
> +static int msm_eud_probe(struct platform_device *pdev)
> +{
> + struct eud_chip *chip;
> + struct resource *res;
> + int ret;
> +
> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->dev = &pdev->dev;
> + platform_set_drvdata(pdev, chip);
> +
> + chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable);
Aren't we moving away from extcon in favor of the usb role switching
thing?
> + if (IS_ERR(chip->extcon))
> + return PTR_ERR(chip->extcon);
> +
> + ret = devm_extcon_dev_register(&pdev->dev, chip->extcon);
> + if (ret)
> + return ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENOMEM;
> +
> + chip->eud_reg_base = devm_ioremap_resource(&pdev->dev, res);
Use devm_platform_ioremap_resource() instead
> + if (IS_ERR(chip->eud_reg_base))
> + return PTR_ERR(chip->eud_reg_base);
> +
> + chip->eud_irq = platform_get_irq(pdev, 0);
> +
> + ret = devm_request_irq(&pdev->dev, chip->eud_irq, handle_eud_irq,
> + IRQF_TRIGGER_HIGH, NULL, chip);
Omit the irq trigger information here and let it come from devicetree.
> + if (ret)
> + return ret;
> +
> + device_init_wakeup(&pdev->dev, true);
> + enable_irq_wake(chip->eud_irq);
> +
> + INIT_WORK(&chip->eud_work, eud_event_notifier);
> +
> + if (ret)
Duplicate of the same check 8 lines up.
> + return ret;
> +
> + /* Enable EUD */
> + if (chip->enable)
I'm not seeing where this would have been written during probe.
> + enable_eud(chip);
> +
> + return 0;
> +}
> +
> +static int msm_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 msm_eud_dt_match[] = {
> + {.compatible = "qcom,msm-eud"},
Is this the one and only, past and future, version of the EUD hardware
block? Or do we need this compatible to be more specific?
Nit. Please add a space after { and before }
Regards,
Bjorn
On 31/01/2020 04:43, Avaneesh Kumar Dwivedi wrote:
> Documentation for Embedded USB Debugger (EUD) device tree bindings.
>
> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
> ---
> .../devicetree/bindings/soc/qcom/qcom,msm-eud.txt | 43 ++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
> new file mode 100644
> index 0000000..57476ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
> @@ -0,0 +1,43 @@
> +* Qualcomm Technologies Inc Embedded USB Debugger (EUD)
> +
> +The EUD (Embedded USB Debugger) is a mini-USB hub implemented
> +on chip to support the USB-based debug and trace capabilities.
on chip to support USB-based debug and trace capabilities.
> +
> +Required properties:
> +
> + - compatible: Should be "qcom,msm-eud"
> + - interrupts: Interrupt number
> + - reg: Should be address and size of EUD register space
> +
> +EUD notifies clients for VBUS attach/detach and charger enable/disable
> +events. The link between event consumer(i.e.USB controller for vbus
missing space
consumer (i.e.
> +attach/detach event) and EUD is established via a directed graph. EUD
> +act
The EUD acts
> as an output link and clients of EUD as input link of this directed
> +graph. Events flows through the directed graph only during debug mode.
Probably this is a very obvious question but, you mean debug and trace
events or do you mean VBUS/charger events?
> +
> +An example for EUD device node:
> +
> + eud: qcom,msm-eud@88e0000 {
> + compatible = "qcom,msm-eud";
> + interrupts = <GIC_SPI 492 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0x88e0000 0x4000>;
> + usb_con: connector {
> + compatible = "usb-c-connector";
> + label = "USB-C";
> + port {
> + eud_usb_output: endpoint {
> + remote-endpoint = <&eud_usb3_input>;
> + };
> + };
> + };
> + };
> +
> +An example for EUD client:
> +
> + usb3 {
> + port {
> + eud_usb3_input: endpoint {
> + remote-endpoint = <&eud_usb_output>;
> + };
> + };
> + };
>
On 03/02/2020 19:35, Bjorn Andersson wrote:
> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
Hi Avaneesh.
> Please aim for keeping the sort order in this file (ignore QCOM_APR
> which obviously is in the wrong place)
>
>> + tristate "QTI Embedded USB Debugger (EUD)"
>> + depends on ARCH_QCOM
If we persist with the model of EXTCON you should "select EXTCON" here.
>> + help
>> + The Embedded USB Debugger (EUD) driver is a driver for the
>> + control peripheral which waits on events like USB attach/detach
>> + and charger enable/disable. The control peripheral further helps
>> + support the USB-based debug and trace capabilities.
>> + This module enables support for Qualcomm Technologies, Inc.
>> + Embedded USB Debugger (EUD).
Suggest.
This module enables support for Qualcomm Technologies, Inc.
Embedded USB Debugger (EUD).
The EUD is a control peripheral which reports VBUS attach/detach,
charger enable/disable and USB-based debug and trace capabilities.
>> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
2020
>> +
>> +static int enable_eud(struct eud_chip *priv)
>> +{
>> + int ret;
>> +
>> + /* write into CSR to enable EUD */
>> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
>> + /* Enable vbus, chgr & safe mode warning interrupts */
>> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
>> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
>> +
>> + /* Ensure Register Writes Complete */
So... You are writing a register in an on-chip PMIC. The PMIC is
responsible for detecting USB ID and supplying VBUS as appropriate.
You then get an interrupt to inform you of the state ?
>> +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);
>> + int enable = 0;
>
> You shouldn't need to initialize this as you're checking the return
> value of sscanf().
>
>> + int ret = 0;
>> +
>> + if (sscanf(buf, "%du", &enable) != 1)
>> + return -EINVAL;
>> +
>> + if (enable == EUD_ENABLE_CMD)
>> + ret = enable_eud(chip);
>
> If ret is !0 you should probably return that, rather than count...
>
>> + else if (enable == EUD_DISABLE_CMD)
>> + disable_eud(chip);
>> + if (!ret)
>
> ...and then you don't need this check, or initialize ret to 0 above.
>
>> + chip->enable = enable;
>
> So if I write 42 to "enable" nothing will change in the hardware, but
> chip->enable will be 42...
>
>> + return count;
>> +}
I was just going to comment on usb_connector but, does the above code
need a synchronization primitive to serialize with the worker and
interrupt handler ?
>> +static int msm_eud_probe(struct platform_device *pdev)
>> +{
>> + struct eud_chip *chip;
>> + struct resource *res;
>> + int ret;
>> +
>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> + if (!chip)
>> + return -ENOMEM;
>> +
>> + chip->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, chip);
>> +
>> + chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable);
>
> Aren't we moving away from extcon in favor of the usb role switching
> thing?
Yes.
For the VBUS notification you could use
usb-role-switch and model the USB connector as a child-node of the
dual-role controller.
See:
https://patchwork.kernel.org/cover/11346247/
https://patchwork.kernel.org/patch/11346295/
https://patchwork.kernel.org/patch/11346263/
Avaneesh do you have any kernel code that cares about the charger state ?
What we are suggesting here is dropping extcon and using role-switching
but, if you have some other code that cares about EXTCON_CHG_USB_SDP
you'd have to do additional work.
But, if I understood the implication of the code above where you write
to the PMIC and let it handle VBUS/CHARGER on/off and you are just
notified of the state change, you should be fine with usb-role-switching.
---
bod
On 31/01/2020 04:43, Avaneesh Kumar Dwivedi wrote:
> Documentation for Embedded USB Debugger (EUD) device tree bindings.
>
> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
> ---
> .../devicetree/bindings/soc/qcom/qcom,msm-eud.txt | 43 ++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
Forgot to mention, this file should be described in YAML.
./Documentation/devicetree/writing-schema.rst
./Documentation/devicetree/bindings/example-schema.yaml
---
bod
On Fri, Jan 31, 2020 at 10:13:31AM +0530, Avaneesh Kumar Dwivedi wrote:
> Add support for control peripheral of EUD (Embedded USB Debugger) to
> listen to events such as USB attach/detach, charger enable/disable, pet
> EUD to indicate software is functional. Reusing the platform device kobj,
> sysfs entry 'enable' is created to enable or disable EUD.
>
> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
> ---
> Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
> drivers/soc/qcom/Kconfig | 12 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/eud.c | 329 ++++++++++++++++++++++++++
> 4 files changed, 347 insertions(+)
> create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
> create mode 100644 drivers/soc/qcom/eud.c
>
> diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
> new file mode 100644
> index 0000000..d96ae05
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
> @@ -0,0 +1,5 @@
> +What: /sys/bus/platform/drivers/msm-eud/enable
> +Date: Jan 2020
> +Contact: Avaneesh Kumar Dwivedi <[email protected]>
> +Description: Enable/Disable use of eud device.
What are valid values to be used here?
> +Users: User space debug application which intend to use EUD h/w block.
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index d0a73e7..6b7c9d0 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -202,4 +202,16 @@ config QCOM_APR
> application processor and QDSP6. APR is
> used by audio driver to configure QDSP6
> ASM, ADM and AFE modules.
> +
> +config QCOM_EUD
> + tristate "QTI Embedded USB Debugger (EUD)"
> + depends on ARCH_QCOM
Why not let everyone test build this?
> + help
> + The Embedded USB Debugger (EUD) driver is a driver for the
> + control peripheral which waits on events like USB attach/detach
> + and charger enable/disable. The control peripheral further helps
> + support the USB-based debug and trace capabilities.
> + This module enables support for Qualcomm Technologies, Inc.
> + Embedded USB Debugger (EUD).
> + If unsure, say N.
> endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 9fb35c8..c15be68 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_QCOM_APR) += apr.o
> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> +obj-$(CONFIG_QCOM_EUD) += eud.o
> diff --git a/drivers/soc/qcom/eud.c b/drivers/soc/qcom/eud.c
> new file mode 100644
> index 0000000..e6c3604
> --- /dev/null
> +++ b/drivers/soc/qcom/eud.c
> @@ -0,0 +1,329 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/extcon.h>
> +#include <linux/extcon-provider.h>
> +#include <linux/delay.h>
> +#include <linux/sysfs.h>
> +#include <linux/io.h>
> +#include <linux/bitops.h>
> +#include <linux/workqueue.h>
> +#include <linux/power_supply.h>
> +
> +#define EUD_ENABLE_CMD 1
> +#define EUD_DISABLE_CMD 0
Don't need these.
> +
> +#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_CHGR_INT_CLR 0x0084
> +#define EUD_REG_CSR_EUD_EN 0x1014
> +#define EUD_REG_SW_ATTACH_DET 0x1018
> +
> +#define EUD_INT_VBUS BIT(2)
> +#define EUD_INT_CHGR BIT(3)
> +#define EUD_INT_SAFE_MODE BIT(4)
> +#define EUD_INT_ALL (EUD_INT_VBUS|EUD_INT_CHGR|\
> + EUD_INT_SAFE_MODE)
> +
> +struct eud_chip {
> + struct device *dev;
> + int eud_irq;
> + unsigned int extcon_id;
> + unsigned int int_status;
> + bool usb_attach;
> + bool chgr_enable;
> + void __iomem *eud_reg_base;
> + struct extcon_dev *extcon;
> + int enable;
> + struct work_struct eud_work;
> +};
> +
> +static const unsigned int eud_extcon_cable[] = {
> + EXTCON_USB,
> + EXTCON_CHG_USB_SDP,
> + EXTCON_NONE,
> +};
> +
> +static int enable_eud(struct eud_chip *priv)
> +{
> + int ret;
> +
> + /* write into CSR to enable EUD */
> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
> + /* Enable vbus, chgr & safe mode warning interrupts */
> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
> +
> + /* Ensure Register Writes Complete */
> + wmb();
> +
> + /*
> + * Set the default cable state to usb connect and charger
> + * enable
> + */
> + ret = extcon_set_state_sync(priv->extcon, EXTCON_USB, true);
> + if (ret)
> + return ret;
> + ret = extcon_set_state_sync(priv->extcon,
> + EXTCON_CHG_USB_SDP, true);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void disable_eud(struct eud_chip *priv)
> +{
> + /* write into CSR to disable EUD */
> + writel_relaxed(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
> +}
> +
> +static ssize_t enable_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct eud_chip *chip = dev_get_drvdata(dev);
> +
> + return snprintf(buf, sizeof(int), "%d", 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);
> + int enable = 0;
> + int ret = 0;
> +
> + if (sscanf(buf, "%du", &enable) != 1)
> + return -EINVAL;
No, use the built-in kernel function to handle reading y/n/Y/N/0/1 from
sysfs files, do not try to roll your own. As you have seen, you will
get it wrong :)
> +
> + if (enable == EUD_ENABLE_CMD)
> + ret = enable_eud(chip);
> + else if (enable == EUD_DISABLE_CMD)
> + disable_eud(chip);
> + if (!ret)
> + chip->enable = enable;
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(enable);
> +
> +static struct attribute *attrs[] = {
> + &dev_attr_enable.attr,
> + NULL
> +};
> +
> +static struct attribute_group attr_group = {
> + .attrs = attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> + &attr_group,
> + NULL
> +};
ATTRIBUTE_GROUPS()?
> +
> +static void eud_event_notifier(struct work_struct *eud_work)
> +{
> + struct eud_chip *chip = container_of(eud_work, struct eud_chip,
> + eud_work);
> + int ret;
> +
> + if (chip->int_status == EUD_INT_VBUS) {
> + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
> + chip->usb_attach);
> + if (ret)
> + return;
> + } else if (chip->int_status == EUD_INT_CHGR) {
> + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
> + chip->chgr_enable);
> + if (ret)
> + return;
> + }
> +}
> +
> +static void usb_attach_detach(struct eud_chip *chip)
> +{
> + u32 reg;
> +
> + chip->extcon_id = EXTCON_USB;
> + /* read ctl_out_1[4] to find USB attach or detach event */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
> + if (reg & BIT(4))
> + chip->usb_attach = true;
> + else
> + chip->usb_attach = false;
> +
> + schedule_work(&chip->eud_work);
> +
> + /* set and clear vbus_int_clr[0] to clear interrupt */
> + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
> + /* Ensure Register Writes Complete */
> + wmb();
> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
> +}
> +
> +static void chgr_enable_disable(struct eud_chip *chip)
> +{
> + u32 reg;
> +
> + chip->extcon_id = EXTCON_CHG_USB_SDP;
> + /* read ctl_out_1[6] to find charger enable or disable event */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
> + if (reg & BIT(6))
> + chip->chgr_enable = true;
> + else
> + chip->chgr_enable = false;
> +
> + schedule_work(&chip->eud_work);
> +
> + /* set and clear chgr_int_clr[0] to clear interrupt */
> + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
> + /* Ensure Register Writes Complete */
> + wmb();
> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
> +}
> +
> +static void pet_eud(struct eud_chip *chip)
> +{
> + u32 reg;
> +
> + /* read sw_attach_det[0] to find attach/detach event */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
> + if (reg & BIT(0)) {
> + /* Detach & Attach pet for EUD */
> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
> + /* Ensure Register Writes Complete */
> + wmb();
> + /* Delay to make sure detach pet is done before attach pet */
> + udelay(100);
> + writel_relaxed(BIT(0), chip->eud_reg_base +
> + EUD_REG_SW_ATTACH_DET);
> + /* Ensure Register Writes Complete */
> + wmb();
> + } else {
> + /* Attach pet for EUD */
> + writel_relaxed(BIT(0), chip->eud_reg_base +
> + EUD_REG_SW_ATTACH_DET);
> + /* Ensure Register Writes Complete */
> + wmb();
> + }
> +}
> +
> +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 */
> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_INT_STATUS_1);
> + switch (reg & EUD_INT_ALL) {
> + case EUD_INT_VBUS:
> + chip->int_status = EUD_INT_VBUS;
> + usb_attach_detach(chip);
> + break;
> + case EUD_INT_CHGR:
> + chip->int_status = EUD_INT_CHGR;
> + chgr_enable_disable(chip);
> + break;
> + case EUD_INT_SAFE_MODE:
> + pet_eud(chip);
> + break;
> + default:
> + return IRQ_NONE;
> + }
> + return IRQ_HANDLED;
> +}
> +
> +static int msm_eud_probe(struct platform_device *pdev)
> +{
> + struct eud_chip *chip;
> + struct resource *res;
> + int ret;
> +
> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->dev = &pdev->dev;
No reference counting???
thanks,
greg k-h
Thank you very much Bryan for your review comments, will be working on
your and other maintainers comment on this patchset and will be posting
new patchset soon.
On 2/4/2020 8:21 AM, Bryan O'Donoghue wrote:
> On 31/01/2020 04:43, Avaneesh Kumar Dwivedi wrote:
>> Documentation for Embedded USB Debugger (EUD) device tree bindings.
>>
>> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
>> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
>> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
>> ---
>> .../devicetree/bindings/soc/qcom/qcom,msm-eud.txt | 43
>> ++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
>> b/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
>> new file mode 100644
>> index 0000000..57476ce
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
>> @@ -0,0 +1,43 @@
>> +* Qualcomm Technologies Inc Embedded USB Debugger (EUD)
>> +
>> +The EUD (Embedded USB Debugger) is a mini-USB hub implemented
>> +on chip to support the USB-based debug and trace capabilities.
>
> on chip to support USB-based debug and trace capabilities.
OK
>
>> +
>> +Required properties:
>> +
>> + - compatible: Should be "qcom,msm-eud"
>> + - interrupts: Interrupt number
>> + - reg: Should be address and size of EUD register space
>> +
>> +EUD notifies clients for VBUS attach/detach and charger enable/disable
>> +events. The link between event consumer(i.e.USB controller for vbus
> missing space
> consumer (i.e.
Ok
>
>> +attach/detach event) and EUD is established via a directed graph. EUD
>> +act
> The EUD acts
OK
>
>> as an output link and clients of EUD as input link of this directed
>> +graph. Events flows through the directed graph only during debug mode.
>
> Probably this is a very obvious question but, you mean debug and trace
> events or do you mean VBUS/charger events?
Directed graph is to show VBUS and charger events flow.
>
>> +
>> +An example for EUD device node:
>> +
>> + eud: qcom,msm-eud@88e0000 {
>> + compatible = "qcom,msm-eud";
>> + interrupts = <GIC_SPI 492 IRQ_TYPE_LEVEL_HIGH>;
>> + reg = <0x88e0000 0x4000>;
>> + usb_con: connector {
>> + compatible = "usb-c-connector";
>> + label = "USB-C";
>> + port {
>> + eud_usb_output: endpoint {
>> + remote-endpoint = <&eud_usb3_input>;
>> + };
>> + };
>> + };
>> + };
>> +
>> +An example for EUD client:
>> +
>> + usb3 {
>> + port {
>> + eud_usb3_input: endpoint {
>> + remote-endpoint = <&eud_usb_output>;
>> + };
>> + };
>> + };
>>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On 2/4/2020 8:47 AM, Bryan O'Donoghue wrote:
> On 31/01/2020 04:43, Avaneesh Kumar Dwivedi wrote:
>> Documentation for Embedded USB Debugger (EUD) device tree bindings.
>>
>> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
>> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
>> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
>> ---
>> .../devicetree/bindings/soc/qcom/qcom,msm-eud.txt | 43
>> ++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
>
> Forgot to mention, this file should be described in YAML.
Ok
>
> ./Documentation/devicetree/writing-schema.rst
> ./Documentation/devicetree/bindings/example-schema.yaml
>
> ---
> bod
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
Thank you very much Bjorn for your comments, will address them and post
latest patchset soon.
On 2/4/2020 1:05 AM, Bjorn Andersson wrote:
> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
>
>> Add support for control peripheral of EUD (Embedded USB Debugger) to
>> listen to events such as USB attach/detach, charger enable/disable, pet
>> EUD to indicate software is functional. Reusing the platform device kobj,
>> sysfs entry 'enable' is created to enable or disable EUD.
>>
>> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
>> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
>> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
> Either ensure Satya is the author, or add some Co-developed-by to
> indicate that all three of you have authored the patch.
Will add Co-developed-by.
>
>> ---
>> Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
>> drivers/soc/qcom/Kconfig | 12 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/eud.c | 329 ++++++++++++++++++++++++++
>> 4 files changed, 347 insertions(+)
>> create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
>> create mode 100644 drivers/soc/qcom/eud.c
>>
>> diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
>> new file mode 100644
>> index 0000000..d96ae05
>> --- /dev/null
>> +++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
>> @@ -0,0 +1,5 @@
>> +What: /sys/bus/platform/drivers/msm-eud/enable
>> +Date: Jan 2020
>> +Contact: Avaneesh Kumar Dwivedi <[email protected]>
>> +Description: Enable/Disable use of eud device.
>> +Users: User space debug application which intend to use EUD h/w block.
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index d0a73e7..6b7c9d0 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -202,4 +202,16 @@ config QCOM_APR
>> application processor and QDSP6. APR is
>> used by audio driver to configure QDSP6
>> ASM, ADM and AFE modules.
>> +
>> +config QCOM_EUD
> Please aim for keeping the sort order in this file (ignore QCOM_APR
> which obviously is in the wrong place)
Please help to elaborate more, do you mean adding configs in
alphabetical order?
>
>> + tristate "QTI Embedded USB Debugger (EUD)"
>> + depends on ARCH_QCOM
>> + help
>> + The Embedded USB Debugger (EUD) driver is a driver for the
>> + control peripheral which waits on events like USB attach/detach
>> + and charger enable/disable. The control peripheral further helps
>> + support the USB-based debug and trace capabilities.
>> + This module enables support for Qualcomm Technologies, Inc.
>> + Embedded USB Debugger (EUD).
>> + If unsure, say N.
>> endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 9fb35c8..c15be68 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -25,3 +25,4 @@ obj-$(CONFIG_QCOM_APR) += apr.o
>> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
>> obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>> obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>> +obj-$(CONFIG_QCOM_EUD) += eud.o
>> diff --git a/drivers/soc/qcom/eud.c b/drivers/soc/qcom/eud.c
>> new file mode 100644
>> index 0000000..e6c3604
>> --- /dev/null
>> +++ b/drivers/soc/qcom/eud.c
>> @@ -0,0 +1,329 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/extcon.h>
>> +#include <linux/extcon-provider.h>
>> +#include <linux/delay.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/io.h>
>> +#include <linux/bitops.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/power_supply.h>
> Please sort these.
OK
>
>> +
>> +#define EUD_ENABLE_CMD 1
>> +#define EUD_DISABLE_CMD 0
> These defines doesn't add much value.
Will remove them.
>
>> +
>> +#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_CHGR_INT_CLR 0x0084
>> +#define EUD_REG_CSR_EUD_EN 0x1014
>> +#define EUD_REG_SW_ATTACH_DET 0x1018
>> +
>> +#define EUD_INT_VBUS BIT(2)
>> +#define EUD_INT_CHGR BIT(3)
>> +#define EUD_INT_SAFE_MODE BIT(4)
>> +#define EUD_INT_ALL (EUD_INT_VBUS|EUD_INT_CHGR|\
>> + EUD_INT_SAFE_MODE)
>> +
>> +struct eud_chip {
>> + struct device *dev;
>> + int eud_irq;
>> + unsigned int extcon_id;
>> + unsigned int int_status;
>> + bool usb_attach;
>> + bool chgr_enable;
>> + void __iomem *eud_reg_base;
>> + struct extcon_dev *extcon;
>> + int enable;
>> + struct work_struct eud_work;
>> +};
>> +
>> +static const unsigned int eud_extcon_cable[] = {
>> + EXTCON_USB,
>> + EXTCON_CHG_USB_SDP,
>> + EXTCON_NONE,
>> +};
>> +
>> +static int enable_eud(struct eud_chip *priv)
>> +{
>> + int ret;
>> +
>> + /* write into CSR to enable EUD */
> Make up a define for BIT(0) and the next line is self explanatory - i.e.
> drop the comment..
OK.
>
>> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
> Don't use _relaxed version of writel/readl unless you have a really good
> reason - and if so provide a comment to why this is.
OK, will change to writel.
>
>> + /* Enable vbus, chgr & safe mode warning interrupts */
> This just repeats exactly what can be read from the next line.
Ok.
>
>> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
>> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
>> +
>> + /* Ensure Register Writes Complete */
> wmb() ensures ordering, it deosn't wait for the operation to complete,
> if you need that readl() the register.
Will fix.
>
>> + wmb();
>> +
>> + /*
>> + * Set the default cable state to usb connect and charger
>> + * enable
>> + */
>> + ret = extcon_set_state_sync(priv->extcon, EXTCON_USB, true);
>> + if (ret)
>> + return ret;
>> + ret = extcon_set_state_sync(priv->extcon,
>> + EXTCON_CHG_USB_SDP, true);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static void disable_eud(struct eud_chip *priv)
>> +{
>> + /* write into CSR to disable EUD */
>> + writel_relaxed(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
> Use writel() and drop the comment.
OK
>
>> +}
>> +
>> +static ssize_t enable_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct eud_chip *chip = dev_get_drvdata(dev);
>> +
>> + return snprintf(buf, sizeof(int), "%d", chip->enable);
> buf is not sizeof(int) big...Just do sprintf()...
OK
>
>> +}
>> +
>> +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);
>> + int enable = 0;
> You shouldn't need to initialize this as you're checking the return
> value of sscanf().
OK
>
>> + int ret = 0;
>> +
>> + if (sscanf(buf, "%du", &enable) != 1)
>> + return -EINVAL;
>> +
>> + if (enable == EUD_ENABLE_CMD)
>> + ret = enable_eud(chip);
> If ret is !0 you should probably return that, rather than count...
ok
>
>> + else if (enable == EUD_DISABLE_CMD)
>> + disable_eud(chip);
>> + if (!ret)
> ...and then you don't need this check, or initialize ret to 0 above.
ok
>
>> + chip->enable = enable;
> So if I write 42 to "enable" nothing will change in the hardware, but
> chip->enable will be 42...
will change enable struct member to bool?
>
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(enable);
>> +
>> +static struct attribute *attrs[] = {
>> + &dev_attr_enable.attr,
>> + NULL
>> +};
>> +
>> +static struct attribute_group attr_group = {
>> + .attrs = attrs,
>> +};
>> +
>> +static const struct attribute_group *attr_groups[] = {
>> + &attr_group,
>> + NULL
>> +};
>> +
>> +static void eud_event_notifier(struct work_struct *eud_work)
> Why do you need a worker for this? Why not just use a threaded handler
> and execute this directly in that context?
Will consider it.
>
>> +{
>> + struct eud_chip *chip = container_of(eud_work, struct eud_chip,
>> + eud_work);
>> + int ret;
>> +
>> + if (chip->int_status == EUD_INT_VBUS) {
> And if you just call this function from the handler, you don't need
> chip->int_status to pass parameters between the handler and the worker.
ok
>
>> + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
>> + chip->usb_attach);
>> + if (ret)
>> + return;
>> + } else if (chip->int_status == EUD_INT_CHGR) {
>> + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
>> + chip->chgr_enable);
>> + if (ret)
>> + return;
>> + }
>> +}
>> +
>> +static void usb_attach_detach(struct eud_chip *chip)
>> +{
>> + u32 reg;
>> +
>> + chip->extcon_id = EXTCON_USB;
>> + /* read ctl_out_1[4] to find USB attach or detach event */
>> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
>> + if (reg & BIT(4))
> Give this bit a define
ok
>
>> + chip->usb_attach = true;
>> + else
>> + chip->usb_attach = false;
>> +
>> + schedule_work(&chip->eud_work);
>> +
>> + /* set and clear vbus_int_clr[0] to clear interrupt */
>> + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
>> + /* Ensure Register Writes Complete */
>> + wmb();
> Use writel() and you probably don't need the wmb() here.
ok
>
>> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
>> +}
>> +
>> +static void chgr_enable_disable(struct eud_chip *chip)
>> +{
>> + u32 reg;
>> +
>> + chip->extcon_id = EXTCON_CHG_USB_SDP;
>> + /* read ctl_out_1[6] to find charger enable or disable event */
>> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
>> + if (reg & BIT(6))
> Again, this deserves a define
ok
>
>> + chip->chgr_enable = true;
>> + else
>> + chip->chgr_enable = false;
>> +
>> + schedule_work(&chip->eud_work);
>> +
>> + /* set and clear chgr_int_clr[0] to clear interrupt */
>> + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
>> +}
>> +
>> +static void pet_eud(struct eud_chip *chip)
>> +{
>> + u32 reg;
>> +
>> + /* read sw_attach_det[0] to find attach/detach event */
>> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
>> + if (reg & BIT(0)) {
> define
ok
>
>> + /* Detach & Attach pet for EUD */
> All comments in this driver relates to the very next line, but this
> seems to document the next two writes - i.e. this seems to be a proper
> comment.
ok
>> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> + /* Delay to make sure detach pet is done before attach pet */
>> + udelay(100);
> Better read back the value if you want to ensure the length of the delay
> between the two writes.
ok
>
>> + writel_relaxed(BIT(0), chip->eud_reg_base +
>> + EUD_REG_SW_ATTACH_DET);
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> + } else {
>> + /* Attach pet for EUD */
>> + writel_relaxed(BIT(0), chip->eud_reg_base +
>> + EUD_REG_SW_ATTACH_DET);
>> + /* Ensure Register Writes Complete */
>> + wmb();
> It will complete, if you need to wait for it to have completed read back
> the value.
ok
>
>> + }
>> +}
>> +
>> +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 */
>> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_INT_STATUS_1);
>> + switch (reg & EUD_INT_ALL) {
> What is the expected outcome if for some reason more than one of these
> bits are set?
USB attach/detach events and Charger enable/disable events are
orthogonal and they both should be processed, will evaluate if break
statement should be removed.
>
>> + case EUD_INT_VBUS:
>> + chip->int_status = EUD_INT_VBUS;
>> + usb_attach_detach(chip);
>> + break;
>> + case EUD_INT_CHGR:
>> + chip->int_status = EUD_INT_CHGR;
>> + chgr_enable_disable(chip);
>> + break;
>> + case EUD_INT_SAFE_MODE:
>> + pet_eud(chip);
>> + break;
>> + default:
>> + return IRQ_NONE;
>> + }
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int msm_eud_probe(struct platform_device *pdev)
>> +{
>> + struct eud_chip *chip;
>> + struct resource *res;
>> + int ret;
>> +
>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> + if (!chip)
>> + return -ENOMEM;
>> +
>> + chip->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, chip);
>> +
>> + chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable);
> Aren't we moving away from extcon in favor of the usb role switching
> thing?
i could see that usb role switch has been implemented for c type
connector and that connector is modeled as child of usb controller, but
EUD is not a true connector, it intercepts PHY signals and reroute it to
USB controller as per EUD Command issued by debug appliaction
i am not sure if i need to implement EUD DT node as child of usb
controller, if i do so, as per my understanding EUD driver need to set
USB controller mode(host or device mode) by calling usb role switch
API's, please let me know if my understanding is correct?
>
>> + if (IS_ERR(chip->extcon))
>> + return PTR_ERR(chip->extcon);
>> +
>> + ret = devm_extcon_dev_register(&pdev->dev, chip->extcon);
>> + if (ret)
>> + return ret;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -ENOMEM;
>> +
>> + chip->eud_reg_base = devm_ioremap_resource(&pdev->dev, res);
> Use devm_platform_ioremap_resource() instead
Ok.
>
>> + if (IS_ERR(chip->eud_reg_base))
>> + return PTR_ERR(chip->eud_reg_base);
>> +
>> + chip->eud_irq = platform_get_irq(pdev, 0);
>> +
>> + ret = devm_request_irq(&pdev->dev, chip->eud_irq, handle_eud_irq,
>> + IRQF_TRIGGER_HIGH, NULL, chip);
> Omit the irq trigger information here and let it come from devicetree.
Ok
>
>> + if (ret)
>> + return ret;
>> +
>> + device_init_wakeup(&pdev->dev, true);
>> + enable_irq_wake(chip->eud_irq);
>> +
>> + INIT_WORK(&chip->eud_work, eud_event_notifier);
>> +
>> + if (ret)
> Duplicate of the same check 8 lines up.
Ok
>
>> + return ret;
>> +
>> + /* Enable EUD */
>> + if (chip->enable)
> I'm not seeing where this would have been written during probe.
Will check and modify.
>
>> + enable_eud(chip);
>> +
>> + return 0;
>> +}
>> +
>> +static int msm_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 msm_eud_dt_match[] = {
>> + {.compatible = "qcom,msm-eud"},
> Is this the one and only, past and future, version of the EUD hardware
> block? Or do we need this compatible to be more specific?
EUD h/w IP is Qualcomm IP, As of now this is only hw IP available, if
future version of EUD IP comes, we can modify and add support then?
>
> Nit. Please add a space after { and before }
Ok
>
> Regards,
> Bjorn
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On 2/4/2020 8:40 AM, Bryan O'Donoghue wrote:
> On 03/02/2020 19:35, Bjorn Andersson wrote:
>> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
>
> Hi Avaneesh.
Hello Bryan, Thank you very much for your review comments.
Will be replying to your comments and will be posting new patchset soon
as per review comments.
>
>> Please aim for keeping the sort order in this file (ignore QCOM_APR
>> which obviously is in the wrong place)
>>
>>> + tristate "QTI Embedded USB Debugger (EUD)"
>>> + depends on ARCH_QCOM
>
> If we persist with the model of EXTCON you should "select EXTCON" here.
I have asked this query with Bjorn Also against his review comments,
whether we need to persist with extcon or need to switch to usb role
switch framework, as we are notifying not only to usb controller but
also to pmic charger so in case we adopt usb role switch then how we
will notify to pmic charger to enable charging battery ? Also as i
mentioned there my dilema is it does not look very apt to model EUD hw
IP as c type connector, so please let me know your views.
>
>>> + help
>>> + The Embedded USB Debugger (EUD) driver is a driver for the
>>> + control peripheral which waits on events like USB
>>> attach/detach
>>> + and charger enable/disable. The control peripheral further
>>> helps
>>> + support the USB-based debug and trace capabilities.
>>> + This module enables support for Qualcomm Technologies, Inc.
>>> + Embedded USB Debugger (EUD).
>
> Suggest.
>
> This module enables support for Qualcomm Technologies, Inc.
> Embedded USB Debugger (EUD).
> The EUD is a control peripheral which reports VBUS attach/detach,
> charger enable/disable and USB-based debug and trace capabilities.
OK.
>
>
>>> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>
> 2020
OK
>
>>> +
>>> +static int enable_eud(struct eud_chip *priv)
>>> +{
>>> + int ret;
>>> +
>>> + /* write into CSR to enable EUD */
>>> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
>>> + /* Enable vbus, chgr & safe mode warning interrupts */
>>> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
>>> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
>>> +
>>> + /* Ensure Register Writes Complete */
>
> So... You are writing a register in an on-chip PMIC. The PMIC is
> responsible for detecting USB ID and supplying VBUS as appropriate.
>
> You then get an interrupt to inform you of the state ?
I am writing to EUD control port so that when EUD is enable, EUD hw IP
can intercept VBUS and d+/d- signal and can reroute to PMIC or USB as
per host application command in debug mode.
so for example in debug mode VBUS signal although asserted on connecting
phone with host PC, but EUD based on debug application command can
notify USB to detach(which otherwise would have detected and attached)
>
>>> +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);
>>> + int enable = 0;
>>
>> You shouldn't need to initialize this as you're checking the return
>> value of sscanf().
OK
>>
>>> + int ret = 0;
>>> +
>>> + if (sscanf(buf, "%du", &enable) != 1)
>>> + return -EINVAL;
>>> +
>>> + if (enable == EUD_ENABLE_CMD)
>>> + ret = enable_eud(chip);
>>
>> If ret is !0 you should probably return that, rather than count...
OK
>>
>>> + else if (enable == EUD_DISABLE_CMD)
>>> + disable_eud(chip);
>>> + if (!ret)
>>
>> ...and then you don't need this check, or initialize ret to 0 above.
>>
>>> + chip->enable = enable;
>>
>> So if I write 42 to "enable" nothing will change in the hardware, but
>> chip->enable will be 42...
>>
>>> + return count;
>>> +}
>
> I was just going to comment on usb_connector but, does the above code
> need a synchronization primitive to serialize with the worker and
> interrupt handler ?
Will evaluate and take corrective action if needed.
>
>>> +static int msm_eud_probe(struct platform_device *pdev)
>>> +{
>>> + struct eud_chip *chip;
>>> + struct resource *res;
>>> + int ret;
>>> +
>>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>> + if (!chip)
>>> + return -ENOMEM;
>>> +
>>> + chip->dev = &pdev->dev;
>>> + platform_set_drvdata(pdev, chip);
>>> +
>>> + chip->extcon = devm_extcon_dev_allocate(&pdev->dev,
>>> eud_extcon_cable);
>>
>> Aren't we moving away from extcon in favor of the usb role switching
>> thing?
>
> Yes.
>
> For the VBUS notification you could use
>
> usb-role-switch and model the USB connector as a child-node of the
> dual-role controller.
I am not sure if EUD interface is true USB connector or should be
modeled so, as EUD can trick usb controller about absence of VBUS/d+/d-
signal. To illustrate, when in debug mode even if phone is connected
with PC, EUD can notify USB controller to stop USB s/w stack, by
notifying USB controller about usb detach event, even when d+\d- signals
are valid. moreover in debug mode USB controller will always configure
in device mode, so let me know if EUD qualifies to be modeled as child
of controller node?
>
> See:
> https://patchwork.kernel.org/cover/11346247/
> https://patchwork.kernel.org/patch/11346295/
> https://patchwork.kernel.org/patch/11346263/
>
> Avaneesh do you have any kernel code that cares about the charger state ?
charger state is to be notified to charger driver to start charging
battery so if i switch to usb role switch framework, how will i notify
to pmic charger? so if i have to adopt usb role switch framework then
also i will have to keep extcon framework, let me know your comment.
>
> What we are suggesting here is dropping extcon and using
> role-switching but, if you have some other code that cares about
> EXTCON_CHG_USB_SDP you'd have to do additional work.
>
> But, if I understood the implication of the code above where you write
> to the PMIC and let it handle VBUS/CHARGER on/off and you are just
> notified of the state change, you should be fine with usb-role-switching.
as i mentioned usb-role-switch will only cater need to notify to usb
controller so please let me know your views.
>
> ---
> bod
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On 2/7/2020 3:34 PM, Greg KH wrote:
> On Fri, Jan 31, 2020 at 10:13:31AM +0530, Avaneesh Kumar Dwivedi wrote:
>> Add support for control peripheral of EUD (Embedded USB Debugger) to
>> listen to events such as USB attach/detach, charger enable/disable, pet
>> EUD to indicate software is functional. Reusing the platform device kobj,
>> sysfs entry 'enable' is created to enable or disable EUD.
>>
>> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
>> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
>> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
>> ---
>> Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
>> drivers/soc/qcom/Kconfig | 12 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/eud.c | 329 ++++++++++++++++++++++++++
>> 4 files changed, 347 insertions(+)
>> create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
>> create mode 100644 drivers/soc/qcom/eud.c
>>
>> diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
>> new file mode 100644
>> index 0000000..d96ae05
>> --- /dev/null
>> +++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
>> @@ -0,0 +1,5 @@
>> +What: /sys/bus/platform/drivers/msm-eud/enable
>> +Date: Jan 2020
>> +Contact: Avaneesh Kumar Dwivedi <[email protected]>
>> +Description: Enable/Disable use of eud device.
> What are valid values to be used here?
it should be bool variable relying on 0 or 1.
>
>> +Users: User space debug application which intend to use EUD h/w block.
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index d0a73e7..6b7c9d0 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -202,4 +202,16 @@ config QCOM_APR
>> application processor and QDSP6. APR is
>> used by audio driver to configure QDSP6
>> ASM, ADM and AFE modules.
>> +
>> +config QCOM_EUD
>> + tristate "QTI Embedded USB Debugger (EUD)"
>> + depends on ARCH_QCOM
> Why not let everyone test build this?
EUD is Qualcomm IP, shall not it be associated with ARCH_QCOM?
>
>> + help
>> + The Embedded USB Debugger (EUD) driver is a driver for the
>> + control peripheral which waits on events like USB attach/detach
>> + and charger enable/disable. The control peripheral further helps
>> + support the USB-based debug and trace capabilities.
>> + This module enables support for Qualcomm Technologies, Inc.
>> + Embedded USB Debugger (EUD).
>> + If unsure, say N.
>> endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 9fb35c8..c15be68 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -25,3 +25,4 @@ obj-$(CONFIG_QCOM_APR) += apr.o
>> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
>> obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>> obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>> +obj-$(CONFIG_QCOM_EUD) += eud.o
>> diff --git a/drivers/soc/qcom/eud.c b/drivers/soc/qcom/eud.c
>> new file mode 100644
>> index 0000000..e6c3604
>> --- /dev/null
>> +++ b/drivers/soc/qcom/eud.c
>> @@ -0,0 +1,329 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/extcon.h>
>> +#include <linux/extcon-provider.h>
>> +#include <linux/delay.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/io.h>
>> +#include <linux/bitops.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/power_supply.h>
>> +
>> +#define EUD_ENABLE_CMD 1
>> +#define EUD_DISABLE_CMD 0
> Don't need these.
OK
>
>> +
>> +#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_CHGR_INT_CLR 0x0084
>> +#define EUD_REG_CSR_EUD_EN 0x1014
>> +#define EUD_REG_SW_ATTACH_DET 0x1018
>> +
>> +#define EUD_INT_VBUS BIT(2)
>> +#define EUD_INT_CHGR BIT(3)
>> +#define EUD_INT_SAFE_MODE BIT(4)
>> +#define EUD_INT_ALL (EUD_INT_VBUS|EUD_INT_CHGR|\
>> + EUD_INT_SAFE_MODE)
>> +
>> +struct eud_chip {
>> + struct device *dev;
>> + int eud_irq;
>> + unsigned int extcon_id;
>> + unsigned int int_status;
>> + bool usb_attach;
>> + bool chgr_enable;
>> + void __iomem *eud_reg_base;
>> + struct extcon_dev *extcon;
>> + int enable;
>> + struct work_struct eud_work;
>> +};
>> +
>> +static const unsigned int eud_extcon_cable[] = {
>> + EXTCON_USB,
>> + EXTCON_CHG_USB_SDP,
>> + EXTCON_NONE,
>> +};
>> +
>> +static int enable_eud(struct eud_chip *priv)
>> +{
>> + int ret;
>> +
>> + /* write into CSR to enable EUD */
>> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
>> + /* Enable vbus, chgr & safe mode warning interrupts */
>> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
>> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
>> +
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> +
>> + /*
>> + * Set the default cable state to usb connect and charger
>> + * enable
>> + */
>> + ret = extcon_set_state_sync(priv->extcon, EXTCON_USB, true);
>> + if (ret)
>> + return ret;
>> + ret = extcon_set_state_sync(priv->extcon,
>> + EXTCON_CHG_USB_SDP, true);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static void disable_eud(struct eud_chip *priv)
>> +{
>> + /* write into CSR to disable EUD */
>> + writel_relaxed(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
>> +}
>> +
>> +static ssize_t enable_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct eud_chip *chip = dev_get_drvdata(dev);
>> +
>> + return snprintf(buf, sizeof(int), "%d", 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);
>> + int enable = 0;
>> + int ret = 0;
>> +
>> + if (sscanf(buf, "%du", &enable) != 1)
>> + return -EINVAL;
> No, use the built-in kernel function to handle reading y/n/Y/N/0/1 from
> sysfs files, do not try to roll your own. As you have seen, you will
> get it wrong :)
ok
>
>
>
>> +
>> + if (enable == EUD_ENABLE_CMD)
>> + ret = enable_eud(chip);
>> + else if (enable == EUD_DISABLE_CMD)
>> + disable_eud(chip);
>> + if (!ret)
>> + chip->enable = enable;
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(enable);
>> +
>> +static struct attribute *attrs[] = {
>> + &dev_attr_enable.attr,
>> + NULL
>> +};
>> +
>> +static struct attribute_group attr_group = {
>> + .attrs = attrs,
>> +};
>> +
>> +static const struct attribute_group *attr_groups[] = {
>> + &attr_group,
>> + NULL
>> +};
> ATTRIBUTE_GROUPS()?
OK.
>
>> +
>> +static void eud_event_notifier(struct work_struct *eud_work)
>> +{
>> + struct eud_chip *chip = container_of(eud_work, struct eud_chip,
>> + eud_work);
>> + int ret;
>> +
>> + if (chip->int_status == EUD_INT_VBUS) {
>> + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
>> + chip->usb_attach);
>> + if (ret)
>> + return;
>> + } else if (chip->int_status == EUD_INT_CHGR) {
>> + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id,
>> + chip->chgr_enable);
>> + if (ret)
>> + return;
>> + }
>> +}
>> +
>> +static void usb_attach_detach(struct eud_chip *chip)
>> +{
>> + u32 reg;
>> +
>> + chip->extcon_id = EXTCON_USB;
>> + /* read ctl_out_1[4] to find USB attach or detach event */
>> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
>> + if (reg & BIT(4))
>> + chip->usb_attach = true;
>> + else
>> + chip->usb_attach = false;
>> +
>> + schedule_work(&chip->eud_work);
>> +
>> + /* set and clear vbus_int_clr[0] to clear interrupt */
>> + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
>> +}
>> +
>> +static void chgr_enable_disable(struct eud_chip *chip)
>> +{
>> + u32 reg;
>> +
>> + chip->extcon_id = EXTCON_CHG_USB_SDP;
>> + /* read ctl_out_1[6] to find charger enable or disable event */
>> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
>> + if (reg & BIT(6))
>> + chip->chgr_enable = true;
>> + else
>> + chip->chgr_enable = false;
>> +
>> + schedule_work(&chip->eud_work);
>> +
>> + /* set and clear chgr_int_clr[0] to clear interrupt */
>> + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_CHGR_INT_CLR);
>> +}
>> +
>> +static void pet_eud(struct eud_chip *chip)
>> +{
>> + u32 reg;
>> +
>> + /* read sw_attach_det[0] to find attach/detach event */
>> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
>> + if (reg & BIT(0)) {
>> + /* Detach & Attach pet for EUD */
>> + writel_relaxed(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> + /* Delay to make sure detach pet is done before attach pet */
>> + udelay(100);
>> + writel_relaxed(BIT(0), chip->eud_reg_base +
>> + EUD_REG_SW_ATTACH_DET);
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> + } else {
>> + /* Attach pet for EUD */
>> + writel_relaxed(BIT(0), chip->eud_reg_base +
>> + EUD_REG_SW_ATTACH_DET);
>> + /* Ensure Register Writes Complete */
>> + wmb();
>> + }
>> +}
>> +
>> +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 */
>> + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_INT_STATUS_1);
>> + switch (reg & EUD_INT_ALL) {
>> + case EUD_INT_VBUS:
>> + chip->int_status = EUD_INT_VBUS;
>> + usb_attach_detach(chip);
>> + break;
>> + case EUD_INT_CHGR:
>> + chip->int_status = EUD_INT_CHGR;
>> + chgr_enable_disable(chip);
>> + break;
>> + case EUD_INT_SAFE_MODE:
>> + pet_eud(chip);
>> + break;
>> + default:
>> + return IRQ_NONE;
>> + }
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int msm_eud_probe(struct platform_device *pdev)
>> +{
>> + struct eud_chip *chip;
>> + struct resource *res;
>> + int ret;
>> +
>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> + if (!chip)
>> + return -ENOMEM;
>> +
>> + chip->dev = &pdev->dev;
> No reference counting???
you mean get/put_device?
>
> thanks,
>
> greg k-h
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On Sun, Feb 16, 2020 at 09:52:19PM +0530, Dwivedi, Avaneesh Kumar (avani) wrote:
>
> On 2/7/2020 3:34 PM, Greg KH wrote:
> > On Fri, Jan 31, 2020 at 10:13:31AM +0530, Avaneesh Kumar Dwivedi wrote:
> > > Add support for control peripheral of EUD (Embedded USB Debugger) to
> > > listen to events such as USB attach/detach, charger enable/disable, pet
> > > EUD to indicate software is functional. Reusing the platform device kobj,
> > > sysfs entry 'enable' is created to enable or disable EUD.
> > >
> > > Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
> > > Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
> > > Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
> > > ---
> > > Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
> > > drivers/soc/qcom/Kconfig | 12 +
> > > drivers/soc/qcom/Makefile | 1 +
> > > drivers/soc/qcom/eud.c | 329 ++++++++++++++++++++++++++
> > > 4 files changed, 347 insertions(+)
> > > create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
> > > create mode 100644 drivers/soc/qcom/eud.c
> > >
> > > diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
> > > new file mode 100644
> > > index 0000000..d96ae05
> > > --- /dev/null
> > > +++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
> > > @@ -0,0 +1,5 @@
> > > +What: /sys/bus/platform/drivers/msm-eud/enable
> > > +Date: Jan 2020
> > > +Contact: Avaneesh Kumar Dwivedi <[email protected]>
> > > +Description: Enable/Disable use of eud device.
> > What are valid values to be used here?
> it should be bool variable relying on 0 or 1.
Then document it.
> >
> > > +Users: User space debug application which intend to use EUD h/w block.
> > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > > index d0a73e7..6b7c9d0 100644
> > > --- a/drivers/soc/qcom/Kconfig
> > > +++ b/drivers/soc/qcom/Kconfig
> > > @@ -202,4 +202,16 @@ config QCOM_APR
> > > application processor and QDSP6. APR is
> > > used by audio driver to configure QDSP6
> > > ASM, ADM and AFE modules.
> > > +
> > > +config QCOM_EUD
> > > + tristate "QTI Embedded USB Debugger (EUD)"
> > > + depends on ARCH_QCOM
> > Why not let everyone test build this?
> EUD is Qualcomm IP, shall not it be associated with ARCH_QCOM?
No, why can't everyone buid it for testing? What about when I want to
build a generic arm64 kernel to run on multiple SoCs?
Do not put dependancies in here that you really do not have. There's no
reason for this to be limited to that one chip, right? And if you allow
others to build the code, you will get proper bug reports when things
break, and others will fix them, which is what you want.
I think the ARCH_RANDOM_SOC_NAME is totally broken and needs to be, at
most, just an arch-specific thing, if even that.
Look at almost all other kernel drivers, they do not have those types of
dependancies.
> > > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > + if (!chip)
> > > + return -ENOMEM;
> > > +
> > > + chip->dev = &pdev->dev;
> > No reference counting???
> you mean get/put_device?
yes.
thanks,
greg k-h
On 2/16/2020 10:05 PM, Greg KH wrote:
> On Sun, Feb 16, 2020 at 09:52:19PM +0530, Dwivedi, Avaneesh Kumar (avani) wrote:
>> On 2/7/2020 3:34 PM, Greg KH wrote:
>>> On Fri, Jan 31, 2020 at 10:13:31AM +0530, Avaneesh Kumar Dwivedi wrote:
>>>> Add support for control peripheral of EUD (Embedded USB Debugger) to
>>>> listen to events such as USB attach/detach, charger enable/disable, pet
>>>> EUD to indicate software is functional. Reusing the platform device kobj,
>>>> sysfs entry 'enable' is created to enable or disable EUD.
>>>>
>>>> Signed-off-by: Satya Durga Srinivasu Prabhala <[email protected]>
>>>> Signed-off-by: Prakruthi Deepak Heragu <[email protected]>
>>>> Signed-off-by: Avaneesh Kumar Dwivedi <[email protected]>
>>>> ---
>>>> Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
>>>> drivers/soc/qcom/Kconfig | 12 +
>>>> drivers/soc/qcom/Makefile | 1 +
>>>> drivers/soc/qcom/eud.c | 329 ++++++++++++++++++++++++++
>>>> 4 files changed, 347 insertions(+)
>>>> create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
>>>> create mode 100644 drivers/soc/qcom/eud.c
>>>>
>>>> diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
>>>> new file mode 100644
>>>> index 0000000..d96ae05
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
>>>> @@ -0,0 +1,5 @@
>>>> +What: /sys/bus/platform/drivers/msm-eud/enable
>>>> +Date: Jan 2020
>>>> +Contact: Avaneesh Kumar Dwivedi <[email protected]>
>>>> +Description: Enable/Disable use of eud device.
>>> What are valid values to be used here?
>> it should be bool variable relying on 0 or 1.
> Then document it.
OK
>
>>>> +Users: User space debug application which intend to use EUD h/w block.
>>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>>> index d0a73e7..6b7c9d0 100644
>>>> --- a/drivers/soc/qcom/Kconfig
>>>> +++ b/drivers/soc/qcom/Kconfig
>>>> @@ -202,4 +202,16 @@ config QCOM_APR
>>>> application processor and QDSP6. APR is
>>>> used by audio driver to configure QDSP6
>>>> ASM, ADM and AFE modules.
>>>> +
>>>> +config QCOM_EUD
>>>> + tristate "QTI Embedded USB Debugger (EUD)"
>>>> + depends on ARCH_QCOM
>>> Why not let everyone test build this?
>> EUD is Qualcomm IP, shall not it be associated with ARCH_QCOM?
> No, why can't everyone buid it for testing? What about when I want to
> build a generic arm64 kernel to run on multiple SoCs?
>
> Do not put dependancies in here that you really do not have. There's no
> reason for this to be limited to that one chip, right? And if you allow
> others to build the code, you will get proper bug reports when things
> break, and others will fix them, which is what you want.
>
> I think the ARCH_RANDOM_SOC_NAME is totally broken and needs to be, at
> most, just an arch-specific thing, if even that.
>
> Look at almost all other kernel drivers, they do not have those types of
> dependancies.
Will check and address concerns.
>
>>>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>>> + if (!chip)
>>>> + return -ENOMEM;
>>>> +
>>>> + chip->dev = &pdev->dev;
>>> No reference counting???
>> you mean get/put_device?
> yes.
>
> thanks,
>
> greg k-h
Thank you very much Greg for your time to review.
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On 16/02/2020 16:07, Dwivedi, Avaneesh Kumar (avani) wrote:
>
> On 2/4/2020 8:40 AM, Bryan O'Donoghue wrote:
>> On 03/02/2020 19:35, Bjorn Andersson wrote:
>>> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
>>
>> Hi Avaneesh.
>
> Hello Bryan, Thank you very much for your review comments.
>
> Will be replying to your comments and will be posting new patchset soon
> as per review comments.
>
>>
>>> Please aim for keeping the sort order in this file (ignore QCOM_APR
>>> which obviously is in the wrong place)
>>>
>>>> + tristate "QTI Embedded USB Debugger (EUD)"
>>>> + depends on ARCH_QCOM
>>
>> If we persist with the model of EXTCON you should "select EXTCON" here.
> I have asked this query with Bjorn Also against his review comments,
> whether we need to persist with extcon or need to switch to usb role
> switch framework, as we are notifying not only to usb controller but
> also to pmic charger so in case we adopt usb role switch then how we
> will notify to pmic charger to enable charging battery ? Also as i
> mentioned there my dilema is it does not look very apt to model EUD hw
> IP as c type connector, so please let me know your views.
I think there's a desire to model USB ports as connector child nodes of
a USB controllers as opposed to the more generic extcon so, I think the
effort should probably be made to model it up as typec.
1. Model as a typec connector
You can use usb-role-switch based on the VBUS interrupt you get
drivers/extcon/extcon-axp288.c::axp288_usb_role_work()
as an exmple
2. Model the registers/gpios in the PMIC interface as regulators
that your typec driver could then own.
You wouldn't have to notify outside of your typec driver then
you'd just be using the regulator API.
You can use regmap to divide up the registers between devices for that.
Can that work for you ?
>>>> +static int enable_eud(struct eud_chip *priv)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + /* write into CSR to enable EUD */
>>>> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
>>>> + /* Enable vbus, chgr & safe mode warning interrupts */
>>>> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
>>>> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
>>>> +
>>>> + /* Ensure Register Writes Complete */
>>
>> So... You are writing a register in an on-chip PMIC. The PMIC is
>> responsible for detecting USB ID and supplying VBUS as appropriate.
>>
>> You then get an interrupt to inform you of the state ?
>
> I am writing to EUD control port so that when EUD is enable, EUD hw IP
> can intercept VBUS and d+/d- signal and can reroute to PMIC or USB as
> per host application command in debug mode.
Reading the dts that goes with this
+The EUD (Embedded USB Debugger) is a mini-USB hub implemented
+on chip to support the USB-based debug and trace capabilities.
Ah so, the EUD is a mux, that sits between the connector and the
controller, routing UTMI signals to an internal USB hub, which in-turn
has debug functions attached to the hub...
Can the Arm core see the hub ? I assume not ?
There are a few different modes - you should probably be clear on which
mode it is you are supporting.
Normal mode: (Bypass)
Port | EUD | Controller
Normal + debug hub mode: (Debug)
Port | EUD | Controller + HUB -> debug functions
Debug hub mode: (Control Peripheral)
Port | EUD | HUB -> debug functions
its not clear to me from the documentation or the code which mode it is
we are targeting to be supported here.
I think you should support Debug mode only here, so that the Arm core
never has to deal with the situation where the USB connector "goes away".
If we were to support Control Peripheral where the local DWC3 controller
has the signals routed away entirely, then I think we would need to look
into modelling that in device tree - and using an overlay to show the
DWC3 controller going away in Control Peripheral mode and coming back.
Also final thought since the EUD can operate in different modes, it
really should be a string that gets passed in - with the string name
aligning to the documentation "bypass", "debug" and so on, so that the
mode we are switching to is obvious to anybody who has the spec and the
driver.
---
bod
On Sun 16 Feb 06:14 PST 2020, Dwivedi, Avaneesh Kumar (avani) wrote:
> Thank you very much Bjorn for your comments, will address them and post
> latest patchset soon.
>
> On 2/4/2020 1:05 AM, Bjorn Andersson wrote:
> > On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
[..]
> > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > > index d0a73e7..6b7c9d0 100644
> > > --- a/drivers/soc/qcom/Kconfig
> > > +++ b/drivers/soc/qcom/Kconfig
> > > @@ -202,4 +202,16 @@ config QCOM_APR
> > > application processor and QDSP6. APR is
> > > used by audio driver to configure QDSP6
> > > ASM, ADM and AFE modules.
> > > +
> > > +config QCOM_EUD
> > Please aim for keeping the sort order in this file (ignore QCOM_APR
> > which obviously is in the wrong place)
> Please help to elaborate more, do you mean adding configs in alphabetical
> order?
Yes, we want to maintain alphabetical sort order of the config options
in the Kconfig file. Unfortunately I must have missed this as I picked
up QCOM_APR - hence my ask to add you entry further up, even if the
order isn't perfect...
> >
> > > + tristate "QTI Embedded USB Debugger (EUD)"
> > > + depends on ARCH_QCOM
> > > + help
> > > + The Embedded USB Debugger (EUD) driver is a driver for the
> > > + control peripheral which waits on events like USB attach/detach
> > > + and charger enable/disable. The control peripheral further helps
> > > + support the USB-based debug and trace capabilities.
> > > + This module enables support for Qualcomm Technologies, Inc.
> > > + Embedded USB Debugger (EUD).
> > > + If unsure, say N.
> > > endmenu
[..]
> > > +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);
> > > + int enable = 0;
> > You shouldn't need to initialize this as you're checking the return
> > value of sscanf().
> OK
> >
> > > + int ret = 0;
> > > +
> > > + if (sscanf(buf, "%du", &enable) != 1)
> > > + return -EINVAL;
> > > +
> > > + if (enable == EUD_ENABLE_CMD)
> > > + ret = enable_eud(chip);
> > If ret is !0 you should probably return that, rather than count...
> ok
> >
> > > + else if (enable == EUD_DISABLE_CMD)
> > > + disable_eud(chip);
> > > + if (!ret)
> > ...and then you don't need this check, or initialize ret to 0 above.
> ok
> >
> > > + chip->enable = enable;
> > So if I write 42 to "enable" nothing will change in the hardware, but
> > chip->enable will be 42...
> will change enable struct member to bool?
> >
The problem I meant was hat if buf is "42", then you will hit the
following code path:
int ret = 0;
sscanf(buf, "%du", &enable);
chip->enable = 42;
As enable isn't 1 or 0, neither conditional path is taken, but you still
store the value in chip->enable.
Changing enable to bool won't change this problem, adding an else and
returning -EINVAL; would.
> > > + return count;
> > > +}
> > > +
> > > +static DEVICE_ATTR_RW(enable);
[..]
> > > +static int msm_eud_probe(struct platform_device *pdev)
> > > +{
> > > + struct eud_chip *chip;
> > > + struct resource *res;
> > > + int ret;
> > > +
> > > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > + if (!chip)
> > > + return -ENOMEM;
> > > +
> > > + chip->dev = &pdev->dev;
> > > + platform_set_drvdata(pdev, chip);
> > > +
> > > + chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable);
> > Aren't we moving away from extcon in favor of the usb role switching
> > thing?
>
> i could see that usb role switch has been implemented for c type connector
> and that connector is modeled as child of usb controller, but EUD is not a
> true connector, it intercepts PHY signals and reroute it to USB controller
> as per EUD Command issued by debug appliaction
>
> i am not sure if i need to implement EUD DT node as child of usb controller,
> if i do so, as per my understanding EUD driver need to set USB controller
> mode(host or device mode) by calling usb role switch API's, please let me
> know if my understanding is correct?
>
I don't know how to properly represent this, but I would like the USB
guys to chime in before merging something.
> >
> > > + if (IS_ERR(chip->extcon))
> > > + return PTR_ERR(chip->extcon);
> > > +
[..]
> > > +static const struct of_device_id msm_eud_dt_match[] = {
> > > + {.compatible = "qcom,msm-eud"},
> > Is this the one and only, past and future, version of the EUD hardware
> > block? Or do we need this compatible to be more specific?
> EUD h/w? IP is Qualcomm IP, As of now this is only hw IP available, if
> future version of EUD IP comes, we can modify and add support then?
You can add additional compatibles, but you can't change this one as
existing devicetree files must continue to function.
If you have a number of platforms that works with this very same
implementation then you could make the binding require a specific
platform and qcom,msm-eud (although qcom,eud should be enough?) and then
keep the implementation as is.
I.e. dt would say:
compatible = "qcom,sc7180-eud", "qcom,eud";
And driver would match on the latter only, for now.
Regards,
Bjorn
On 2/18/2020 9:05 AM, Bjorn Andersson wrote:
> On Sun 16 Feb 06:14 PST 2020, Dwivedi, Avaneesh Kumar (avani) wrote:
>
>> Thank you very much Bjorn for your comments, will address them and post
>> latest patchset soon.
>>
>> On 2/4/2020 1:05 AM, Bjorn Andersson wrote:
>>> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
> [..]
>>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>>> index d0a73e7..6b7c9d0 100644
>>>> --- a/drivers/soc/qcom/Kconfig
>>>> +++ b/drivers/soc/qcom/Kconfig
>>>> @@ -202,4 +202,16 @@ config QCOM_APR
>>>> application processor and QDSP6. APR is
>>>> used by audio driver to configure QDSP6
>>>> ASM, ADM and AFE modules.
>>>> +
>>>> +config QCOM_EUD
>>> Please aim for keeping the sort order in this file (ignore QCOM_APR
>>> which obviously is in the wrong place)
>> Please help to elaborate more, do you mean adding configs in alphabetical
>> order?
> Yes, we want to maintain alphabetical sort order of the config options
> in the Kconfig file. Unfortunately I must have missed this as I picked
> up QCOM_APR - hence my ask to add you entry further up, even if the
> order isn't perfect...
Ok
>
>>>> + tristate "QTI Embedded USB Debugger (EUD)"
>>>> + depends on ARCH_QCOM
>>>> + help
>>>> + The Embedded USB Debugger (EUD) driver is a driver for the
>>>> + control peripheral which waits on events like USB attach/detach
>>>> + and charger enable/disable. The control peripheral further helps
>>>> + support the USB-based debug and trace capabilities.
>>>> + This module enables support for Qualcomm Technologies, Inc.
>>>> + Embedded USB Debugger (EUD).
>>>> + If unsure, say N.
>>>> endmenu
> [..]
>>>> +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);
>>>> + int enable = 0;
>>> You shouldn't need to initialize this as you're checking the return
>>> value of sscanf().
>> OK
>>>> + int ret = 0;
>>>> +
>>>> + if (sscanf(buf, "%du", &enable) != 1)
>>>> + return -EINVAL;
>>>> +
>>>> + if (enable == EUD_ENABLE_CMD)
>>>> + ret = enable_eud(chip);
>>> If ret is !0 you should probably return that, rather than count...
>> ok
>>>> + else if (enable == EUD_DISABLE_CMD)
>>>> + disable_eud(chip);
>>>> + if (!ret)
>>> ...and then you don't need this check, or initialize ret to 0 above.
>> ok
>>>> + chip->enable = enable;
>>> So if I write 42 to "enable" nothing will change in the hardware, but
>>> chip->enable will be 42...
>> will change enable struct member to bool?
> The problem I meant was hat if buf is "42", then you will hit the
> following code path:
>
> int ret = 0;
> sscanf(buf, "%du", &enable);
> chip->enable = 42;
>
> As enable isn't 1 or 0, neither conditional path is taken, but you still
> store the value in chip->enable.
>
> Changing enable to bool won't change this problem, adding an else and
> returning -EINVAL; would.
ok.
>>>> + return count;
>>>> +}
>>>> +
>>>> +static DEVICE_ATTR_RW(enable);
> [..]
>>>> +static int msm_eud_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct eud_chip *chip;
>>>> + struct resource *res;
>>>> + int ret;
>>>> +
>>>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>>> + if (!chip)
>>>> + return -ENOMEM;
>>>> +
>>>> + chip->dev = &pdev->dev;
>>>> + platform_set_drvdata(pdev, chip);
>>>> +
>>>> + chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable);
>>> Aren't we moving away from extcon in favor of the usb role switching
>>> thing?
>> i could see that usb role switch has been implemented for c type connector
>> and that connector is modeled as child of usb controller, but EUD is not a
>> true connector, it intercepts PHY signals and reroute it to USB controller
>> as per EUD Command issued by debug appliaction
>>
>> i am not sure if i need to implement EUD DT node as child of usb controller,
>> if i do so, as per my understanding EUD driver need to set USB controller
>> mode(host or device mode) by calling usb role switch API's, please let me
>> know if my understanding is correct?
>>
> I don't know how to properly represent this, but I would like the USB
> guys to chime in before merging something.
I will check with USB folks if they can give their opinion.
based on that will proceed.
>
>>>> + if (IS_ERR(chip->extcon))
>>>> + return PTR_ERR(chip->extcon);
>>>> +
> [..]
>>>> +static const struct of_device_id msm_eud_dt_match[] = {
>>>> + {.compatible = "qcom,msm-eud"},
>>> Is this the one and only, past and future, version of the EUD hardware
>>> block? Or do we need this compatible to be more specific?
>> EUD h/w IP is Qualcomm IP, As of now this is only hw IP available, if
>> future version of EUD IP comes, we can modify and add support then?
> You can add additional compatibles, but you can't change this one as
> existing devicetree files must continue to function.
>
> If you have a number of platforms that works with this very same
> implementation then you could make the binding require a specific
> platform and qcom,msm-eud (although qcom,eud should be enough?) and then
> keep the implementation as is.
>
> I.e. dt would say:
> compatible = "qcom,sc7180-eud", "qcom,eud";
>
> And driver would match on the latter only, for now.
Ok.
>
> Regards,
> Bjorn
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On 2/18/2020 1:14 AM, Bryan O'Donoghue wrote:
> On 16/02/2020 16:07, Dwivedi, Avaneesh Kumar (avani) wrote:
>>
>> On 2/4/2020 8:40 AM, Bryan O'Donoghue wrote:
>>> On 03/02/2020 19:35, Bjorn Andersson wrote:
>>>> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
>>>
>>> Hi Avaneesh.
>>
>> Hello Bryan, Thank you very much for your review comments.
>>
>> Will be replying to your comments and will be posting new patchset
>> soon as per review comments.
>>
>>>
>>>> Please aim for keeping the sort order in this file (ignore QCOM_APR
>>>> which obviously is in the wrong place)
>>>>
>>>>> + tristate "QTI Embedded USB Debugger (EUD)"
>>>>> + depends on ARCH_QCOM
>>>
>>> If we persist with the model of EXTCON you should "select EXTCON" here.
>
>> I have asked this query with Bjorn Also against his review comments,
>> whether we need to persist with extcon or need to switch to usb role
>> switch framework, as we are notifying not only to usb controller but
>> also to pmic charger so in case we adopt usb role switch then how we
>> will notify to pmic charger to enable charging battery ? Also as i
>> mentioned there my dilema is it does not look very apt to model EUD
>> hw IP as c type connector, so please let me know your views.
>
> I think there's a desire to model USB ports as connector child nodes
> of a USB controllers as opposed to the more generic extcon so, I think
> the effort should probably be made to model it up as typec.
this comment is irrespective of your below comment (If we were to
support Control Peripheral where the local DWC3 controller has the
signals routed away entirely, then I think we would need to look into
modelling that in device tree - and using an overlay to show the DWC3
controller going away in Control Peripheral mode and coming back. )?
>
> 1. Model as a typec connector
> You can use usb-role-switch based on the VBUS interrupt you get
> drivers/extcon/extcon-axp288.c::axp288_usb_role_work()
> as an exmple
Will look into this example, but seems this driver uses both extcon and
usb-role-switch for notification.
>
> 2. Model the registers/gpios in the PMIC interface as regulators
> that your typec driver could then own.
>
> You wouldn't have to notify outside of your typec driver then
> you'd just be using the regulator API.
>
> You can use regmap to divide up the registers between devices for that.
>
> Can that work for you ?
Did not comprehend this comment fully. if possible can you give some
example.
>
>>>>> +static int enable_eud(struct eud_chip *priv)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + /* write into CSR to enable EUD */
>>>>> + writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
>>>>> + /* Enable vbus, chgr & safe mode warning interrupts */
>>>>> + writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
>>>>> + priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
>>>>> +
>>>>> + /* Ensure Register Writes Complete */
>>>
>>> So... You are writing a register in an on-chip PMIC. The PMIC is
>>> responsible for detecting USB ID and supplying VBUS as appropriate.
>>>
>>> You then get an interrupt to inform you of the state ?
>>
>> I am writing to EUD control port so that when EUD is enable, EUD hw
>> IP can intercept VBUS and d+/d- signal and can reroute to PMIC or USB
>> as per host application command in debug mode.
>
> Reading the dts that goes with this
>
> +The EUD (Embedded USB Debugger) is a mini-USB hub implemented
> +on chip to support the USB-based debug and trace capabilities.
>
> Ah so, the EUD is a mux, that sits between the connector and the
> controller, routing UTMI signals to an internal USB hub, which in-turn
> has debug functions attached to the hub...
Yes that is correct understanding.
>
> Can the Arm core see the hub ? I assume not ?
Not sure what is it mean by "Can the Arm core see the hub"?
>
> There are a few different modes - you should probably be clear on
> which mode it is you are supporting.
>
> Normal mode: (Bypass)
> Port | EUD | Controller
>
> Normal + debug hub mode: (Debug)
> Port | EUD | Controller + HUB -> debug functions
>
> Debug hub mode: (Control Peripheral)
> Port | EUD | HUB -> debug functions
>
> its not clear to me from the documentation or the code which mode it
> is we are targeting to be supported here.
Its debug mode which we are supporting in driver.
>
> I think you should support Debug mode only here, so that the Arm core
> never has to deal with the situation where the USB connector "goes away".
Can you please help what you mean by "so that the Arm core never has to
deal with the situation where the USB connector "goes away""
>
> If we were to support Control Peripheral where the local DWC3
> controller has the signals routed away entirely, then I think we would
> need to look into modelling that in device tree - and using an overlay
> to show the DWC3 controller going away in Control Peripheral mode and
> coming back.
debug mode is set run time via user, i will check how we can model such
scenario where device tree corresponding to a h/w module is only valid
in some scenario at run time. if possible please elaborate bit more on
your suggestion
>
> Also final thought since the EUD can operate in different modes, it
> really should be a string that gets passed in - with the string name
> aligning to the documentation "bypass", "debug" and so on, so that the
> mode we are switching to is obvious to anybody who has the spec and
> the driver.
you mean we should document that this driver works in debug mode only?
not clear on where one should pass "debug" and "bypass" string?
I will also be discussing modelling EUD as typec connector with USB
folks and will come back soon with clarity on this. Thanks for your
valuable comments and suggestions.
>
> ---
> bod
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
On 18/02/2020 13:23, Dwivedi, Avaneesh Kumar (avani) wrote:
>
> On 2/18/2020 1:14 AM, Bryan O'Donoghue wrote:
>> On 16/02/2020 16:07, Dwivedi, Avaneesh Kumar (avani) wrote:
>>>
>>> On 2/4/2020 8:40 AM, Bryan O'Donoghue wrote:
>>>> On 03/02/2020 19:35, Bjorn Andersson wrote:
>>>>> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
>>>>
>>>> Hi Avaneesh.
>>>
>>> Hello Bryan, Thank you very much for your review comments.
>>>
>>> Will be replying to your comments and will be posting new patchset
>>> soon as per review comments.
>>>
>>>>
>>>>> Please aim for keeping the sort order in this file (ignore QCOM_APR
>>>>> which obviously is in the wrong place)
>>>>>
>>>>>> + tristate "QTI Embedded USB Debugger (EUD)"
>>>>>> + depends on ARCH_QCOM
>>>>
>>>> If we persist with the model of EXTCON you should "select EXTCON" here.
>>
>>> I have asked this query with Bjorn Also against his review comments,
>>> whether we need to persist with extcon or need to switch to usb role
>>> switch framework, as we are notifying not only to usb controller but
>>> also to pmic charger so in case we adopt usb role switch then how we
>>> will notify to pmic charger to enable charging battery ? Also as i
>>> mentioned there my dilema is it does not look very apt to model EUD
>>> hw IP as c type connector, so please let me know your views.
>>
>> I think there's a desire to model USB ports as connector child nodes
>> of a USB controllers as opposed to the more generic extcon so, I think
>> the effort should probably be made to model it up as typec.
> this comment is irrespective of your below comment (If we were to
> support Control Peripheral where the local DWC3 controller has the
> signals routed away entirely, then I think we would need to look into
> modelling that in device tree - and using an overlay to show the DWC3
> controller going away in Control Peripheral mode and coming back. )?
Yes, I think irrespective we should model this as a connector not an
extcon and I think you could do think you could do that as a typec
1. Using role-switch
2. Use the regulator API to capture EUD related charger messages
and trigger changes in the PMIC as opposed to using extcon
to notify.
I could be wrong about #2
>> Can that work for you ?
> Did not comprehend this comment fully. if possible can you give some
> example.
My understanding is we are generally being encouraged to model ports as
connectors instead of extcon. I think it is possible to model your port
driver as a typec connector using USB role-switching and the regulator
API i.e. I don't think you really need extcon here.
>> Ah so, the EUD is a mux, that sits between the connector and the
>> controller, routing UTMI signals to an internal USB hub, which in-turn
>> has debug functions attached to the hub...
> Yes that is correct understanding.
>>
>> Can the Arm core see the hub ? I assume not ?
> Not sure what is it mean by "Can the Arm core see the hub"?
In Debug mode will a DWC3 controller in host mode enumerate the internal
hub ? If so, is that a supported use-case ?
>> There are a few different modes - you should probably be clear on
>> which mode it is you are supporting.
>>
>> Normal mode: (Bypass)
>> Port | EUD | Controller
>>
>> Normal + debug hub mode: (Debug)
>> Port | EUD | Controller + HUB -> debug functions
>>
>> Debug hub mode: (Control Peripheral)
>> Port | EUD | HUB -> debug functions
>>
>> its not clear to me from the documentation or the code which mode it
>> is we are targeting to be supported here.
> Its debug mode which we are supporting in driver.
>>
>> I think you should support Debug mode only here, so that the Arm core
>> never has to deal with the situation where the USB connector "goes away".
> Can you please help what you mean by "so that the Arm core never has to
> deal with the situation where the USB connector "goes away""
So my thinking is
- DWC3 in host mode
For argument sake, lets say an external self-powered hub is connected
and a number of USB devices are enumerated
- EUD switches to Control Peripheral mode
In this case what would happen ?
>>
>> If we were to support Control Peripheral where the local DWC3
>> controller has the signals routed away entirely, then I think we would
>> need to look into modelling that in device tree - and using an overlay
>> to show the DWC3 controller going away in Control Peripheral mode and
>> coming back.
> debug mode is set run time via user, i will check how we can model such
> scenario where device tree corresponding to a h/w module is only valid
> in some scenario at run time. if possible please elaborate bit more on
> your suggestion
If Debug mode is all you are trying to do support then I don't think you
really need to model that in DT.
However if intend to support Control Peripheral mode which as I
understand it, switches the UTMI signals away from a DWC3 controller in
Host mode, then I think you would need to use a DT overlay to switch off
the controller, before switching.
That's why I'm asking you about Control Peripheral mode - do you want to
support it - and if so, then what happens to DWC3 in host mode when the
UTMI signals go away ?
I think you've said you only want to support Debug mode, which makes
more sense to me.
Is Debug mode only valid when the DWC3 controller is in
peripheral/device mode and if so, should we be checking/enforcing that
somewhere - DT or EUD-driver code ?
>> Also final thought since the EUD can operate in different modes, it
>> really should be a string that gets passed in - with the string name
>> aligning to the documentation "bypass", "debug" and so on, so that the
>> mode we are switching to is obvious to anybody who has the spec and
>> the driver.
>
> you mean we should document that this driver works in debug mode only?
> not clear on where one should pass "debug" and "bypass" string?
You have a routine to switch to debug mode that takes a parameter from
user-space right ?
Bjorn mentioned you could write 42. My question/suggestion is why isn't
the value written a string which corresponds to the supported modes from
the EUD spec ?
"bypass" as default "debug" the mode you want to add, at a later time
you could optionally add in "control-periperhal" mode.
Makes a little more sense to me than writing just 0, 1 or 42 :) into
your store routine.
---
bod
On 2/18/2020 8:18 PM, Bryan O'Donoghue wrote:
> On 18/02/2020 13:23, Dwivedi, Avaneesh Kumar (avani) wrote:
>>
>> On 2/18/2020 1:14 AM, Bryan O'Donoghue wrote:
>>> On 16/02/2020 16:07, Dwivedi, Avaneesh Kumar (avani) wrote:
>>>>
>>>> On 2/4/2020 8:40 AM, Bryan O'Donoghue wrote:
>>>>> On 03/02/2020 19:35, Bjorn Andersson wrote:
>>>>>> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
>>>>>
>>>>> Hi Avaneesh.
>>>>
>>>> Hello Bryan, Thank you very much for your review comments.
>>>>
>>>> Will be replying to your comments and will be posting new patchset
>>>> soon as per review comments.
>>>>
>>>>>
>>>>>> Please aim for keeping the sort order in this file (ignore QCOM_APR
>>>>>> which obviously is in the wrong place)
>>>>>>
>>>>>>> + tristate "QTI Embedded USB Debugger (EUD)"
>>>>>>> + depends on ARCH_QCOM
>>>>>
>>>>> If we persist with the model of EXTCON you should "select EXTCON"
>>>>> here.
>>>
>>>> I have asked this query with Bjorn Also against his review
>>>> comments, whether we need to persist with extcon or need to switch
>>>> to usb role switch framework, as we are notifying not only to usb
>>>> controller but also to pmic charger so in case we adopt usb role
>>>> switch then how we will notify to pmic charger to enable charging
>>>> battery ? Also as i mentioned there my dilema is it does not look
>>>> very apt to model EUD hw IP as c type connector, so please let me
>>>> know your views.
>>>
>>> I think there's a desire to model USB ports as connector child nodes
>>> of a USB controllers as opposed to the more generic extcon so, I
>>> think the effort should probably be made to model it up as typec.
>> this comment is irrespective of your below comment (If we were to
>> support Control Peripheral where the local DWC3 controller has the
>> signals routed away entirely, then I think we would need to look into
>> modelling that in device tree - and using an overlay to show the DWC3
>> controller going away in Control Peripheral mode and coming back. )?
>
> Yes, I think irrespective we should model this as a connector not an
> extcon and I think you could do think you could do that as a typec
>
> 1. Using role-switch
> 2. Use the regulator API to capture EUD related charger messages
> and trigger changes in the PMIC as opposed to using extcon
> to notify.
>
> I could be wrong about #2
HI Bryan,
Sorry for long pause on this thread, I went through USB role switch
framework and yes we can move to it for notification of VBUS event, but
i am not able to find a good example in upstream, of how battery charger
module can be notified about charger stop and charger start event if we
don't use extcon interface for notification. I am not sure it would be
simple regulator enable and disable call, i will discuss with PMIC guys
on this and will come back.
>
>>> Can that work for you ?
>> Did not comprehend this comment fully. if possible can you give some
>> example.
>
> My understanding is we are generally being encouraged to model ports
> as connectors instead of extcon. I think it is possible to model your
> port driver as a typec connector using USB role-switching and the
> regulator API i.e. I don't think you really need extcon here.
>
>>> Ah so, the EUD is a mux, that sits between the connector and the
>>> controller, routing UTMI signals to an internal USB hub, which
>>> in-turn has debug functions attached to the hub...
>> Yes that is correct understanding.
>>>
>>> Can the Arm core see the hub ? I assume not ?
>> Not sure what is it mean by "Can the Arm core see the hub"?
>
> In Debug mode will a DWC3 controller in host mode enumerate the
> internal hub ? If so, is that a supported use-case ?
In debug mode DWC3 controller will only enumerate in device mode.
>
>>> There are a few different modes - you should probably be clear on
>>> which mode it is you are supporting.
>>>
>>> Normal mode: (Bypass)
>>> Port | EUD | Controller
>>>
>>> Normal + debug hub mode: (Debug)
>>> Port | EUD | Controller + HUB -> debug functions
>>>
>>> Debug hub mode: (Control Peripheral)
>>> Port | EUD | HUB -> debug functions
>>>
>>> its not clear to me from the documentation or the code which mode it
>>> is we are targeting to be supported here.
>> Its debug mode which we are supporting in driver.
>>>
>>> I think you should support Debug mode only here, so that the Arm
>>> core never has to deal with the situation where the USB connector
>>> "goes away".
>> Can you please help what you mean by "so that the Arm core never has
>> to deal with the situation where the USB connector "goes away""
>
> So my thinking is
>
> - DWC3 in host mode
> For argument sake, lets say an external self-powered hub is connected
> and a number of USB devices are enumerated
> - EUD switches to Control Peripheral mode
>
> In this case what would happen ?
I am not getting clarity about this from spec document, what i
understand is in this case PHY signal to USB controller will get
stop(UTMI switch will block signal from USB PHY to USB controller), so
before to switching to control peripheral mode EUD should send detach
event to USB controller so that it can enter low power mode, let me know
if it is grossly wrong understanding. In any case we are not supporting
control peripheral mode in present state of driver.
>
>>>
>>> If we were to support Control Peripheral where the local DWC3
>>> controller has the signals routed away entirely, then I think we
>>> would need to look into modelling that in device tree - and using an
>>> overlay to show the DWC3 controller going away in Control Peripheral
>>> mode and coming back.
>> debug mode is set run time via user, i will check how we can model
>> such scenario where device tree corresponding to a h/w module is only
>> valid in some scenario at run time. if possible please elaborate bit
>> more on your suggestion
>
> If Debug mode is all you are trying to do support then I don't think
> you really need to model that in DT.
>
> However if intend to support Control Peripheral mode which as I
> understand it, switches the UTMI signals away from a DWC3 controller
> in Host mode, then I think you would need to use a DT overlay to
> switch off the controller, before switching.
>
> That's why I'm asking you about Control Peripheral mode - do you want
> to support it - and if so, then what happens to DWC3 in host mode when
> the UTMI signals go away ?
>
> I think you've said you only want to support Debug mode, which makes
> more sense to me.
>
> Is Debug mode only valid when the DWC3 controller is in
> peripheral/device mode and if so, should we be checking/enforcing that
> somewhere - DT or EUD-driver code ?
Yes in debug mode DWC3 controller should always be in device mode, and i
believe this we can insure when we inform USB controller about attach
event after starting in debug mode, using role-switch framework isnt it?
may be i am not getting your statement, how device mode enumeration can
be enforced using DT ?
>
>>> Also final thought since the EUD can operate in different modes, it
>>> really should be a string that gets passed in - with the string name
>>> aligning to the documentation "bypass", "debug" and so on, so that
>>> the mode we are switching to is obvious to anybody who has the spec
>>> and the driver.
>>
>> you mean we should document that this driver works in debug mode
>> only? not clear on where one should pass "debug" and "bypass" string?
>
> You have a routine to switch to debug mode that takes a parameter from
> user-space right ?
>
> Bjorn mentioned you could write 42. My question/suggestion is why
> isn't the value written a string which corresponds to the supported
> modes from the EUD spec ?
> "bypass" as default "debug" the mode you want to add, at a later time
> you could optionally add in "control-periperhal" mode.
>
> Makes a little more sense to me than writing just 0, 1 or 42 :) into
> your store routine.
OK.
>
> ---
> bod
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.