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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux