mbox series

[v5,0/5] Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support

Message ID 20230516213308.2432018-1-bhupesh.sharma@linaro.org
Headers show
Series Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support | expand

Message

Bhupesh Sharma May 16, 2023, 9:33 p.m. UTC
Changes since v4:
----------------
- v4 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230505064039.1630025-1-bhupesh.sharma@linaro.org/
- Addressed Konrad's review comments regarding EUD driver code.
- Also collected his R-B for [PATCH 4/5 and 5/5].
- Fixed the dt-bindings as per Krzysztof's comments.

Changes since v3:
----------------
- v3 can be viewed here: https://www.spinics.net/lists/linux-arm-msm/msg137025.html 
- Addressed Konrad's review comments regarding mainly the driver code.
  Also fixed the .dtsi as per his comments.
- Also collected his R-B for [PATCH 1/5].

Changes since v2:
----------------
- v2 can be viewed here: https://www.spinics.net/lists/linux-arm-msm/msg137025.html 
- Addressed Bjorn and Krzysztof's comments.
- Added [PATCH 1/5] which fixes the 'qcom_eud' sysfs path. 
- Added [PATCH 5/5] to enable EUD for Qualcomm QRB4210-RB2 boards.

Changes since v1:
----------------
- v1 can be viewed here: https://lore.kernel.org/linux-arm-msm/20221231130743.3285664-1-bhupesh.sharma@linaro.org
- Added Krzysztof in Cc list.
- Fixed the following issue reported by kernel test bot:
  >> ERROR: modpost: "qcom_scm_io_writel" [drivers/usb/misc/qcom_eud.ko] undefined!

This series adds the dt-binding and driver support for SM6115 / SM4250
EUD (Embedded USB Debugger) block available on Qualcomm SoCs.

It also enables the same for QRB4210-RB2 boards by default (the user
still needs to enable the same via sysfs).

The EUD is a mini-USB hub implemented on chip to support the USB-based debug
and trace capabilities.

EUD driver listens to events like USB attach or detach and then
informs the USB about these events via ROLE-SWITCH.

Bhupesh Sharma (5):
  usb: misc: eud: Fix eud sysfs path (use 'qcom_eud')
  dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support
  usb: misc: eud: Add driver support for SM6115 / SM4250
  arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector
  arm64: dts: qcom: qrb4210-rb2: Enable EUD debug peripheral

 Documentation/ABI/testing/sysfs-driver-eud    |  2 +-
 .../bindings/soc/qcom/qcom,eud.yaml           | 42 ++++++++++-
 arch/arm64/boot/dts/qcom/qrb4210-rb2.dts      | 27 +++++++-
 arch/arm64/boot/dts/qcom/sm6115.dtsi          | 50 ++++++++++++++
 drivers/usb/misc/Kconfig                      |  1 +
 drivers/usb/misc/qcom_eud.c                   | 69 +++++++++++++++++--
 6 files changed, 179 insertions(+), 12 deletions(-)

Comments

Greg KH May 17, 2023, 4:50 a.m. UTC | #1
On Wed, May 17, 2023 at 03:03:06AM +0530, Bhupesh Sharma wrote:
> Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.

Why is the subject line duplicated here?

> On some SoCs (like the SM6115 / SM4250 SoC), the mode manager
> needs to be accessed only via the secure world (through 'scm'
> calls).
> 
> Also, the enable bit inside 'tcsr_check_reg' needs to be set
> first to set the eud in 'enable' mode on these SoCs.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
>  drivers/usb/misc/Kconfig    |  1 +
>  drivers/usb/misc/qcom_eud.c | 69 +++++++++++++++++++++++++++++++++----

Given that you didn't cc the usb maintainer, I'm guessing you don't want
this patch applied?

>  2 files changed, 63 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 99b15b77dfd5..fe1b5fec1dfc 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -147,6 +147,7 @@ config USB_APPLEDISPLAY
>  config USB_QCOM_EUD
>  	tristate "QCOM Embedded USB Debugger(EUD) Driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
> +	select QCOM_SCM

How well is that going to work on building on non-QCOM systems?  Can
QCOM_SCM build if COMPILE_TEST is enabled?  select is rough to get
right, are you sure it's correct here?  If so, some documentation in the
changelog would be appreciated.

>  	select USB_ROLE_SWITCH
>  	help
>  	  This module enables support for Qualcomm Technologies, Inc.
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index b7f13df00764..10d194604d4c 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -5,12 +5,14 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/err.h>
> +#include <linux/firmware/qcom/qcom_scm.h>

There's no rule to keep these sorted, but it's your choice...

>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> @@ -22,23 +24,33 @@
>  #define EUD_REG_VBUS_INT_CLR	0x0080
>  #define EUD_REG_CSR_EUD_EN	0x1014
>  #define EUD_REG_SW_ATTACH_DET	0x1018
> -#define EUD_REG_EUD_EN2        0x0000
> +#define EUD_REG_EUD_EN2		0x0000

Why the coding style cleanup in the same patch?  Remember, changes only
do one thing, and you have already listed 2 things in your commit
message :(

>  
>  #define EUD_ENABLE		BIT(0)
> -#define EUD_INT_PET_EUD	BIT(0)
> +#define EUD_INT_PET_EUD		BIT(0)

Again, why this change?

thanks,

greg k-h
Bhupesh Sharma May 17, 2023, 6:28 a.m. UTC | #2
On Wed, 17 May 2023 at 11:08, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, May 17, 2023 at 03:03:04AM +0530, Bhupesh Sharma wrote:
> > The eud sysfs enablement path is currently mentioned in the
> > Documentation as:
> >   /sys/bus/platform/drivers/eud/.../enable
> >
> > Instead it should be:
> >   /sys/bus/platform/drivers/qcom_eud/.../enable
> >
> > Fix the same.
> >
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>
> I believe the path has changed during one of the EUD patch iterations. In that
> case, the documentation is wrong from day one. So this patch should have the
> relevant Fixes tag.
>
> With that,
>
> Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Ok, I will add a Fixes tag.

Thanks,
Bhupesh

> > ---
> >  Documentation/ABI/testing/sysfs-driver-eud | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-eud b/Documentation/ABI/testing/sysfs-driver-eud
> > index 83f3872182a4..2bab0db2d2f0 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-eud
> > +++ b/Documentation/ABI/testing/sysfs-driver-eud
> > @@ -1,4 +1,4 @@
> > -What:                /sys/bus/platform/drivers/eud/.../enable
> > +What:                /sys/bus/platform/drivers/qcom_eud/.../enable
> >  Date:           February 2022
> >  Contact:        Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> >  Description:
> > --
> > 2.38.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்