| Message ID | 20210120081921.3315847-4-steen.hegelund@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/3] dt-bindings: reset: microchip sparx5 reset driver bindings | expand |
On Wed, Jan 20, 2021 at 09:19:21AM +0100, Steen Hegelund wrote: > This provides reset driver support for the Microchip Sparx5 PCB134 and > PCB135 reference boards. This isn't a compatible change. You need an explanation why that's okay if that's intended. > > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > --- > arch/arm64/boot/dts/microchip/sparx5.dtsi | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi > index 380281f312d8..4edbb9fcdce0 100644 > --- a/arch/arm64/boot/dts/microchip/sparx5.dtsi > +++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi > @@ -132,9 +132,17 @@ mux: mux-controller { > }; > }; > > - reset@611010008 { > - compatible = "microchip,sparx5-chip-reset"; > - reg = <0x6 0x11010008 0x4>; > + gcb_ctrl: syscon@611010000 { > + compatible = "microchip,sparx5-gcb-syscon", "syscon"; > + reg = <0x6 0x11010000 0x10000>; > + }; > + > + reset: reset-controller@0 { > + compatible = "microchip,sparx5-switch-reset"; > + reg = <0x6 0x0 0x0>; Your register length is 0? > + #reset-cells = <1>; > + cpu-syscon = <&cpu_ctrl>; Can't you accomplish the same thing adding these to "microchip,sparx5-chip-reset"? Or possibly as a child node. Define nodes based on h/w blocks, not as containers of things you happen to want for some driver. > + gcb-syscon = <&gcb_ctrl>; > }; > > uart0: serial@600100000 { > -- > 2.29.2 >
Hi Rob, On Fri, 2021-02-05 at 16:35 -0600, Rob Herring wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Wed, Jan 20, 2021 at 09:19:21AM +0100, Steen Hegelund wrote: > > This provides reset driver support for the Microchip Sparx5 PCB134 > > and > > PCB135 reference boards. > > This isn't a compatible change. You need an explanation why that's > okay > if that's intended. > > > > > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > > --- > > arch/arm64/boot/dts/microchip/sparx5.dtsi | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi > > b/arch/arm64/boot/dts/microchip/sparx5.dtsi > > index 380281f312d8..4edbb9fcdce0 100644 > > --- a/arch/arm64/boot/dts/microchip/sparx5.dtsi > > +++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi > > @@ -132,9 +132,17 @@ mux: mux-controller { > > }; > > }; > > > > - reset@611010008 { > > - compatible = "microchip,sparx5-chip-reset"; > > - reg = <0x6 0x11010008 0x4>; > > + gcb_ctrl: syscon@611010000 { > > + compatible = "microchip,sparx5-gcb-syscon", > > "syscon"; > > + reg = <0x6 0x11010000 0x10000>; > > + }; > > + > > + reset: reset-controller@0 { > > + compatible = "microchip,sparx5-switch-reset"; > > + reg = <0x6 0x0 0x0>; > > Your register length is 0? Yes, I only use the syscons. > > > + #reset-cells = <1>; > > + cpu-syscon = <&cpu_ctrl>; > > Can't you accomplish the same thing adding these to > "microchip,sparx5-chip-reset"? Or possibly as a child node. > > Define nodes based on h/w blocks, not as containers of things you > happen > to want for some driver. Yes - I think the idea of using syscons came from another reset driver, but it probably makes more sense to just use IO ranges directly. I will try that out. > > > + gcb-syscon = <&gcb_ctrl>; > > }; > > > > uart0: serial@600100000 { > > -- > > 2.29.2 > > Thanks for your comments BR Steen
Hi Rob, On Fri, 2021-02-05 at 16:35 -0600, Rob Herring wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Wed, Jan 20, 2021 at 09:19:21AM +0100, Steen Hegelund wrote: > > This provides reset driver support for the Microchip Sparx5 PCB134 > > and > > PCB135 reference boards. > > This isn't a compatible change. You need an explanation why that's > okay > if that's intended. I am not entirely sure that I get your point. The change (adding the reset driver) is new, and it is optional (but strongly recommended) for the clients to use it, so will this be a incompatible change? I can certainly add a an explanation. Would that be needed here in the bindings, or where do you suggest? > > > > > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> > > --- > > arch/arm64/boot/dts/microchip/sparx5.dtsi | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi > > b/arch/arm64/boot/dts/microchip/sparx5.dtsi > > index 380281f312d8..4edbb9fcdce0 100644 > > --- a/arch/arm64/boot/dts/microchip/sparx5.dtsi > > +++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi > > @@ -132,9 +132,17 @@ mux: mux-controller { > > }; > > }; > > > > - reset@611010008 { > > - compatible = "microchip,sparx5-chip-reset"; > > - reg = <0x6 0x11010008 0x4>; > > + gcb_ctrl: syscon@611010000 { > > + compatible = "microchip,sparx5-gcb-syscon", > > "syscon"; > > + reg = <0x6 0x11010000 0x10000>; > > + }; > > + > > + reset: reset-controller@0 { > > + compatible = "microchip,sparx5-switch-reset"; > > + reg = <0x6 0x0 0x0>; > > Your register length is 0? > > > + #reset-cells = <1>; > > + cpu-syscon = <&cpu_ctrl>; > > Can't you accomplish the same thing adding these to > "microchip,sparx5-chip-reset"? Or possibly as a child node. > > Define nodes based on h/w blocks, not as containers of things you > happen > to want for some driver. > > > + gcb-syscon = <&gcb_ctrl>; > > }; > > > > uart0: serial@600100000 { > > -- > > 2.29.2 > > Thanks for your comments BR Steen
On Mon, Feb 8, 2021 at 7:20 AM Steen Hegelund <steen.hegelund@microchip.com> wrote: > > Hi Rob, > > On Fri, 2021-02-05 at 16:35 -0600, Rob Herring wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On Wed, Jan 20, 2021 at 09:19:21AM +0100, Steen Hegelund wrote: > > > This provides reset driver support for the Microchip Sparx5 PCB134 > > > and > > > PCB135 reference boards. > > > > This isn't a compatible change. You need an explanation why that's > > okay > > if that's intended. > > I am not entirely sure that I get your point. > > The change (adding the reset driver) is new, and it is optional (but > strongly recommended) for the clients to use it, so will this be a > incompatible change? I'm assuming something used "microchip,sparx5-chip-reset"? > I can certainly add a an explanation. Would that be needed here in the > bindings, or where do you suggest? Just need to spell out why dropping that is okay in the commit msg. But I guess what this looks like will change anyways. Rob
diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi index 380281f312d8..4edbb9fcdce0 100644 --- a/arch/arm64/boot/dts/microchip/sparx5.dtsi +++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi @@ -132,9 +132,17 @@ mux: mux-controller { }; }; - reset@611010008 { - compatible = "microchip,sparx5-chip-reset"; - reg = <0x6 0x11010008 0x4>; + gcb_ctrl: syscon@611010000 { + compatible = "microchip,sparx5-gcb-syscon", "syscon"; + reg = <0x6 0x11010000 0x10000>; + }; + + reset: reset-controller@0 { + compatible = "microchip,sparx5-switch-reset"; + reg = <0x6 0x0 0x0>; + #reset-cells = <1>; + cpu-syscon = <&cpu_ctrl>; + gcb-syscon = <&gcb_ctrl>; }; uart0: serial@600100000 {
This provides reset driver support for the Microchip Sparx5 PCB134 and PCB135 reference boards. Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com> --- arch/arm64/boot/dts/microchip/sparx5.dtsi | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)