[v4,2/2] Embedded USB Debugger (EUD) driver

Commit Message

Dwivedi, Avaneesh Kumar (avani) Jan. 31, 2020, 4:43 a.m. UTC

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 <[e-mail]>
Signed-off-by: Prakruthi Deepak Heragu <[e-mail]>
Signed-off-by: Avaneesh Kumar Dwivedi <[e-mail]>
---
 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

Comments

Bjorn Andersson Feb. 3, 2020, 7:35 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Prakruthi Deepak Heragu <[e-mail]>
> Signed-off-by: Avaneesh Kumar Dwivedi <[e-mail]>

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 <[e-mail]>
> +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

Bryan O'Donoghue Feb. 4, 2020, 3:10 a.m. UTC | #2

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

Greg KH Feb. 7, 2020, 10:04 a.m. UTC | #3

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 <[e-mail]>
> Signed-off-by: Prakruthi Deepak Heragu <[e-mail]>
> Signed-off-by: Avaneesh Kumar Dwivedi <[e-mail]>
> ---
>  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 <[e-mail]>
> +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

Dwivedi, Avaneesh Kumar (avani) Feb. 16, 2020, 2:14 p.m. UTC | #4

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 <[e-mail]>
>> Signed-off-by: Prakruthi Deepak Heragu <[e-mail]>
>> Signed-off-by: Avaneesh Kumar Dwivedi <[e-mail]>
> 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 <[e-mail]>
>> +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

Dwivedi, Avaneesh Kumar (avani) Feb. 16, 2020, 4:07 p.m. UTC | #5

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

Dwivedi, Avaneesh Kumar (avani) Feb. 16, 2020, 4:22 p.m. UTC | #6

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 <[e-mail]>
>> Signed-off-by: Prakruthi Deepak Heragu <[e-mail]>
>> Signed-off-by: Avaneesh Kumar Dwivedi <[e-mail]>
>> ---
>>   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 <[e-mail]>
>> +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

Greg KH Feb. 16, 2020, 4:35 p.m. UTC | #7

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 <[e-mail]>
> > > Signed-off-by: Prakruthi Deepak Heragu <[e-mail]>
> > > Signed-off-by: Avaneesh Kumar Dwivedi <[e-mail]>
> > > ---
> > >   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 <[e-mail]>
> > > +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

Dwivedi, Avaneesh Kumar (avani) Feb. 17, 2020, 7:21 a.m. UTC | #8

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 <[e-mail]>
>>>> Signed-off-by: Prakruthi Deepak Heragu <[e-mail]>
>>>> Signed-off-by: Avaneesh Kumar Dwivedi <[e-mail]>
>>>> ---
>>>>    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 <[e-mail]>
>>>> +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.

Bryan O'Donoghue Feb. 17, 2020, 7:44 p.m. UTC | #9

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

Bjorn Andersson Feb. 18, 2020, 3:35 a.m. UTC | #10

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

Dwivedi, Avaneesh Kumar (avani) Feb. 18, 2020, 1 p.m. UTC | #11

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

Dwivedi, Avaneesh Kumar (avani) Feb. 18, 2020, 1:23 p.m. UTC | #12

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

Bryan O'Donoghue Feb. 18, 2020, 2:48 p.m. UTC | #13

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

Dwivedi, Avaneesh Kumar (avani) April 4, 2020, 2:12 p.m. UTC | #14

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

Patch

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 <[e-mail]>
+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");