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

[Daniel Thompson]

On Wed, Mar 20, 2019 at 05:29:56PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Mar 20, 2019 at 12:26:58PM +0530, Manivannan Sadhasivam wrote:
> > 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.
> 
> I've no reference to base an opinion on as I have no hardware that
> would use this facility.
> 
> However, it seems obvious to me that if a reset line is shared between
> several devices, and when the reset line is asserted, it blocks reading
> the ID, then we do need a way for the AMBA primecell code to be able
> to lift the reset to read the device ID, and we need to do it in a way
> that is capable of being done even when another device/driver has
> already bound and taken control of the reset line.
> 
> That said, if a reset line is shared between multiple devices, and a
> driver wants to assert the reset line, it would disrupt the operation
> of all those devices, so there would need to be some kind of
> synchronisation between the drivers.

That is what shared ownership of the reset line provides. When a line is
shared a single driver does not have the authority to unilaterally
assert reset because deasserts and asserts are counted and the line only
goes high again when they balance.


> A different way to look at it though is that such a reset line is not
> a property of the individual devices, but of the bus - in which case
> it should be specified at bus level and controlled at bus level, not
> at individual device level.
> 
> What is appropriate is entirely dependent on the hardware setup, and
> as I said above, with no view of the hardware I am unable to give a
> meaningful opinion.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up