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.thomp...@linaro.org> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasi...@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. > > > + > > + 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.