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.
