From: Serge Semin <[email protected]>
Optional regmap property will be used to refer to a syscon-controller
having a reboot tolerant register mapped.
Signed-off-by: Serge Semin <[email protected]>
Signed-off-by: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
---
.../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 06, 2020 at 04:03:40PM +0300, [email protected] wrote:
> From: Serge Semin <[email protected]>
>
> Optional regmap property will be used to refer to a syscon-controller
> having a reboot tolerant register mapped.
>
> Signed-off-by: Serge Semin <[email protected]>
> Signed-off-by: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> ---
Acked-by: Sebastian Reichel <[email protected]>
-- 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, [email protected] wrote:
> From: Serge Semin <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> ---
> .../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, [email protected] wrote:
> > From: Serge Semin <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Alexey Malahov <[email protected]>
> > Cc: Thomas Bogendoerfer <[email protected]>
> > Cc: Paul Burton <[email protected]>
> > Cc: Ralf Baechle <[email protected]>
> > ---
> > .../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, [email protected] wrote:
> > > From: Serge Semin <[email protected]>
> > > 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
<[email protected]> wrote:
>
> On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > On Fri, Mar 06, 2020 at 04:03:40PM +0300, [email protected] wrote:
> > > From: Serge Semin <[email protected]>
> > >
> > > 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
> <[email protected]> wrote:
> >
> > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, [email protected] wrote:
> > > > From: Serge Semin <[email protected]>
> > > >
> > > > 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
> > <[email protected]> wrote:
> > >
> > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, [email protected] wrote:
> > > > > From: Serge Semin <[email protected]>
> > > > >
> > > > > 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
> > > <[email protected]> wrote:
> > > >
> > > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, [email protected] wrote:
> > > > > > From: Serge Semin <[email protected]>
> > > > > >
> > > > > > 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
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, [email protected] wrote:
> > > > > > > From: Serge Semin <[email protected]>
> > > > > > >
> > > > > > > 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