| Message ID | 20230516213308.2432018-1-bhupesh.sharma@linaro.org |
|---|---|
| Headers | show |
| Series | Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support | expand |
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
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> - Mani > --- > 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 >
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 > > > > -- > மணிவண்ணன் சதாசிவம்
Hi Greg, On Wed, 17 May 2023 at 10:21, Greg KH <gregkh@linuxfoundation.org> wrote: > > 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? Oops, I will do that in the next version. > > 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. Ok, I will double check. > > 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... Sure. > > #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 :( Sure, will spin a separate patch for cleanups. > > #define EUD_ENABLE BIT(0) > > -#define EUD_INT_PET_EUD BIT(0) > > +#define EUD_INT_PET_EUD BIT(0) > > Again, why this change? Ack. Will send a v6 shortly. Thanks, Bhupesh
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(-)