| Message ID | 20221121065144.3667658-2-ping.bai@nxp.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add nxp bbnsm module support | expand |
On 21/11/2022 07:51, Jacky Bai wrote: > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > --- > .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 ++++++++++++++++++ > 1 file changed, 103 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > new file mode 100644 > index 000000000000..b3f22b0daea6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > @@ -0,0 +1,103 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/nxp,bbnsm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NXP Battery-Backed Non-Secure Module bindings > + > +maintainers: > + - Jacky Bai <ping.bai@nxp.com> > + > +description: | > + NXP BBNSM serves as non-volatile logic and storage for the system. > + it Intergrates RTC & ON/OFF control. > + The RTC can retain its state and continues counting even when the > + main chip is power down. A time alarm is generated once the most > + significant 32 bits of the real-time counter match the value in the > + Time Alarm register. > + The ON/OFF logic inside the BBNSM allows for connecting directly to > + a PMIC or other voltage regulator device. both smart PMIC mode and > + Dumb PMIC mode supported. > + > +properties: > + compatible: > + items: > + - enum: > + - nxp,bbnsm > + - const: syscon > + - const: simple-mfd > + > + reg: > + maxItems: 1 > + > + rtc: > + type: object > + > + properties: > + compatible: > + const: nxp,bbnsm-rtc Missing ref to rtc.yaml. > + > + regmap: Use vendor prefix, descriptive name and description. Where is the type of 'regmap' defined? > + maxItems: 1 I don't think this is correct. Rob explained the simple-mfd means children do not depend on anything from the parent, but taking a regmap from its parent contradicts it. > + > + interrupts: > + maxItems: 1 You have same interrupt and same address space used by two devices. Both arguments (depending on parent regmap, sharing interrupt) suggests that this is one device block, so describing it with simple-mfd is quite unflexible. > + > + required: > + - compatible > + - regmap > + - interrupts > + > + additionalProperties: false > + > + pwrkey: > + type: object > + $ref: /schemas/input/input.yaml# > + > + properties: > + compatible: > + const: nxp,bbnsm-pwrkey > + > + regmap: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + linux,code: true > + > + required: > + - compatible > + - regmap > + - interrupts > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - rtc > + - pwrkey > + > +additionalProperties: false > + > +examples: > + - | > + bbnsm: bbnsm@44440000 { > + compatible = "nxp,bbnsm", "syscon", "simple-mfd"; > + reg = <0x44440000 0x10000>; > + > + bbnsm_rtc: rtc { > + compatible = "nxp,bbnsm-rtc"; Use 4 spaces for example indentation. > + regmap = <&bbnsm>; > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > + }; > + > + bbnsm_pwrkey: pwrkey { > + compatible = "nxp,bbnsm-pwrkey"; > + regmap = <&bbnsm>; > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > + linux,code = <KEY_POWER>; > + }; > + }; Best regards, Krzysztof
Also few nits: On 21/11/2022 07:51, Jacky Bai wrote: > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). Subject: drop second, redundant "bindings". > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > --- > .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 ++++++++++++++++++ > 1 file changed, 103 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > new file mode 100644 > index 000000000000..b3f22b0daea6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > @@ -0,0 +1,103 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/nxp,bbnsm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NXP Battery-Backed Non-Secure Module bindings Drop "bindings" > + > +maintainers: > + - Jacky Bai <ping.bai@nxp.com> Best regards, Krzysztof
On 21/11/2022 10:09:40+0100, Krzysztof Kozlowski wrote: > On 21/11/2022 07:51, Jacky Bai wrote: > > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > --- > > .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 ++++++++++++++++++ > > 1 file changed, 103 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > > > > diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > > new file mode 100644 > > index 000000000000..b3f22b0daea6 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > > @@ -0,0 +1,103 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mfd/nxp,bbnsm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: NXP Battery-Backed Non-Secure Module bindings > > + > > +maintainers: > > + - Jacky Bai <ping.bai@nxp.com> > > + > > +description: | > > + NXP BBNSM serves as non-volatile logic and storage for the system. > > + it Intergrates RTC & ON/OFF control. > > + The RTC can retain its state and continues counting even when the > > + main chip is power down. A time alarm is generated once the most > > + significant 32 bits of the real-time counter match the value in the > > + Time Alarm register. > > + The ON/OFF logic inside the BBNSM allows for connecting directly to > > + a PMIC or other voltage regulator device. both smart PMIC mode and > > + Dumb PMIC mode supported. > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - nxp,bbnsm > > + - const: syscon > > + - const: simple-mfd > > + > > + reg: > > + maxItems: 1 > > + > > + rtc: > > + type: object > > + > > + properties: > > + compatible: > > + const: nxp,bbnsm-rtc > > > Missing ref to rtc.yaml. > This is also missing start-year > > + > > + regmap: > > Use vendor prefix, descriptive name and description. Where is the type > of 'regmap' defined? > > > + maxItems: 1 > > I don't think this is correct. Rob explained the simple-mfd means > children do not depend on anything from the parent, but taking a regmap > from its parent contradicts it. > > > + > > + interrupts: > > + maxItems: 1 > > You have same interrupt and same address space used by two devices. > > Both arguments (depending on parent regmap, sharing interrupt) suggests > that this is one device block, so describing it with simple-mfd is quite > unflexible. > > > + > > + required: > > + - compatible > > + - regmap > > + - interrupts > > + > > + additionalProperties: false > > + > > + pwrkey: > > + type: object > > + $ref: /schemas/input/input.yaml# > > + > > + properties: > > + compatible: > > + const: nxp,bbnsm-pwrkey > > + > > + regmap: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + linux,code: true > > + > > + required: > > + - compatible > > + - regmap > > + - interrupts > > + > > + additionalProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - rtc > > + - pwrkey > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + bbnsm: bbnsm@44440000 { > > + compatible = "nxp,bbnsm", "syscon", "simple-mfd"; > > + reg = <0x44440000 0x10000>; > > + > > + bbnsm_rtc: rtc { > > + compatible = "nxp,bbnsm-rtc"; > > Use 4 spaces for example indentation. > > > + regmap = <&bbnsm>; > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > + }; > > + > > + bbnsm_pwrkey: pwrkey { > > + compatible = "nxp,bbnsm-pwrkey"; > > + regmap = <&bbnsm>; > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > + linux,code = <KEY_POWER>; > > + }; > > + }; > > Best regards, > Krzysztof >
> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > bbnsm > > On 21/11/2022 07:51, Jacky Bai wrote: > > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > --- ... > > + > > + properties: > > + compatible: > > + const: nxp,bbnsm-rtc > > > Missing ref to rtc.yaml. Ok will include in v2. > > > + > > + regmap: > > Use vendor prefix, descriptive name and description. Where is the type of > 'regmap' defined? Type is missed. Will add a description and type define if necessary. > > > + maxItems: 1 > > I don't think this is correct. Rob explained the simple-mfd means children do > not depend on anything from the parent, but taking a regmap from its parent > contradicts it. For this BBNSM module, basically, it provides two sperate & different function: RTC and ON/OFF button control. But no separate register offset range for each of these functions. For example, the interrupt enable control, Interrupt status and basic function control are mixed in the same registers' different bit. Any good suggestion on how to handle such case? ^_^ > > > + > > + interrupts: > > + maxItems: 1 > > You have same interrupt and same address space used by two devices. > > Both arguments (depending on parent regmap, sharing interrupt) suggests > that this is one device block, so describing it with simple-mfd is quite > unflexible. > It is depends on how SoC integrates this BBNSM module. From the BBNSM side, it has separate IRQ lines for RTC function and ON/OFF function. Different IRQ lines can be used for RTC and ON/OFF button. But in current i.MX93 SoC, those interrupts are ORed together at SoC level. That's why same interrupt in the example. > > + > > + required: > > + - compatible > > + - regmap > > + - interrupts > > + > > + additionalProperties: false > > + > > + pwrkey: > > + type: object > > + $ref: /schemas/input/input.yaml# > > + > > + properties: > > + compatible: > > + const: nxp,bbnsm-pwrkey > > + > > + regmap: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + linux,code: true > > + > > + required: > > + - compatible > > + - regmap > > + - interrupts > > + > > + additionalProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - rtc > > + - pwrkey > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + bbnsm: bbnsm@44440000 { > > + compatible = "nxp,bbnsm", "syscon", "simple-mfd"; > > + reg = <0x44440000 0x10000>; > > + > > + bbnsm_rtc: rtc { > > + compatible = "nxp,bbnsm-rtc"; > > Use 4 spaces for example indentation. > Ok, will fix it. Thx BR > > + regmap = <&bbnsm>; > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > + }; > > + > > + bbnsm_pwrkey: pwrkey { > > + compatible = "nxp,bbnsm-pwrkey"; > > + regmap = <&bbnsm>; > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > + linux,code = <KEY_POWER>; > > + }; > > + }; > > Best regards, > Krzysztof
> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > bbnsm > > Also few nits: > > On 21/11/2022 07:51, Jacky Bai wrote: > > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > Subject: drop second, redundant "bindings". > Do you mean the redundant 'bindings' in below title line? > > + > > +title: NXP Battery-Backed Non-Secure Module bindings > > Drop "bindings" > Ok will drop it in v2. BR > > + > > +maintainers: > > + - Jacky Bai <ping.bai@nxp.com> > > Best regards, > Krzysztof
> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > bbnsm > > On 21/11/2022 10:09:40+0100, Krzysztof Kozlowski wrote: > > On 21/11/2022 07:51, Jacky Bai wrote: > > > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > > --- > > > .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 > ++++++++++++++++++ > > > 1 file changed, 103 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml ... > > > + > > > +title: NXP Battery-Backed Non-Secure Module bindings > > > + > > > +maintainers: > > > + - Jacky Bai <ping.bai@nxp.com> > > > + > > > +description: | > > > + NXP BBNSM serves as non-volatile logic and storage for the system. > > > + it Intergrates RTC & ON/OFF control. > > > + The RTC can retain its state and continues counting even when the > > > + main chip is power down. A time alarm is generated once the most > > > + significant 32 bits of the real-time counter match the value in > > > +the > > > + Time Alarm register. > > > + The ON/OFF logic inside the BBNSM allows for connecting directly > > > +to > > > + a PMIC or other voltage regulator device. both smart PMIC mode > > > +and > > > + Dumb PMIC mode supported. > > > + > > > +properties: > > > + compatible: > > > + items: > > > + - enum: > > > + - nxp,bbnsm > > > + - const: syscon > > > + - const: simple-mfd > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + rtc: > > > + type: object > > > + > > > + properties: > > > + compatible: > > > + const: nxp,bbnsm-rtc > > > > > > Missing ref to rtc.yaml. > > > > This is also missing start-year The RTC counter will be reset to 0 after PoR reset, do we still need to add this property? BR > > > > + > > > + regmap: > > > > Use vendor prefix, descriptive name and description. Where is the type > > of 'regmap' defined? > > > > > + maxItems: 1 > > > > I don't think this is correct. Rob explained the simple-mfd means > > children do not depend on anything from the parent, but taking a > > regmap from its parent contradicts it. > > > > > + > > > + interrupts: > > > + maxItems: 1 > > > > You have same interrupt and same address space used by two devices. > > > > Both arguments (depending on parent regmap, sharing interrupt) > > suggests that this is one device block, so describing it with > > simple-mfd is quite unflexible. > > > > > + > > > + required: > > > + - compatible > > > + - regmap > > > + - interrupts > > > + > > > + additionalProperties: false > > > + > > > + pwrkey: > > > + type: object > > > + $ref: /schemas/input/input.yaml# > > > + > > > + properties: > > > + compatible: > > > + const: nxp,bbnsm-pwrkey > > > + > > > + regmap: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + linux,code: true > > > + > > > + required: > > > + - compatible > > > + - regmap > > > + - interrupts > > > + > > > + additionalProperties: false > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - rtc > > > + - pwrkey > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + bbnsm: bbnsm@44440000 { > > > + compatible = "nxp,bbnsm", "syscon", "simple-mfd"; > > > + reg = <0x44440000 0x10000>; > > > + > > > + bbnsm_rtc: rtc { > > > + compatible = "nxp,bbnsm-rtc"; > > > > Use 4 spaces for example indentation. > > > > > + regmap = <&bbnsm>; > > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > > + }; > > > + > > > + bbnsm_pwrkey: pwrkey { > > > + compatible = "nxp,bbnsm-pwrkey"; > > > + regmap = <&bbnsm>; > > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > > + linux,code = <KEY_POWER>; > > > + }; > > > + }; > > > > Best regards, > > Krzysztof > > > > -- > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel > engineering > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin > .com%2F&data=05%7C01%7Cping.bai%40nxp.com%7Cd188bbb7b6ec40 > 5c481f08dacba2af8b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > 7C638046196834682924%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7 > C%7C&sdata=L%2FTZNaG01NTrKvbKwz9%2FNFEFQ6JqdnsOIzUydww1D > ZU%3D&reserved=0
On 21/11/2022 10:33:15+0000, Jacky Bai wrote: > > Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > > bbnsm > > > > On 21/11/2022 10:09:40+0100, Krzysztof Kozlowski wrote: > > > On 21/11/2022 07:51, Jacky Bai wrote: > > > > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > > > > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > > > --- > > > > .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 > > ++++++++++++++++++ > > > > 1 file changed, 103 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > > ... > > > > > + > > > > +title: NXP Battery-Backed Non-Secure Module bindings > > > > + > > > > +maintainers: > > > > + - Jacky Bai <ping.bai@nxp.com> > > > > + > > > > +description: | > > > > + NXP BBNSM serves as non-volatile logic and storage for the system. > > > > + it Intergrates RTC & ON/OFF control. > > > > + The RTC can retain its state and continues counting even when the > > > > + main chip is power down. A time alarm is generated once the most > > > > + significant 32 bits of the real-time counter match the value in > > > > +the > > > > + Time Alarm register. > > > > + The ON/OFF logic inside the BBNSM allows for connecting directly > > > > +to > > > > + a PMIC or other voltage regulator device. both smart PMIC mode > > > > +and > > > > + Dumb PMIC mode supported. > > > > + > > > > +properties: > > > > + compatible: > > > > + items: > > > > + - enum: > > > > + - nxp,bbnsm > > > > + - const: syscon > > > > + - const: simple-mfd > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + rtc: > > > > + type: object > > > > + > > > > + properties: > > > > + compatible: > > > > + const: nxp,bbnsm-rtc > > > > > > > > > Missing ref to rtc.yaml. > > > > > > > This is also missing start-year > > The RTC counter will be reset to 0 after PoR reset, do we still need to add > this property? > Is this really an RTC then? > BR > > > > > > + > > > > + regmap: > > > > > > Use vendor prefix, descriptive name and description. Where is the type > > > of 'regmap' defined? > > > > > > > + maxItems: 1 > > > > > > I don't think this is correct. Rob explained the simple-mfd means > > > children do not depend on anything from the parent, but taking a > > > regmap from its parent contradicts it. > > > > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > > > You have same interrupt and same address space used by two devices. > > > > > > Both arguments (depending on parent regmap, sharing interrupt) > > > suggests that this is one device block, so describing it with > > > simple-mfd is quite unflexible. > > > > > > > + > > > > + required: > > > > + - compatible > > > > + - regmap > > > > + - interrupts > > > > + > > > > + additionalProperties: false > > > > + > > > > + pwrkey: > > > > + type: object > > > > + $ref: /schemas/input/input.yaml# > > > > + > > > > + properties: > > > > + compatible: > > > > + const: nxp,bbnsm-pwrkey > > > > + > > > > + regmap: > > > > + maxItems: 1 > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > + > > > > + linux,code: true > > > > + > > > > + required: > > > > + - compatible > > > > + - regmap > > > > + - interrupts > > > > + > > > > + additionalProperties: false > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - rtc > > > > + - pwrkey > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + bbnsm: bbnsm@44440000 { > > > > + compatible = "nxp,bbnsm", "syscon", "simple-mfd"; > > > > + reg = <0x44440000 0x10000>; > > > > + > > > > + bbnsm_rtc: rtc { > > > > + compatible = "nxp,bbnsm-rtc"; > > > > > > Use 4 spaces for example indentation. > > > > > > > + regmap = <&bbnsm>; > > > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > > > + }; > > > > + > > > > + bbnsm_pwrkey: pwrkey { > > > > + compatible = "nxp,bbnsm-pwrkey"; > > > > + regmap = <&bbnsm>; > > > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > > > + linux,code = <KEY_POWER>; > > > > + }; > > > > + }; > > > > > > Best regards, > > > Krzysztof > > > > > > > -- > > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel > > engineering > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin > > .com%2F&data=05%7C01%7Cping.bai%40nxp.com%7Cd188bbb7b6ec40 > > 5c481f08dacba2af8b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > > 7C638046196834682924%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > > wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7 > > C%7C&sdata=L%2FTZNaG01NTrKvbKwz9%2FNFEFQ6JqdnsOIzUydww1D > > ZU%3D&reserved=0
My email client takes around 20s to open your emails.
Please fix that.
[-- Begin signature information --]
Problem signature from: CN=nxa19010,OU=Developers,OU=Managed Users,OU=CN,OU=NXP,DC=wbi,DC=nxp,DC=com
aka: <ping.bai@nxp.com>
created: Mon 21 Nov 2022 10:26:32 GMT
expires: Mon 08 Apr 2024 10:15:04 BST
[-- End signature information --]
> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > bbnsm > > On 21/11/2022 10:33:15+0000, Jacky Bai wrote: > > > Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding > > > for nxp bbnsm > > > > > > On 21/11/2022 10:09:40+0100, Krzysztof Kozlowski wrote: > > > > On 21/11/2022 07:51, Jacky Bai wrote: > > > > > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > > > > > > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > > > > --- > > > > > .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 > > > ++++++++++++++++++ > > > > > 1 file changed, 103 insertions(+) create mode 100644 > > > > > Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > > > > ... > > > > > > > + > > > > > +title: NXP Battery-Backed Non-Secure Module bindings > > > > > + > > > > > +maintainers: > > > > > + - Jacky Bai <ping.bai@nxp.com> > > > > > + > > > > > +description: | > > > > > + NXP BBNSM serves as non-volatile logic and storage for the > system. > > > > > + it Intergrates RTC & ON/OFF control. > > > > > + The RTC can retain its state and continues counting even when > > > > > +the > > > > > + main chip is power down. A time alarm is generated once the > > > > > +most > > > > > + significant 32 bits of the real-time counter match the value > > > > > +in the > > > > > + Time Alarm register. > > > > > + The ON/OFF logic inside the BBNSM allows for connecting > > > > > +directly to > > > > > + a PMIC or other voltage regulator device. both smart PMIC > > > > > +mode and > > > > > + Dumb PMIC mode supported. > > > > > + > > > > > +properties: > > > > > + compatible: > > > > > + items: > > > > > + - enum: > > > > > + - nxp,bbnsm > > > > > + - const: syscon > > > > > + - const: simple-mfd > > > > > + > > > > > + reg: > > > > > + maxItems: 1 > > > > > + > > > > > + rtc: > > > > > + type: object > > > > > + > > > > > + properties: > > > > > + compatible: > > > > > + const: nxp,bbnsm-rtc > > > > > > > > > > > > Missing ref to rtc.yaml. > > > > > > > > > > This is also missing start-year > > > > The RTC counter will be reset to 0 after PoR reset, do we still need > > to add this property? > > > > Is this really an RTC then? Sorry, I think I misunderstand your previous comment. The 'start-year' is used to expand the rtc range, I will add this property in V2. Thx. BR > > > BR > > > > > > > > + > > > > > + regmap: > > > > > > > > Use vendor prefix, descriptive name and description. Where is the > > > > type of 'regmap' defined? > > > > > > > > > + maxItems: 1 > > > > > > > > I don't think this is correct. Rob explained the simple-mfd means > > > > children do not depend on anything from the parent, but taking a > > > > regmap from its parent contradicts it. > > > > > > > > > + > > > > > + interrupts: > > > > > + maxItems: 1 > > > > > > > > You have same interrupt and same address space used by two devices. > > > > > > > > Both arguments (depending on parent regmap, sharing interrupt) > > > > suggests that this is one device block, so describing it with > > > > simple-mfd is quite unflexible. > > > > > > > > > + > > > > > + required: > > > > > + - compatible > > > > > + - regmap > > > > > + - interrupts > > > > > + > > > > > + additionalProperties: false > > > > > + > > > > > + pwrkey: > > > > > + type: object > > > > > + $ref: /schemas/input/input.yaml# > > > > > + > > > > > + properties: > > > > > + compatible: > > > > > + const: nxp,bbnsm-pwrkey > > > > > + > > > > > + regmap: > > > > > + maxItems: 1 > > > > > + > > > > > + interrupts: > > > > > + maxItems: 1 > > > > > + > > > > > + linux,code: true > > > > > + > > > > > + required: > > > > > + - compatible > > > > > + - regmap > > > > > + - interrupts > > > > > + > > > > > + additionalProperties: false > > > > > + > > > > > +required: > > > > > + - compatible > > > > > + - reg > > > > > + - rtc > > > > > + - pwrkey > > > > > + > > > > > +additionalProperties: false > > > > > + > > > > > +examples: > > > > > + - | > > > > > + bbnsm: bbnsm@44440000 { > > > > > + compatible = "nxp,bbnsm", "syscon", "simple-mfd"; > > > > > + reg = <0x44440000 0x10000>; > > > > > + > > > > > + bbnsm_rtc: rtc { > > > > > + compatible = "nxp,bbnsm-rtc"; > > > > > > > > Use 4 spaces for example indentation. > > > > > > > > > + regmap = <&bbnsm>; > > > > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > > > > + }; > > > > > + > > > > > + bbnsm_pwrkey: pwrkey { > > > > > + compatible = "nxp,bbnsm-pwrkey"; > > > > > + regmap = <&bbnsm>; > > > > > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; > > > > > + linux,code = <KEY_POWER>; > > > > > + }; > > > > > + }; > > > > > > > > Best regards, > > > > Krzysztof > > > > > > > > > > --
On 21/11/2022 11:26, Jacky Bai wrote: >> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp >> bbnsm >> >> On 21/11/2022 07:51, Jacky Bai wrote: >>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). >>> >>> Signed-off-by: Jacky Bai <ping.bai@nxp.com> >>> --- > > ... > >>> + >>> + properties: >>> + compatible: >>> + const: nxp,bbnsm-rtc >> >> >> Missing ref to rtc.yaml. > > Ok will include in v2. >> >>> + >>> + regmap: >> >> Use vendor prefix, descriptive name and description. Where is the type of >> 'regmap' defined? > > Type is missed. Will add a description and type define if necessary. > >> >>> + maxItems: 1 >> >> I don't think this is correct. Rob explained the simple-mfd means children > do >> not depend on anything from the parent, but taking a regmap from its > parent >> contradicts it. > > For this BBNSM module, basically, it provides two sperate & different > function: RTC and ON/OFF button control. But > no separate register offset range for each of these functions. For example, > the interrupt enable control, > Interrupt status and basic function control are mixed in the same registers' > different bit. > > Any good suggestion on how to handle such case? ^_^ The solution for more complex common parts, dedicated device driver (MFD driver) with its own binding. However I understand why it would be overshoot here. > >> >>> + >>> + interrupts: >>> + maxItems: 1 >> >> You have same interrupt and same address space used by two devices. >> >> Both arguments (depending on parent regmap, sharing interrupt) suggests >> that this is one device block, so describing it with simple-mfd is quite >> unflexible. >> > > It is depends on how SoC integrates this BBNSM module. From the BBNSM side, > it has separate IRQ lines for RTC function and ON/OFF function. Different > IRQ lines > can be used for RTC and ON/OFF button. But in current i.MX93 SoC, those > interrupts > are ORed together at SoC level. That's why same interrupt in the example. It's fine then. Best regards, Krzysztof
On 21/11/2022 11:30, Jacky Bai wrote: >> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp >> bbnsm >> >> Also few nits: >> >> On 21/11/2022 07:51, Jacky Bai wrote: >>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). >> >> Subject: drop second, redundant "bindings". >> > > Do you mean the redundant 'bindings' in below title line? No, I meant subject. Best regards, Krzysztof
On 21/11/2022 13:45:27+0000, Jacky Bai wrote: > > > > > Missing ref to rtc.yaml. > > > > > > > > > > > > > This is also missing start-year > > > > > > The RTC counter will be reset to 0 after PoR reset, do we still need > > > to add this property? > > > > > > > Is this really an RTC then? > > Sorry, I think I misunderstand your previous comment. The 'start-year' is used to expand the rtc range, > I will add this property in V2. Thx. > I think my question is still valid, if the counter is reset to 0 on POR, what is the use case?
On Mon, 21 Nov 2022 14:51:41 +0800, Jacky Bai wrote: > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > --- > .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 ++++++++++++++++++ > 1 file changed, 103 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Error: Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dts:28.27-28 syntax error FATAL ERROR: Unable to parse input tree make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dtb] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1492: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221121065144.3667658-2-ping.bai@nxp.com This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command.
> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > bbnsm > > On 21/11/2022 11:26, Jacky Bai wrote: > >> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for > >> nxp bbnsm > >> > >> On 21/11/2022 07:51, Jacky Bai wrote: > >>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > >>> > >>> Signed-off-by: Jacky Bai <ping.bai@nxp.com> > >>> --- > > > > ... > > > >>> + > >>> + properties: > >>> + compatible: > >>> + const: nxp,bbnsm-rtc > >> > >> > >> Missing ref to rtc.yaml. > > > > Ok will include in v2. > >> > >>> + > >>> + regmap: > >> > >> Use vendor prefix, descriptive name and description. Where is the > >> type of 'regmap' defined? > > > > Type is missed. Will add a description and type define if necessary. > > > >> > >>> + maxItems: 1 > >> > >> I don't think this is correct. Rob explained the simple-mfd means > >> children > > do > >> not depend on anything from the parent, but taking a regmap from its > > parent > >> contradicts it. > > > > For this BBNSM module, basically, it provides two sperate & different > > function: RTC and ON/OFF button control. But no separate register > > offset range for each of these functions. For example, the interrupt > > enable control, Interrupt status and basic function control are mixed > > in the same registers' > > different bit. > > > > Any good suggestion on how to handle such case? ^_^ > > The solution for more complex common parts, dedicated device driver (MFD > driver) with its own binding. However I understand why it would be overshoot > here. > Is it ok to keep current implementation rather than reimplement a new dedicate MFD wrapper driver? BR > > > >> > >>> + > >>> + interrupts: > >>> + maxItems: 1 > >> > >> You have same interrupt and same address space used by two devices. > >> > >> Both arguments (depending on parent regmap, sharing interrupt) > >> suggests that this is one device block, so describing it with > >> simple-mfd is quite unflexible. > >> > > > > It is depends on how SoC integrates this BBNSM module. From the BBNSM > > side, it has separate IRQ lines for RTC function and ON/OFF function. > > Different IRQ lines can be used for RTC and ON/OFF button. But in > > current i.MX93 SoC, those interrupts are ORed together at SoC level. > > That's why same interrupt in the example. > > It's fine then. > > Best regards, > Krzysztof
Hi Alexandre, > Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > bbnsm > > On 21/11/2022 13:45:27+0000, Jacky Bai wrote: > > > > > > Missing ref to rtc.yaml. > > > > > > > > > > > > > > > > This is also missing start-year > > > > > > > > The RTC counter will be reset to 0 after PoR reset, do we still > > > > need to add this property? > > > > > > > > > > Is this really an RTC then? > > > > Sorry, I think I misunderstand your previous comment. The 'start-year' > > is used to expand the rtc range, I will add this property in V2. Thx. > > > > I think my question is still valid, if the counter is reset to 0 on POR, what is the > use case? > The RTC can keep running without losing the state if the power rail to it is not cut off. It has a battery backed supply even if the main power rail is off when device shutdown. I thought the 'start-year' is to indicate the initial value of the RTC count after first power up. BR > > --
Hi Rob, > Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > bbnsm > > > On Mon, 21 Nov 2022 14:51:41 +0800, Jacky Bai wrote: > > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com> > > --- > > .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 > ++++++++++++++++++ > > 1 file changed, 103 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m > dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Error: > Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dts:28.27-28 > syntax error FATAL ERROR: Unable to parse input tree > make[1]: *** [scripts/Makefile.lib:406: > Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dtb] Error 1 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1492: dt_binding_check] Error 2 > This error should be related to the 'interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;' Do we need to change it a magic number define? BR Jacky Bai > doc reference errors (make refcheckdocs): > ... > > This check can fail if there are any dependencies. The base for a patch series > is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above error(s), > then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit after running the above command.
On 23/11/2022 08:43, Jacky Bai wrote: >> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp >> bbnsm >> >> On 21/11/2022 11:26, Jacky Bai wrote: >>>> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for >>>> nxp bbnsm >>>> >>>> On 21/11/2022 07:51, Jacky Bai wrote: >>>>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). >>>>> >>>>> Signed-off-by: Jacky Bai <ping.bai@nxp.com> >>>>> --- >>> >>> ... >>> >>>>> + >>>>> + properties: >>>>> + compatible: >>>>> + const: nxp,bbnsm-rtc >>>> >>>> >>>> Missing ref to rtc.yaml. >>> >>> Ok will include in v2. >>>> >>>>> + >>>>> + regmap: >>>> >>>> Use vendor prefix, descriptive name and description. Where is the >>>> type of 'regmap' defined? >>> >>> Type is missed. Will add a description and type define if necessary. >>> >>>> >>>>> + maxItems: 1 >>>> >>>> I don't think this is correct. Rob explained the simple-mfd means >>>> children >>> do >>>> not depend on anything from the parent, but taking a regmap from its >>> parent >>>> contradicts it. >>> >>> For this BBNSM module, basically, it provides two sperate & different >>> function: RTC and ON/OFF button control. But no separate register >>> offset range for each of these functions. For example, the interrupt >>> enable control, Interrupt status and basic function control are mixed >>> in the same registers' >>> different bit. >>> >>> Any good suggestion on how to handle such case? ^_^ >> >> The solution for more complex common parts, dedicated device driver (MFD >> driver) with its own binding. However I understand why it would be overshoot >> here. >> > > Is it ok to keep current implementation rather than reimplement a new dedicate MFD wrapper driver? Yes Best regards, Krzysztof
On 23/11/2022 08:54, Jacky Bai wrote: > Hi Rob, > >> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp >> bbnsm >> >> >> On Mon, 21 Nov 2022 14:51:41 +0800, Jacky Bai wrote: >>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). >>> >>> Signed-off-by: Jacky Bai <ping.bai@nxp.com> >>> --- >>> .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 >> ++++++++++++++++++ >>> 1 file changed, 103 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml >>> >> >> My bot found errors running 'make DT_CHECKER_FLAGS=-m >> dt_binding_check' >> on your patch (DT_CHECKER_FLAGS is new in v5.13): >> >> yamllint warnings/errors: >> >> dtschema/dtc warnings/errors: >> Error: >> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dts:28.27-28 >> syntax error FATAL ERROR: Unable to parse input tree >> make[1]: *** [scripts/Makefile.lib:406: >> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dtb] Error 1 >> make[1]: *** Waiting for unfinished jobs.... >> make: *** [Makefile:1492: dt_binding_check] Error 2 >> > > This error should be related to the 'interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;' > Do we need to change it a magic number define? You should include a proper header. Look at other bindings. Best regards, Krzysztof
> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp > bbnsm > > On 23/11/2022 08:54, Jacky Bai wrote: > > Hi Rob, > > > >> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for > >> nxp bbnsm > >> > >> > >> On Mon, 21 Nov 2022 14:51:41 +0800, Jacky Bai wrote: > >>> Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). > >>> > >>> Signed-off-by: Jacky Bai <ping.bai@nxp.com> > >>> --- > >>> .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 > >> ++++++++++++++++++ > >>> 1 file changed, 103 insertions(+) > >>> create mode 100644 > >>> Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml > >>> > >> > >> My bot found errors running 'make DT_CHECKER_FLAGS=-m > >> dt_binding_check' > >> on your patch (DT_CHECKER_FLAGS is new in v5.13): > >> > >> yamllint warnings/errors: > >> > >> dtschema/dtc warnings/errors: > >> Error: > >> > Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dts:28.27-28 > >> syntax error FATAL ERROR: Unable to parse input tree > >> make[1]: *** [scripts/Makefile.lib:406: > >> Documentation/devicetree/bindings/mfd/nxp,bbnsm.example.dtb] Error 1 > >> make[1]: *** Waiting for unfinished jobs.... > >> make: *** [Makefile:1492: dt_binding_check] Error 2 > >> > > > > This error should be related to the 'interrupts = <GIC_SPI 73 > IRQ_TYPE_LEVEL_HIGH>;' > > Do we need to change it a magic number define? > > You should include a proper header. Look at other bindings. > Great thx. I missed the header file including . Will fix in V2. ^_^ BR > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml new file mode 100644 index 000000000000..b3f22b0daea6 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml @@ -0,0 +1,103 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/nxp,bbnsm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP Battery-Backed Non-Secure Module bindings + +maintainers: + - Jacky Bai <ping.bai@nxp.com> + +description: | + NXP BBNSM serves as non-volatile logic and storage for the system. + it Intergrates RTC & ON/OFF control. + The RTC can retain its state and continues counting even when the + main chip is power down. A time alarm is generated once the most + significant 32 bits of the real-time counter match the value in the + Time Alarm register. + The ON/OFF logic inside the BBNSM allows for connecting directly to + a PMIC or other voltage regulator device. both smart PMIC mode and + Dumb PMIC mode supported. + +properties: + compatible: + items: + - enum: + - nxp,bbnsm + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + rtc: + type: object + + properties: + compatible: + const: nxp,bbnsm-rtc + + regmap: + maxItems: 1 + + interrupts: + maxItems: 1 + + required: + - compatible + - regmap + - interrupts + + additionalProperties: false + + pwrkey: + type: object + $ref: /schemas/input/input.yaml# + + properties: + compatible: + const: nxp,bbnsm-pwrkey + + regmap: + maxItems: 1 + + interrupts: + maxItems: 1 + + linux,code: true + + required: + - compatible + - regmap + - interrupts + + additionalProperties: false + +required: + - compatible + - reg + - rtc + - pwrkey + +additionalProperties: false + +examples: + - | + bbnsm: bbnsm@44440000 { + compatible = "nxp,bbnsm", "syscon", "simple-mfd"; + reg = <0x44440000 0x10000>; + + bbnsm_rtc: rtc { + compatible = "nxp,bbnsm-rtc"; + regmap = <&bbnsm>; + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; + }; + + bbnsm_pwrkey: pwrkey { + compatible = "nxp,bbnsm-pwrkey"; + regmap = <&bbnsm>; + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>; + linux,code = <KEY_POWER>; + }; + };
Add binding for NXP BBNSM(Battery-Backed Non-Secure Module). Signed-off-by: Jacky Bai <ping.bai@nxp.com> --- .../devicetree/bindings/mfd/nxp,bbnsm.yaml | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml