Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 12 Mar 2019 12:27:11 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc:     linux@armlinux.org.uk, xuwei5@hisilicon.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linus.walleij@linaro.org, peter.griffin@linaro.org,
        guodong.xu@linaro.org, haojian.zhuang@linaro.org
Subject: Re: [PATCH 1/2] amba: Take device out of reset before reading pid
 and cid values
Message-ID: <20190312122711.b2ibipico2dgavl7@holly.lan>
References: <20190309015635.5401-1-manivannan.sadhasivam@linaro.org>
 <20190309015635.5401-2-manivannan.sadhasivam@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190309015635.5401-2-manivannan.sadhasivam@linaro.org>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Mar 09, 2019 at 07:26:34AM +0530, Manivannan Sadhasivam wrote:
> For the AMBA Primecell devices having the reset lines wired, it is
> necessary to take them out of reset before reading the pid and cid values.
> Earlier we were dependent on the bootloader to do this but a more cleaner
> approach would be to do it in the kernel itself. Hence, this commit
> deasserts the reset line just before reading the pid and cid values.
> 
> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/amba/bus.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 41b706403ef7..da8f1aac5315 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -21,6 +21,7 @@
>  #include <linux/limits.h>
>  #include <linux/clk/clk-conf.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  
>  #include <asm/irq.h>
>  
> @@ -352,6 +353,7 @@ static void amba_device_release(struct device *dev)
>  
>  static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>  {
> +	struct reset_control *rst;
>  	u32 size;
>  	void __iomem *tmp;
>  	int i, ret;
> @@ -388,6 +390,13 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>  	if (ret == 0) {
>  		u32 pid, cid;
>  
> +		/* De-assert the reset line to take the device out of reset */
> +		rst = reset_control_get_optional_exclusive(&dev->dev, NULL);
> +		if (IS_ERR(rst))
> +			return PTR_ERR(rst);

It is really correct to propagate an error if we cannot get exclusive
ownership of the reset line.

With drivers for vendor specific cells it is ok to "just know" that the
reset line is never shared but we cannot know this for generic cells and
we certainly can't know this for the bus.

I think it *might* be OK to propagate an error if you used
reset_control_get_optional_shared() instead because if that reports an
error than arguably we have either a mistake in the DT or a bug in the
driver we are sharing a reset with.


> +
> +		reset_control_deassert(rst);

Perhaps we might also need to explain why we can ignore -ENOTSUPP
here. Perhaps something like the following based on the comment
found in in reset_control_deassert():

		/*
		 * -ENOTSUPP means occurs when the reset controller
		 * does not implement .deassert(), in which case the
		 * the reset lines should be self-deasserting (and
		 * deasserted by default).
		 */
		WARN_ON(deassert did not return 0 or -ENOTSUPP);
> +
>  		/*
>  		 * Read pid and cid based on size of resource
>  		 * they are located at end of region

This looks like it will leak the control reference... shouldn't there
be a put after you have read the pid/cid?


Daniel.