Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2adb5ee2-7876-cdf1-d0c6-437615b4f52c@quicinc.com>
Date:   Tue, 18 Jul 2023 13:21:59 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v9 4/7] usb: misc: eud: Add driver support for SM6115 /
 SM4250
Content-Language: en-US
To:     Bhupesh Sharma <bhupesh.sharma@linaro.org>
CC:     <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <linux-kernel@vger.kernel.org>,
        <bhupesh.linux@gmail.com>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>,
        <krzysztof.kozlowski@linaro.org>, <gregkh@linuxfoundation.org>,
        <stephan@gerhold.net>, <linux-usb@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>
References: <20230718061052.1332993-1-bhupesh.sharma@linaro.org>
 <20230718061052.1332993-5-bhupesh.sharma@linaro.org>
From:   Souradeep Chowdhury <quic_schowdhu@quicinc.com>
In-Reply-To: <20230718061052.1332993-5-bhupesh.sharma@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);