Re: [PATCH v2 2/2] usb: misc: eud: Add driver support for SM6115 / SM4250

[Bjorn Andersson]

On Tue, Jan 03, 2023 at 08:34:19PM +0530, 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).
> 

This difference does not come from the platform number, but how the
firmware is configured.

How about relying on the presence of the extra DT properties to identify
if the secure access is needed?

> Also, the enable bit inside 'tcsr_check_reg' needs to be set
> first to set the eud in 'enable' mode on these SoCs.
> 
> Cc: Souradeep Chowdhury <quic_schowdhu@xxxxxxxxxxx>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxxxxxx>
> ---
>  drivers/usb/misc/Kconfig    |  1 +
>  drivers/usb/misc/qcom_eud.c | 49 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index a5f7652db7da1..ed4ae32ff1577 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -140,6 +140,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 b7f13df007646..a96ca28a4899b 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -11,7 +11,9 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/qcom_scm.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/usb/role.h>
> @@ -30,15 +32,24 @@
>  #define EUD_INT_SAFE_MODE	BIT(4)
>  #define EUD_INT_ALL		(EUD_INT_VBUS | EUD_INT_SAFE_MODE)
>  
> +struct eud_soc_data {
> +	bool secure_eud_en;
> +	bool tcsr_check_enable;
> +};
> +
>  struct eud_chip {
>  	struct device			*dev;
>  	struct usb_role_switch		*role_sw;
> +	const struct eud_soc_data	*eud_data;
>  	void __iomem			*base;
>  	void __iomem			*mode_mgr;
>  	unsigned int			int_status;
>  	int				irq;
>  	bool				enabled;
>  	bool				usb_attached;
> +	phys_addr_t			mode_mgr_phys_base;

Given that mode_mgr doesn't come with the "_phys_base" suffix, how about
naming this "secure_mode_mgr"?

> +	phys_addr_t			tcsr_check_phys_base;

For now this could be a local variable in eud_probe(). Also while
mode_mgr_phys_base denotes an address range, afaict this points directly
to a single register. As such the suffix isn't appropriate.

How about "tcsr_check"?

> +
>  };
>  
>  static int enable_eud(struct eud_chip *priv)
> @@ -46,7 +57,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->eud_data->secure_eud_en)

if (priv->mode_mgr_phys_base)

Would avoid the notion of having two separate variables to keep track of
the validity of one entity.

> +		qcom_scm_io_writel(priv->mode_mgr_phys_base + EUD_REG_EUD_EN2, BIT(0));
> +	else
> +		writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
>  
>  	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
>  }
> @@ -54,7 +69,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->eud_data->secure_eud_en)
> +		qcom_scm_io_writel(priv->mode_mgr_phys_base + 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 +197,15 @@ static void eud_role_switch_release(void *data)
>  static int eud_probe(struct platform_device *pdev)
>  {
>  	struct eud_chip *chip;
> +	struct resource *res;
>  	int ret;
>  
>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
>  		return -ENOMEM;
>  
> +	chip->eud_data = of_device_get_match_data(&pdev->dev);
> +
>  	chip->dev = &pdev->dev;
>  
>  	chip->role_sw = usb_role_switch_get(&pdev->dev);
> @@ -200,10 +222,25 @@ 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);
> +	chip->mode_mgr = devm_platform_get_and_ioremap_resource(pdev, 1, &res);

Perhaps it would be best not to ioremap the region when secure accesses
are required?

Regards,
Bjorn

>  	if (IS_ERR(chip->mode_mgr))
>  		return PTR_ERR(chip->mode_mgr);
>  
> +	if (chip->eud_data->secure_eud_en)
> +		chip->mode_mgr_phys_base = res->start;
> +
> +	if (chip->eud_data->tcsr_check_enable) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +		if (!res)
> +			return dev_err_probe(chip->dev, -ENODEV, "failed to get tcsr reg base\n");
> +
> +		chip->tcsr_check_phys_base = res->start;
> +
> +		ret = qcom_scm_io_writel(chip->tcsr_check_phys_base, BIT(0));
> +		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,
>  			handle_eud_irq_thread, IRQF_ONESHOT, NULL, chip);
> @@ -230,8 +267,14 @@ static int eud_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct eud_soc_data sm6115_eud_data = {
> +	.secure_eud_en = true,
> +	.tcsr_check_enable = true,
> +};
> +
>  static const struct of_device_id eud_dt_match[] = {
>  	{ .compatible = "qcom,sc7280-eud" },
> +	{ .compatible = "qcom,sm6115-eud", .data = &sm6115_eud_data },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, eud_dt_match);
> -- 
> 2.38.1
>