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;
MIME-Version: 1.0
References: <20230517211756.2483552-1-bhupesh.sharma@linaro.org>
 <20230517211756.2483552-5-bhupesh.sharma@linaro.org> <e3ed8dc6-61f1-e73c-2bc7-4e57e86b8647@linaro.org>
In-Reply-To: <e3ed8dc6-61f1-e73c-2bc7-4e57e86b8647@linaro.org>
From:   Bhupesh Sharma <bhupesh.sharma@linaro.org>
Date:   Thu, 18 May 2023 16:07:51 +0530
Message-ID: <CAH=2Ntwrs=+dg8_KRRoEaKg3j6boyMNd6XDxaEK45=BVXqyoQA@mail.gmail.com>
Subject: Re: [PATCH v6 4/6] usb: misc: eud: Add driver support for SM6115 / SM4250
To:     Konrad Dybcio <konrad.dybcio@linaro.org>
Cc:     linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-usb@vger.kernel.org, agross@kernel.org, andersson@kernel.org,
        linux-kernel@vger.kernel.org, bhupesh.linux@gmail.com,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        krzysztof.kozlowski@linaro.org, quic_schowdhu@quicinc.com,
        gregkh@linuxfoundation.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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