| Message ID | 20230517211756.2483552-5-bhupesh.sharma@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support | expand |
On 17.05.2023 23:17, 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 | 65 ++++++++++++++++++++++++++++++++++--- > 2 files changed, 61 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > index 99b15b77dfd5..51eb5140caa1 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 74f2aeaccdcb..6face21b7fb7 100644 > --- a/drivers/usb/misc/qcom_eud.c > +++ b/drivers/usb/misc/qcom_eud.c > @@ -11,9 +11,11 @@ > #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> > +#include <linux/firmware/qcom/qcom_scm.h> > #include <linux/usb/role.h> > > #define EUD_REG_INT1_EN_MASK 0x0024 > @@ -30,15 +32,25 @@ > #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_soc_cfg { > + u32 tcsr_check_offset; > +}; > + > 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; > + phys_addr_t secure_mode_mgr; > }; > > static int enable_eud(struct eud_chip *priv) > @@ -46,7 +58,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 +70,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, > @@ -178,6 +198,8 @@ 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_check; > int ret; > > chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > @@ -200,9 +222,37 @@ 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. > + */ > + if (of_device_is_compatible(chip->dev->of_node, "qcom,sm6115-eud")) { I didn't notice that this changed between v4 and v5, but in my v4 review I suggested using if (of_property_read_bool(chip->dev->of_node, "qcom,secure-mode-enable")) as this was the only place where the value of that function was checked and caching it in the driver struct simply made no sense (as of today, anyway) checking the device compatible does not scale very well for something generic, as now it'd require adding each qcom,smABCD-eud to this condition as well. > + 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"); This suggests the reg-name is "secure_mode_mgr" which is not true, according to your binding patch. I thought about adding a separate entry, but ultimately this would be against the DT philosophy, as it references the same physical region as "eud-mode-mgr", just that due to ACL software running at a higher exception level it's not directly accessible.. I was debating suggesting moving it to SoC configuration, but that also depends on the software stack (e.g. there are windows and cros 7280 laptops with different security restrictions).. so I think the dt property is the way to go. Konrad > + > + 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_byname(pdev, IORESOURCE_MEM, "tcsr-base"); > + if (!res) > + return dev_err_probe(chip->dev, -ENODEV, > + "failed to get tcsr reg base\n"); > + > + tcsr_check = res->start + chip->eud_cfg->tcsr_check_offset; > + > + 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"); > + } > > chip->irq = platform_get_irq(pdev, 0); > ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq, > @@ -230,8 +280,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 Thu, 18 May 2023 at 15:58, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 17.05.2023 23:17, 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 | 65 ++++++++++++++++++++++++++++++++++--- > > 2 files changed, 61 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > > index 99b15b77dfd5..51eb5140caa1 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 74f2aeaccdcb..6face21b7fb7 100644 > > --- a/drivers/usb/misc/qcom_eud.c > > +++ b/drivers/usb/misc/qcom_eud.c > > @@ -11,9 +11,11 @@ > > #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> > > +#include <linux/firmware/qcom/qcom_scm.h> > > #include <linux/usb/role.h> > > > > #define EUD_REG_INT1_EN_MASK 0x0024 > > @@ -30,15 +32,25 @@ > > #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_soc_cfg { > > + u32 tcsr_check_offset; > > +}; > > + > > 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; > > + phys_addr_t secure_mode_mgr; > > }; > > > > static int enable_eud(struct eud_chip *priv) > > @@ -46,7 +58,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 +70,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, > > @@ -178,6 +198,8 @@ 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_check; > > int ret; > > > > chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > > @@ -200,9 +222,37 @@ 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. > > + */ > > + if (of_device_is_compatible(chip->dev->of_node, "qcom,sm6115-eud")) { > I didn't notice that this changed between v4 and v5, but in my v4 review > I suggested using > > if (of_property_read_bool(chip->dev->of_node, "qcom,secure-mode-enable")) > > as this was the only place where the value of that function was checked > and caching it in the driver struct simply made no sense (as of today, anyway) > > checking the device compatible does not scale very well for something > generic, as now it'd require adding each qcom,smABCD-eud to this condition > as well. > > > + 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"); > This suggests the reg-name is "secure_mode_mgr" which is not true, > according to your binding patch. I thought about adding a separate > entry, but ultimately this would be against the DT philosophy, as it > references the same physical region as "eud-mode-mgr", just that due > to ACL software running at a higher exception level it's not > directly accessible.. > > I was debating suggesting moving it to SoC configuration, but that > also depends on the software stack (e.g. there are windows and cros > 7280 laptops with different security restrictions).. so I think > the dt property is the way to go. Well, the changes were done as per Krzysztof's comments on the earlier versions (see [1] and [2]): [1]. https://lore.kernel.org/linux-arm-msm/fe326d38-ee52-b0a4-21d8-f00f22449417@linaro.org/ [2]. https://lore.kernel.org/linux-arm-msm/e60af365-4260-a56f-1ec1-c7c60d172b38@linaro.org/ I am fine with either approach. As I originally argued, having a dt-property seems more useful to me to indicate a secure-mode access. May be @Krzysztof Kozlowski , can share his opinions on the same. Thanks, Bhupesh
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig index 99b15b77dfd5..51eb5140caa1 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 74f2aeaccdcb..6face21b7fb7 100644 --- a/drivers/usb/misc/qcom_eud.c +++ b/drivers/usb/misc/qcom_eud.c @@ -11,9 +11,11 @@ #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> +#include <linux/firmware/qcom/qcom_scm.h> #include <linux/usb/role.h> #define EUD_REG_INT1_EN_MASK 0x0024 @@ -30,15 +32,25 @@ #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_soc_cfg { + u32 tcsr_check_offset; +}; + 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; + phys_addr_t secure_mode_mgr; }; static int enable_eud(struct eud_chip *priv) @@ -46,7 +58,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 +70,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, @@ -178,6 +198,8 @@ 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_check; int ret; chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); @@ -200,9 +222,37 @@ 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. + */ + if (of_device_is_compatible(chip->dev->of_node, "qcom,sm6115-eud")) { + 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_byname(pdev, IORESOURCE_MEM, "tcsr-base"); + if (!res) + return dev_err_probe(chip->dev, -ENODEV, + "failed to get tcsr reg base\n"); + + tcsr_check = res->start + chip->eud_cfg->tcsr_check_offset; + + 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"); + } chip->irq = platform_get_irq(pdev, 0); ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq, @@ -230,8 +280,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);
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 | 65 ++++++++++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 6 deletions(-)