| Message ID | 20190731082339.20163-3-ccaione@baylibre.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Rework secure-monitor driver | expand |
On Wed 31 Jul 2019 at 09:23, Carlo Caione <ccaione@baylibre.com> wrote: > Add a new property to link the nvmem driver to the secure-monitor. The > nvmem driver needs to access the secure-monitor to be able to access the > fuses. > Reviewed-by: Jerome Brunet <jbrunet@baylibre.com> > Signed-off-by: Carlo Caione <ccaione@baylibre.com>
Rob, Carlo Caione <ccaione@baylibre.com> writes: > Add a new property to link the nvmem driver to the secure-monitor. The > nvmem driver needs to access the secure-monitor to be able to access the > fuses. > > Signed-off-by: Carlo Caione <ccaione@baylibre.com> > --- > Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt > index 2e0723ab3384..f7b3ed74db54 100644 > --- a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt > +++ b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt > @@ -4,6 +4,7 @@ Required properties: > - compatible: should be "amlogic,meson-gxbb-efuse" > - clocks: phandle to the efuse peripheral clock provided by the > clock controller. > +- secure-monitor: phandle to the secure-monitor node > > = Data cells = > Are child nodes of eFuse, bindings of which as described in > @@ -16,6 +17,7 @@ Example: > clocks = <&clkc CLKID_EFUSE>; > #address-cells = <1>; > #size-cells = <1>; > + secure-monitor = <&sm>; > > sn: sn@14 { > reg = <0x14 0x10>; > @@ -30,6 +32,10 @@ Example: > }; > }; > > + sm: secure-monitor { > + compatible = "amlogic,meson-gxbb-sm"; > + }; > + > = Data consumers = > Are device nodes which consume nvmem data cells. > With your review/ack, I'll take this through with the driver changes. Thanks, Kevin
On Wed, Jul 31, 2019 at 09:23:37AM +0100, Carlo Caione wrote: > Add a new property to link the nvmem driver to the secure-monitor. The > nvmem driver needs to access the secure-monitor to be able to access the > fuses. > > Signed-off-by: Carlo Caione <ccaione@baylibre.com> > --- > Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt > index 2e0723ab3384..f7b3ed74db54 100644 > --- a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt > +++ b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt > @@ -4,6 +4,7 @@ Required properties: > - compatible: should be "amlogic,meson-gxbb-efuse" > - clocks: phandle to the efuse peripheral clock provided by the > clock controller. > +- secure-monitor: phandle to the secure-monitor node > > = Data cells = > Are child nodes of eFuse, bindings of which as described in > @@ -16,6 +17,7 @@ Example: > clocks = <&clkc CLKID_EFUSE>; > #address-cells = <1>; > #size-cells = <1>; > + secure-monitor = <&sm>; > > sn: sn@14 { > reg = <0x14 0x10>; > @@ -30,6 +32,10 @@ Example: > }; > }; > > + sm: secure-monitor { > + compatible = "amlogic,meson-gxbb-sm"; > + }; I guess I acked this a while back, but I'm not sure I would today. It seems incomplete and a node with only a compatible string and no resources doesn't need to be in DT. But that's already done... There's no need for 'secure-monitor' anyways. Just do 'of_find_compatible_node(NULL, NULL, "amlogic,meson-gxbb-sm")' or search for the driver directly. It's not like there's more than one secure monitor... Rob
On 21/08/2019 19:14, Rob Herring wrote: > On Wed, Jul 31, 2019 at 09:23:37AM +0100, Carlo Caione wrote: > There's no need for 'secure-monitor' anyways. Just do > 'of_find_compatible_node(NULL, NULL, "amlogic,meson-gxbb-sm")' or search > for the driver directly. It's not like there's more than one secure > monitor... How is hardcoding the secure-monitor directly into the efuse driver better than having it referenced in the DT? Yes, there is one single secure monitor but (even if this is not currently the case) several drivers can use it making the secure-monitor a resource to be potentially used by several devices. -- Carlo Caione
On Wed 21 Aug 2019 at 13:14, Rob Herring <robh@kernel.org> wrote: > On Wed, Jul 31, 2019 at 09:23:37AM +0100, Carlo Caione wrote: >> Add a new property to link the nvmem driver to the secure-monitor. The >> nvmem driver needs to access the secure-monitor to be able to access the >> fuses. >> >> Signed-off-by: Carlo Caione <ccaione@baylibre.com> >> --- >> Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt >> index 2e0723ab3384..f7b3ed74db54 100644 >> --- a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt >> +++ b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt >> @@ -4,6 +4,7 @@ Required properties: >> - compatible: should be "amlogic,meson-gxbb-efuse" >> - clocks: phandle to the efuse peripheral clock provided by the >> clock controller. >> +- secure-monitor: phandle to the secure-monitor node >> >> = Data cells = >> Are child nodes of eFuse, bindings of which as described in >> @@ -16,6 +17,7 @@ Example: >> clocks = <&clkc CLKID_EFUSE>; >> #address-cells = <1>; >> #size-cells = <1>; >> + secure-monitor = <&sm>; >> >> sn: sn@14 { >> reg = <0x14 0x10>; >> @@ -30,6 +32,10 @@ Example: >> }; >> }; >> >> + sm: secure-monitor { >> + compatible = "amlogic,meson-gxbb-sm"; >> + }; > > I guess I acked this a while back, but I'm not sure I would today. It > seems incomplete and a node with only a compatible string and no > resources doesn't need to be in DT. But that's already done... It does have ressources (the shared memory) but the mistake (we should maybe think about fixing) is not expressing these in DT I think the shared memory is already in our DT, maybe the secure monitor should get a phandle to it ? > > There's no need for 'secure-monitor' anyways. Just do > 'of_find_compatible_node(NULL, NULL, "amlogic,meson-gxbb-sm")' or search > for the driver directly. It's not like there's more than one secure > monitor... IMO the two methods show different constraints: - Carlo's proposition show that the efuse driver need a ressource, which is *a* secure monitor, whatever the variant is. - Your proposition shows that the efuse driver depends on a particular secure monitor variant, which is the one provided by "amlogic,meson-gxbb-sm" Yes, we could make your proposition work by the keeping the "amlogic,meson-gxbb-sm" last in the compatible list but it isn't great if a newer variant is actually not compatible with it. Carlo represent the HW the way it is. It seems more flexible to me, without adding (unbearable) complexity > > Rob
diff --git a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt index 2e0723ab3384..f7b3ed74db54 100644 --- a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt +++ b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt @@ -4,6 +4,7 @@ Required properties: - compatible: should be "amlogic,meson-gxbb-efuse" - clocks: phandle to the efuse peripheral clock provided by the clock controller. +- secure-monitor: phandle to the secure-monitor node = Data cells = Are child nodes of eFuse, bindings of which as described in @@ -16,6 +17,7 @@ Example: clocks = <&clkc CLKID_EFUSE>; #address-cells = <1>; #size-cells = <1>; + secure-monitor = <&sm>; sn: sn@14 { reg = <0x14 0x10>; @@ -30,6 +32,10 @@ Example: }; }; + sm: secure-monitor { + compatible = "amlogic,meson-gxbb-sm"; + }; + = Data consumers = Are device nodes which consume nvmem data cells.
Add a new property to link the nvmem driver to the secure-monitor. The nvmem driver needs to access the secure-monitor to be able to access the fuses. Signed-off-by: Carlo Caione <ccaione@baylibre.com> --- Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt | 6 ++++++ 1 file changed, 6 insertions(+)