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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 2 Aug 2019 09:29:50 -0600
From:   Rob Herring <robh+dt@...nel.org>
To:     Dinh Nguyen <dinguyen@...nel.org>
Cc:     devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>,
        Dinh Nguyen <dinh.nguyen@...el.com>
Subject: Re: [PATCH] drivers/amba: add reset control to primecell probe

On Fri, Aug 2, 2019 at 8:42 AM Dinh Nguyen <dinguyen@...nel.org> wrote:
>
>
>
> On 8/2/19 9:37 AM, Rob Herring wrote:
> > On Thu, Aug 1, 2019 at 12:44 PM Dinh Nguyen <dinguyen@...nel.org> wrote:
> >>
> >> From: Dinh Nguyen <dinh.nguyen@...el.com>
> >>
> >> 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@...nel.org>
> >> ---
> >>  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