| Message ID | E1oTkeH-003t9A-3K@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Apple Mac System Management Controller GPIOs | expand |
On 01/09/2022 16:54, Russell King (Oracle) wrote: > Add a DT binding for the Apple Mac System Management Controller. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thank you for your patch. There is something to discuss/improve. > + > +maintainers: > + - Hector Martin <marcan@marcan.st> > + > +description: > + Apple Mac System Management Controller implements various functions > + such as GPIO, RTC, power, reboot. > + > +properties: > + compatible: > + items: > + - enum: > + - apple,t8103-smc You miss two spaces of indentation on this level. > + - apple,t8112-smc > + - apple,t6000-smc Bring some order here - either alphabetical or by date of release (as in other Apple schemas). I think t6000 was before t8112, so it's none of that orders. > + - const: apple,smc > + > + reg: > + description: Two regions, one for the SMC area and one for the SRAM area. You need constraints for size/order, so in this context list with described items. > + > + reg-names: > + items: > + - const: smc > + - const: sram > + > + mboxes: > + description: > + A phandle to the mailbox channel Missing maxItems > + > +additionalProperties: false > + Best regards, Krzysztof
On Thu, Sep 01, 2022 at 06:06:17PM +0300, Krzysztof Kozlowski wrote: > On 01/09/2022 16:54, Russell King (Oracle) wrote: > > Add a DT binding for the Apple Mac System Management Controller. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Thank you for your patch. There is something to discuss/improve. > > > + > > +maintainers: > > + - Hector Martin <marcan@marcan.st> > > + > > +description: > > + Apple Mac System Management Controller implements various functions > > + such as GPIO, RTC, power, reboot. > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - apple,t8103-smc > > You miss two spaces of indentation on this level. Should that be picked up by the dt checker? > > + - apple,t8112-smc > > + - apple,t6000-smc > > Bring some order here - either alphabetical or by date of release (as in > other Apple schemas). I think t6000 was before t8112, so it's none of > that orders. Ok. > > + - const: apple,smc > > + > > + reg: > > + description: Two regions, one for the SMC area and one for the SRAM area. > > You need constraints for size/order, so in this context list with > described items. How do I do that? I tried maxItems/minItems set to 2, but the dt checker objected to it. > > + reg-names: > > + items: > > + - const: smc > > + - const: sram > > + > > + mboxes: > > + description: > > + A phandle to the mailbox channel > > Missing maxItems Ok. Would be helpful if the dt checker identified that.
On 01/09/2022 18:12, Russell King (Oracle) wrote: >>> + compatible: >>> + items: >>> + - enum: >>> + - apple,t8103-smc >> >> You miss two spaces of indentation on this level. > > Should that be picked up by the dt checker? I think yamllint complains about it. It is not a hard-dependency, so maybe you don't have it installed. > >>> + - apple,t8112-smc >>> + - apple,t6000-smc >> >> Bring some order here - either alphabetical or by date of release (as in >> other Apple schemas). I think t6000 was before t8112, so it's none of >> that orders. > > Ok. > >>> + - const: apple,smc >>> + >>> + reg: >>> + description: Two regions, one for the SMC area and one for the SRAM area. >> >> You need constraints for size/order, so in this context list with >> described items. > > How do I do that? I tried maxItems/minItems set to 2, but the dt checker > objected to it. One way: reg: items: - description: SMC area - description: SRAM area but actually this is very similar what you wrote for reg-names - kind of obvious, so easier way: reg: maxItems: 2 > >>> + reg-names: >>> + items: >>> + - const: smc >>> + - const: sram >>> + >>> + mboxes: >>> + description: >>> + A phandle to the mailbox channel >> >> Missing maxItems > > Ok. Would be helpful if the dt checker identified that. Patches are welcomed :) Best regards, Krzysztof
On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote: > On 01/09/2022 18:12, Russell King (Oracle) wrote: > >>> + compatible: > >>> + items: > >>> + - enum: > >>> + - apple,t8103-smc > >> > >> You miss two spaces of indentation on this level. > > > > Should that be picked up by the dt checker? > > I think yamllint complains about it. It is not a hard-dependency, so > maybe you don't have it installed. > > > > >>> + - apple,t8112-smc > >>> + - apple,t6000-smc > >> > >> Bring some order here - either alphabetical or by date of release (as in > >> other Apple schemas). I think t6000 was before t8112, so it's none of > >> that orders. > > > > Ok. > > > >>> + - const: apple,smc > >>> + > >>> + reg: > >>> + description: Two regions, one for the SMC area and one for the SRAM area. > >> > >> You need constraints for size/order, so in this context list with > >> described items. > > > > How do I do that? I tried maxItems/minItems set to 2, but the dt checker > > objected to it. > > One way: > reg: > items: > - description: SMC area > - description: SRAM area > > but actually this is very similar what you wrote for reg-names - kind of > obvious, so easier way: > > reg: > maxItems: 2 Doesn't work. With maxItems: 2, the example fails, yet it correctly lists two regs which are 64-bit address and 64-bit size - so in total 8 32-bit ints. Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml Hence, I originally had maxItems: 2, and ended up deleting it because of the dt checker. With the two descriptions, it's the same failure. I think the problem is that the checker has no knowledge in the example of how big each address and size element of the reg property is. So, it's interpreting it as four entries of 32-bit address,size pairs instead of two entries of 64-bit address,size pairs. Yep, that's it, if I increase the number of "- description" entries to four then it's happy. So, what's the solution?
On 01/09/2022 18:24, Russell King (Oracle) wrote: > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote: >> On 01/09/2022 18:12, Russell King (Oracle) wrote: >>>>> + compatible: >>>>> + items: >>>>> + - enum: >>>>> + - apple,t8103-smc >>>> >>>> You miss two spaces of indentation on this level. >>> >>> Should that be picked up by the dt checker? >> >> I think yamllint complains about it. It is not a hard-dependency, so >> maybe you don't have it installed. >> >>> >>>>> + - apple,t8112-smc >>>>> + - apple,t6000-smc >>>> >>>> Bring some order here - either alphabetical or by date of release (as in >>>> other Apple schemas). I think t6000 was before t8112, so it's none of >>>> that orders. >>> >>> Ok. >>> >>>>> + - const: apple,smc >>>>> + >>>>> + reg: >>>>> + description: Two regions, one for the SMC area and one for the SRAM area. >>>> >>>> You need constraints for size/order, so in this context list with >>>> described items. >>> >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker >>> objected to it. >> >> One way: >> reg: >> items: >> - description: SMC area >> - description: SRAM area >> >> but actually this is very similar what you wrote for reg-names - kind of >> obvious, so easier way: >> >> reg: >> maxItems: 2 > > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit > ints. > > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long > From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > Hence, I originally had maxItems: 2, and ended up deleting it because of > the dt checker. > > With the two descriptions, it's the same failure. Yeah, they should create same result. > > I think the problem is that the checker has no knowledge in the example > of how big each address and size element of the reg property is. So, > it's interpreting it as four entries of 32-bit address,size pairs > instead of two entries of 64-bit address,size pairs. Yep, that's it, > if I increase the number of "- description" entries to four then it's > happy. > > So, what's the solution? > If you open generated DTS examples (in your kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which address/size cells are expected. By default it is I think address/size cells=1, so you need a bus node setting it to 2. Best regards, Krzysztof
On Thu, Sep 01, 2022 at 06:45:52PM +0300, Krzysztof Kozlowski wrote: > On 01/09/2022 18:24, Russell King (Oracle) wrote: > > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote: > >> On 01/09/2022 18:12, Russell King (Oracle) wrote: > >>>>> + compatible: > >>>>> + items: > >>>>> + - enum: > >>>>> + - apple,t8103-smc > >>>> > >>>> You miss two spaces of indentation on this level. > >>> > >>> Should that be picked up by the dt checker? > >> > >> I think yamllint complains about it. It is not a hard-dependency, so > >> maybe you don't have it installed. > >> > >>> > >>>>> + - apple,t8112-smc > >>>>> + - apple,t6000-smc > >>>> > >>>> Bring some order here - either alphabetical or by date of release (as in > >>>> other Apple schemas). I think t6000 was before t8112, so it's none of > >>>> that orders. > >>> > >>> Ok. > >>> > >>>>> + - const: apple,smc > >>>>> + > >>>>> + reg: > >>>>> + description: Two regions, one for the SMC area and one for the SRAM area. > >>>> > >>>> You need constraints for size/order, so in this context list with > >>>> described items. > >>> > >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker > >>> objected to it. > >> > >> One way: > >> reg: > >> items: > >> - description: SMC area > >> - description: SRAM area > >> > >> but actually this is very similar what you wrote for reg-names - kind of > >> obvious, so easier way: > >> > >> reg: > >> maxItems: 2 > > > > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists > > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit > > ints. > > > > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long > > From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > > > Hence, I originally had maxItems: 2, and ended up deleting it because of > > the dt checker. > > > > With the two descriptions, it's the same failure. > > Yeah, they should create same result. > > > > > I think the problem is that the checker has no knowledge in the example > > of how big each address and size element of the reg property is. So, > > it's interpreting it as four entries of 32-bit address,size pairs > > instead of two entries of 64-bit address,size pairs. Yep, that's it, > > if I increase the number of "- description" entries to four then it's > > happy. > > > > So, what's the solution? > > > > If you open generated DTS examples (in your > kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which > address/size cells are expected. By default it is I think address/size > cells=1, so you need a bus node setting it to 2. Thanks, that works. The patch with all those points addressed now looks like: 8<=== From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management Controller Add a DT binding for the Apple Mac System Management Controller. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- .../devicetree/bindings/mfd/apple,smc.yaml | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml new file mode 100644 index 000000000000..168f237c2962 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple Mac System Management Controller + +maintainers: + - Hector Martin <marcan@marcan.st> + +description: + Apple Mac System Management Controller implements various functions + such as GPIO, RTC, power, reboot. + +properties: + compatible: + items: + - enum: + - apple,t6000-smc + - apple,t8103-smc + - apple,t8112-smc + - const: apple,smc + + reg: + items: + - description: SMC area + - description: SRAM area + + reg-names: + items: + - const: smc + - const: sram + + mboxes: + maxItems: 1 + description: + A phandle to the mailbox channel + +additionalProperties: false + +required: + - compatible + - reg + - reg-names + - mboxes + +examples: + - | + soc { + #address-cells = <2>; + #size-cells = <2>; + + smc@23e400000 { + compatible = "apple,t8103-smc", "apple,smc"; + reg = <0x2 0x3e400000 0x0 0x4000>, + <0x2 0x3fe00000 0x0 0x100000>; + reg-names = "smc", "sram"; + mboxes = <&smc_mbox>; + }; + };
On 01/09/2022 18:56, Russell King (Oracle) wrote: > > 8<=== > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > Controller > > Add a DT binding for the Apple Mac System Management Controller. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Yes, looks good. I won't add Reviewed-by tag, because I think it would confuse Patchwork, so please send a v2 at some point. Best regards, Krzysztof
On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote: > On 01/09/2022 18:56, Russell King (Oracle) wrote: > > > > 8<=== > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > > Controller > > > > Add a DT binding for the Apple Mac System Management Controller. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Yes, looks good. > > I won't add Reviewed-by tag, because I think it would confuse Patchwork, > so please send a v2 at some point. Thanks. Do you have any suggestions for patch 2? Should I merge the description in patch 2 into this file? The full dts for this series looks like this: smc: smc@23e400000 { compatible = "apple,t8103-smc", "apple,smc"; reg = <0x2 0x3e400000 0x0 0x4000>, <0x2 0x3fe00000 0x0 0x100000>; reg-names = "smc", "sram"; mboxes = <&smc_mbox>; smc_gpio: gpio { gpio-controller; #gpio-cells = <2>; }; }; but the fuller version in the asahi linux tree looks like: smc: smc@23e400000 { compatible = "apple,t8103-smc", "apple,smc"; reg = <0x2 0x3e400000 0x0 0x4000>, <0x2 0x3fe00000 0x0 0x100000>; reg-names = "smc", "sram"; mboxes = <&smc_mbox>; smc_gpio: gpio { gpio-controller; #gpio-cells = <2>; }; smc_rtc: rtc { nvmem-cells = <&rtc_offset>; nvmem-cell-names = "rtc_offset"; }; smc_reboot: reboot { nvmem-cells = <&shutdown_flag>, <&boot_stage>, <&boot_error_count>, <&panic_count>, <&pm_setting>; nvmem-cell-names = "shutdown_flag", "boot_stage", "boot_error_count", "panic_count", "pm_setting"; }; }; So whatever is decided for the gpio node would eventually also apply to the other nodes too. Thanks.
On Thu, 01 Sep 2022 14:54:25 +0100, Russell King (Oracle) wrote: > Add a DT binding for the Apple Mac System Management Controller. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > .../devicetree/bindings/mfd/apple,smc.yaml | 53 +++++++++++++++++++ > 1 file changed, 53 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.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: ./Documentation/devicetree/bindings/mfd/apple,smc.yaml:20:9: [warning] wrong indentation: expected 10 but found 8 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ 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.
On Thu, Sep 1, 2022 at 10:56 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Thu, Sep 01, 2022 at 06:45:52PM +0300, Krzysztof Kozlowski wrote: > > On 01/09/2022 18:24, Russell King (Oracle) wrote: > > > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote: > > >> On 01/09/2022 18:12, Russell King (Oracle) wrote: > > >>>>> + compatible: > > >>>>> + items: > > >>>>> + - enum: > > >>>>> + - apple,t8103-smc > > >>>> > > >>>> You miss two spaces of indentation on this level. > > >>> > > >>> Should that be picked up by the dt checker? I have a problem that Krzysztof is quicker. ;) Maybe I should stop screening the emails (for the times I break things mostly). > > >> > > >> I think yamllint complains about it. It is not a hard-dependency, so > > >> maybe you don't have it installed. > > >> > > >>> > > >>>>> + - apple,t8112-smc > > >>>>> + - apple,t6000-smc > > >>>> > > >>>> Bring some order here - either alphabetical or by date of release (as in > > >>>> other Apple schemas). I think t6000 was before t8112, so it's none of > > >>>> that orders. > > >>> > > >>> Ok. > > >>> > > >>>>> + - const: apple,smc > > >>>>> + > > >>>>> + reg: > > >>>>> + description: Two regions, one for the SMC area and one for the SRAM area. > > >>>> > > >>>> You need constraints for size/order, so in this context list with > > >>>> described items. > > >>> > > >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker > > >>> objected to it. > > >> > > >> One way: > > >> reg: > > >> items: > > >> - description: SMC area > > >> - description: SRAM area > > >> > > >> but actually this is very similar what you wrote for reg-names - kind of > > >> obvious, so easier way: > > >> > > >> reg: > > >> maxItems: 2 > > > > > > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists > > > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit > > > ints. > > > > > > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long > > > From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > > > > > Hence, I originally had maxItems: 2, and ended up deleting it because of > > > the dt checker. > > > > > > With the two descriptions, it's the same failure. > > > > Yeah, they should create same result. > > > > > > > > I think the problem is that the checker has no knowledge in the example > > > of how big each address and size element of the reg property is. So, > > > it's interpreting it as four entries of 32-bit address,size pairs > > > instead of two entries of 64-bit address,size pairs. Yep, that's it, > > > if I increase the number of "- description" entries to four then it's > > > happy. > > > > > > So, what's the solution? > > > > > > > If you open generated DTS examples (in your > > kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which > > address/size cells are expected. By default it is I think address/size > > cells=1, so you need a bus node setting it to 2. > > Thanks, that works. The patch with all those points addressed now looks > like: > > 8<=== > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > Controller > > Add a DT binding for the Apple Mac System Management Controller. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > .../devicetree/bindings/mfd/apple,smc.yaml | 61 +++++++++++++++++++ > 1 file changed, 61 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml > new file mode 100644 > index 000000000000..168f237c2962 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Apple Mac System Management Controller > + > +maintainers: > + - Hector Martin <marcan@marcan.st> > + > +description: > + Apple Mac System Management Controller implements various functions > + such as GPIO, RTC, power, reboot. > + > +properties: > + compatible: > + items: > + - enum: > + - apple,t6000-smc > + - apple,t8103-smc > + - apple,t8112-smc > + - const: apple,smc > + > + reg: > + items: > + - description: SMC area > + - description: SRAM area Based on the disjoint addresses, is this really one device? Perhaps the SRAM area should use mmio-sram binding? That already supports sub-dividing the sram for different uses. I'll comment more on the full example. Rob
On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote: > > On 01/09/2022 18:56, Russell King (Oracle) wrote: > > > > > > 8<=== > > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > > > Controller > > > > > > Add a DT binding for the Apple Mac System Management Controller. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > Yes, looks good. > > > > I won't add Reviewed-by tag, because I think it would confuse Patchwork, > > so please send a v2 at some point. > > Thanks. Do you have any suggestions for patch 2? Should I merge the > description in patch 2 into this file? > > The full dts for this series looks like this: > > smc: smc@23e400000 { > compatible = "apple,t8103-smc", "apple,smc"; > reg = <0x2 0x3e400000 0x0 0x4000>, > <0x2 0x3fe00000 0x0 0x100000>; > reg-names = "smc", "sram"; > mboxes = <&smc_mbox>; > > smc_gpio: gpio { > gpio-controller; > #gpio-cells = <2>; > }; > }; > > but the fuller version in the asahi linux tree looks like: > > smc: smc@23e400000 { > compatible = "apple,t8103-smc", "apple,smc"; > reg = <0x2 0x3e400000 0x0 0x4000>, > <0x2 0x3fe00000 0x0 0x100000>; > reg-names = "smc", "sram"; > mboxes = <&smc_mbox>; > > smc_gpio: gpio { > gpio-controller; > #gpio-cells = <2>; Only 2 properties doesn't really need its own schema doc. However, I would just move these to the parent node. > }; > > smc_rtc: rtc { > nvmem-cells = <&rtc_offset>; > nvmem-cell-names = "rtc_offset"; > }; > > smc_reboot: reboot { > nvmem-cells = <&shutdown_flag>, <&boot_stage>, > <&boot_error_count>, <&panic_count>, <&pm_setting>; > nvmem-cell-names = "shutdown_flag", "boot_stage", > "boot_error_count", "panic_count", "pm_setting"; Not really much reason to split these up either because you can just fetch the entry you want by name. How confident are the asahi folks that this is a complete binding? Rob
> From: Rob Herring <robh+dt@kernel.org> > Date: Thu, 1 Sep 2022 17:26:18 -0500 > > On Thu, Sep 1, 2022 at 10:56 AM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Thu, Sep 01, 2022 at 06:45:52PM +0300, Krzysztof Kozlowski wrote: > > > On 01/09/2022 18:24, Russell King (Oracle) wrote: > > > > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote: > > > >> On 01/09/2022 18:12, Russell King (Oracle) wrote: > > > >>>>> + compatible: > > > >>>>> + items: > > > >>>>> + - enum: > > > >>>>> + - apple,t8103-smc > > > >>>> > > > >>>> You miss two spaces of indentation on this level. > > > >>> > > > >>> Should that be picked up by the dt checker? > > I have a problem that Krzysztof is quicker. ;) Maybe I should stop > screening the emails (for the times I break things mostly). > > > > >> > > > >> I think yamllint complains about it. It is not a hard-dependency, so > > > >> maybe you don't have it installed. > > > >> > > > >>> > > > >>>>> + - apple,t8112-smc > > > >>>>> + - apple,t6000-smc > > > >>>> > > > >>>> Bring some order here - either alphabetical or by date of release (as in > > > >>>> other Apple schemas). I think t6000 was before t8112, so it's none of > > > >>>> that orders. > > > >>> > > > >>> Ok. > > > >>> > > > >>>>> + - const: apple,smc > > > >>>>> + > > > >>>>> + reg: > > > >>>>> + description: Two regions, one for the SMC area and one for the SRAM area. > > > >>>> > > > >>>> You need constraints for size/order, so in this context list with > > > >>>> described items. > > > >>> > > > >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker > > > >>> objected to it. > > > >> > > > >> One way: > > > >> reg: > > > >> items: > > > >> - description: SMC area > > > >> - description: SRAM area > > > >> > > > >> but actually this is very similar what you wrote for reg-names - kind of > > > >> obvious, so easier way: > > > >> > > > >> reg: > > > >> maxItems: 2 > > > > > > > > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists > > > > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit > > > > ints. > > > > > > > > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long > > > > From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > > > > > > > Hence, I originally had maxItems: 2, and ended up deleting it because of > > > > the dt checker. > > > > > > > > With the two descriptions, it's the same failure. > > > > > > Yeah, they should create same result. > > > > > > > > > > > I think the problem is that the checker has no knowledge in the example > > > > of how big each address and size element of the reg property is. So, > > > > it's interpreting it as four entries of 32-bit address,size pairs > > > > instead of two entries of 64-bit address,size pairs. Yep, that's it, > > > > if I increase the number of "- description" entries to four then it's > > > > happy. > > > > > > > > So, what's the solution? > > > > > > > > > > If you open generated DTS examples (in your > > > kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which > > > address/size cells are expected. By default it is I think address/size > > > cells=1, so you need a bus node setting it to 2. > > > > Thanks, that works. The patch with all those points addressed now looks > > like: > > > > 8<=== > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > > Controller > > > > Add a DT binding for the Apple Mac System Management Controller. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > .../devicetree/bindings/mfd/apple,smc.yaml | 61 +++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml > > > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > new file mode 100644 > > index 000000000000..168f237c2962 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > @@ -0,0 +1,61 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Apple Mac System Management Controller > > + > > +maintainers: > > + - Hector Martin <marcan@marcan.st> > > + > > +description: > > + Apple Mac System Management Controller implements various functions > > + such as GPIO, RTC, power, reboot. > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - apple,t6000-smc > > + - apple,t8103-smc > > + - apple,t8112-smc > > + - const: apple,smc > > + > > + reg: > > + items: > > + - description: SMC area > > + - description: SRAM area > > Based on the disjoint addresses, is this really one device? Perhaps > the SRAM area should use mmio-sram binding? That already supports > sub-dividing the sram for different uses. I'll comment more on the > full example. To me it does look as if the SRAM is part of the SMC coprocessor block. It is probably part of a larger SRAM on the side of the SMC coprocessor. There is a gap, but the addresses are close. The only thing in between is the SMC mailbox, which is represented by a separate node. The address of the SRAM can be discovered by sending SMC commands. I believe Hector added it in order to verify the address that the SMC firmware provides. My OpenBSD driver doesn't use it, so in that sense changing the binding to use a separate node with a "mmio-sram" compatible (and presumably an "apple,sram" property to reference that node using a phandle) would work fine. The extra level of indirection obviously would mean more code in the Linux SMC driver though.
> From: Rob Herring <robh+dt@kernel.org> > Date: Thu, 1 Sep 2022 17:33:31 -0500 > > On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote: > > > On 01/09/2022 18:56, Russell King (Oracle) wrote: > > > > > > > > 8<=== > > > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > > > > Controller > > > > > > > > Add a DT binding for the Apple Mac System Management Controller. > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > > Yes, looks good. > > > > > > I won't add Reviewed-by tag, because I think it would confuse Patchwork, > > > so please send a v2 at some point. > > > > Thanks. Do you have any suggestions for patch 2? Should I merge the > > description in patch 2 into this file? > > > > The full dts for this series looks like this: > > > > smc: smc@23e400000 { > > compatible = "apple,t8103-smc", "apple,smc"; > > reg = <0x2 0x3e400000 0x0 0x4000>, > > <0x2 0x3fe00000 0x0 0x100000>; > > reg-names = "smc", "sram"; > > mboxes = <&smc_mbox>; > > > > smc_gpio: gpio { > > gpio-controller; > > #gpio-cells = <2>; > > }; > > }; > > > > but the fuller version in the asahi linux tree looks like: > > > > smc: smc@23e400000 { > > compatible = "apple,t8103-smc", "apple,smc"; > > reg = <0x2 0x3e400000 0x0 0x4000>, > > <0x2 0x3fe00000 0x0 0x100000>; > > reg-names = "smc", "sram"; > > mboxes = <&smc_mbox>; > > > > smc_gpio: gpio { > > gpio-controller; > > #gpio-cells = <2>; > > Only 2 properties doesn't really need its own schema doc. However, I > would just move these to the parent node. When we designed the bindings, it was our understanding that having separate nodes better matches Linux's MFD driver model. Please be aware that OpenBSD is already using these bindings. If there are good reasons for moving things, we can probably deal with that. But this sounds a bit like a toss up. > > > }; > > > > smc_rtc: rtc { > > nvmem-cells = <&rtc_offset>; > > nvmem-cell-names = "rtc_offset"; > > }; > > > > smc_reboot: reboot { > > nvmem-cells = <&shutdown_flag>, <&boot_stage>, > > <&boot_error_count>, <&panic_count>, <&pm_setting>; > > nvmem-cell-names = "shutdown_flag", "boot_stage", > > "boot_error_count", "panic_count", "pm_setting"; > > Not really much reason to split these up either because you can just > fetch the entry you want by name. Again the separate nodes are there because the RTC and the reboot functionality are logically separate and handled by different MFD sub-drivers in Linux. > How confident are the asahi folks that this is a complete binding? There is a lot of functionality in the SMC that is still largely unexplored. The idea of the design of the binding is that additional functionality may be added by adding more subnodes to the smc node. But we expect that the main SMC node itself should be complete with the "smc" and "sram" regions and the reference to the mailbox. Cheers, Mark
On Fri, Sep 02, 2022 at 04:49:37PM +0200, Mark Kettenis wrote: > > From: Rob Herring <robh+dt@kernel.org> > > Date: Thu, 1 Sep 2022 17:26:18 -0500 > > > > On Thu, Sep 1, 2022 at 10:56 AM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > > > > On Thu, Sep 01, 2022 at 06:45:52PM +0300, Krzysztof Kozlowski wrote: > > > > On 01/09/2022 18:24, Russell King (Oracle) wrote: > > > > > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote: > > > > >> On 01/09/2022 18:12, Russell King (Oracle) wrote: > > > > >>>>> + compatible: > > > > >>>>> + items: > > > > >>>>> + - enum: > > > > >>>>> + - apple,t8103-smc > > > > >>>> > > > > >>>> You miss two spaces of indentation on this level. > > > > >>> > > > > >>> Should that be picked up by the dt checker? > > > > I have a problem that Krzysztof is quicker. ;) Maybe I should stop > > screening the emails (for the times I break things mostly). > > > > > > >> > > > > >> I think yamllint complains about it. It is not a hard-dependency, so > > > > >> maybe you don't have it installed. > > > > >> > > > > >>> > > > > >>>>> + - apple,t8112-smc > > > > >>>>> + - apple,t6000-smc > > > > >>>> > > > > >>>> Bring some order here - either alphabetical or by date of release (as in > > > > >>>> other Apple schemas). I think t6000 was before t8112, so it's none of > > > > >>>> that orders. > > > > >>> > > > > >>> Ok. > > > > >>> > > > > >>>>> + - const: apple,smc > > > > >>>>> + > > > > >>>>> + reg: > > > > >>>>> + description: Two regions, one for the SMC area and one for the SRAM area. > > > > >>>> > > > > >>>> You need constraints for size/order, so in this context list with > > > > >>>> described items. > > > > >>> > > > > >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker > > > > >>> objected to it. > > > > >> > > > > >> One way: > > > > >> reg: > > > > >> items: > > > > >> - description: SMC area > > > > >> - description: SRAM area > > > > >> > > > > >> but actually this is very similar what you wrote for reg-names - kind of > > > > >> obvious, so easier way: > > > > >> > > > > >> reg: > > > > >> maxItems: 2 > > > > > > > > > > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists > > > > > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit > > > > > ints. > > > > > > > > > > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long > > > > > From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > > > > > > > > > Hence, I originally had maxItems: 2, and ended up deleting it because of > > > > > the dt checker. > > > > > > > > > > With the two descriptions, it's the same failure. > > > > > > > > Yeah, they should create same result. > > > > > > > > > > > > > > I think the problem is that the checker has no knowledge in the example > > > > > of how big each address and size element of the reg property is. So, > > > > > it's interpreting it as four entries of 32-bit address,size pairs > > > > > instead of two entries of 64-bit address,size pairs. Yep, that's it, > > > > > if I increase the number of "- description" entries to four then it's > > > > > happy. > > > > > > > > > > So, what's the solution? > > > > > > > > > > > > > If you open generated DTS examples (in your > > > > kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which > > > > address/size cells are expected. By default it is I think address/size > > > > cells=1, so you need a bus node setting it to 2. > > > > > > Thanks, that works. The patch with all those points addressed now looks > > > like: > > > > > > 8<=== > > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > > > Controller > > > > > > Add a DT binding for the Apple Mac System Management Controller. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > --- > > > .../devicetree/bindings/mfd/apple,smc.yaml | 61 +++++++++++++++++++ > > > 1 file changed, 61 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > > new file mode 100644 > > > index 000000000000..168f237c2962 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml > > > @@ -0,0 +1,61 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Apple Mac System Management Controller > > > + > > > +maintainers: > > > + - Hector Martin <marcan@marcan.st> > > > + > > > +description: > > > + Apple Mac System Management Controller implements various functions > > > + such as GPIO, RTC, power, reboot. > > > + > > > +properties: > > > + compatible: > > > + items: > > > + - enum: > > > + - apple,t6000-smc > > > + - apple,t8103-smc > > > + - apple,t8112-smc > > > + - const: apple,smc > > > + > > > + reg: > > > + items: > > > + - description: SMC area > > > + - description: SRAM area > > > > Based on the disjoint addresses, is this really one device? Perhaps > > the SRAM area should use mmio-sram binding? That already supports > > sub-dividing the sram for different uses. I'll comment more on the > > full example. > > To me it does look as if the SRAM is part of the SMC coprocessor > block. It is probably part of a larger SRAM on the side of the SMC > coprocessor. There is a gap, but the addresses are close. The only > thing in between is the SMC mailbox, which is represented by a > separate node. Okay, fair enough. Let's keep them together. Rob
On Fri, Sep 02, 2022 at 05:06:43PM +0200, Mark Kettenis wrote: > > From: Rob Herring <robh+dt@kernel.org> > > Date: Thu, 1 Sep 2022 17:33:31 -0500 > > > > On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > > > > On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote: > > > > On 01/09/2022 18:56, Russell King (Oracle) wrote: > > > > > > > > > > 8<=== > > > > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > > > > > Controller > > > > > > > > > > Add a DT binding for the Apple Mac System Management Controller. > > > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > > > > Yes, looks good. > > > > > > > > I won't add Reviewed-by tag, because I think it would confuse Patchwork, > > > > so please send a v2 at some point. > > > > > > Thanks. Do you have any suggestions for patch 2? Should I merge the > > > description in patch 2 into this file? > > > > > > The full dts for this series looks like this: > > > > > > smc: smc@23e400000 { > > > compatible = "apple,t8103-smc", "apple,smc"; > > > reg = <0x2 0x3e400000 0x0 0x4000>, > > > <0x2 0x3fe00000 0x0 0x100000>; > > > reg-names = "smc", "sram"; > > > mboxes = <&smc_mbox>; > > > > > > smc_gpio: gpio { > > > gpio-controller; > > > #gpio-cells = <2>; > > > }; > > > }; > > > > > > but the fuller version in the asahi linux tree looks like: > > > > > > smc: smc@23e400000 { > > > compatible = "apple,t8103-smc", "apple,smc"; > > > reg = <0x2 0x3e400000 0x0 0x4000>, > > > <0x2 0x3fe00000 0x0 0x100000>; > > > reg-names = "smc", "sram"; > > > mboxes = <&smc_mbox>; > > > > > > smc_gpio: gpio { > > > gpio-controller; > > > #gpio-cells = <2>; > > > > Only 2 properties doesn't really need its own schema doc. However, I > > would just move these to the parent node. > > When we designed the bindings, it was our understanding that having > separate nodes better matches Linux's MFD driver model. Well, it is convenient to have subnodes with compatibles so that your drivers automagically probe. So yes, a 1:1 relationship of nodes to drivers is nice and tidy. But h/w is not always packaged up neatly and it's not DT's job to try to abstract it such that it is. Also, we shouldn't design bindings around the *current* driver partitioning of some OS. This one is actually pretty odd in that the child nodes don't have a compatible string which breaks the automagical probing. > Please be aware that OpenBSD is already using these bindings. If > there are good reasons for moving things, we can probably deal with > that. But this sounds a bit like a toss up. Sigh. If there are other bindings in use, please submit them even if the Linux driver isn't ready. If a Linux subsystem maintainer doesn't want to take it, then I will. It is a toss up though... > > > }; > > > > > > smc_rtc: rtc { > > > nvmem-cells = <&rtc_offset>; > > > nvmem-cell-names = "rtc_offset"; > > > }; > > > > > > smc_reboot: reboot { > > > nvmem-cells = <&shutdown_flag>, <&boot_stage>, > > > <&boot_error_count>, <&panic_count>, <&pm_setting>; > > > nvmem-cell-names = "shutdown_flag", "boot_stage", > > > "boot_error_count", "panic_count", "pm_setting"; > > > > Not really much reason to split these up either because you can just > > fetch the entry you want by name. > > Again the separate nodes are there because the RTC and the reboot > functionality are logically separate and handled by different MFD > sub-drivers in Linux. It's really a question of whether the subset of functionality is going to get reused on its own or has its own resources in DT. MFD bindings are done both ways. Rob
On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote: > On Fri, Sep 02, 2022 at 05:06:43PM +0200, Mark Kettenis wrote: > > > From: Rob Herring <robh+dt@kernel.org> > > > Date: Thu, 1 Sep 2022 17:33:31 -0500 > > > > > > On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle) > > > <linux@armlinux.org.uk> wrote: > > > > > > > > On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote: > > > > > On 01/09/2022 18:56, Russell King (Oracle) wrote: > > > > > > > > > > > > 8<=== > > > > > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > > > > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management > > > > > > Controller > > > > > > > > > > > > Add a DT binding for the Apple Mac System Management Controller. > > > > > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > > > > > > Yes, looks good. > > > > > > > > > > I won't add Reviewed-by tag, because I think it would confuse Patchwork, > > > > > so please send a v2 at some point. > > > > > > > > Thanks. Do you have any suggestions for patch 2? Should I merge the > > > > description in patch 2 into this file? > > > > > > > > The full dts for this series looks like this: > > > > > > > > smc: smc@23e400000 { > > > > compatible = "apple,t8103-smc", "apple,smc"; > > > > reg = <0x2 0x3e400000 0x0 0x4000>, > > > > <0x2 0x3fe00000 0x0 0x100000>; > > > > reg-names = "smc", "sram"; > > > > mboxes = <&smc_mbox>; > > > > > > > > smc_gpio: gpio { > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > }; > > > > }; > > > > > > > > but the fuller version in the asahi linux tree looks like: > > > > > > > > smc: smc@23e400000 { > > > > compatible = "apple,t8103-smc", "apple,smc"; > > > > reg = <0x2 0x3e400000 0x0 0x4000>, > > > > <0x2 0x3fe00000 0x0 0x100000>; > > > > reg-names = "smc", "sram"; > > > > mboxes = <&smc_mbox>; > > > > > > > > smc_gpio: gpio { > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > > > Only 2 properties doesn't really need its own schema doc. However, I > > > would just move these to the parent node. > > > > When we designed the bindings, it was our understanding that having > > separate nodes better matches Linux's MFD driver model. > > Well, it is convenient to have subnodes with compatibles so that your > drivers automagically probe. So yes, a 1:1 relationship of nodes to > drivers is nice and tidy. But h/w is not always packaged up neatly and > it's not DT's job to try to abstract it such that it is. Also, we > shouldn't design bindings around the *current* driver partitioning of > some OS. > > This one is actually pretty odd in that the child nodes don't have a > compatible string which breaks the automagical probing. > > > Please be aware that OpenBSD is already using these bindings. If > > there are good reasons for moving things, we can probably deal with > > that. But this sounds a bit like a toss up. > > Sigh. If there are other bindings in use, please submit them even if the > Linux driver isn't ready. If a Linux subsystem maintainer doesn't want > to take it, then I will. > > It is a toss up though... > > > > > }; > > > > > > > > smc_rtc: rtc { > > > > nvmem-cells = <&rtc_offset>; > > > > nvmem-cell-names = "rtc_offset"; > > > > }; > > > > > > > > smc_reboot: reboot { > > > > nvmem-cells = <&shutdown_flag>, <&boot_stage>, > > > > <&boot_error_count>, <&panic_count>, <&pm_setting>; > > > > nvmem-cell-names = "shutdown_flag", "boot_stage", > > > > "boot_error_count", "panic_count", "pm_setting"; > > > > > > Not really much reason to split these up either because you can just > > > fetch the entry you want by name. > > > > Again the separate nodes are there because the RTC and the reboot > > functionality are logically separate and handled by different MFD > > sub-drivers in Linux. > > It's really a question of whether the subset of functionality is going > to get reused on its own or has its own resources in DT. MFD bindings > are done both ways. I'm guessing this series is blocked until a resolution is found for the DT binding description. Please can the Asahi folk and the DT maintainers sort this out and let me know when I can proceed with this patch set. I'm just the man in the middle here. Thanks.
On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote: > This one is actually pretty odd in that the child nodes don't have a > compatible string which breaks the automagical probing. I don't think that is necessarily true, and I don't think it's true in this case. The SMC core driver instructs the MFD core to create devices for the individual functional items: static const struct mfd_cell apple_smc_devs[] = { { .name = "macsmc-gpio", }, { .name = "macsmc-hid", }, { .name = "macsmc-power", }, { .name = "macsmc-reboot", }, { .name = "macsmc-rtc", }, }; Since MFD uses platform devices for these, they get all the normal functionality that these devices have, which include matching by device name ot the driver name, and udev events being appropriately triggered. As long as the platform drivers for these devices have the correct modalias lines, autoloading of the modules will work and the drivers will be correctly matched and probed. The Asahi kernel builds most of the platform support as modules, including these, so we know it works (if it didn't, then lots of module autoloading would be broken on non-DT platforms.) > > Again the separate nodes are there because the RTC and the reboot > > functionality are logically separate and handled by different MFD > > sub-drivers in Linux. > > It's really a question of whether the subset of functionality is going > to get reused on its own or has its own resources in DT. MFD bindings > are done both ways. I think the current position on what to do about these is that everyone is looking for someone else to make a decision, and no one wants to! Firstly, I don't think that the number of properties in a node should have a bearing on the design of the DT binding - what should have a bearing is the logical partitioning of functionality. Mark suggests that it would take six months for OpenBSD to transition to some other description - for example, if we merged the nodes. Hector says that MacOS's firmware description has the nodes merged, but their description is a mess. The overall preference seems to be to keep the sub-nodes unless there is a strong technical reason not to. The feeling I am getting from the review is that there doesn't seem to be a strong technical reason to merge the nodes - there are desires and preferences, but nothing concrete. So at this point, I think it would make sense if I post a v2 with all the updates so far (sorry, given the long drawn out discussions on this, I've lost track of what changes have been made to the code, so I won't include a detailed change log.)
> Date: Tue, 6 Sep 2022 10:04:45 +0100 > From: "Russell King (Oracle)" <linux@armlinux.org.uk> > > On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote: > > This one is actually pretty odd in that the child nodes don't have a > > compatible string which breaks the automagical probing. > > I don't think that is necessarily true, and I don't think it's true in > this case. > > The SMC core driver instructs the MFD core to create devices for the > individual functional items: > > static const struct mfd_cell apple_smc_devs[] = { > { > .name = "macsmc-gpio", > }, > { > .name = "macsmc-hid", > }, > { > .name = "macsmc-power", > }, > { > .name = "macsmc-reboot", > }, > { > .name = "macsmc-rtc", > }, > }; > > Since MFD uses platform devices for these, they get all the normal > functionality that these devices have, which include matching by > device name ot the driver name, and udev events being appropriately > triggered. As long as the platform drivers for these devices have the > correct modalias lines, autoloading of the modules will work and the > drivers will be correctly matched and probed. > > The Asahi kernel builds most of the platform support as modules, > including these, so we know it works (if it didn't, then lots of > module autoloading would be broken on non-DT platforms.) > > > > Again the separate nodes are there because the RTC and the reboot > > > functionality are logically separate and handled by different MFD > > > sub-drivers in Linux. > > > > It's really a question of whether the subset of functionality is going > > to get reused on its own or has its own resources in DT. MFD bindings > > are done both ways. > > I think the current position on what to do about these is that everyone > is looking for someone else to make a decision, and no one wants to! > > Firstly, I don't think that the number of properties in a node should > have a bearing on the design of the DT binding - what should have a > bearing is the logical partitioning of functionality. > > Mark suggests that it would take six months for OpenBSD to transition to > some other description - for example, if we merged the nodes. Note that we're totally willing to do that if there is a good technical reason for changes to the binding. We just have to find a way to upgrade existing installations of OpenBSD 7.1 which is a bit of a challenge as the SMC GPIOs are essential for wifi which on the laptops is the only network connection available. > Hector says that MacOS's firmware description has the nodes merged, but > their description is a mess. To elaborate on that a bit more, the use of sub-nodes provides a nice separation between the SMC hardware interface (the main node) and the functionality offered by the firmware running on the SMC (the sub-nodes). Another argument for having sub-nodes is that the firmware actually exposes *two* GPIO controllers. For now we only support the "master" PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a separate set of SMC "keys". We currently don't need any of the pins on the "slave", so we don't expose it in the DT yet. > The overall preference seems to be to keep the sub-nodes unless there > is a strong technical reason not to. > > The feeling I am getting from the review is that there doesn't seem to > be a strong technical reason to merge the nodes - there are desires and > preferences, but nothing concrete. > > So at this point, I think it would make sense if I post a v2 with all > the updates so far (sorry, given the long drawn out discussions on > this, I've lost track of what changes have been made to the code, so > I won't include a detailed change log.) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! > >
On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > Another argument for having sub-nodes is that the firmware actually > exposes *two* GPIO controllers. For now we only support the "master" > PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a > separate set of SMC "keys". We currently don't need any of the pins > on the "slave", so we don't expose it in the DT yet. That sounds backward, like we don't expose device X as DT node because $OS doesn't use it yet. DT should just expose (by nodes or other ways) all hardware that exist or at least all hardware we know about no matter what $OS is using. FWIW I think nodes makes most sense because no doubt for example the RTC is a separate hardware unit somewhere, and so is the GPIO. The fact that it is hidden behind a software abstraction doesn't change the fact that the HW definitely has these discrete units. Yours, Linus Walleij
On 06/09/2022 20.22, Linus Walleij wrote: > On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > >> Another argument for having sub-nodes is that the firmware actually >> exposes *two* GPIO controllers. For now we only support the "master" >> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a >> separate set of SMC "keys". We currently don't need any of the pins >> on the "slave", so we don't expose it in the DT yet. > > That sounds backward, like we don't expose device X as DT node > because $OS doesn't use it yet. DT should just expose (by nodes or > other ways) all hardware that exist or at least all hardware we know > about no matter what $OS is using. How so? The are piles and piles of unused hardware not exposed in the DT, and piles and piles of hardware that will be used but we haven't figured out how to do it yet, so it's not exposed. For example, we know there are like 8 or so UARTs, but we don't define them in the DT because they are not connected to anything on any existing device and we don't need them. Apple does the same thing in their DTs (only used hardware is defined). I don't really see the point of exposing a GPIO controller when we don't actually do anything with the pins yet, and might never do so. Having drivers bind and stay unused just increases the amount of code running without any ultimate purpose, so why do it? It's not like any other OS would use the hardware either - GPIOs are only useful if they are referenced in the DT for something, and we don't have anything that would reference these. For SMC in particular, there's a huge amount of functionality we don't have drivers for yet, and I don't see the point of trying to conjure up DT bindings for it until someone writes a driver (and has a reason to do so) :) > FWIW I think nodes makes most sense because no doubt for example > the RTC is a separate hardware unit somewhere, and so is the > GPIO. The fact that it is hidden behind a software abstraction doesn't > change the fact that the HW definitely has these discrete units. The RTC and the GPIO happen to be part of the same physical IC (PMU), but yes, I agree. - Hector
On Tue, Sep 6, 2022 at 1:36 PM Hector Martin <marcan@marcan.st> wrote: > On 06/09/2022 20.22, Linus Walleij wrote: > > On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > >> Another argument for having sub-nodes is that the firmware actually > >> exposes *two* GPIO controllers. For now we only support the "master" > >> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a > >> separate set of SMC "keys". We currently don't need any of the pins > >> on the "slave", so we don't expose it in the DT yet. > > > > That sounds backward, like we don't expose device X as DT node > > because $OS doesn't use it yet. DT should just expose (by nodes or > > other ways) all hardware that exist or at least all hardware we know > > about no matter what $OS is using. > > How so? The are piles and piles of unused hardware not exposed in the > DT, and piles and piles of hardware that will be used but we haven't > figured out how to do it yet, so it's not exposed. For example, we know > there are like 8 or so UARTs, but we don't define them in the DT because > they are not connected to anything on any existing device and we don't > need them. Apple does the same thing in their DTs (only used hardware is > defined). > > I don't really see the point of exposing a GPIO controller when we don't > actually do anything with the pins yet, and might never do so. Having > drivers bind and stay unused just increases the amount of code running > without any ultimate purpose, so why do it? It's not like any other OS > would use the hardware either - GPIOs are only useful if they are > referenced in the DT for something, and we don't have anything that > would reference these. This comes from the FDT background in OpenFirmware, and there is definitely a timeline perspective (also called "waterfall model") in that line of thinking: a DT is written that describes the hardware and flashed into the BIOS and never changed, then the operating system is implemented at a later point. This is how e.g. the PC ACPI BIOS tables are also thinking about themselves. Of course the world does not work like that, but as a standard process DT and ACPI works with the same ambition as any such process: slowly and impersonal, like the planets, or the plants. The ambition that a DT should always remain backward compatible comes from the same historical root and reasoning. Any agile development or (heh) hardware discovers through reverse engineering will of course drive a truck through that line of thinking. Realistically, use the same approach as with everything else, I like the IETF motto "rough consensus and running code", any ambition taken too far will just stifle development. So I'd say make an honest effort to resonably describe the nodes we see will be useful in the foreseeable future. Yours, Linus Walleij
On 06/09/2022 20.57, Linus Walleij wrote: > On Tue, Sep 6, 2022 at 1:36 PM Hector Martin <marcan@marcan.st> wrote: >> On 06/09/2022 20.22, Linus Walleij wrote: >>> On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: >>> >>>> Another argument for having sub-nodes is that the firmware actually >>>> exposes *two* GPIO controllers. For now we only support the "master" >>>> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a >>>> separate set of SMC "keys". We currently don't need any of the pins >>>> on the "slave", so we don't expose it in the DT yet. >>> >>> That sounds backward, like we don't expose device X as DT node >>> because $OS doesn't use it yet. DT should just expose (by nodes or >>> other ways) all hardware that exist or at least all hardware we know >>> about no matter what $OS is using. >> >> How so? The are piles and piles of unused hardware not exposed in the >> DT, and piles and piles of hardware that will be used but we haven't >> figured out how to do it yet, so it's not exposed. For example, we know >> there are like 8 or so UARTs, but we don't define them in the DT because >> they are not connected to anything on any existing device and we don't >> need them. Apple does the same thing in their DTs (only used hardware is >> defined). >> >> I don't really see the point of exposing a GPIO controller when we don't >> actually do anything with the pins yet, and might never do so. Having >> drivers bind and stay unused just increases the amount of code running >> without any ultimate purpose, so why do it? It's not like any other OS >> would use the hardware either - GPIOs are only useful if they are >> referenced in the DT for something, and we don't have anything that >> would reference these. > > This comes from the FDT background in OpenFirmware, and there is > definitely a timeline perspective (also called "waterfall model") in that > line of thinking: a DT is written that describes the hardware and flashed > into the BIOS and never changed, then the operating system is > implemented at a later point. This is how e.g. the PC ACPI BIOS tables > are also thinking about themselves. Yes, but again, that only makes sense from the point of view of describing hardware that exists, is useful, and could be used by the OS. For any given platform X, if platform X does not use the secondary GPIO controller for any service describable in the DT, then there is no point in describing that GPIO controller. Same way ACPI tables do not describe every single physical GPIO available on a platform, just whatever is used by the AML. This is the case for every existing Apple device to my knowledge. Apple's device trees have zero references to the secondary controller's GPIOs. It's possible they are only used internally by SMC, or they were added to support future platforms that don't exist yet, or for debugging purposes, or who knows. If some day we find a use for those GPIOs, that would require a DT change *anyway*, to describe that usage, and the controller could be described then (we did something like that, using a GPIO that Apple doesn't, for the interim display-backlight power control support, though that is a temporary hack that will go away). Heck, we don't even know what these GPIOs are connected to right now! Ultimately, we're working with a reverse engineered platform here, and DTs will inevitaby be incremental. But in this particular case, of hardware that has no known useful purpose to an OS, I don't see the point in gratuitously describing it. And besides, the way we set things up, forward-compatible DT upgrades are trivial, and no actual user on this platform is going to be stuck with an old DT and newer software (if their software supports the platform properly, and that takes more than the relatively trivial DT upgrade stuff anyway). I'm a lot more interested in getting bindings upstreamed ASAP (so we can start guaranteeing no backwards-compat breaks, which is important to avoid outright breakage) than I am in trying to be exhaustive up front with device instances. It's perfectly fine to say that users have to upgrade both their DTs and kernels to get newer hardware support, on these platforms. - Hector
On Tue, Sep 06, 2022 at 10:28:25PM +0900, Hector Martin wrote: > Ultimately, we're working with a reverse engineered platform here, and > DTs will inevitaby be incremental. But in this particular case, of > hardware that has no known useful purpose to an OS, I don't see the > point in gratuitously describing it. And besides, the way we set things > up, forward-compatible DT upgrades are trivial, and no actual user on > this platform is going to be stuck with an old DT and newer software (if > their software supports the platform properly, and that takes more than > the relatively trivial DT upgrade stuff anyway). I'm a lot more > interested in getting bindings upstreamed ASAP (so we can start > guaranteeing no backwards-compat breaks, which is important to avoid > outright breakage) than I am in trying to be exhaustive up front with > device instances. It's perfectly fine to say that users have to upgrade > both their DTs and kernels to get newer hardware support, on these > platforms. It's also a very common process for SoCs - almost no one writes the DT first and then writes the drivers. You always see on the mailing list series of patches that add a driver for some bit of hardware, along with patches adding the DT binding and the DT description. I don't think you're doing anything different here to what is common practice within the mainline kernel community with this approach. The exception to that is when adding a driver for feature X in a SoC, it's common to add all instances of X to the dtsi with ``status = "disabled"'' and only enable the appropriate blocks on platforms that need it. So, for example, if a SoC has three network interfaces, all of them identical, when adding a network driver and the bindings for the network hardware, one would add all three to the SoC description whether or not the platform one was working with makes use of them. It means that one has to think about how to support all instances of the hardware on the platform and design the binding to allow that flexibility, rather than having to augment the binding later. In the case of gpio-macsmc, how would we later add support for the slave PMU GPIOs, given that these use keys "gpXX" rather than "gPxx"? How do we tell the gpio-macsmc code to use a different set of keys? Should DT describe the key "prefix" (in other words "gp" vs "gP"), or should it describe it some other way. What if Apple decides to instantiate another GPIO controller in a later platform with a different prefix, could that be accomodated without breaking any solution we come up today? Maybe the solution to this would be to describe the key prefix in DT as that's effectively its "reg". Or maybe we use "reg" to describe it somehow (which is value of the key, which seems to have an "address" like quality to it?) We don't have to implement code for this now, we just need to get a reasonably correct DT binding for the gpio controller. Thanks.
On Tue, Sep 6, 2022 at 3:28 PM Hector Martin <marcan@marcan.st> wrote: > On 06/09/2022 20.57, Linus Walleij wrote: > > On Tue, Sep 6, 2022 at 1:36 PM Hector Martin <marcan@marcan.st> wrote: > > This comes from the FDT background in OpenFirmware, and there is > > definitely a timeline perspective (also called "waterfall model") in that > > line of thinking: a DT is written that describes the hardware and flashed > > into the BIOS and never changed, then the operating system is > > implemented at a later point. This is how e.g. the PC ACPI BIOS tables > > are also thinking about themselves. > > Yes, but again, that only makes sense from the point of view of > describing hardware that exists, is useful, and could be used by the OS. > > For any given platform X, if platform X does not use the secondary GPIO > controller for any service describable in the DT, then there is no point > in describing that GPIO controller. Same way ACPI tables do not describe > every single physical GPIO available on a platform, just whatever is > used by the AML. Good point. I don't know what ambition DT should have here. If there is a discrete component on I2C for example, we tend to describe it fully, this kind of stuff with misc "dark silicon" didn't exist when DT was first conceived. > If some day we find a use for those GPIOs, that would require a DT > change *anyway*, to describe that usage, and the controller could be > described then (we did something like that, using a GPIO that Apple > doesn't, for the interim display-backlight power control support, though > that is a temporary hack that will go away). Heck, we don't even know > what these GPIOs are connected to right now! > > Ultimately, we're working with a reverse engineered platform here, and > DTs will inevitaby be incremental. (...) That's OK, I think the patch series is good enough as it is and should be merged, so I have added my Reviewed-by. I think the world is a better place with support for Apple silicon being developed in-tree. > I'm a lot more > interested in getting bindings upstreamed ASAP (so we can start > guaranteeing no backwards-compat breaks, which is important to avoid > outright breakage) than I am in trying to be exhaustive up front with > device instances. It's perfectly fine to say that users have to upgrade > both their DTs and kernels to get newer hardware support, on these > platforms. I agree. Yours, Linus Walleij
On 06/09/2022 22.43, Russell King (Oracle) wrote: > On Tue, Sep 06, 2022 at 10:28:25PM +0900, Hector Martin wrote: >> Ultimately, we're working with a reverse engineered platform here, and >> DTs will inevitaby be incremental. But in this particular case, of >> hardware that has no known useful purpose to an OS, I don't see the >> point in gratuitously describing it. And besides, the way we set things >> up, forward-compatible DT upgrades are trivial, and no actual user on >> this platform is going to be stuck with an old DT and newer software (if >> their software supports the platform properly, and that takes more than >> the relatively trivial DT upgrade stuff anyway). I'm a lot more >> interested in getting bindings upstreamed ASAP (so we can start >> guaranteeing no backwards-compat breaks, which is important to avoid >> outright breakage) than I am in trying to be exhaustive up front with >> device instances. It's perfectly fine to say that users have to upgrade >> both their DTs and kernels to get newer hardware support, on these >> platforms. > > It's also a very common process for SoCs - almost no one writes the > DT first and then writes the drivers. You always see on the mailing > list series of patches that add a driver for some bit of hardware, > along with patches adding the DT binding and the DT description. > > I don't think you're doing anything different here to what is common > practice within the mainline kernel community with this approach. > > The exception to that is when adding a driver for feature X in a SoC, > it's common to add all instances of X to the dtsi with ``status = > "disabled"'' and only enable the appropriate blocks on platforms that > need it. > > So, for example, if a SoC has three network interfaces, all of them > identical, when adding a network driver and the bindings for the > network hardware, one would add all three to the SoC description > whether or not the platform one was working with makes use of them. Right, and that makes sense for SoCs that third parties can incorporate into any arbitrary platform, and which have documentation listing those instances, because then board-level DTs can just enable the ones they need. But here we're working with SoCs where the only reference for what hardware exists *is* Apple's DT, and it only lists hardware that is in use on existing platforms. So there is no sane way for us to know what hardware exists, except what we can infer from reading the tea leaves (e.g. you can tell how many UARTs there are from the power domains and leaked schematics, but this doesn't extend to everything else). For example, we also know that these SoCs have a second Neural Engine that Apple have disabled on all shipping platforms, that the t600x series has an unused USB3 controller, etc... but there's no way we can safely describe these things in the DT if we can't test that they work and we don't have documentation even telling us how they're supposed to work :) > In the case of gpio-macsmc, how would we later add support for the > slave PMU GPIOs, given that these use keys "gpXX" rather than "gPxx"? > How do we tell the gpio-macsmc code to use a different set of keys? > Should DT describe the key "prefix" (in other words "gp" vs "gP"), > or should it describe it some other way. What if Apple decides to > instantiate another GPIO controller in a later platform with a > different prefix, could that be accomodated without breaking any > solution we come up today? > > Maybe the solution to this would be to describe the key prefix in DT > as that's effectively its "reg". Or maybe we use "reg" to describe > it somehow (which is value of the key, which seems to have an > "address" like quality to it?) > > We don't have to implement code for this now, we just need to get a > reasonably correct DT binding for the gpio controller. I agree that this is something to think about (I was about to reply on the subject). I can think of two ways: using `reg` for the key name, but that feels icky since it's ASCII and not *really* a register number/address, or something like this: gpio@0 { apple,smc-key-base = "gP00"; ... } gpio@1 { apple,smc-key-base = "gp00"; ... } But this ties back to the device enumeration too, since right now the DT does not drive that (we'd have to add the subdevice to the mfd subdevice list somehow anyway, if we don't switch to compatibles). I'd love to hear Rob's opinion on this one, and also whether the existing Linux and OpenBSD code would currently find gpio@0 {} instead of gpio {} for backwards compat. - Hector
> Date: Tue, 6 Sep 2022 22:53:47 +0900 > From: Hector Martin <marcan@marcan.st> > > On 06/09/2022 22.43, Russell King (Oracle) wrote: > > In the case of gpio-macsmc, how would we later add support for the > > slave PMU GPIOs, given that these use keys "gpXX" rather than "gPxx"? > > How do we tell the gpio-macsmc code to use a different set of keys? > > Should DT describe the key "prefix" (in other words "gp" vs "gP"), > > or should it describe it some other way. What if Apple decides to > > instantiate another GPIO controller in a later platform with a > > different prefix, could that be accomodated without breaking any > > solution we come up today? > > > > Maybe the solution to this would be to describe the key prefix in DT > > as that's effectively its "reg". Or maybe we use "reg" to describe > > it somehow (which is value of the key, which seems to have an > > "address" like quality to it?) > > > > We don't have to implement code for this now, we just need to get a > > reasonably correct DT binding for the gpio controller. > > I agree that this is something to think about (I was about to reply on > the subject). > > I can think of two ways: using `reg` for the key name, but that feels > icky since it's ASCII and not *really* a register number/address, or > something like this: > > gpio@0 { > apple,smc-key-base = "gP00"; > ... > } > > gpio@1 { > apple,smc-key-base = "gp00"; > ... > } This would still require us to add a (one-cell) "reg" property and would require adding the appropriate "#address-cells" and "#size-cells" properties to the SMC node. > But this ties back to the device enumeration too, since right now the DT > does not drive that (we'd have to add the subdevice to the mfd subdevice > list somehow anyway, if we don't switch to compatibles). > > I'd love to hear Rob's opinion on this one, and also whether the > existing Linux and OpenBSD code would currently find gpio@0 {} instead > of gpio {} for backwards compat. The OpenBSD driver does a lookup by name and the "@0" is part of that name. So that would break backwards compat. Maybe just name the slave GPIO controller "gpio-slave"? If we add compatibles, the compatibles for the nodes should propbably be different such that we can switch to do a lookup by compatible?
On Tue, Sep 06, 2022 at 04:25:49PM +0200, Mark Kettenis wrote: > > Date: Tue, 6 Sep 2022 22:53:47 +0900 > > From: Hector Martin <marcan@marcan.st> > > > > I agree that this is something to think about (I was about to reply on > > the subject). > > > > I can think of two ways: using `reg` for the key name, but that feels > > icky since it's ASCII and not *really* a register number/address, or > > something like this: > > > > gpio@0 { > > apple,smc-key-base = "gP00"; > > ... > > } > > > > gpio@1 { > > apple,smc-key-base = "gp00"; > > ... > > } > > This would still require us to add a (one-cell) "reg" property and > would require adding the appropriate "#address-cells" and > "#size-cells" properties to the SMC node. Yes, and at that point, as I suggested, it probably would be better to use: #address-cells = <1>; #size-cells = <0>; gpio@67503030 { reg = <0x67503030>; }; gpio@67703030 { reg = <0x67703030>; }; Then the "reg" has a meaning that is directly related to the SMC. > > But this ties back to the device enumeration too, since right now the DT > > does not drive that (we'd have to add the subdevice to the mfd subdevice > > list somehow anyway, if we don't switch to compatibles). > > > > I'd love to hear Rob's opinion on this one, and also whether the > > existing Linux and OpenBSD code would currently find gpio@0 {} instead > > of gpio {} for backwards compat. > > The OpenBSD driver does a lookup by name and the "@0" is part of that > name. So that would break backwards compat. Oh, that's annoying - and is a different behaviour to Linux. On Linux, we only look at the node name up to the @ when matching (see of_node_name_eq() in drivers/of/base.c, so it doesn't matter to Linux what follows the @ when you try to look up a node named "gpio" - you'll find gpio@anythingyoulike. > Maybe just name the slave GPIO controller "gpio-slave"? If we add > compatibles, the compatibles for the nodes should propbably be > different such that we can switch to do a lookup by compatible? I don't think the DT folk would be happy with "gpio-slave" because node names are supposed to be generic. Also, "slave" probably isn't a good choice of name in this modern era given past history. Rather than the above, we could use "reg" to indicate which GPIO controller we're talking about, and lookup the reg value in a table to give the key. So gpio@0, reg=<0> => gP00, gpio@1, reg=<1> => gp00. gpio@2, reg=<2> => whatever next. That sounds like it won't break the existing OpenBSD.
On Tue, Sep 06, 2022 at 08:36:25PM +0900, Hector Martin wrote: > On 06/09/2022 20.22, Linus Walleij wrote: > > On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > >> Another argument for having sub-nodes is that the firmware actually > >> exposes *two* GPIO controllers. For now we only support the "master" > >> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a > >> separate set of SMC "keys". We currently don't need any of the pins > >> on the "slave", so we don't expose it in the DT yet. > > > > That sounds backward, like we don't expose device X as DT node > > because $OS doesn't use it yet. DT should just expose (by nodes or > > other ways) all hardware that exist or at least all hardware we know > > about no matter what $OS is using. > > How so? The are piles and piles of unused hardware not exposed in the > DT, and piles and piles of hardware that will be used but we haven't > figured out how to do it yet, so it's not exposed. For example, we know > there are like 8 or so UARTs, but we don't define them in the DT because > they are not connected to anything on any existing device and we don't > need them. Apple does the same thing in their DTs (only used hardware is > defined). > > I don't really see the point of exposing a GPIO controller when we don't > actually do anything with the pins yet, and might never do so. Having > drivers bind and stay unused just increases the amount of code running > without any ultimate purpose, so why do it? It's not like any other OS > would use the hardware either - GPIOs are only useful if they are > referenced in the DT for something, and we don't have anything that > would reference these. > > For SMC in particular, there's a huge amount of functionality we don't > have drivers for yet, and I don't see the point of trying to conjure up > DT bindings for it until someone writes a driver (and has a reason to do > so) :) Exposing in a DT is one thing. Defining in the binding is another. Even if it's not complete bindings, but just a fuller description of what functionality the MFD contains is. For example, just knowing there are 2 instances of GPIO, I'm much more inclined to agree GPIO should be a subnode. Rob
> Date: Tue, 6 Sep 2022 15:54:50 +0100 > From: "Russell King (Oracle)" <linux@armlinux.org.uk> > > On Tue, Sep 06, 2022 at 04:25:49PM +0200, Mark Kettenis wrote: > > > Date: Tue, 6 Sep 2022 22:53:47 +0900 > > > From: Hector Martin <marcan@marcan.st> > > > > > > I agree that this is something to think about (I was about to reply on > > > the subject). > > > > > > I can think of two ways: using `reg` for the key name, but that feels > > > icky since it's ASCII and not *really* a register number/address, or > > > something like this: > > > > > > gpio@0 { > > > apple,smc-key-base = "gP00"; > > > ... > > > } > > > > > > gpio@1 { > > > apple,smc-key-base = "gp00"; > > > ... > > > } > > > > This would still require us to add a (one-cell) "reg" property and > > would require adding the appropriate "#address-cells" and > > "#size-cells" properties to the SMC node. > > Yes, and at that point, as I suggested, it probably would be better > to use: > > #address-cells = <1>; > #size-cells = <0>; > > gpio@67503030 { > reg = <0x67503030>; > }; > > gpio@67703030 { > reg = <0x67703030>; > }; > > Then the "reg" has a meaning that is directly related to the SMC. > > > > But this ties back to the device enumeration too, since right now the DT > > > does not drive that (we'd have to add the subdevice to the mfd subdevice > > > list somehow anyway, if we don't switch to compatibles). > > > > > > I'd love to hear Rob's opinion on this one, and also whether the > > > existing Linux and OpenBSD code would currently find gpio@0 {} instead > > > of gpio {} for backwards compat. > > > > The OpenBSD driver does a lookup by name and the "@0" is part of that > > name. So that would break backwards compat. > > Oh, that's annoying - and is a different behaviour to Linux. > > On Linux, we only look at the node name up to the @ when matching (see > of_node_name_eq() in drivers/of/base.c, so it doesn't matter to Linux > what follows the @ when you try to look up a node named "gpio" - you'll > find gpio@anythingyoulike. So maybe I should change our code to match what Linux does. OpenBSD 7.2 is scheduled for release on November 1st, and I can probably get that change in quickly. If we can hold off updating the device trees in the Asahi installer until then the transition should be smooth enough. > > Maybe just name the slave GPIO controller "gpio-slave"? If we add > > compatibles, the compatibles for the nodes should propbably be > > different such that we can switch to do a lookup by compatible? > > I don't think the DT folk would be happy with "gpio-slave" because > node names are supposed to be generic. I don't think that rule applies to "named" sub-nodes like this where the name is actually significant. > Also, "slave" probably isn't a good choice of name in this modern > era given past history. Yes, we don't have to follow Apple's terminology here. > Rather than the above, we could use "reg" to indicate which GPIO > controller we're talking about, and lookup the reg value in a table > to give the key. So gpio@0, reg=<0> => gP00, gpio@1, reg=<1> => gp00. > gpio@2, reg=<2> => whatever next. > > That sounds like it won't break the existing OpenBSD. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
On Tue, Sep 06, 2022 at 03:54:50PM +0100, Russell King (Oracle) wrote: > On Tue, Sep 06, 2022 at 04:25:49PM +0200, Mark Kettenis wrote: > > > Date: Tue, 6 Sep 2022 22:53:47 +0900 > > > From: Hector Martin <marcan@marcan.st> > > > > > > I agree that this is something to think about (I was about to reply on > > > the subject). > > > > > > I can think of two ways: using `reg` for the key name, but that feels > > > icky since it's ASCII and not *really* a register number/address, or > > > something like this: > > > > > > gpio@0 { > > > apple,smc-key-base = "gP00"; > > > ... > > > } > > > > > > gpio@1 { > > > apple,smc-key-base = "gp00"; > > > ... > > > } > > > > This would still require us to add a (one-cell) "reg" property and > > would require adding the appropriate "#address-cells" and > > "#size-cells" properties to the SMC node. > > Yes, and at that point, as I suggested, it probably would be better > to use: > > #address-cells = <1>; > #size-cells = <0>; > > gpio@67503030 { > reg = <0x67503030>; > }; > > gpio@67703030 { > reg = <0x67703030>; > }; > > Then the "reg" has a meaning that is directly related to the SMC. That's certainly odd, but if that's how it is addressed, then okay I suppose. > > > > But this ties back to the device enumeration too, since right now the DT > > > does not drive that (we'd have to add the subdevice to the mfd subdevice > > > list somehow anyway, if we don't switch to compatibles). > > > > > > I'd love to hear Rob's opinion on this one, and also whether the > > > existing Linux and OpenBSD code would currently find gpio@0 {} instead > > > of gpio {} for backwards compat. Node names are generally not considered ABI except when they are. :) Generally, core code doesn't care. Specific bindings with defined child nodes often do, but finding nodes by compatible is strongly preferred. Linux drivers can bind by node name (w/o unit-address), but that's really only ever done on ancient h/w. > > > > The OpenBSD driver does a lookup by name and the "@0" is part of that > > name. So that would break backwards compat. > > Oh, that's annoying - and is a different behaviour to Linux. > > On Linux, we only look at the node name up to the @ when matching (see > of_node_name_eq() in drivers/of/base.c, so it doesn't matter to Linux > what follows the @ when you try to look up a node named "gpio" - you'll > find gpio@anythingyoulike. > > > Maybe just name the slave GPIO controller "gpio-slave"? If we add > > compatibles, the compatibles for the nodes should propbably be > > different such that we can switch to do a lookup by compatible? > > I don't think the DT folk would be happy with "gpio-slave" because > node names are supposed to be generic. Also, "slave" probably isn't > a good choice of name in this modern era given past history. Yeah, not a great choice for both reasons. > Rather than the above, we could use "reg" to indicate which GPIO > controller we're talking about, and lookup the reg value in a table > to give the key. So gpio@0, reg=<0> => gP00, gpio@1, reg=<1> => gp00. > gpio@2, reg=<2> => whatever next. Keep in mind that for any level, there is only 1 address space. So if there's anything else with multiple instances, they share the same address space. IOW, you couldn't have say 'rtc@1'. (Another example of why I want to see a full picture.) > > That sounds like it won't break the existing OpenBSD. No? Isn't OpenBSD looking for 'gpio' which wouldn't find 'gpio@0'? Rob
On Tue, Sep 06, 2022 at 10:04:45AM +0100, Russell King (Oracle) wrote: > On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote: > > This one is actually pretty odd in that the child nodes don't have a > > compatible string which breaks the automagical probing. > > I don't think that is necessarily true, and I don't think it's true in > this case. It is, because you are creating the devices in the driver rather than creating devices based on child nodes that exist. > > The SMC core driver instructs the MFD core to create devices for the > individual functional items: > > static const struct mfd_cell apple_smc_devs[] = { > { > .name = "macsmc-gpio", > }, > { > .name = "macsmc-hid", > }, > { > .name = "macsmc-power", > }, > { > .name = "macsmc-reboot", > }, > { > .name = "macsmc-rtc", > }, > }; > > Since MFD uses platform devices for these, they get all the normal > functionality that these devices have, which include matching by > device name ot the driver name, and udev events being appropriately > triggered. As long as the platform drivers for these devices have the > correct modalias lines, autoloading of the modules will work and the > drivers will be correctly matched and probed. Yes, and that's how MFD devices with no child nodes work. The difference is those get any DT info out of the parent node. Here you are presumably manually getting the child DT node you want for each driver. > The Asahi kernel builds most of the platform support as modules, > including these, so we know it works (if it didn't, then lots of > module autoloading would be broken on non-DT platforms.) > > > > Again the separate nodes are there because the RTC and the reboot > > > functionality are logically separate and handled by different MFD > > > sub-drivers in Linux. > > > > It's really a question of whether the subset of functionality is going > > to get reused on its own or has its own resources in DT. MFD bindings > > are done both ways. > > I think the current position on what to do about these is that everyone > is looking for someone else to make a decision, and no one wants to! I'm just looking for more information first. > Firstly, I don't think that the number of properties in a node should > have a bearing on the design of the DT binding - what should have a > bearing is the logical partitioning of functionality. It's not strictly about number of properties. Though nodes with only a compatible string is generally a red flag for me. > Mark suggests that it would take six months for OpenBSD to transition to > some other description - for example, if we merged the nodes. The risk you take when using undocumented bindings... > Hector says that MacOS's firmware description has the nodes merged, but > their description is a mess. > > The overall preference seems to be to keep the sub-nodes unless there > is a strong technical reason not to. > > The feeling I am getting from the review is that there doesn't seem to > be a strong technical reason to merge the nodes - there are desires and > preferences, but nothing concrete. > > So at this point, I think it would make sense if I post a v2 with all > the updates so far (sorry, given the long drawn out discussions on > this, I've lost track of what changes have been made to the code, so > I won't include a detailed change log.) As I said elsewhere, sub-nodes is probably the right choice here. I think they need compatible strings in the child nodes, and addressing has to be sorted out which it seems may also break OpenBSD. Rob
On 07/09/2022 01.10, Rob Herring wrote: >> So at this point, I think it would make sense if I post a v2 with all >> the updates so far (sorry, given the long drawn out discussions on >> this, I've lost track of what changes have been made to the code, so >> I won't include a detailed change log.) > > As I said elsewhere, sub-nodes is probably the right choice here. I > think they need compatible strings in the child nodes, and addressing > has to be sorted out which it seems may also break OpenBSD. So addressing only makes sense for GPIO, out of the nodes we have so far - that's the only thing with two discrete instances whose access can be entirely described by a single base key name, and which are otherwise compatible. Everything else is pretty much single-instance, and talks to multiple keys, so there isn't one single "address" key that would make semantic sense to use as the node address. There are some indexed keys, but at a deeper level (e.g. multiple battery cells part of the charge control subsystem, multiple Type C ports as part of the AC/power input subsystem, etc.). And in those cases, these subdevices are mostly homogeneous and we would never need multiple nodes for them at the DT level, they'd just be implicitly handled by those drivers. GPIO is quite special in that 1) it only has a single key type (which is overloaded using advanced features to provide, effectively, sub-registers to control all the GPIO features per pin), 2) a single key represents a single pin, 3) keys are numbered in a reasoanble way, and 4) there are two prefixes for two discrete GPIO controllers. For pretty much everything else SMC does, we just have a bag of keys with no real rhyme nor reason from the point of view of an "address space". Given that, how would this work? Is it legal/reasonable for only the gpio nodes to have addressing/reg properties, and everything else to just have a node name with no concept of address? Does it even make sense to special case gpio in this way, vs. just having something like gpio {} / gpio-sec {} (if we ever even need gpio-sec, which is an open question)? - Hector
On Wed, Sep 07, 2022 at 02:00:53AM +0900, Hector Martin wrote: > On 07/09/2022 01.10, Rob Herring wrote: > >> So at this point, I think it would make sense if I post a v2 with all > >> the updates so far (sorry, given the long drawn out discussions on > >> this, I've lost track of what changes have been made to the code, so > >> I won't include a detailed change log.) > > > > As I said elsewhere, sub-nodes is probably the right choice here. I > > think they need compatible strings in the child nodes, and addressing > > has to be sorted out which it seems may also break OpenBSD. > > So addressing only makes sense for GPIO, out of the nodes we have so far > - that's the only thing with two discrete instances whose access can be > entirely described by a single base key name, and which are otherwise > compatible. > > Everything else is pretty much single-instance, and talks to multiple > keys, so there isn't one single "address" key that would make semantic > sense to use as the node address. Unit-addresses are just the first address in 'reg'. So multiple addresses or not doesn't really matter. > There are some indexed keys, but at a > deeper level (e.g. multiple battery cells part of the charge control > subsystem, multiple Type C ports as part of the AC/power input > subsystem, etc.). And in those cases, these subdevices are mostly > homogeneous and we would never need multiple nodes for them at the DT > level, they'd just be implicitly handled by those drivers. Type-C will be fun depending on how much of the muxing/altmode details have to get exposed. > GPIO is quite special in that 1) it only has a single key type (which is > overloaded using advanced features to provide, effectively, > sub-registers to control all the GPIO features per pin), 2) a single key > represents a single pin, 3) keys are numbered in a reasoanble way, and > 4) there are two prefixes for two discrete GPIO controllers. For pretty > much everything else SMC does, we just have a bag of keys with no real > rhyme nor reason from the point of view of an "address space". > > Given that, how would this work? Is it legal/reasonable for only the > gpio nodes to have addressing/reg properties, and everything else to > just have a node name with no concept of address? Not ideal, but allowed. > Does it even make > sense to special case gpio in this way, vs. just having something like > gpio {} / gpio-sec {} (if we ever even need gpio-sec, which is an open > question)? If not unit-addresses, the 2nd choice we do is 'gpio-0', 'gpio-1', etc. Though that is mainly in the GPIO based consumer bindings like gpio-pwm, spi-gpio, etc. where there's really not anything to use for an address. If only those 2 nodes, then I really don't care so much and gpio-sec would be fine. It's 1 node in 1 binding. Rob
On Tue, Sep 6, 2022, at 19:35, Rob Herring wrote: > On Wed, Sep 07, 2022 at 02:00:53AM +0900, Hector Martin wrote: >> On 07/09/2022 01.10, Rob Herring wrote: >> >> So at this point, I think it would make sense if I post a v2 with all >> >> the updates so far (sorry, given the long drawn out discussions on >> >> this, I've lost track of what changes have been made to the code, so >> >> I won't include a detailed change log.) >> > >> > As I said elsewhere, sub-nodes is probably the right choice here. I >> > think they need compatible strings in the child nodes, and addressing >> > has to be sorted out which it seems may also break OpenBSD. >> >> So addressing only makes sense for GPIO, out of the nodes we have so far >> - that's the only thing with two discrete instances whose access can be >> entirely described by a single base key name, and which are otherwise >> compatible. >> >> Everything else is pretty much single-instance, and talks to multiple >> keys, so there isn't one single "address" key that would make semantic >> sense to use as the node address. > > Unit-addresses are just the first address in 'reg'. So multiple > addresses or not doesn't really matter. > >> There are some indexed keys, but at a >> deeper level (e.g. multiple battery cells part of the charge control >> subsystem, multiple Type C ports as part of the AC/power input >> subsystem, etc.). And in those cases, these subdevices are mostly >> homogeneous and we would never need multiple nodes for them at the DT >> level, they'd just be implicitly handled by those drivers. > > Type-C will be fun depending on how much of the muxing/altmode > details have to get exposed. Type-C is going to be a lot of "fun", but the SMC is not directly involved. I still don't have a full picture but these boards have TPS6598x chips which trigger the entire mess whenever a new mode was negotiated and the "Apple Type-C PHY" contains the actual mux. The SMC has a side channel to these TPS6598x chips as well but it seems to only handle charging without having to communicate with whatever kernel is running on the main processor. Sven
On 07/09/2022 02.35, Rob Herring wrote: > On Wed, Sep 07, 2022 at 02:00:53AM +0900, Hector Martin wrote: >> On 07/09/2022 01.10, Rob Herring wrote: >>>> So at this point, I think it would make sense if I post a v2 with all >>>> the updates so far (sorry, given the long drawn out discussions on >>>> this, I've lost track of what changes have been made to the code, so >>>> I won't include a detailed change log.) >>> >>> As I said elsewhere, sub-nodes is probably the right choice here. I >>> think they need compatible strings in the child nodes, and addressing >>> has to be sorted out which it seems may also break OpenBSD. >> >> So addressing only makes sense for GPIO, out of the nodes we have so far >> - that's the only thing with two discrete instances whose access can be >> entirely described by a single base key name, and which are otherwise >> compatible. >> >> Everything else is pretty much single-instance, and talks to multiple >> keys, so there isn't one single "address" key that would make semantic >> sense to use as the node address. > > Unit-addresses are just the first address in 'reg'. So multiple > addresses or not doesn't really matter. Okay, but we're obviously not going to list every single SMC key used by any given node in the device tree (that'd be a giant mess, other than for hwmon which is a story for another time), and it doesn't make sense to pick an arbitrary one as the reg address and then ignore it in the driver. >> There are some indexed keys, but at a >> deeper level (e.g. multiple battery cells part of the charge control >> subsystem, multiple Type C ports as part of the AC/power input >> subsystem, etc.). And in those cases, these subdevices are mostly >> homogeneous and we would never need multiple nodes for them at the DT >> level, they'd just be implicitly handled by those drivers. > > Type-C will be fun depending on how much of the muxing/altmode > details have to get exposed. As sven mentioned that will be fun, but thankfully not part of this binding (SMC only cares for power supply purposes, and since our direct driver already exposes power config info in Linux (or should, and we can improve that), I don't particularly have plans to expose the SMC Type-C data other than perhaps something in the existing power supply driver to indicate which port is the currently chosen power input. > If only those 2 nodes, then I really don't care so much and gpio-sec > would be fine. It's 1 node in 1 binding. I think it might make sense to just go with this then. If Apple ever introduces yet another GPIO sub-controller we can just add another one, and honestly I don't think that's very likely, given they don't even use any of the GPIOs from the second one from the AP yet. I don't see SMC growing a big list of GPIO controllers any time soon, such that we regret doing it this way. And then the node-name can just map to a given key prefix statically in the driver, and thus we don't even need a property for that (gpio would be gP?? and gpio-sec gp?? right now). - Hector
> Date: Wed, 7 Sep 2022 03:38:09 +0900 > From: Hector Martin <marcan@marcan.st> > > On 07/09/2022 02.35, Rob Herring wrote: > > > If only those 2 nodes, then I really don't care so much and gpio-sec > > would be fine. It's 1 node in 1 binding. > > I think it might make sense to just go with this then. If Apple ever > introduces yet another GPIO sub-controller we can just add another one, > and honestly I don't think that's very likely, given they don't even use > any of the GPIOs from the second one from the AP yet. I don't see SMC > growing a big list of GPIO controllers any time soon, such that we > regret doing it this way. And then the node-name can just map to a given > key prefix statically in the driver, and thus we don't even need a > property for that (gpio would be gP?? and gpio-sec gp?? right now). We could also use a compatible property to map the key prefix. For example we could have "apple,smc-gpio-primary" map to gP?? and "apple,smc-gpio-secondary" map to gp??. Then we can keep the generic "gpio" name for both GPIO nodes. And if Apple introduces yet another GPIO sub-controller we just have to invent a new compatible for it. Probably the cleanest solution if Rob still thinks it is better for these nodes to have a compatible property anyway.
diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml new file mode 100644 index 000000000000..794d3a6eb04a --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple Mac System Management Controller + +maintainers: + - Hector Martin <marcan@marcan.st> + +description: + Apple Mac System Management Controller implements various functions + such as GPIO, RTC, power, reboot. + +properties: + compatible: + items: + - enum: + - apple,t8103-smc + - apple,t8112-smc + - apple,t6000-smc + - const: apple,smc + + reg: + description: Two regions, one for the SMC area and one for the SRAM area. + + reg-names: + items: + - const: smc + - const: sram + + mboxes: + description: + A phandle to the mailbox channel + +additionalProperties: false + +required: + - compatible + - reg + - reg-names + - mboxes + +examples: + - | + smc@23e400000 { + compatible = "apple,t8103-smc", "apple,smc"; + reg = <0x2 0x3e400000 0x0 0x4000>, + <0x2 0x3fe00000 0x0 0x100000>; + reg-names = "smc", "sram"; + mboxes = <&smc_mbox>; + };
Add a DT binding for the Apple Mac System Management Controller. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- .../devicetree/bindings/mfd/apple,smc.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml