| Message ID | 20230718061052.1332993-1-bhupesh.sharma@linaro.org |
|---|---|
| Headers | show |
| Series | Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support | expand |
On 18/07/2023 08:10, 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. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > drivers/usb/misc/Kconfig | 2 +- > drivers/usb/misc/qcom_eud.c | 76 ++++++++++++++++++++++++++++++++++--- > 2 files changed, 72 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > index 99b15b77dfd57..51eb5140caa14 100644 > --- a/drivers/usb/misc/Kconfig > +++ b/drivers/usb/misc/Kconfig > @@ -146,7 +146,7 @@ config USB_APPLEDISPLAY > > config USB_QCOM_EUD > tristate "QCOM Embedded USB Debugger(EUD) Driver" > - depends on ARCH_QCOM || COMPILE_TEST > + depends on (ARCH_QCOM && QCOM_SCM) || COMPILE_TEST > 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 7f371ea1248c3..a5b28fc24116a 100644 > --- a/drivers/usb/misc/qcom_eud.c > +++ b/drivers/usb/misc/qcom_eud.c > @@ -11,9 +11,12 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/slab.h> > #include <linux/sysfs.h> > +#include <linux/firmware/qcom/qcom_scm.h> > #include <linux/usb/role.h> > > #define EUD_REG_INT1_EN_MASK 0x0024 > @@ -30,6 +33,10 @@ > #define EUD_INT_SAFE_MODE BIT(4) > #define EUD_INT_ALL (EUD_INT_VBUS | EUD_INT_SAFE_MODE) > > +#define EUD_EN2_EN BIT(0) > +#define EUD_EN2_DISABLE (0) > +#define TCSR_CHECK_EN BIT(0) > + > struct eud_chip { > struct device *dev; > struct usb_role_switch *role_sw; > @@ -39,6 +46,7 @@ struct eud_chip { > int irq; > bool enabled; > bool usb_attached; > + phys_addr_t secure_mode_mgr; > }; > > static int enable_eud(struct eud_chip *priv) > @@ -46,7 +54,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, EUD_EN2_EN); > + else > + writel(EUD_EN2_EN, priv->mode_mgr + EUD_REG_EUD_EN2); > > return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); > } > @@ -54,7 +66,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, EUD_EN2_DISABLE); > + else > + writel(EUD_EN2_DISABLE, priv->mode_mgr + EUD_REG_EUD_EN2); > } > > static ssize_t enable_show(struct device *dev, > @@ -175,9 +191,37 @@ static void eud_role_switch_release(void *data) > usb_role_switch_put(chip->role_sw); > } > > +static int eud_find_secure_reg_addr(struct device *dev, u64 *addr) > +{ > + struct device_node *tcsr; > + struct device_node *np = dev->of_node; > + struct resource res; > + u32 offset; > + int ret; > + > + tcsr = of_parse_phandle(np, "qcom,secure-eud-reg", 0); > + if (!tcsr) > + return 0; > + > + ret = of_address_to_resource(tcsr, 0, &res); > + of_node_put(tcsr); > + if (ret) > + return ret; > + > + ret = of_property_read_u32_index(np, "qcom,secure-eud-reg", 1, &offset); > + if (ret < 0) > + return ret; > + > + *addr = res.start + offset; > + > + return 0; > +} > + > static int eud_probe(struct platform_device *pdev) > { > struct eud_chip *chip; > + struct resource *res; > + phys_addr_t tcsr_check = 0; > int ret; > > chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > @@ -200,9 +244,30 @@ 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); > + /* > + * EUD block on a few Qualcomm SoCs needs secure register access. > + * Check for the same via vendor-specific dt property. > + */ > + ret = eud_find_secure_reg_addr(&pdev->dev, &tcsr_check); > + if (ret < 0) > + return ret; > + > + if (tcsr_check) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); I don't understand this code. If you have syscon to eud reg, then why you are not using it, but instead map again second address space? > + if (!res) > + return dev_err_probe(chip->dev, -ENODEV, > + "failed to get secure_mode_mgr reg base\n"); > + > + chip->secure_mode_mgr = res->start; > + > + ret = qcom_scm_io_writel(tcsr_check, TCSR_CHECK_EN); That's not how syscon is used. Your bindings defined it as syscon, so are you sure you are using it correctly? Best regards, Krzysztof
Hi Bhupesh, On 7/18/2023 11:40 AM, 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. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > drivers/usb/misc/Kconfig | 2 +- > drivers/usb/misc/qcom_eud.c | 76 ++++++++++++++++++++++++++++++++++--- > 2 files changed, 72 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > index 99b15b77dfd57..51eb5140caa14 100644 > --- a/drivers/usb/misc/Kconfig > +++ b/drivers/usb/misc/Kconfig > @@ -146,7 +146,7 @@ config USB_APPLEDISPLAY > > config USB_QCOM_EUD > tristate "QCOM Embedded USB Debugger(EUD) Driver" > - depends on ARCH_QCOM || COMPILE_TEST > + depends on (ARCH_QCOM && QCOM_SCM) || COMPILE_TEST > 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 7f371ea1248c3..a5b28fc24116a 100644 > --- a/drivers/usb/misc/qcom_eud.c > +++ b/drivers/usb/misc/qcom_eud.c > @@ -11,9 +11,12 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/slab.h> > #include <linux/sysfs.h> > +#include <linux/firmware/qcom/qcom_scm.h> > #include <linux/usb/role.h> > > #define EUD_REG_INT1_EN_MASK 0x0024 > @@ -30,6 +33,10 @@ > #define EUD_INT_SAFE_MODE BIT(4) > #define EUD_INT_ALL (EUD_INT_VBUS | EUD_INT_SAFE_MODE) > > +#define EUD_EN2_EN BIT(0) > +#define EUD_EN2_DISABLE (0) > +#define TCSR_CHECK_EN BIT(0) > + > struct eud_chip { > struct device *dev; > struct usb_role_switch *role_sw; > @@ -39,6 +46,7 @@ struct eud_chip { > int irq; > bool enabled; > bool usb_attached; > + phys_addr_t secure_mode_mgr; > }; > > static int enable_eud(struct eud_chip *priv) > @@ -46,7 +54,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, EUD_EN2_EN); > + else > + writel(EUD_EN2_EN, priv->mode_mgr + EUD_REG_EUD_EN2); > > return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE); > } > @@ -54,7 +66,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, EUD_EN2_DISABLE); > + else > + writel(EUD_EN2_DISABLE, priv->mode_mgr + EUD_REG_EUD_EN2); > } > > static ssize_t enable_show(struct device *dev, > @@ -175,9 +191,37 @@ static void eud_role_switch_release(void *data) > usb_role_switch_put(chip->role_sw); > } > > +static int eud_find_secure_reg_addr(struct device *dev, u64 *addr) > +{ > + struct device_node *tcsr; > + struct device_node *np = dev->of_node; > + struct resource res; > + u32 offset; > + int ret; > + > + tcsr = of_parse_phandle(np, "qcom,secure-eud-reg", 0); > + if (!tcsr) > + return 0; > + > + ret = of_address_to_resource(tcsr, 0, &res); > + of_node_put(tcsr); > + if (ret) > + return ret; > + > + ret = of_property_read_u32_index(np, "qcom,secure-eud-reg", 1, &offset); > + if (ret < 0) > + return ret; > + > + *addr = res.start + offset; > + > + return 0; > +} > + > static int eud_probe(struct platform_device *pdev) > { > struct eud_chip *chip; > + struct resource *res; > + phys_addr_t tcsr_check = 0; > int ret; > > chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > @@ -200,9 +244,30 @@ 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); > + /* > + * EUD block on a few Qualcomm SoCs needs secure register access. > + * Check for the same via vendor-specific dt property. > + */ > + ret = eud_find_secure_reg_addr(&pdev->dev, &tcsr_check); > + if (ret < 0) > + return ret; > + > + if (tcsr_check) { > + 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; There are multiple instances where the addresses are being mapped from the dt property without using the devm version. Either we should switch to the later or ensure that these addresses are unmapped in the removal path of the driver. Thanks, Souradeep > + > + ret = qcom_scm_io_writel(tcsr_check, TCSR_CHECK_EN); > + if (ret) > + return dev_err_probe(chip->dev, ret, "failed to write tcsr check reg\n"); > + } else { > + chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(chip->mode_mgr)) > + return PTR_ERR(chip->mode_mgr); > + } > > chip->irq = platform_get_irq(pdev, 0); > ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq, > @@ -230,6 +295,7 @@ static void eud_remove(struct platform_device *pdev) > > static const struct of_device_id eud_dt_match[] = { > { .compatible = "qcom,sc7280-eud" }, > + { .compatible = "qcom,sm6115-eud" }, > { } > }; > MODULE_DEVICE_TABLE(of, eud_dt_match);
On 18.07.2023 08:10, Bhupesh Sharma wrote: > Add the TCSR syscon dt node for SM6115 / SM4250 SoC. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
Changes since v8: ------------------- - v8 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230717103236.1246771-1-bhupesh.sharma@linaro.org/ - Konrad and Stephan pointed that I should define 'tcsr syscon' node for sm6115.dtsi, and use phandle for the same inside the EUD node, which would eventually be used inside the eud driver. Added [PATCH 1/7] and [PATCH 5/7] for the same in this series. - Rebased on latest linux-next/master. Changes since v6/v7: ------------------- - v6 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230517211756.2483552-1-bhupesh.sharma@linaro.org/ - Konrad and Krzysztof had different suggestions on how to tackle different SoCs inside the eud driver which require access to secure mode manager register space. While Konrad's suggestion was to use a dt property, other comments suggested using optional platform data for determining the same. Modified [PATCH 2/4] accordingly to use the optional platform data for now. - Added Krzysztof's RB for [PATCH 1/4] and also addressed his review comments received on v5. - Dropped eud cleanup patches (which were sent a v7) as they have been accepted in linux-next. - Rebased on latest linux-next/master. Changes since v5: ---------------- - v5 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230516213308.2432018-1-bhupesh.sharma@linaro.org/ - Addressed Mani's comment and added Fixes tag for [PATCH 1/6]. Also collected his Ack for this patch. - Fixed [PATCH 4/6] as per Greg's comments and added a separate patch for identation issues -> [PATCH 3/6]. 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 (7): dt-bindings: mfd: qcom,tcsr: Add the compatible for SM6115 dt-bindings: soc: qcom: eud: Document vendor-specific 'secure mode' property dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support usb: misc: eud: Add driver support for SM6115 / SM4250 arm64: dts: qcom: sm6115: Add tcsr syscon node arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector arm64: dts: qcom: qrb4210-rb2: Enable EUD debug peripheral .../devicetree/bindings/mfd/qcom,tcsr.yaml | 1 + .../bindings/soc/qcom/qcom,eud.yaml | 9 +++ arch/arm64/boot/dts/qcom/qrb4210-rb2.dts | 27 ++++++- arch/arm64/boot/dts/qcom/sm6115.dtsi | 56 ++++++++++++++ drivers/usb/misc/Kconfig | 2 +- drivers/usb/misc/qcom_eud.c | 76 +++++++++++++++++-- 6 files changed, 164 insertions(+), 7 deletions(-)