| Message ID | 20200306130402.1F4F0803079F@mail.baikalelectronics.ru |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/4] dt-bindings: syscon: Add syscon endian properties support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
Hi, On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Optional regmap property will be used to refer to a syscon-controller > having a reboot tolerant register mapped. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Paul Burton <paulburton@kernel.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > --- Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com> -- Sebastian > .../bindings/power/reset/syscon-reboot-mode.yaml | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > index e09bb07b1abb..f47bf52ad983 100644 > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > @@ -13,9 +13,8 @@ description: | > This driver gets reboot mode magic value from reboot-mode driver > and stores it in a SYSCON mapped register. Then the bootloader > can read it and take different action according to the magic > - value stored. The SYSCON mapped register is retrieved from the > - parental dt-node plus the offset. So the SYSCON reboot-mode node > - should be represented as a sub-node of a "syscon", "simple-mfd" node. > + value stored. The SYSCON mapped register is retrieved either from > + the parental dt-node or from a regmap phandle plus the offset. > > properties: > compatible: > @@ -29,6 +28,10 @@ properties: > $ref: /schemas/types.yaml#/definitions/uint32 > description: Offset in the register map for the mode register (in bytes). > > + regmap: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: Phandle to the register map node. > + > patternProperties: > "^mode-.+": > $ref: /schemas/types.yaml#/definitions/uint32 > -- > 2.25.1 >
On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Optional regmap property will be used to refer to a syscon-controller > having a reboot tolerant register mapped. NAK. It should simply be a child node of the 'syscon-controller'. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Paul Burton <paulburton@kernel.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > --- > .../bindings/power/reset/syscon-reboot-mode.yaml | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > index e09bb07b1abb..f47bf52ad983 100644 > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > @@ -13,9 +13,8 @@ description: | > This driver gets reboot mode magic value from reboot-mode driver > and stores it in a SYSCON mapped register. Then the bootloader > can read it and take different action according to the magic > - value stored. The SYSCON mapped register is retrieved from the > - parental dt-node plus the offset. So the SYSCON reboot-mode node > - should be represented as a sub-node of a "syscon", "simple-mfd" node. > + value stored. The SYSCON mapped register is retrieved either from > + the parental dt-node or from a regmap phandle plus the offset. > > properties: > compatible: > @@ -29,6 +28,10 @@ properties: > $ref: /schemas/types.yaml#/definitions/uint32 > description: Offset in the register map for the mode register (in bytes). > > + regmap: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: Phandle to the register map node. > + > patternProperties: > "^mode-.+": > $ref: /schemas/types.yaml#/definitions/uint32 > -- > 2.25.1 >
On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote: > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > Optional regmap property will be used to refer to a syscon-controller > > having a reboot tolerant register mapped. > > NAK. It should simply be a child node of the 'syscon-controller'. Hm, It's dilemma. The driver maintainer said ack, while you disagree.) So the code change will be merged while the doc-part won't? Lets discuss then to settle the issue. Why 'syscon-reboot' can be out of syscon-controller node, while 'syscon-reboot-mode' can't? They both belong to the same usecase: save cause id and reboot. So having similar properties-set and declaring their nodes someplace nearby is natural. According to the driver 'syscon-reboot' can't lack the regmap property because it's mandatory, while here you refuse to have even optional support. Additionally in most of the cases the 'syscon-reboot' nodes aren't declared as a child of a system controller node. Why 'syscon-reboot-mode' can't work in a similar way? Regards, -Sergey > > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Paul Burton <paulburton@kernel.org> > > Cc: Ralf Baechle <ralf@linux-mips.org> > > --- > > .../bindings/power/reset/syscon-reboot-mode.yaml | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > > index e09bb07b1abb..f47bf52ad983 100644 > > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml > > @@ -13,9 +13,8 @@ description: | > > This driver gets reboot mode magic value from reboot-mode driver > > and stores it in a SYSCON mapped register. Then the bootloader > > can read it and take different action according to the magic > > - value stored. The SYSCON mapped register is retrieved from the > > - parental dt-node plus the offset. So the SYSCON reboot-mode node > > - should be represented as a sub-node of a "syscon", "simple-mfd" node. > > + value stored. The SYSCON mapped register is retrieved either from > > + the parental dt-node or from a regmap phandle plus the offset. > > > > properties: > > compatible: > > @@ -29,6 +28,10 @@ properties: > > $ref: /schemas/types.yaml#/definitions/uint32 > > description: Offset in the register map for the mode register (in bytes). > > > > + regmap: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: Phandle to the register map node. > > + > > patternProperties: > > "^mode-.+": > > $ref: /schemas/types.yaml#/definitions/uint32 > > -- > > 2.25.1 > >
Hi, On Fri, Mar 13, 2020 at 04:02:31PM +0300, Sergey Semin wrote: > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote: > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > Optional regmap property will be used to refer to a syscon-controller > > > having a reboot tolerant register mapped. > > > > NAK. It should simply be a child node of the 'syscon-controller'. > > Hm, It's dilemma. The driver maintainer said ack, while you > disagree. So the code change will be merged while the doc-part > won't? FWIW I do not merge with bindings being NAK'd by Rob. -- Sebastian
On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin <Sergey.Semin@baikalelectronics.ru> wrote: > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote: > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > Optional regmap property will be used to refer to a syscon-controller > > > having a reboot tolerant register mapped. > > > > NAK. It should simply be a child node of the 'syscon-controller'. > > Hm, It's dilemma. The driver maintainer said ack, while you disagree.) > So the code change will be merged while the doc-part won't? Lets discuss then > to settle the issue. > > Why 'syscon-reboot' can be out of syscon-controller node, while > 'syscon-reboot-mode' can't? Look at the history and you will see one was reviewed by DT maintainers and one wasn't. > They both belong to the same usecase: save > cause id and reboot. So having similar properties-set and declaring their > nodes someplace nearby is natural. Which is what I'm asking for. Where else in the tree does it make sense to locate the 'syscon-reboot-mode' node? Locate nodes where they logically belong. > According to the driver 'syscon-reboot' > can't lack the regmap property because it's mandatory, while here you refuse > to have even optional support. Additionally in most of the cases the > 'syscon-reboot' nodes aren't declared as a child of a system controller > node. Why 'syscon-reboot-mode' can't work in a similar way? There's plenty of bad or "don't follow current best practice" examples in the tree for all sorts of things. That is not a reason for doing something in a new binding or adding to an existing one. Rob
On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote: > On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin > <Sergey.Semin@baikalelectronics.ru> wrote: > > > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote: > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > > > Optional regmap property will be used to refer to a syscon-controller > > > > having a reboot tolerant register mapped. > > > > > > NAK. It should simply be a child node of the 'syscon-controller'. > > > > Hm, It's dilemma. The driver maintainer said ack, while you disagree.) > > So the code change will be merged while the doc-part won't? Lets discuss then > > to settle the issue. > > > > Why 'syscon-reboot' can be out of syscon-controller node, while > > 'syscon-reboot-mode' can't? > > Look at the history and you will see one was reviewed by DT > maintainers and one wasn't. > > > They both belong to the same usecase: save > > cause id and reboot. So having similar properties-set and declaring their > > nodes someplace nearby is natural. > > Which is what I'm asking for. Where else in the tree does it make > sense to locate the 'syscon-reboot-mode' node? Locate nodes where they > logically belong. > > > According to the driver 'syscon-reboot' > > can't lack the regmap property because it's mandatory, while here you refuse > > to have even optional support. Additionally in most of the cases the > > 'syscon-reboot' nodes aren't declared as a child of a system controller > > node. Why 'syscon-reboot-mode' can't work in a similar way? > > There's plenty of bad or "don't follow current best practice" examples > in the tree for all sorts of things. That is not a reason for doing > something in a new binding or adding to an existing one. > > Rob Alright. I see your point. What about I'd provide a sort of opposite implementation? I could make the "regmap"-phandle reference being optional in the !"syscon-reboot"! driver instead of adding the regmap-property support to the "syscon-reboot-mode" driver. So if regmap property isn't defined in the "syscon-reboot"-compatible node, the driver will try to get a syscon regmap from the parental node as it's done in the "syscon-reboot-mode" driver. Seeing you think that regmap-property-based design is a bad practice in this case, I also could mark the property as deprecated in the "syscon-reboot" dt schema and print a warning from the "syscon-reboot" driver if one is defined. What do you think? Regards, -Sergey
Rob, Any comment on my suggestion below? Regards, -Sergey On Tue, Mar 31, 2020 at 10:50:53PM +0300, Sergey Semin wrote: > On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote: > > On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin > > <Sergey.Semin@baikalelectronics.ru> wrote: > > > > > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote: > > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > > > > > Optional regmap property will be used to refer to a syscon-controller > > > > > having a reboot tolerant register mapped. > > > > > > > > NAK. It should simply be a child node of the 'syscon-controller'. > > > > > > Hm, It's dilemma. The driver maintainer said ack, while you disagree.) > > > So the code change will be merged while the doc-part won't? Lets discuss then > > > to settle the issue. > > > > > > Why 'syscon-reboot' can be out of syscon-controller node, while > > > 'syscon-reboot-mode' can't? > > > > Look at the history and you will see one was reviewed by DT > > maintainers and one wasn't. > > > > > They both belong to the same usecase: save > > > cause id and reboot. So having similar properties-set and declaring their > > > nodes someplace nearby is natural. > > > > Which is what I'm asking for. Where else in the tree does it make > > sense to locate the 'syscon-reboot-mode' node? Locate nodes where they > > logically belong. > > > > > According to the driver 'syscon-reboot' > > > can't lack the regmap property because it's mandatory, while here you refuse > > > to have even optional support. Additionally in most of the cases the > > > 'syscon-reboot' nodes aren't declared as a child of a system controller > > > node. Why 'syscon-reboot-mode' can't work in a similar way? > > > > There's plenty of bad or "don't follow current best practice" examples > > in the tree for all sorts of things. That is not a reason for doing > > something in a new binding or adding to an existing one. > > > > Rob > > Alright. I see your point. What about I'd provide a sort of opposite > implementation? I could make the "regmap"-phandle reference being optional > in the !"syscon-reboot"! driver instead of adding the regmap-property > support to the "syscon-reboot-mode" driver. So if regmap property isn't > defined in the "syscon-reboot"-compatible node, the driver will try to > get a syscon regmap from the parental node as it's done in the > "syscon-reboot-mode" driver. > > Seeing you think that regmap-property-based design is a bad practice in > this case, I also could mark the property as deprecated in the "syscon-reboot" > dt schema and print a warning from the "syscon-reboot" driver if one is defined. > > What do you think? > > Regards, > -Sergey
On Thu, Apr 16, 2020 at 10:56:20PM +0300, Sergey Semin wrote: > Rob, > Any comment on my suggestion below? > > Regards, > -Sergey > > On Tue, Mar 31, 2020 at 10:50:53PM +0300, Sergey Semin wrote: > > On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote: > > > On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin > > > <Sergey.Semin@baikalelectronics.ru> wrote: > > > > > > > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote: > > > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > > > > > > > Optional regmap property will be used to refer to a syscon-controller > > > > > > having a reboot tolerant register mapped. > > > > > > > > > > NAK. It should simply be a child node of the 'syscon-controller'. > > > > > > > > Hm, It's dilemma. The driver maintainer said ack, while you disagree.) > > > > So the code change will be merged while the doc-part won't? Lets discuss then > > > > to settle the issue. > > > > > > > > Why 'syscon-reboot' can be out of syscon-controller node, while > > > > 'syscon-reboot-mode' can't? > > > > > > Look at the history and you will see one was reviewed by DT > > > maintainers and one wasn't. > > > > > > > They both belong to the same usecase: save > > > > cause id and reboot. So having similar properties-set and declaring their > > > > nodes someplace nearby is natural. > > > > > > Which is what I'm asking for. Where else in the tree does it make > > > sense to locate the 'syscon-reboot-mode' node? Locate nodes where they > > > logically belong. > > > > > > > According to the driver 'syscon-reboot' > > > > can't lack the regmap property because it's mandatory, while here you refuse > > > > to have even optional support. Additionally in most of the cases the > > > > 'syscon-reboot' nodes aren't declared as a child of a system controller > > > > node. Why 'syscon-reboot-mode' can't work in a similar way? > > > > > > There's plenty of bad or "don't follow current best practice" examples > > > in the tree for all sorts of things. That is not a reason for doing > > > something in a new binding or adding to an existing one. > > > > > > Rob > > > > Alright. I see your point. What about I'd provide a sort of opposite > > implementation? I could make the "regmap"-phandle reference being optional > > in the !"syscon-reboot"! driver instead of adding the regmap-property > > support to the "syscon-reboot-mode" driver. So if regmap property isn't > > defined in the "syscon-reboot"-compatible node, the driver will try to > > get a syscon regmap from the parental node as it's done in the > > "syscon-reboot-mode" driver. That seems fine. > > Seeing you think that regmap-property-based design is a bad practice in > > this case, I also could mark the property as deprecated in the "syscon-reboot" > > dt schema and print a warning from the "syscon-reboot" driver if one is defined. Depends on how many platforms will start getting warnings. I think just marking deprecated is enough. Rob
On Thu, Apr 16, 2020 at 04:28:42PM -0500, Rob Herring wrote: > On Thu, Apr 16, 2020 at 10:56:20PM +0300, Sergey Semin wrote: > > Rob, > > Any comment on my suggestion below? > > > > Regards, > > -Sergey > > > > On Tue, Mar 31, 2020 at 10:50:53PM +0300, Sergey Semin wrote: > > > On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote: > > > > On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin > > > > <Sergey.Semin@baikalelectronics.ru> wrote: > > > > > > > > > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote: > > > > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > > > > > > > > > Optional regmap property will be used to refer to a syscon-controller > > > > > > > having a reboot tolerant register mapped. > > > > > > > > > > > > NAK. It should simply be a child node of the 'syscon-controller'. > > > > > > > > > > Hm, It's dilemma. The driver maintainer said ack, while you disagree.) > > > > > So the code change will be merged while the doc-part won't? Lets discuss then > > > > > to settle the issue. > > > > > > > > > > Why 'syscon-reboot' can be out of syscon-controller node, while > > > > > 'syscon-reboot-mode' can't? > > > > > > > > Look at the history and you will see one was reviewed by DT > > > > maintainers and one wasn't. > > > > > > > > > They both belong to the same usecase: save > > > > > cause id and reboot. So having similar properties-set and declaring their > > > > > nodes someplace nearby is natural. > > > > > > > > Which is what I'm asking for. Where else in the tree does it make > > > > sense to locate the 'syscon-reboot-mode' node? Locate nodes where they > > > > logically belong. > > > > > > > > > According to the driver 'syscon-reboot' > > > > > can't lack the regmap property because it's mandatory, while here you refuse > > > > > to have even optional support. Additionally in most of the cases the > > > > > 'syscon-reboot' nodes aren't declared as a child of a system controller > > > > > node. Why 'syscon-reboot-mode' can't work in a similar way? > > > > > > > > There's plenty of bad or "don't follow current best practice" examples > > > > in the tree for all sorts of things. That is not a reason for doing > > > > something in a new binding or adding to an existing one. > > > > > > > > Rob > > > > > > Alright. I see your point. What about I'd provide a sort of opposite > > > implementation? I could make the "regmap"-phandle reference being optional > > > in the !"syscon-reboot"! driver instead of adding the regmap-property > > > support to the "syscon-reboot-mode" driver. So if regmap property isn't > > > defined in the "syscon-reboot"-compatible node, the driver will try to > > > get a syscon regmap from the parental node as it's done in the > > > "syscon-reboot-mode" driver. > > That seems fine. > > > > Seeing you think that regmap-property-based design is a bad practice in > > > this case, I also could mark the property as deprecated in the "syscon-reboot" > > > dt schema and print a warning from the "syscon-reboot" driver if one is defined. > > Depends on how many platforms will start getting warnings. I think just > marking deprecated is enough. Ok. Thanks. I'll do this in v2. Regards, -Sergey > > Rob
diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml index e09bb07b1abb..f47bf52ad983 100644 --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml @@ -13,9 +13,8 @@ description: | This driver gets reboot mode magic value from reboot-mode driver and stores it in a SYSCON mapped register. Then the bootloader can read it and take different action according to the magic - value stored. The SYSCON mapped register is retrieved from the - parental dt-node plus the offset. So the SYSCON reboot-mode node - should be represented as a sub-node of a "syscon", "simple-mfd" node. + value stored. The SYSCON mapped register is retrieved either from + the parental dt-node or from a regmap phandle plus the offset. properties: compatible: @@ -29,6 +28,10 @@ properties: $ref: /schemas/types.yaml#/definitions/uint32 description: Offset in the register map for the mode register (in bytes). + regmap: + $ref: /schemas/types.yaml#/definitions/phandle + description: Phandle to the register map node. + patternProperties: "^mode-.+": $ref: /schemas/types.yaml#/definitions/uint32