| Message ID | YxC5eZjGgd8xguDr@shell.armlinux.org.uk |
|---|---|
| Headers | show |
| Series | Add Apple Mac System Management Controller GPIOs | expand |
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 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 Thu, Sep 1, 2022, at 15:54, Russell King wrote: > From: Hector Martin <marcan@marcan.st> > > This allows a client to receive messages in atomic context, by polling. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Sven Peter <sven@svenpeter.dev>
On Thu, 1 Sept 2022 at 15:03, Russell King <rmk+kernel@armlinux.org.uk> wrote: > > From: Hector Martin <marcan@marcan.st> > > This allows a client to receive messages in atomic context, by polling. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Eric Curtin <ecurtin@redhat.com>
> On 1. 9. 2022, at 20:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: >> + ret = apple_smc_write_u32(smcgp->smc, key, CMD_OUTPUT | value); >> + if (ret < 0) >> + dev_err(smcgp->dev, "GPIO set failed %p4ch = 0x%x\n", &key, value); > > Strange specifier... It seems like a hashed pointer with added (or > skipped? I don't remember) '4ch'. Perhaps you meant one of '%pE', > '%p4cc'? > Ditto for other cases. As was pointed out by Sven elsewhere in the thread, this is an unupstreamed specifier (that was missed as a dependency of this code). https://github.com/AsahiLinux/linux/blob/f8c0d18173a7b649999ee27515393f7aae40310c/Documentation/core-api/printk-formats.rst#generic-fourcc-code Martin
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
On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: > > On 1. 9. 2022, at 20:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > >> + ret = apple_smc_write_u32(smcgp->smc, key, CMD_OUTPUT | value); > >> + if (ret < 0) > >> + dev_err(smcgp->dev, "GPIO set failed %p4ch = 0x%x\n", &key, value); > > > > Strange specifier... It seems like a hashed pointer with added (or > > skipped? I don't remember) '4ch'. Perhaps you meant one of '%pE', > > '%p4cc'? > > Ditto for other cases. > > As was pointed out by Sven elsewhere in the thread, this is an > unupstreamed specifier (that was missed as a dependency of this > code). > > https://github.com/AsahiLinux/linux/blob/f8c0d18173a7b649999ee27515393f7aae40310c/Documentation/core-api/printk-formats.rst#generic-fourcc-code I don't see why we need that. The %.4s (0x%08x) is repeating that with the existing codebase. (I do understand why v4l2/drm have it). Ideally the first should use %4pE, but it might not be suitable in some cases.
On Thu, Sep 1, 2022 at 3:54 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > From: Hector Martin <marcan@marcan.st> > > This driver implements the GPIO service on top of the SMC framework > on Apple Mac machines. In particular, these are the GPIOs present in the > PMU IC which are used to control power to certain on-board devices. > > Although the underlying hardware supports various pin config settings > (input/output, open drain, etc.), this driver does not implement that > functionality and leaves it up to the firmware to configure things > properly. We also don't yet support interrupts/events. This is > sufficient for device power control, which is the only thing we need to > support at this point. More features will be implemented when needed. > > To our knowledge, only Apple Silicon Macs implement this SMC feature. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Overall this looks very good provided the SMC API is fine with the platform/MFD maintainers, I like the usage of .init_valid_mask which is used just as intended. Andy's detailed review points should be addressed reasonably after that it's: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: > > +static int macsmc_gpio_nr(smc_key key) > > +{ > > + int low = hex_to_bin(key & 0xff); > > + int high = hex_to_bin((key >> 8) & 0xff); > > + > > + if (low < 0 || high < 0) > > + return -1; > > + > > + return low | (high << 4); > > +} > > NIH hex2bin(). Is using hex2bin really better? static int macsmc_gpio_nr(smc_key key) { char k[2]; u8 result; int ret; k[0] = key; k[1] = key >> 8; ret = hex2bin(&result, k, 2); if (ret < 0) return ret; return result; } This looks to me like it consumes more CPU cycles - because we have to write each "character" to the stack, then call a function, only to then call the hex_to_bin() function. One can't just pass "key" into hex2bin because that will bring with it endian issues. > > +static int macsmc_gpio_key(unsigned int offset) > > +{ > > + return _SMC_KEY("gP\0\0") | (hex_asc_hi(offset) << 8) | hex_asc_lo(offset); > > +} > > NIH hex_byte_pack(). This would become: char buf[2]; hex_byte_pack(buf, offset); return _SMC_KEY("gP\0\0") | buf[0] << 8 | buf[1]; to avoid the endian issues. It just seems to be a more complex way to do the conversion. One could then argue that this is just a NIH sprintf(), so it could then be written: char buf[5]; snprintf(buf, sizeof(buf), "gP%02x", offset); return _SMC_KEY(buf); which looks nicer, but involves a lot more code. Since this is called for every GPIO operation, and you were worred above about the layout of the macsmc_gpio structure (which is a micro- optimisation), it seems weird to be concerned about the efficiency of the structure layout and then suggest less efficient code in each of the functional paths of the driver. There seems to be a contradiction. > > + /* First try reading the explicit pin mode register */ > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_PINMODE, &val); > > + if (!ret) > > + return (val & MODE_OUTPUT) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN; > > + > > + /* > > + * Less common IRQ configs cause CMD_PINMODE to fail, and so does open drain mode. > > + * Fall back to reading IRQ mode, which will only succeed for inputs. > > + */ > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val); > > + return (!ret) ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT; > > What is the meaning of val in this case? Reading the comment, it seems that "val" is irrelevant. I'm not sure that needs explaining given there's a comment that's already explaining what is going on here. > > + if (ret == GPIO_LINE_DIRECTION_OUT) > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_OUTPUT, &val); > > + else > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_INPUT, &val); > > > + > > Unnecessary blank line. I think that's personal style preference, it isn't mentioned in the coding style. However, the following is much nicer and likely produces better code: if (ret == GPIO_LINE_DIRECTION_OUT) cmd = CMD_OUTPUT; else cmd = CMD_INPUT; ret = apple_smc_rw_u32(smcgp->smc, key, cmd, &val); if (ret < 0) return ret; > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > > + int count = apple_smc_get_key_count(smcgp->smc) - smcgp->first_index; > > I would split this assignment and move it closer to the first user. > > > + int i; > > + > > + if (count > MAX_GPIO) > > + count = MAX_GPIO; > > Hmm... When can it be the case? That's a question for the Asahi folk - it's not obvious whether it could or could not be from the code. I think it depends on firmware. > > + bitmap_zero(valid_mask, ngpios); > > + > > + for (i = 0; i < count; i++) { > > + smc_key key; > > + int gpio_nr; > > > + int ret = apple_smc_get_key_by_index(smcgp->smc, smcgp->first_index + i, &key); > > Ditto. What does "ditto" here mean, because I don't think you mean "Hmm... When can it be the case?" and "I would split this assignment and move it closer to the first user." doesn't seem to be relevant either. > > + pdev->dev.of_node = of_get_child_by_name(pdev->dev.parent->of_node, "gpio"); > > Can we use fwnode APIs instead? > Or do you really need this? Ouch, that's not nice. I can change this to: fwnode = device_get_named_child_node(pdev->dev.parent, "gpio"); device_set_node(&pdev->dev, fwnode); but even that isn't _that_ nice. I'd like to hear comments from the Asahi folk about whether these sub-blocks of the SMC can have compatibles, so that the MFD layer can automatically fill in the firmware nodes on the struct device before the probe function gets called. If not, then I think it would be reasonable to have a discussion with Lee about extending MFD to be able to have mfd cells name a child, so that MFD can do the above instead of having it littered amongst drivers. Thanks for the review.
On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: ... > > > +static int macsmc_gpio_nr(smc_key key) > > > +{ > > > + int low = hex_to_bin(key & 0xff); > > > + int high = hex_to_bin((key >> 8) & 0xff); > > > + > > > + if (low < 0 || high < 0) > > > + return -1; > > > + > > > + return low | (high << 4); > > > +} > > > > NIH hex2bin(). > > Is using hex2bin really better? Yes. > static int macsmc_gpio_nr(smc_key key) > { > char k[2]; > u8 result; > int ret; > > k[0] = key; > k[1] = key >> 8; > > ret = hex2bin(&result, k, 2); > if (ret < 0) > return ret; > > return result; > } > > This looks to me like it consumes more CPU cycles - because we have to > write each "character" to the stack, then call a function, only to then > call the hex_to_bin() function. One can't just pass "key" into hex2bin > because that will bring with it endian issues. With one detail missed, why do you need all that if you can use byteorder helpers()? What's the stack? Just replace this entire function with the respectful calls to hex2bin(). ... > > > +static int macsmc_gpio_key(unsigned int offset) > > > +{ > > > + return _SMC_KEY("gP\0\0") | (hex_asc_hi(offset) << 8) | hex_asc_lo(offset); > > > +} > > > > NIH hex_byte_pack(). > > This would become: > > char buf[2]; > > hex_byte_pack(buf, offset); > > return _SMC_KEY("gP\0\0") | buf[0] << 8 | buf[1]; You have a point here. > to avoid the endian issues. It just seems to be a more complex way to > do the conversion. One could then argue that this is just a NIH > sprintf(), so it could then be written: No, no. snprintf() is too much here. > which looks nicer, but involves a lot more code. > > Since this is called for every GPIO operation, and you were worred above > about the layout of the macsmc_gpio structure (which is a micro- > optimisation), it seems weird to be concerned about the efficiency of > the structure layout and then suggest less efficient code in each of the > functional paths of the driver. There seems to be a contradiction. ... > > > + /* First try reading the explicit pin mode register */ > > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_PINMODE, &val); > > > + if (!ret) > > > + return (val & MODE_OUTPUT) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN; > > > + > > > + /* > > > + * Less common IRQ configs cause CMD_PINMODE to fail, and so does open drain mode. > > > + * Fall back to reading IRQ mode, which will only succeed for inputs. > > > + */ > > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val); > > > + return (!ret) ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT; > > > > What is the meaning of val in this case? > > Reading the comment, it seems that "val" is irrelevant. I'm not sure that > needs explaining given there's a comment that's already explaining what > is going on here. OK. Just convert then (!ret) --> ret. ... > > > + if (ret == GPIO_LINE_DIRECTION_OUT) > > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_OUTPUT, &val); > > > + else > > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_INPUT, &val); > > > > > + > > > > Unnecessary blank line. > > I think that's personal style preference, it isn't mentioned in the coding > style. However, the following is much nicer and likely produces better > code: > > if (ret == GPIO_LINE_DIRECTION_OUT) > cmd = CMD_OUTPUT; > else > cmd = CMD_INPUT; > > ret = apple_smc_rw_u32(smcgp->smc, key, cmd, &val); > if (ret < 0) > return ret; Go for it! ... > > > + if (count > MAX_GPIO) > > > + count = MAX_GPIO; > > > > Hmm... When can it be the case? > > That's a question for the Asahi folk - it's not obvious whether it could > or could not be from the code. I think it depends on firmware. If it's the case, why does the code not support higher GPIOs? Needs at least a comment. ... > > > + bitmap_zero(valid_mask, ngpios); > > > + > > > + for (i = 0; i < count; i++) { > > > + smc_key key; > > > + int gpio_nr; > > > > > + int ret = apple_smc_get_key_by_index(smcgp->smc, smcgp->first_index + i, &key); > > > > Ditto. > > What does "ditto" here mean, because I don't think you mean "Hmm... > When can it be the case?" and "I would split this assignment and move > it closer to the first user." doesn't seem to be relevant either. Split int ret = foo(); to int ret; ret = foo(); ... > > > + pdev->dev.of_node = of_get_child_by_name(pdev->dev.parent->of_node, "gpio"); > > > > Can we use fwnode APIs instead? > > Or do you really need this? > > Ouch, that's not nice. I can change this to: (Some background on why my eye caught this. We as GPIO SIG in the kernel want to move the library to be fwnode one without looking into the underneath property provider. This kind of lines makes driver look a bit ugly from that perspective) > fwnode = device_get_named_child_node(pdev->dev.parent, "gpio"); > device_set_node(&pdev->dev, fwnode); > > but even that isn't _that_ nice. I'd like to hear comments from the Asahi > folk about whether these sub-blocks of the SMC can have compatibles, so > that the MFD layer can automatically fill in the firmware nodes on the > struct device before the probe function gets called. > If not, then I think it would be reasonable to have a discussion with > Lee about extending MFD to be able to have mfd cells name a child, so > that MFD can do the above instead of having it littered amongst drivers. MFD cells can be matched by compatible strings.
Pardon, I lost the CC list in my earlier reply. Adding it back now. > On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@cutebit.org> wrote: >>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: >>>>> On 1. 9. 2022, at 20:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>>> On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: >>>> >>>>>> + ret = apple_smc_write_u32(smcgp->smc, key, CMD_OUTPUT | value); >>>>>> + if (ret < 0) >>>>>> + dev_err(smcgp->dev, "GPIO set failed %p4ch = 0x%x\n", &key, value); >>>>> >>>>> Strange specifier... It seems like a hashed pointer with added (or >>>>> skipped? I don't remember) '4ch'. Perhaps you meant one of '%pE', >>>>> '%p4cc'? >>>>> Ditto for other cases. >>>> >>>> As was pointed out by Sven elsewhere in the thread, this is an >>>> unupstreamed specifier (that was missed as a dependency of this >>>> code). >>>> >>>> https://github.com/AsahiLinux/linux/blob/f8c0d18173a7b649999ee27515393f7aae40310c/Documentation/core-api/printk-formats.rst#generic-fourcc-code >>> >>> I don't see why we need that. The %.4s (0x%08x) is repeating that with >>> the existing codebase. (I do understand why v4l2/drm have it). Ideally >>> the first should use %4pE, but it might not be suitable in some cases. >> >> Just from a superficial understanding of things: %p4ch on little-endian >> will print in a reversed order to %.4s. As I see it the handling of >> endianness is the value proposition of the new specifiers. > > So, what prevents you from adding this to %pE? > The preferred way is not adding a specifier for a single user with a > particular case, esp. when it's covered by the existing ones. Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’? If you think that would be accepted... I guess this was added on the assumption that keys like this will be a common occurrence in interaction with Apple firmware. Though greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the SMC code (9 times): ./drivers/power/reset/macsmc-reboot.c ./drivers/platform/apple/smc_core.c ./drivers/gpio/gpio-macsmc.c >> So >> >> %p4ch - interpret as an u32, print the character in most significant byte first > > %.4s + be32_to_cpu() Well, AIUI instead of printk(“%p4ch = ...\n”, &key); you need to do u32 key_be = cpu_to_be32(key); printk(“%.4s = ...\n”, &key_be); in at least 9 places now, the number of which will probably grow. Just to make the case for *some* printk helper. > >> %p4cb - the same as %.4s > >> %p4cl - reversed to %.4s > > %.4s + swab32() Sure, these two are uninteresting, probably added for completeness. > > So? Well, so nothing. I am primarily explaining how that strange specifier came to be. Martin > -- > With Best Regards, > Andy Shevchenko
On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: > > > > +static int macsmc_gpio_nr(smc_key key) > > > > +{ > > > > + int low = hex_to_bin(key & 0xff); > > > > + int high = hex_to_bin((key >> 8) & 0xff); > > > > + > > > > + if (low < 0 || high < 0) > > > > + return -1; > > > > + > > > > + return low | (high << 4); > > > > +} > > > > > > NIH hex2bin(). > > > > Is using hex2bin really better? > > Yes. > > > static int macsmc_gpio_nr(smc_key key) > > { > > char k[2]; > > u8 result; > > int ret; > > > > k[0] = key; > > k[1] = key >> 8; > > > > ret = hex2bin(&result, k, 2); > > if (ret < 0) > > return ret; > > > > return result; > > } > > > > This looks to me like it consumes more CPU cycles - because we have to > > write each "character" to the stack, then call a function, only to then > > call the hex_to_bin() function. One can't just pass "key" into hex2bin > > because that will bring with it endian issues. > > With one detail missed, why do you need all that if you can use > byteorder helpers()? What's the stack? Just replace this entire > function with the respectful calls to hex2bin(). Sorry, I don't understand what you're suggesting, because it doesn't make sense to me. The byteorder helpers do not give a char array, which is what hex2bin() wants, so we end up with something like: __le16 foo = cpu_to_le16(key); u8 result; ret = hex2bin(&result, (char *)&foo, 1); if (ret < 0) return ret; return result; This to me looks like yucky code, It still results in "foo" having to be on the stack, because the out-of-line hex2bin() requires a pointer to be passed as the second argument. Maybe you could provide an example of what you're thinking of, because I'm at a loss to understand what you're thinking this should look like. > > > > + /* First try reading the explicit pin mode register */ > > > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_PINMODE, &val); > > > > + if (!ret) > > > > + return (val & MODE_OUTPUT) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN; > > > > + > > > > + /* > > > > + * Less common IRQ configs cause CMD_PINMODE to fail, and so does open drain mode. > > > > + * Fall back to reading IRQ mode, which will only succeed for inputs. > > > > + */ > > > > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val); > > > > + return (!ret) ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT; > > > > > > What is the meaning of val in this case? > > > > Reading the comment, it seems that "val" is irrelevant. I'm not sure that > > needs explaining given there's a comment that's already explaining what > > is going on here. > > OK. > Just convert then (!ret) --> ret. Already done, thanks. > > > > + pdev->dev.of_node = of_get_child_by_name(pdev->dev.parent->of_node, "gpio"); > > > > > > Can we use fwnode APIs instead? > > > Or do you really need this? > > > > Ouch, that's not nice. I can change this to: > > (Some background on why my eye caught this. We as GPIO SIG in the > kernel want to move the library to be fwnode one without looking into > the underneath property provider. This kind of lines makes driver look > a bit ugly from that perspective) I agree, I'd prefer it not to be there. > > fwnode = device_get_named_child_node(pdev->dev.parent, "gpio"); > > device_set_node(&pdev->dev, fwnode); > > > > but even that isn't _that_ nice. I'd like to hear comments from the Asahi > > folk about whether these sub-blocks of the SMC can have compatibles, so > > that the MFD layer can automatically fill in the firmware nodes on the > > struct device before the probe function gets called. > > > If not, then I think it would be reasonable to have a discussion with > > Lee about extending MFD to be able to have mfd cells name a child, so > > that MFD can do the above instead of having it littered amongst drivers. > > MFD cells can be matched by compatible strings. Yes, that's what I meant in my preceeding paragraph above, but it needs involvement and decisions from the Asahi maintainers.
On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: > > On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@cutebit.org> wrote: > >>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: ... > >>> I don't see why we need that. The %.4s (0x%08x) is repeating that with > >>> the existing codebase. (I do understand why v4l2/drm have it). Ideally > >>> the first should use %4pE, but it might not be suitable in some cases. > >> > >> Just from a superficial understanding of things: %p4ch on little-endian > >> will print in a reversed order to %.4s. As I see it the handling of > >> endianness is the value proposition of the new specifiers. > > > > So, what prevents you from adding this to %pE? > > The preferred way is not adding a specifier for a single user with a > > particular case, esp. when it's covered by the existing ones. > > Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’? > If you think that would be accepted... > > I guess this was added on the assumption that keys like this will > be a common occurrence in interaction with Apple firmware. Though > greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the > SMC code (9 times): > > ./drivers/power/reset/macsmc-reboot.c > ./drivers/platform/apple/smc_core.c > ./drivers/gpio/gpio-macsmc.c > >> %p4ch - interpret as an u32, print the character in most significant byte first > > > > %.4s + be32_to_cpu() > > Well, AIUI instead of > > printk(“%p4ch = ...\n”, &key); > > you need to do > > u32 key_be = cpu_to_be32(key); > printk(“%.4s = ...\n”, &key_be); > > in at least 9 places now, the number of which will probably grow. > Just to make the case for *some* printk helper. Wouldn't this be one line printk(“%.4s = ...\n”, &cpu_to_be32(key)); ? So I don't see disadvantages right now. Later on we can consider a new specifier _separately_, otherwise this series would be dragging for no sense.
On Fri, Sep 2, 2022 at 4:33 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: ... > > in at least 9 places now, the number of which will probably grow. > > Just to make the case for *some* printk helper. > > Wouldn't this be one line > > printk(“%.4s = ...\n”, &cpu_to_be32(key)); > > ? > > So I don't see disadvantages right now. Later on we can consider a new > specifier _separately_, otherwise this series would be dragging for no > sense. Just to make my point clear. The single user or small subset of users does not justify the new specifier. New specifier is a lot of burden to printf() code. Find 3+ independent users and we can talk again. That's why I suggest to either create a local (to apple whatever stuff) helper or use existing specifiers in _this_ series.
> On 2. 9. 2022, at 15:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: >>> On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@cutebit.org> wrote: >>>>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: > > ... > >>>>> I don't see why we need that. The %.4s (0x%08x) is repeating that with >>>>> the existing codebase. (I do understand why v4l2/drm have it). Ideally >>>>> the first should use %4pE, but it might not be suitable in some cases. >>>> >>>> Just from a superficial understanding of things: %p4ch on little-endian >>>> will print in a reversed order to %.4s. As I see it the handling of >>>> endianness is the value proposition of the new specifiers. >>> >>> So, what prevents you from adding this to %pE? >>> The preferred way is not adding a specifier for a single user with a >>> particular case, esp. when it's covered by the existing ones. >> >> Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’? >> If you think that would be accepted... >> >> I guess this was added on the assumption that keys like this will >> be a common occurrence in interaction with Apple firmware. Though >> greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the >> SMC code (9 times): >> >> ./drivers/power/reset/macsmc-reboot.c >> ./drivers/platform/apple/smc_core.c >> ./drivers/gpio/gpio-macsmc.c > >>>> %p4ch - interpret as an u32, print the character in most significant byte first >>> >>> %.4s + be32_to_cpu() >> >> Well, AIUI instead of >> >> printk(“%p4ch = ...\n”, &key); >> >> you need to do >> >> u32 key_be = cpu_to_be32(key); >> printk(“%.4s = ...\n”, &key_be); >> >> in at least 9 places now, the number of which will probably grow. >> Just to make the case for *some* printk helper. > > Wouldn't this be one line > > printk(“%.4s = ...\n”, &cpu_to_be32(key)); > > ? That would compile? I thought that’s not valid C, taking an address of function’s return value. > > So I don't see disadvantages right now. Later on we can consider a new > specifier _separately_, otherwise this series would be dragging for no > sense. Absolutely agreed on the latter point. Martin
On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: > > > > > +static int macsmc_gpio_nr(smc_key key) > > > > > +{ > > > > > + int low = hex_to_bin(key & 0xff); > > > > > + int high = hex_to_bin((key >> 8) & 0xff); > > > > > + > > > > > + if (low < 0 || high < 0) > > > > > + return -1; > > > > > + > > > > > + return low | (high << 4); > > > > > +} > > > > > > > > NIH hex2bin(). > > > > > > Is using hex2bin really better? > > > > Yes. > > > > > static int macsmc_gpio_nr(smc_key key) > > > { > > > char k[2]; > > > u8 result; > > > int ret; > > > > > > k[0] = key; > > > k[1] = key >> 8; > > > > > > ret = hex2bin(&result, k, 2); > > > if (ret < 0) > > > return ret; > > > > > > return result; > > > } > > > > > > This looks to me like it consumes more CPU cycles - because we have to > > > write each "character" to the stack, then call a function, only to then > > > call the hex_to_bin() function. One can't just pass "key" into hex2bin > > > because that will bring with it endian issues. > > > > With one detail missed, why do you need all that if you can use > > byteorder helpers()? What's the stack? Just replace this entire > > function with the respectful calls to hex2bin(). > > Sorry, I don't understand what you're suggesting, because it doesn't > make sense to me. The byteorder helpers do not give a char array, which > is what hex2bin() wants, so we end up with something like: > > __le16 foo = cpu_to_le16(key); > u8 result; > > ret = hex2bin(&result, (char *)&foo, 1); > if (ret < 0) > return ret; > > return result; > > This to me looks like yucky code, It still results in "foo" having to > be on the stack, because the out-of-line hex2bin() requires a pointer > to be passed as the second argument. > > Maybe you could provide an example of what you're thinking of, because > I'm at a loss to understand what you're thinking this should look like. So, let's look into the real callers to see, oh wait, it's a single caller! Why can't you simply do ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); if (ret < 0) return ret; in-place there?
On Fri, Sep 2, 2022 at 4:37 PM Martin Povišer <povik@cutebit.org> wrote: > > On 2. 9. 2022, at 15:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: ... > >> you need to do > >> > >> u32 key_be = cpu_to_be32(key); > >> printk(“%.4s = ...\n”, &key_be); > >> > >> in at least 9 places now, the number of which will probably grow. > >> Just to make the case for *some* printk helper. > > > > Wouldn't this be one line > > > > printk(“%.4s = ...\n”, &cpu_to_be32(key)); > > > > ? > > That would compile? I thought that’s not valid C, taking an > address of function’s return value. Ah, you are right. My bad.
On Fri, Sep 02, 2022 at 03:37:27PM +0200, Martin Povišer wrote: > > > On 2. 9. 2022, at 15:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: > >>> On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>> On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@cutebit.org> wrote: > >>>>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>>>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: > > > > ... > > > >>>>> I don't see why we need that. The %.4s (0x%08x) is repeating that with > >>>>> the existing codebase. (I do understand why v4l2/drm have it). Ideally > >>>>> the first should use %4pE, but it might not be suitable in some cases. > >>>> > >>>> Just from a superficial understanding of things: %p4ch on little-endian > >>>> will print in a reversed order to %.4s. As I see it the handling of > >>>> endianness is the value proposition of the new specifiers. > >>> > >>> So, what prevents you from adding this to %pE? > >>> The preferred way is not adding a specifier for a single user with a > >>> particular case, esp. when it's covered by the existing ones. > >> > >> Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’? > >> If you think that would be accepted... > >> > >> I guess this was added on the assumption that keys like this will > >> be a common occurrence in interaction with Apple firmware. Though > >> greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the > >> SMC code (9 times): > >> > >> ./drivers/power/reset/macsmc-reboot.c > >> ./drivers/platform/apple/smc_core.c > >> ./drivers/gpio/gpio-macsmc.c > > > >>>> %p4ch - interpret as an u32, print the character in most significant byte first > >>> > >>> %.4s + be32_to_cpu() > >> > >> Well, AIUI instead of > >> > >> printk(“%p4ch = ...\n”, &key); > >> > >> you need to do > >> > >> u32 key_be = cpu_to_be32(key); > >> printk(“%.4s = ...\n”, &key_be); > >> > >> in at least 9 places now, the number of which will probably grow. > >> Just to make the case for *some* printk helper. > > > > Wouldn't this be one line > > > > printk(“%.4s = ...\n”, &cpu_to_be32(key)); > > > > ? > > That would compile? I thought that’s not valid C, taking an > address of function’s return value. It isn't legal C. int foo(int bar); int blah(int *v); int test(int v) { return blah(&foo(v)); } t.c: In function ‘test’: t.c:7:14: error: lvalue required as unary ‘&’ operand And just to make sure that it's not just my test that is wrong, and there's something magical about cpu_to_be32()... In file included from include/linux/device.h:15, from drivers/gpio/gpio-macsmc.c:11: drivers/gpio/gpio-macsmc.c: In function 'macsmc_gpio_probe': drivers/gpio/gpio-macsmc.c:356:49: error: lvalue required as unary '&' operand 356 | dev_info(smcgp->dev, "First GPIO key: %.4s\n", &cpu_to_be32(key)); | ^ include/linux/dev_printk.h:110:23: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~~~~~~~~~ drivers/gpio/gpio-macsmc.c:356:2: note: in expansion of macro 'dev_info' 356 | dev_info(smcgp->dev, "First GPIO key: %.4s\n", &cpu_to_be32(key)); | ^~~~~~~~ make[3]: *** [scripts/Makefile.build:249: drivers/gpio/gpio-macsmc.o] Error 1 make[2]: *** [scripts/Makefile.build:466: drivers/gpio] Error 2 make[1]: *** [Makefile:1843: drivers] Error 2 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:219: __sub-make] Error 2 So, sorry Andy, but this suggestion does not appear to be legal C. This also applies to your suggestion in the other sub-thread of: ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); As we've now discovered that this is not legal C, can we back up *both* discussions and start again on these points.
On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > <linux@armlinux.org.uk> wrote: > > > > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: > > > > > > +static int macsmc_gpio_nr(smc_key key) > > > > > > +{ > > > > > > + int low = hex_to_bin(key & 0xff); > > > > > > + int high = hex_to_bin((key >> 8) & 0xff); > > > > > > + > > > > > > + if (low < 0 || high < 0) > > > > > > + return -1; > > > > > > + > > > > > > + return low | (high << 4); > > > > > > +} > > > > > > > > > > NIH hex2bin(). > > > > > > > > Is using hex2bin really better? > > > > > > Yes. > > > > > > > static int macsmc_gpio_nr(smc_key key) > > > > { > > > > char k[2]; > > > > u8 result; > > > > int ret; > > > > > > > > k[0] = key; > > > > k[1] = key >> 8; > > > > > > > > ret = hex2bin(&result, k, 2); > > > > if (ret < 0) > > > > return ret; > > > > > > > > return result; > > > > } > > > > > > > > This looks to me like it consumes more CPU cycles - because we have to > > > > write each "character" to the stack, then call a function, only to then > > > > call the hex_to_bin() function. One can't just pass "key" into hex2bin > > > > because that will bring with it endian issues. > > > > > > With one detail missed, why do you need all that if you can use > > > byteorder helpers()? What's the stack? Just replace this entire > > > function with the respectful calls to hex2bin(). > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > make sense to me. The byteorder helpers do not give a char array, which > > is what hex2bin() wants, so we end up with something like: > > > > __le16 foo = cpu_to_le16(key); > > u8 result; > > > > ret = hex2bin(&result, (char *)&foo, 1); > > if (ret < 0) > > return ret; > > > > return result; > > > > This to me looks like yucky code, It still results in "foo" having to > > be on the stack, because the out-of-line hex2bin() requires a pointer > > to be passed as the second argument. > > > > Maybe you could provide an example of what you're thinking of, because > > I'm at a loss to understand what you're thinking this should look like. > > So, let's look into the real callers to see, oh wait, it's a single caller! > Why can't you simply do > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > if (ret < 0) > return ret; > > in-place there? This is not legal C. Please can we back up this discussion, and start over with legal C suggestions. Thanks.
> 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.
On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > > <linux@armlinux.org.uk> wrote: > > > > > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: > > > > > > > +static int macsmc_gpio_nr(smc_key key) > > > > > > > +{ > > > > > > > + int low = hex_to_bin(key & 0xff); > > > > > > > + int high = hex_to_bin((key >> 8) & 0xff); > > > > > > > + > > > > > > > + if (low < 0 || high < 0) > > > > > > > + return -1; > > > > > > > + > > > > > > > + return low | (high << 4); > > > > > > > +} > > > > > > > > > > > > NIH hex2bin(). > > > > > > > > > > Is using hex2bin really better? > > > > > > > > Yes. > > > > > > > > > static int macsmc_gpio_nr(smc_key key) > > > > > { > > > > > char k[2]; > > > > > u8 result; > > > > > int ret; > > > > > > > > > > k[0] = key; > > > > > k[1] = key >> 8; > > > > > > > > > > ret = hex2bin(&result, k, 2); > > > > > if (ret < 0) > > > > > return ret; > > > > > > > > > > return result; > > > > > } > > > > > > > > > > This looks to me like it consumes more CPU cycles - because we have to > > > > > write each "character" to the stack, then call a function, only to then > > > > > call the hex_to_bin() function. One can't just pass "key" into hex2bin > > > > > because that will bring with it endian issues. > > > > > > > > With one detail missed, why do you need all that if you can use > > > > byteorder helpers()? What's the stack? Just replace this entire > > > > function with the respectful calls to hex2bin(). > > > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > > make sense to me. The byteorder helpers do not give a char array, which > > > is what hex2bin() wants, so we end up with something like: > > > > > > __le16 foo = cpu_to_le16(key); > > > u8 result; > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > if (ret < 0) > > > return ret; > > > > > > return result; > > > > > > This to me looks like yucky code, It still results in "foo" having to > > > be on the stack, because the out-of-line hex2bin() requires a pointer > > > to be passed as the second argument. > > > > > > Maybe you could provide an example of what you're thinking of, because > > > I'm at a loss to understand what you're thinking this should look like. > > > > So, let's look into the real callers to see, oh wait, it's a single caller! > > Why can't you simply do > > > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > > if (ret < 0) > > return ret; > > > > in-place there? > > This is not legal C. I acknowledged this, sorry. > Please can we back up this discussion, and start > over with legal C suggestions. Thanks. Suggestion was given as well, let's create a helper used by apple stuff and later on we will consider the separate submission for the (new) specifier. Would it work for you?
> 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 05:53:25PM +0300, Andy Shevchenko wrote: > On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > > > <linux@armlinux.org.uk> wrote: > > > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > > > <linux@armlinux.org.uk> wrote: > > > > > > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: > > > > > > > > +static int macsmc_gpio_nr(smc_key key) > > > > > > > > +{ > > > > > > > > + int low = hex_to_bin(key & 0xff); > > > > > > > > + int high = hex_to_bin((key >> 8) & 0xff); > > > > > > > > + > > > > > > > > + if (low < 0 || high < 0) > > > > > > > > + return -1; > > > > > > > > + > > > > > > > > + return low | (high << 4); > > > > > > > > +} > > > > > > > > > > > > > > NIH hex2bin(). > > > > > > > > > > > > Is using hex2bin really better? > > > > > > > > > > Yes. > > > > > > > > > > > static int macsmc_gpio_nr(smc_key key) > > > > > > { > > > > > > char k[2]; > > > > > > u8 result; > > > > > > int ret; > > > > > > > > > > > > k[0] = key; > > > > > > k[1] = key >> 8; > > > > > > > > > > > > ret = hex2bin(&result, k, 2); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > > > > > > > return result; > > > > > > } > > > > > > > > > > > > This looks to me like it consumes more CPU cycles - because we have to > > > > > > write each "character" to the stack, then call a function, only to then > > > > > > call the hex_to_bin() function. One can't just pass "key" into hex2bin > > > > > > because that will bring with it endian issues. > > > > > > > > > > With one detail missed, why do you need all that if you can use > > > > > byteorder helpers()? What's the stack? Just replace this entire > > > > > function with the respectful calls to hex2bin(). > > > > > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > > > make sense to me. The byteorder helpers do not give a char array, which > > > > is what hex2bin() wants, so we end up with something like: > > > > > > > > __le16 foo = cpu_to_le16(key); > > > > u8 result; > > > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > > if (ret < 0) > > > > return ret; > > > > > > > > return result; > > > > > > > > This to me looks like yucky code, It still results in "foo" having to > > > > be on the stack, because the out-of-line hex2bin() requires a pointer > > > > to be passed as the second argument. > > > > > > > > Maybe you could provide an example of what you're thinking of, because > > > > I'm at a loss to understand what you're thinking this should look like. > > > > > > So, let's look into the real callers to see, oh wait, it's a single caller! > > > Why can't you simply do > > > > > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > > > if (ret < 0) > > > return ret; > > > > > > in-place there? > > > > This is not legal C. > > I acknowledged this, sorry. > > > Please can we back up this discussion, and start > > over with legal C suggestions. Thanks. > > Suggestion was given as well, let's create a helper used by apple > stuff and later on we will consider the separate submission for the > (new) specifier. Would it work for you? This sub-thread isn't about the %p4ch specifier. It's about a reasonable implementation of macsmc_gpio_nr(). Extracting from the context above, the original code was: static int macsmc_gpio_nr(smc_key key) { int low = hex_to_bin(key & 0xff); int high = hex_to_bin((key >> 8) & 0xff); if (low < 0 || high < 0) return -1; return low | (high << 4); } I suggested: static int macsmc_gpio_nr(smc_key key) { char k[2]; u8 result; int ret; k[0] = key; k[1] = key >> 8; ret = hex2bin(&result, k, 2); if (ret < 0) return ret; return result; } You didn't like that, so I then suggested: static int macsmc_gpio_nr(smc_key key) { __le16 foo = cpu_to_le16(key); u8 result; int ret; ret = hex2bin(&result, (char *)&foo, 1); if (ret < 0) return ret; return result; } which you also didn't like, and then you suggested something that isn't legal C. So, I then asked you to backup this discussion... As I've made a number of suggestions, and you've essentially rejected them all, I still need to know what you would find acceptable for this, because I'm out of ideas. (I haven't bothered to check whether my last suggestion even works - I am hoping to find out what general style of code you would accept here.
On Fri, Sep 2, 2022 at 6:34 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Sep 02, 2022 at 05:53:25PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > > > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > > > > <linux@armlinux.org.uk> wrote: > > > > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > > > > <linux@armlinux.org.uk> wrote: > > > > > > > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: > > > > > > > > > +static int macsmc_gpio_nr(smc_key key) > > > > > > > > > +{ > > > > > > > > > + int low = hex_to_bin(key & 0xff); > > > > > > > > > + int high = hex_to_bin((key >> 8) & 0xff); > > > > > > > > > + > > > > > > > > > + if (low < 0 || high < 0) > > > > > > > > > + return -1; > > > > > > > > > + > > > > > > > > > + return low | (high << 4); > > > > > > > > > +} > > > > > > > > > > > > > > > > NIH hex2bin(). > > > > > > > > > > > > > > Is using hex2bin really better? > > > > > > > > > > > > Yes. > > > > > > > > > > > > > static int macsmc_gpio_nr(smc_key key) > > > > > > > { > > > > > > > char k[2]; > > > > > > > u8 result; > > > > > > > int ret; > > > > > > > > > > > > > > k[0] = key; > > > > > > > k[1] = key >> 8; > > > > > > > > > > > > > > ret = hex2bin(&result, k, 2); > > > > > > > if (ret < 0) > > > > > > > return ret; > > > > > > > > > > > > > > return result; > > > > > > > } > > > > > > > > > > > > > > This looks to me like it consumes more CPU cycles - because we have to > > > > > > > write each "character" to the stack, then call a function, only to then > > > > > > > call the hex_to_bin() function. One can't just pass "key" into hex2bin > > > > > > > because that will bring with it endian issues. > > > > > > > > > > > > With one detail missed, why do you need all that if you can use > > > > > > byteorder helpers()? What's the stack? Just replace this entire > > > > > > function with the respectful calls to hex2bin(). > > > > > > > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > > > > make sense to me. The byteorder helpers do not give a char array, which > > > > > is what hex2bin() wants, so we end up with something like: > > > > > > > > > > __le16 foo = cpu_to_le16(key); > > > > > u8 result; > > > > > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > > > if (ret < 0) > > > > > return ret; > > > > > > > > > > return result; > > > > > > > > > > This to me looks like yucky code, It still results in "foo" having to > > > > > be on the stack, because the out-of-line hex2bin() requires a pointer > > > > > to be passed as the second argument. > > > > > > > > > > Maybe you could provide an example of what you're thinking of, because > > > > > I'm at a loss to understand what you're thinking this should look like. > > > > > > > > So, let's look into the real callers to see, oh wait, it's a single caller! > > > > Why can't you simply do > > > > > > > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > > > > if (ret < 0) > > > > return ret; > > > > > > > > in-place there? > > > > > > This is not legal C. > > > > I acknowledged this, sorry. > > > > > Please can we back up this discussion, and start > > > over with legal C suggestions. Thanks. > > > > Suggestion was given as well, let's create a helper used by apple > > stuff and later on we will consider the separate submission for the > > (new) specifier. Would it work for you? > > This sub-thread isn't about the %p4ch specifier. It's about a > reasonable implementation of macsmc_gpio_nr(). > > Extracting from the context above, the original code was: > > static int macsmc_gpio_nr(smc_key key) > { > int low = hex_to_bin(key & 0xff); > int high = hex_to_bin((key >> 8) & 0xff); > > if (low < 0 || high < 0) > return -1; > > return low | (high << 4); > } > > I suggested: > > static int macsmc_gpio_nr(smc_key key) > { > char k[2]; > u8 result; > int ret; > > k[0] = key; > k[1] = key >> 8; > > ret = hex2bin(&result, k, 2); > if (ret < 0) > return ret; > > return result; > } > > You didn't like that, so I then suggested: > > static int macsmc_gpio_nr(smc_key key) > { > __le16 foo = cpu_to_le16(key); > u8 result; > int ret; > > ret = hex2bin(&result, (char *)&foo, 1); > if (ret < 0) > return ret; > > return result; > } > > which you also didn't like, ...based on the wrong suggestion below. That said, the above is fine to me. > and then you suggested something that isn't > legal C. So, I then asked you to backup this discussion... > > As I've made a number of suggestions, and you've essentially rejected > them all, I still need to know what you would find acceptable for this, > because I'm out of ideas.
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 06/09/2022 04.10, Linus Walleij wrote: > On Mon, Sep 5, 2022 at 6:13 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > >> I suggest that I try resubmitting the series with IRQ support dropped, >> and with the %p4ch support in it and we'll see what happens. > > You can add my > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > on the result, and the code should go in when Hector & you are > happy with it. I surely trust you to fix the final polish. > > I don't mind the IRQ patch either, but I understand it's a bit > annoying if you can't test it on anything. Thank you! FWIW, I wrote that commit as part of working on PCIe power saving and the SD card reader specifically, where the SMC GPIO card detect pin can replace the internal card detect, and this allows us to completely power down the SD card reader when there is no card inserted. But that PCIe power management code isn't anywhere near done yet, I just tested that using the SMC card detect IRQ works as expected with sdhci-pci. So it's perfectly fine to hold off on that patch until later, and submit it once we've actually been using it downstream for a bit and it gets some testing. - Hector
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 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 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
> 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 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