| Message ID | 20230504082644.1461582-1-bhupesh.sharma@linaro.org |
|---|---|
| Headers | show |
| Series | Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support | expand |
On 4.05.2023 10:26, 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. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad > 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:
On 4.05.2023 10:26, Bhupesh Sharma wrote: > Add SM6115 / SM4250 SoC EUD support in qcom_eud driver. > > 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. > > Since this difference comes from how the firmware is configured, so > the driver now relies on the presence of an extra boolean DT property > to identify if secure access is needed. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > drivers/usb/misc/Kconfig | 1 + > drivers/usb/misc/qcom_eud.c | 66 ++++++++++++++++++++++++++++++++++--- > 2 files changed, 62 insertions(+), 5 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 > 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..b4736edcc64c 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> > #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> > @@ -30,15 +32,22 @@ > #define EUD_INT_SAFE_MODE BIT(4) > #define EUD_INT_ALL (EUD_INT_VBUS | EUD_INT_SAFE_MODE) > > +struct eud_soc_cfg { > + u32 tcsr_check_offset; > +}; Not sure if turning this into a struct is necessary.. can't we just store the offset, or do we expect more changes? > + > struct eud_chip { > struct device *dev; > struct usb_role_switch *role_sw; > + const struct eud_soc_cfg *eud_cfg; > void __iomem *base; > void __iomem *mode_mgr; > unsigned int int_status; > int irq; > bool enabled; > bool usb_attached; > + bool secure_mode_enable; > + phys_addr_t secure_mode_mgr; > }; > > static int enable_eud(struct eud_chip *priv) > @@ -46,7 +55,11 @@ static int enable_eud(struct eud_chip *priv) > writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN); > writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE, > priv->base + EUD_REG_INT1_EN_MASK); > - writel(1, priv->mode_mgr + EUD_REG_EUD_EN2); > + > + if (priv->secure_mode_mgr) > + qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, BIT(0)); #define [field name] BIT(0) > + else > + writel(1, priv->mode_mgr + EUD_REG_EUD_EN2); s/1/[field name]/ > > return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); > } > @@ -54,7 +67,11 @@ static int enable_eud(struct eud_chip *priv) > static void disable_eud(struct eud_chip *priv) > { > writel(0, priv->base + EUD_REG_CSR_EUD_EN); > - writel(0, priv->mode_mgr + EUD_REG_EUD_EN2); > + > + if (priv->secure_mode_mgr) > + qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, 0); > + else > + writel(0, priv->mode_mgr + EUD_REG_EUD_EN2); > } > > static ssize_t enable_show(struct device *dev, > @@ -178,12 +195,15 @@ static void eud_role_switch_release(void *data) > static int eud_probe(struct platform_device *pdev) > { > struct eud_chip *chip; > + struct resource *res; > + phys_addr_t tcsr_base, tcsr_check; > int ret; > > chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > if (!chip) > return -ENOMEM; > > + ? > chip->dev = &pdev->dev; > > chip->role_sw = usb_role_switch_get(&pdev->dev); > @@ -200,9 +220,40 @@ static int eud_probe(struct platform_device *pdev) > if (IS_ERR(chip->base)) > return PTR_ERR(chip->base); > > - chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1); > - if (IS_ERR(chip->mode_mgr)) > - return PTR_ERR(chip->mode_mgr); > + chip->secure_mode_enable = of_property_read_bool(chip->dev->of_node, > + "qcom,secure-mode-enable"); If we map this region iff it's supposed to be used, we may just check for its presence and skip the additional property. Then, the address being non-NULL would invalidate the boolean property. > + /* > + * EUD block on a few Qualcomm SoCs need secure register access. > + * Check for the same. > + */ > + if (chip->secure_mode_enable) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) > + return dev_err_probe(chip->dev, -ENODEV, > + "failed to get secure_mode_mgr reg base\n"); > + > + chip->secure_mode_mgr = res->start; > + } else { > + chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(chip->mode_mgr)) > + return PTR_ERR(chip->mode_mgr); > + } > + > + /* Check for any SoC specific config data */ > + chip->eud_cfg = of_device_get_match_data(&pdev->dev); > + if (chip->eud_cfg) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); I'd vouch to use _byname, in case we get some EUD impl that needs a different sort of a register set.. > + if (!res) > + return dev_err_probe(chip->dev, -ENODEV, > + "failed to get tcsr reg base\n"); > + > + tcsr_base = res->start; > + tcsr_check = tcsr_base + chip->eud_cfg->tcsr_check_offset; > + > + ret = qcom_scm_io_writel(tcsr_check, BIT(0)); s/BIT(0)/.. Konrad > + if (ret) > + return dev_err_probe(chip->dev, ret, "failed to write tcsr check reg\n"); > + } > > chip->irq = platform_get_irq(pdev, 0); > ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq, > @@ -230,8 +281,13 @@ static int eud_remove(struct platform_device *pdev) > return 0; > } > > +static const struct eud_soc_cfg sm6115_eud_cfg = { > + .tcsr_check_offset = 0x25018, > +}; > + > static const struct of_device_id eud_dt_match[] = { > { .compatible = "qcom,sc7280-eud" }, > + { .compatible = "qcom,sm6115-eud", .data = &sm6115_eud_cfg }, > { } > }; > MODULE_DEVICE_TABLE(of, eud_dt_match);
On 4.05.2023 10:26, Bhupesh Sharma wrote: > Add the Embedded USB Debugger(EUD) device tree node for > SM6115 / SM4250 SoC. > > The node contains EUD base register region, EUD mode manager > register region and TCSR Base register region along with the > interrupt entry. > > Also add the typec connector node for EUD which is attached to > EUD node via port. EUD is also attached to DWC3 node via port. > > To enable the role switch, we need to set dr_mode = "otg" property > for 'usb_dwc3' sub-node in the board dts file. > > Also the EUD device can be enabled on a board once linux is boot'ed > by setting: > $ echo 1 > /sys/bus/platform/drivers/qcom_eud/../enable > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > arch/arm64/boot/dts/qcom/sm6115.dtsi | 48 ++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > index f67863561f3f..6865342fc42f 100644 > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > @@ -180,6 +180,18 @@ core3 { > }; > }; > > + eud_typec: connector { > + compatible = "usb-c-connector"; > + > + ports { > + port@0 { > + con_eud: endpoint { > + remote-endpoint = <&eud_con>; > + }; > + }; > + }; > + }; > + > firmware { > scm: scm { > compatible = "qcom,scm-sm6115", "qcom,scm"; > @@ -647,6 +659,35 @@ gcc: clock-controller@1400000 { > #power-domain-cells = <1>; > }; > > + eud: eud@1610000 { > + compatible = "qcom,sm6115-eud", "qcom,eud"; > + reg = <0x0 0x01610000 0x0 0x2000>, > + <0x0 0x01612000 0x0 0x1000>, > + <0x0 0x003c0000 0x0 0x40000>; > + interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>; > + qcom,secure-mode-enable; > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; Please insert a newline before each child node. Konrad > + eud_ep: endpoint { > + remote-endpoint = <&usb2_role_switch>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + eud_con: endpoint { > + remote-endpoint = <&con_eud>; > + }; > + }; > + }; > + }; > + > usb_hsphy: phy@1613000 { > compatible = "qcom,sm6115-qusb2-phy"; > reg = <0x0 0x01613000 0x0 0x180>; > @@ -1144,6 +1185,13 @@ usb_dwc3: usb@4e00000 { > snps,has-lpm-erratum; > snps,hird-threshold = /bits/ 8 <0x10>; > snps,usb3_lpm_capable; > + usb-role-switch; > + > + port { > + usb2_role_switch: endpoint { > + remote-endpoint = <&eud_ep>; > + }; > + }; > }; > }; >
On 4.05.2023 10:26, Bhupesh Sharma wrote: > Since the USB-C type port on the Qualcomm QRB4210-RB2 board > can be set primarily in a 'device' configuration (with the default > DIP switch settings), it makes sense to enable the EUD debug > peripheral on the board by default by setting the USB 'dr_mode' property > as 'otg'. > > Now, the EUD debug peripheral can be enabled by executing: > $ echo 1 > /sys/bus/platform/drivers/qcom_eud/1610000.eud/enable > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > arch/arm64/boot/dts/qcom/qrb4210-rb2.dts | 27 +++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts b/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts > index 1a0776a0cfd0..0ce72f1ebc10 100644 > --- a/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts > +++ b/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts > @@ -30,6 +30,10 @@ vph_pwr: vph-pwr-regulator { > }; > }; > > +&eud { > + status = "okay"; > +}; > + > &qupv3_id_0 { > status = "okay"; > }; > @@ -253,7 +257,28 @@ &usb { > > &usb_dwc3 { > maximum-speed = "super-speed"; > - dr_mode = "peripheral"; > + > + /* > + * There is only one USB DWC3 controller on QRB4210 board and it is connected > + * via a DIP Switch: > + * - to either an USB - C type connector or an USB - A type connector > + * (via a GL3590-S hub), and > + * - to either an USB - A type connector (via a GL3590-S hub) or a connector > + * for further connection with a mezzanine board. > + * > + * All of the above hardware muxes would allow us to hook things up in > + * different ways to some potential benefit for static configurations (for e.g. > + * on one hand we can have two USB - A type connectors and a USB - Ethernet > + * connection available and on the other we can use the USB - C type in > + * peripheral mode). > + * > + * Note that since the USB - C type can be used only in peripehral mode, > + * so hardcoding the mode to 'peripheral' here makes sense. > + * > + * However since we want to use the EUD debug device, we set the mode as > + * 'otg' here. > + */ > + dr_mode = "otg"; So if I understand correctly, EUD works via the type-C connector and only when the switch is turned such that the type-C connector is in use? Konrad > }; > > &usb_hsphy {
On Thu, 4 May 2023 at 15:38, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > On 4.05.2023 10:26, Bhupesh Sharma wrote: > > Since the USB-C type port on the Qualcomm QRB4210-RB2 board > > can be set primarily in a 'device' configuration (with the default > > DIP switch settings), it makes sense to enable the EUD debug > > peripheral on the board by default by setting the USB 'dr_mode' property > > as 'otg'. > > > > Now, the EUD debug peripheral can be enabled by executing: > > $ echo 1 > /sys/bus/platform/drivers/qcom_eud/1610000.eud/enable > > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/qrb4210-rb2.dts | 27 +++++++++++++++++++++++- > > 1 file changed, 26 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts b/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts > > index 1a0776a0cfd0..0ce72f1ebc10 100644 > > --- a/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts > > +++ b/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts > > @@ -30,6 +30,10 @@ vph_pwr: vph-pwr-regulator { > > }; > > }; > > > > +&eud { > > + status = "okay"; > > +}; > > + > > &qupv3_id_0 { > > status = "okay"; > > }; > > @@ -253,7 +257,28 @@ &usb { > > > > &usb_dwc3 { > > maximum-speed = "super-speed"; > > - dr_mode = "peripheral"; > > + > > + /* > > + * There is only one USB DWC3 controller on QRB4210 board and it is connected > > + * via a DIP Switch: > > + * - to either an USB - C type connector or an USB - A type connector > > + * (via a GL3590-S hub), and > > + * - to either an USB - A type connector (via a GL3590-S hub) or a connector > > + * for further connection with a mezzanine board. > > + * > > + * All of the above hardware muxes would allow us to hook things up in > > + * different ways to some potential benefit for static configurations (for e.g. > > + * on one hand we can have two USB - A type connectors and a USB - Ethernet > > + * connection available and on the other we can use the USB - C type in > > + * peripheral mode). > > + * > > + * Note that since the USB - C type can be used only in peripehral mode, > > + * so hardcoding the mode to 'peripheral' here makes sense. > > + * > > + * However since we want to use the EUD debug device, we set the mode as > > + * 'otg' here. > > + */ > > + dr_mode = "otg"; > So if I understand correctly, EUD works via the type-C connector and > only when the switch is turned such that the type-C connector is in use? That's right - which by the way is also the default selection (USB-c type) as per the board schematics. The whole point of adding the text here was to explain the possible connections available on the board and why we selected the 'otg' mode. Thanks, Bhupesh
On Thu, 4 May 2023 at 15:23, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 4.05.2023 10:26, Bhupesh Sharma wrote: > > Add the Embedded USB Debugger(EUD) device tree node for > > SM6115 / SM4250 SoC. > > > > The node contains EUD base register region, EUD mode manager > > register region and TCSR Base register region along with the > > interrupt entry. > > > > Also add the typec connector node for EUD which is attached to > > EUD node via port. EUD is also attached to DWC3 node via port. > > > > To enable the role switch, we need to set dr_mode = "otg" property > > for 'usb_dwc3' sub-node in the board dts file. > > > > Also the EUD device can be enabled on a board once linux is boot'ed > > by setting: > > $ echo 1 > /sys/bus/platform/drivers/qcom_eud/../enable > > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/sm6115.dtsi | 48 ++++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > > index f67863561f3f..6865342fc42f 100644 > > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > > @@ -180,6 +180,18 @@ core3 { > > }; > > }; > > > > + eud_typec: connector { > > + compatible = "usb-c-connector"; > > + > > + ports { > > + port@0 { > > + con_eud: endpoint { > > + remote-endpoint = <&eud_con>; > > + }; > > + }; > > + }; > > + }; > > + > > firmware { > > scm: scm { > > compatible = "qcom,scm-sm6115", "qcom,scm"; > > @@ -647,6 +659,35 @@ gcc: clock-controller@1400000 { > > #power-domain-cells = <1>; > > }; > > > > + eud: eud@1610000 { > > + compatible = "qcom,sm6115-eud", "qcom,eud"; > > + reg = <0x0 0x01610000 0x0 0x2000>, > > + <0x0 0x01612000 0x0 0x1000>, > > + <0x0 0x003c0000 0x0 0x40000>; > > + interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>; > > + qcom,secure-mode-enable; > > + status = "disabled"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > Please insert a newline before each child node. Ok, I will do it in next version. Thanks, Bhupesh
On Thu, 4 May 2023 at 15:22, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 4.05.2023 10:26, Bhupesh Sharma wrote: > > Add SM6115 / SM4250 SoC EUD support in qcom_eud driver. > > > > 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. > > > > Since this difference comes from how the firmware is configured, so > > the driver now relies on the presence of an extra boolean DT property > > to identify if secure access is needed. > > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > --- > > drivers/usb/misc/Kconfig | 1 + > > drivers/usb/misc/qcom_eud.c | 66 ++++++++++++++++++++++++++++++++++--- > > 2 files changed, 62 insertions(+), 5 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 > > 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..b4736edcc64c 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> > > #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> > > @@ -30,15 +32,22 @@ > > #define EUD_INT_SAFE_MODE BIT(4) > > #define EUD_INT_ALL (EUD_INT_VBUS | EUD_INT_SAFE_MODE) > > > > +struct eud_soc_cfg { > > + u32 tcsr_check_offset; > > +}; > Not sure if turning this into a struct is necessary.. can't > we just store the offset, or do we expect more changes? I can see future versions already supporting newer features, so I kept it a struct for now. > > + > > struct eud_chip { > > struct device *dev; > > struct usb_role_switch *role_sw; > > + const struct eud_soc_cfg *eud_cfg; > > void __iomem *base; > > void __iomem *mode_mgr; > > unsigned int int_status; > > int irq; > > bool enabled; > > bool usb_attached; > > + bool secure_mode_enable; > > + phys_addr_t secure_mode_mgr; > > }; > > > > static int enable_eud(struct eud_chip *priv) > > @@ -46,7 +55,11 @@ static int enable_eud(struct eud_chip *priv) > > writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN); > > writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE, > > priv->base + EUD_REG_INT1_EN_MASK); > > - writel(1, priv->mode_mgr + EUD_REG_EUD_EN2); > > + > > + if (priv->secure_mode_mgr) > > + qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, BIT(0)); > #define [field name] BIT(0) Ok. > > + else > > + writel(1, priv->mode_mgr + EUD_REG_EUD_EN2); > s/1/[field name]/ Ok. > > return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); > > } > > @@ -54,7 +67,11 @@ static int enable_eud(struct eud_chip *priv) > > static void disable_eud(struct eud_chip *priv) > > { > > writel(0, priv->base + EUD_REG_CSR_EUD_EN); > > - writel(0, priv->mode_mgr + EUD_REG_EUD_EN2); > > + > > + if (priv->secure_mode_mgr) > > + qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, 0); > > + else > > + writel(0, priv->mode_mgr + EUD_REG_EUD_EN2); > > } > > > > static ssize_t enable_show(struct device *dev, > > @@ -178,12 +195,15 @@ static void eud_role_switch_release(void *data) > > static int eud_probe(struct platform_device *pdev) > > { > > struct eud_chip *chip; > > + struct resource *res; > > + phys_addr_t tcsr_base, tcsr_check; > > int ret; > > > > chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > > if (!chip) > > return -ENOMEM; > > > > + > ? Oops, I will fix it in v4. > > chip->dev = &pdev->dev; > > > > chip->role_sw = usb_role_switch_get(&pdev->dev); > > @@ -200,9 +220,40 @@ static int eud_probe(struct platform_device *pdev) > > if (IS_ERR(chip->base)) > > return PTR_ERR(chip->base); > > > > - chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1); > > - if (IS_ERR(chip->mode_mgr)) > > - return PTR_ERR(chip->mode_mgr); > > + chip->secure_mode_enable = of_property_read_bool(chip->dev->of_node, > > + "qcom,secure-mode-enable"); > If we map this region iff it's supposed to be used, we may just check > for its presence and skip the additional property. Then, the address > being non-NULL would invalidate the boolean property. Bjorn requested during the review of the last version that we should not ioremap the secure mode_mgr region. So, I followed this approach instead. > > + /* > > + * EUD block on a few Qualcomm SoCs need secure register access. > > + * Check for the same. > > + */ > > + if (chip->secure_mode_enable) { > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + if (!res) > > + return dev_err_probe(chip->dev, -ENODEV, > > + "failed to get secure_mode_mgr reg base\n"); > > + > > + chip->secure_mode_mgr = res->start; > > + } else { > > + chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1); > > + if (IS_ERR(chip->mode_mgr)) > > + return PTR_ERR(chip->mode_mgr); > > + } > > + > > + /* Check for any SoC specific config data */ > > + chip->eud_cfg = of_device_get_match_data(&pdev->dev); > > + if (chip->eud_cfg) { > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); > I'd vouch to use _byname, in case we get some EUD impl that needs a > different sort of a register set.. Sure, it makes sense. > > + if (!res) > > + return dev_err_probe(chip->dev, -ENODEV, > > + "failed to get tcsr reg base\n"); > > + > > + tcsr_base = res->start; > > + tcsr_check = tcsr_base + chip->eud_cfg->tcsr_check_offset; > > + > > + ret = qcom_scm_io_writel(tcsr_check, BIT(0)); > s/BIT(0)/.. Ok. Thanks, Bhupesh
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 | 16 +++++ arch/arm64/boot/dts/qcom/qrb4210-rb2.dts | 27 +++++++- arch/arm64/boot/dts/qcom/sm6115.dtsi | 48 ++++++++++++++ drivers/usb/misc/Kconfig | 1 + drivers/usb/misc/qcom_eud.c | 66 +++++++++++++++++-- 6 files changed, 153 insertions(+), 7 deletions(-)