Messages in this thread Patch in this message | | | Date | Thu, 21 Mar 2019 11:17:16 +0000 | From | Daniel Thompson <> | Subject | Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values |
| |
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@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. > > > > > > > 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
| |