Re: [PATCH] drivers/amba: add reset control to primecell probe

[Rob Herring]

On Fri, Aug 2, 2019 at 8:42 AM Dinh Nguyen <dinguyen@xxxxxxxxxx> wrote:
>
>
>
> On 8/2/19 9:37 AM, Rob Herring wrote:
> > On Thu, Aug 1, 2019 at 12:44 PM Dinh Nguyen <dinguyen@xxxxxxxxxx> wrote:
> >>
> >> From: Dinh Nguyen <dinh.nguyen@xxxxxxxxx>
> >>
> >> The primecell controller on some SoCs, i.e. SoCFPGA, is held in reset by
> >> default. Until recently, the DMA controller was brought out of reset by the
> >> bootloader(i.e. U-Boot). But a recent change in U-Boot, the peripherals that
> >> are not used are held in reset and are left to Linux to bring them out of
> >> reset.
> >
> > You can fix this in the kernel, but any versions before this change
> > will remain broken. IMO, the u-boot change should be reverted because
> > it is breaking an ABI (though not a good one).
> >
>
> Right, there exists in U-Boot to support legacy platforms before this
> recent change. This would be for future versions.
>
> >> Add a mechanism for getting the reset property and de-assert the primecell
> >> module from reset if found. This is a not a hard fail if the reset property
> >> is not present in the device tree node, so the driver will continue to probe.
> >
> > I think this belongs in the AMBA bus code, not the DT code, as that is
> > where we already have clock control code for similar reasons.
> >
>
> Ok.
>
> >>
> >> Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> >> ---
> >>  drivers/of/platform.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index 7801e25e6895..d8945705313d 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -21,6 +21,7 @@
> >>  #include <linux/of_irq.h>
> >>  #include <linux/of_platform.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/reset.h>
> >>
> >>  const struct of_device_id of_default_bus_match_table[] = {
> >>         { .compatible = "simple-bus", },
> >> @@ -229,6 +230,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
> >>         struct amba_device *dev;
> >>         const void *prop;
> >>         int i, ret;
> >> +       struct reset_control *rstc;
> >>
> >>         pr_debug("Creating amba device %pOF\n", node);
> >>
> >> @@ -270,6 +272,18 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
> >>                 goto err_free;
> >>         }
> >>
> >> +       /*
> >> +        * reset control of the primecell block is optional
> >> +        * and will not fail if the reset property is not found.
> >> +        */
> >> +       rstc = of_reset_control_get_exclusive(node, "dma");
> >
> > 'dma' doesn't sound very generic.
> >
>
> how about 'primecell' ?

It should be based on what is in the TRMs. Unlike pclk, there doesn't
appear to be a standard name or number of resets:

pl011: PRESETn and nUARTRST
pl330: ARESETn

Can't you just retrieve all of them and deassert them all and ignore the name?

Also, you might need to use the shared variant as the core code has to
work for either dedicated or shared resets.

Rob