Re: [PATCH 3/6] remoteproc: mtk_vpu_rproc: Add support of JTAG

From: Alexandre Bailon <abailon@baylibre.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: ohad@wizery.com, bjorn.andersson@linaro.org, robh+dt@kernel.org,
	matthias.bgg@gmail.com, linux-remoteproc@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/6] remoteproc: mtk_vpu_rproc: Add support of JTAG
Date: Thu, 6 Aug 2020 15:49:49 +0200	[thread overview]
Message-ID: <cc53a2c1-4349-e489-0087-a31a13edef8f@baylibre.com> (raw)
In-Reply-To: <20200721195231.GA1227776@xps15>

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@baylibre.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
>>