Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values

[Manivannan Sadhasivam]

Hi Daniel,

On Tue, Mar 12, 2019 at 12:27:11PM +0000, Daniel Thompson wrote:
> 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@xxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> >  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.
> 

Hmm. I'm not sure whether we can assume shared reset lines here or not! Maybe
Russell can share his opinion here.

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

Ack.

> > +
> >  		/*
> >  		 * 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?
> 

Yes, but it can come before this as well. Right after deassert.

Thanks,
Mani

> 
> Daniel.