| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cc53a2c1-4349-e489-0087-a31a13edef8f@baylibre.com>
Date: Thu, 6 Aug 2020 15:49:49 +0200
From: Alexandre Bailon <abailon@...libre.com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: ohad@...ery.com, bjorn.andersson@...aro.org, robh+dt@...nel.org,
matthias.bgg@...il.com, linux-remoteproc@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] remoteproc: mtk_vpu_rproc: Add support of JTAG
On 7/21/20 9:52 PM, Mathieu Poirier wrote:
> On Mon, Jul 13, 2020 at 03:29:24PM +0200, Alexandre Bailon wrote:
>> The DSP could be debugged using JTAG.
>> The support of JTAG could enabled at build time and it could be enabled
>> using debugfs.
>>
>> Signed-off-by: Alexandre Bailon <abailon@...libre.com>
>> ---
>> drivers/remoteproc/Kconfig | 9 ++
>> drivers/remoteproc/mtk_apu_rproc.c | 156 ++++++++++++++++++++++++++++-
>> 2 files changed, 162 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index e116d4a12ac3..e1158563e2e8 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -52,6 +52,15 @@ config MTK_APU
>>
>> It's safe to say N here.
>>
>> +config MTK_APU_JTAG
>> + bool "Enable support of JTAG"
>> + depends on MTK_APU
>> + help
>> + Say y to enable support of JTAG.
>> + By default, JTAG will remain disabled until it is enabled using
>> + debugfs: remoteproc/remoteproc0/jtag. Write 1 to enable it and
>> + 0 to disable it.
>> +
>> config OMAP_REMOTEPROC
>> tristate "OMAP remoteproc support"
>> depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX
>> diff --git a/drivers/remoteproc/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
>> index fb416a817ef3..f2342b747a35 100644
>> --- a/drivers/remoteproc/mtk_apu_rproc.c
>> +++ b/drivers/remoteproc/mtk_apu_rproc.c
>> @@ -5,6 +5,7 @@
>>
>> #include <linux/bitops.h>
>> #include <linux/clk.h>
>> +#include <linux/debugfs.h>
>> #include <linux/delay.h>
>> #include <linux/interrupt.h>
>> #include <linux/io.h>
>> @@ -14,6 +15,7 @@
>> #include <linux/highmem.h>
>> #include <linux/module.h>
>> #include <linux/of_reserved_mem.h>
>> +#include <linux/pinctrl/consumer.h>
>> #include <linux/platform_device.h>
>> #include <linux/remoteproc.h>
>>
>> @@ -48,6 +50,11 @@
>> #define CORE_DEFAULT1 (0x00000140)
>> #define CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU (0x10 << 0)
>> #define CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU (0x10 << 5)
>> +#define CORE_DEFAULT2 (0x00000144)
>> +#define CORE_DEFAULT2_DBG_EN BIT(3)
>> +#define CORE_DEFAULT2_NIDEN BIT(2)
>> +#define CORE_DEFAULT2_SPNIDEN BIT(1)
>> +#define CORE_DEFAULT2_SPIDEN BIT(0)
>> #define CORE_XTENSA_ALTRESETVEC (0x000001F8)
>>
>> struct mtk_vpu_rproc {
>> @@ -59,6 +66,13 @@ struct mtk_vpu_rproc {
>> struct clk *axi;
>> struct clk *ipu;
>> struct clk *jtag;
>> +
>> +#ifdef CONFIG_MTK_APU_JTAG
>> + struct pinctrl *pinctrl;
>> + struct pinctrl_state *pinctrl_default;
>> + struct pinctrl_state *pinctrl_jtag;
>> + bool jtag_enabled;
>> +#endif
>> };
>>
>> static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off)
>> @@ -149,6 +163,133 @@ static irqreturn_t handle_event(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>>
>> +#ifdef CONFIG_MTK_APU_JTAG
>> +
>> +static int vpu_enable_jtag(struct mtk_vpu_rproc *vpu_rproc)
>> +{
>> + int ret = 0;
>> +
>> + if (vpu_rproc->jtag_enabled)
>> + return -EINVAL;
>> +
>> + ret = pinctrl_select_state(vpu_rproc->pinctrl,
>> + vpu_rproc->pinctrl_jtag);
>> + if (ret < 0) {
>> + dev_err(vpu_rproc->dev, "Failed to configure pins for JTAG\n");
>> + return ret;
>> + }
>> +
>> + vpu_write32(vpu_rproc, CORE_DEFAULT2,
>> + CORE_DEFAULT2_SPNIDEN | CORE_DEFAULT2_SPIDEN |
>> + CORE_DEFAULT2_NIDEN | CORE_DEFAULT2_DBG_EN);
>> +
>> + vpu_rproc->jtag_enabled = 1;
> There should be mutex that gets taken at the beginning and released at the end of
> this function.
>
>> +
>> + return ret;
>> +}
>> +
>> +static int vpu_disable_jtag(struct mtk_vpu_rproc *vpu_rproc)
>> +{
>> + int ret = 0;
>> +
>> + if (!vpu_rproc->jtag_enabled)
>> + return -EINVAL;
>> +
>> + vpu_write32(vpu_rproc, CORE_DEFAULT2, 0);
>> +
>> + ret = pinctrl_select_state(vpu_rproc->pinctrl,
>> + vpu_rproc->pinctrl_default);
>> + if (ret < 0) {
>> + dev_err(vpu_rproc->dev,
>> + "Failed to configure pins to default\n");
>> + return ret;
>> + }
>> +
>> + vpu_rproc->jtag_enabled = 0;
> Same comment as above.
>
>> +
>> + return ret;
>> +}
>> +
>> +static ssize_t rproc_jtag_read(struct file *filp, char __user *userbuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct rproc *rproc = filp->private_data;
>> + struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
>> + char *buf = vpu_rproc->jtag_enabled ? "enabled\n" : "disabled\n";
>> +
>> + return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
>> +}
>> +
>> +static ssize_t rproc_jtag_write(struct file *filp, const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct rproc *rproc = filp->private_data;
>> + struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
>> + char buf[10];
>> + int ret;
>> +
>> + if (count < 1 || count > sizeof(buf))
>> + return -EINVAL;
>> +
>> + ret = copy_from_user(buf, user_buf, count);
>> + if (ret)
>> + return -EFAULT;
>> +
>> + /* remove end of line */
>> + if (buf[count - 1] == '\n')
>> + buf[count - 1] = '\0';
>> +
>> + if (!strncmp(buf, "1", count) || !strncmp(buf, "enabled", count))
>> + ret = vpu_enable_jtag(vpu_rproc);
>> + else if (!strncmp(buf, "0", count) || !strncmp(buf, "disabled", count))
>> + ret = vpu_disable_jtag(vpu_rproc);
>> + else
>> + return -EINVAL;
> I think we should simply stick with "enabled" and "disabled" to be in line with
> what is done in rproc_recovery_write().
>
>> +
>> + return ret ? ret : count;
>> +}
>> +
>> +static const struct file_operations rproc_jtag_ops = {
>> + .read = rproc_jtag_read,
>> + .write = rproc_jtag_write,
>> + .open = simple_open,
>> +};
>> +
>> +static int vpu_jtag_probe(struct mtk_vpu_rproc *vpu_rproc)
>> +{
>> + int ret;
>> +
>> + if (!vpu_rproc->rproc->dbg_dir)
>> + return -ENODEV;
>> +
>> + vpu_rproc->pinctrl = devm_pinctrl_get(vpu_rproc->dev);
>> + if (IS_ERR(vpu_rproc->pinctrl)) {
>> + dev_warn(vpu_rproc->dev, "Failed to find JTAG pinctrl\n");
>> + return PTR_ERR(vpu_rproc->pinctrl);
>> + }
>> +
>> + vpu_rproc->pinctrl_default = pinctrl_lookup_state(vpu_rproc->pinctrl,
>> + PINCTRL_STATE_DEFAULT);
> Indentation problem.
>
>> + if (IS_ERR(vpu_rproc->pinctrl_default))
>> + return PTR_ERR(vpu_rproc->pinctrl_default);
>> +
>> + vpu_rproc->pinctrl_jtag = pinctrl_lookup_state(vpu_rproc->pinctrl,
>> + "jtag");
>> + if (IS_ERR(vpu_rproc->pinctrl_jtag))
>> + return PTR_ERR(vpu_rproc->pinctrl_jtag);
>> +
>> + ret = pinctrl_select_state(vpu_rproc->pinctrl,
>> + vpu_rproc->pinctrl_default);
> What is the default configuration for? It does not seem to be needed to
> properly boot the remote processor since it is not part of the example in the
> bindings or dts patch included in this set. Moreover it is part of a
> configuration option so I really don't understand what it does.
I have a poor knowledge of pinctrl framework so I may have done things
wrong here.
This is not really needed for the remote processor.
By default, I don't want pin to be configured for JTAG until we enable
it and
I want to be able to revert it the default state.
May be this is too much and I should assume that if we build the driver
with JTAG enabled
then we want the pins to be configured for JTAG by default.
>
>
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + debugfs_create_file("jtag", 0600, vpu_rproc->rproc->dbg_dir,
>> + vpu_rproc->rproc, &rproc_jtag_ops);
>> +
>> + return 0;
>> +}
>> +#endif /* CONFIG_MTK_APU_JTAG */
>> +
>> static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -228,16 +369,16 @@ static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>> goto clk_disable_ipu;
>> }
>>
>> - vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag");
>> + vpu_rproc->jtag = devm_clk_get(vpu_rproc->dev, "jtag");
> As I remarked in my comments on the previous patch, this should have been
> devm_clk_get() from the start. Either that or the bindings are wrong.
I should have not made the change in this patch. I will fix it.
Thanks,
Alexandre
>
>> if (IS_ERR(vpu_rproc->jtag)) {
>> - dev_err(dev, "Failed to enable jtag clock\n");
>> + dev_err(vpu_rproc->dev, "Failed to get jtag clock\n");
> Why go from dev to vpu_rproc->dev?
>
>> ret = PTR_ERR(vpu_rproc->jtag);
>> goto clk_disable_axi;
>> }
>>
>> ret = clk_prepare_enable(vpu_rproc->jtag);
>> if (ret) {
>> - dev_err(dev, "Failed to enable jtag clock\n");
>> + dev_err(vpu_rproc->dev, "Failed to enable jtag clock\n");
> Same here.
>
>> goto clk_disable_axi;
>> }
>>
>> @@ -253,6 +394,12 @@ static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>> goto free_mem;
>> }
>>
>> +#ifdef CONFIG_MTK_APU_JTAG
>> + ret = vpu_jtag_probe(vpu_rproc);
>> + if (ret)
>> + dev_warn(dev, "Failed to configure jtag\n");
>> +#endif
> Please don't use #ifdefs in the code like that. It is better to introduce a
> #else (above) with stubs that don't do anything.
>
>> +
>> return 0;
>>
>> free_mem:
>> @@ -277,6 +424,9 @@ static int mtk_vpu_rproc_remove(struct platform_device *pdev)
>>
>> disable_irq(vpu_rproc->irq);
>>
>> +#ifdef CONFIG_MTK_APU_JTAG
>> + vpu_disable_jtag(vpu_rproc);
>> +#endif
>> rproc_del(rproc);
>> of_reserved_mem_device_release(dev);
>> clk_disable_unprepare(vpu_rproc->jtag);
>> --
>> 2.26.2
>>
Powered by blists - more mailing lists