Re: [PATCH v4 2/2] Embedded USB Debugger (EUD) driver

From: Dwivedi, Avaneesh Kumar (avani)
Date: Sun Feb 16 2020 - 11:07:43 EST


On 2/4/2020 8:40 AM, Bryan O'Donoghue wrote:
On 03/02/2020 19:35, Bjorn Andersson wrote:
On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:

Hi Avaneesh.

Hello Bryan, Thank you very much for your review comments.

Will be replying to your comments and will be posting new patchset soon as per review comments.


Please aim for keeping the sort order in this file (ignore QCOM_APR
which obviously is in the wrong place)

+ÂÂÂÂÂÂ tristate "QTI Embedded USB Debugger (EUD)"
+ÂÂÂÂÂÂ depends on ARCH_QCOM

If we persist with the model of EXTCON you should "select EXTCON" here.
I have asked this query with Bjorn Also against his review comments, whether we need to persist with extcon or need to switch to usb role switch framework, as we are notifying not only to usb controller but also to pmic charger so in case we adopt usb role switch then how we will notify to pmic charger to enable charging battery ? Also as i mentioned there my dilema is it does not look very apt to model EUD hw IP as c type connector, so please let me know your views.

+ÂÂÂÂÂÂ help
+ÂÂÂÂÂÂÂÂ The Embedded USB Debugger (EUD) driver is a driver for the
+ÂÂÂÂÂÂÂÂ control peripheral which waits on events like USB attach/detach
+ÂÂÂÂÂÂÂÂ and charger enable/disable. The control peripheral further helps
+ÂÂÂÂÂÂÂÂ support the USB-based debug and trace capabilities.
+ÂÂÂÂÂÂÂÂ This module enables support for Qualcomm Technologies, Inc.
+ÂÂÂÂÂÂÂÂ Embedded USB Debugger (EUD).

Suggest.

This module enables support for Qualcomm Technologies, Inc.
Embedded USB Debugger (EUD).
The EUD is a control peripheral which reports VBUS attach/detach, charger enable/disable and USB-based debug and trace capabilities.
OK.


+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.

2020
OK

+
+static int enable_eud(struct eud_chip *priv)
+{
+ÂÂÂ int ret;
+
+ÂÂÂ /* write into CSR to enable EUD */
+ÂÂÂ writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
+ÂÂÂ /* Enable vbus, chgr & safe mode warning interrupts */
+ÂÂÂ writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
+ÂÂÂÂÂÂÂÂÂÂÂ priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
+
+ÂÂÂ /* Ensure Register Writes Complete */

So... You are writing a register in an on-chip PMIC. The PMIC is responsible for detecting USB ID and supplying VBUS as appropriate.

You then get an interrupt to inform you of the state ?

I am writing to EUD control port so that when EUD is enable, EUD hw IP can intercept VBUS and d+/d- signal and can reroute to PMIC or USB as per host application command in debug mode.

so for example in debug mode VBUS signal although asserted on connecting phone with host PC, but EUD based on debug application command can notify USB to detach(which otherwise would have detected and attached)


+static ssize_t enable_store(struct device *dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct device_attribute *attr,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const char *buf, size_t count)
+{
+ÂÂÂ struct eud_chip *chip = dev_get_drvdata(dev);
+ÂÂÂ int enable = 0;

You shouldn't need to initialize this as you're checking the return
value of sscanf().
OK

+ÂÂÂ int ret = 0;
+
+ÂÂÂ if (sscanf(buf, "%du", &enable) != 1)
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ if (enable == EUD_ENABLE_CMD)
+ÂÂÂÂÂÂÂ ret = enable_eud(chip);

If ret is !0 you should probably return that, rather than count...
OK

+ÂÂÂ else if (enable == EUD_DISABLE_CMD)
+ÂÂÂÂÂÂÂ disable_eud(chip);
+ÂÂÂ if (!ret)

...and then you don't need this check, or initialize ret to 0 above.

+ÂÂÂÂÂÂÂ chip->enable = enable;

So if I write 42 to "enable" nothing will change in the hardware, but
chip->enable will be 42...

+ÂÂÂ return count;
+}

I was just going to comment on usb_connector but, does the above code need a synchronization primitive to serialize with the worker and interrupt handler ?
Will evaluate and take corrective action if needed.

+static int msm_eud_probe(struct platform_device *pdev)
+{
+ÂÂÂ struct eud_chip *chip;
+ÂÂÂ struct resource *res;
+ÂÂÂ int ret;
+
+ÂÂÂ chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+ÂÂÂ if (!chip)
+ÂÂÂÂÂÂÂ return -ENOMEM;
+
+ÂÂÂ chip->dev = &pdev->dev;
+ÂÂÂ platform_set_drvdata(pdev, chip);
+
+ÂÂÂ chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable);

Aren't we moving away from extcon in favor of the usb role switching
thing?

Yes.

For the VBUS notification you could use

usb-role-switch and model the USB connector as a child-node of the dual-role controller.

I am not sure if EUD interface is true USB connector or should be modeled so, as EUD can trick usb controller about absence of VBUS/d+/d- signal. To illustrate, when in debug mode even if phone is connected with PC, EUD can notify USB controller to stop USB s/w stack, by notifying USB controller about usb detach event, even when d+\d- signals are valid. moreover in debug mode USB controller will always configure in device mode, so let me know if EUD qualifies to be modeled as child of controller node?



See:
https://patchwork.kernel.org/cover/11346247/
https://patchwork.kernel.org/patch/11346295/
https://patchwork.kernel.org/patch/11346263/

Avaneesh do you have any kernel code that cares about the charger state ?

charger state is to be notified to charger driver to start charging battery so if i switch to usb role switch framework, how will i notify to pmic charger? so if i have to adopt usb role switch framework then also i will have to keep extcon framework, let me know your comment.


What we are suggesting here is dropping extcon and using role-switching but, if you have some other code that cares about EXTCON_CHG_USB_SDP you'd have to do additional work.

But, if I understood the implication of the code above where you write to the PMIC and let it handle VBUS/CHARGER on/off and you are just notified of the state change, you should be fine with usb-role-switching.
as i mentioned usb-role-switch will only cater need to notify to usb controller so please let me know your views.


---
bod

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.