Message ID | 20230424123522.18302-1-nikita.shubin@maquefel.me |
---|---|
Headers | show |
Series | ep93xx device tree conversion | expand |
On Mon, Apr 24, 2023, at 14:34, Nikita Shubin wrote: > This series aims to convert ep93xx from platform to full device tree support. > > Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302. > > Thank you Linus and Arnd for your support, review and comments, sorry > if i missed something - > these series are quite big for me. > > Big thanks to Alexander Sverdlin for his testing, support, review, > fixes and patches. Thanks a lot for your continued work. I can't merge any of this at the moment since the upstream merge window just opened, but I'm happy to take this all through the soc tree for 6.5, provided we get the sufficient Acks from the subsystem maintainers. Merging it through each individual tree would take a lot longer, so I hope we can avoid that. Arnd
On Mon, 24 Apr 2023 15:34:19 +0300, Nikita Shubin wrote: > Add YAML bindings ep93xx SoC. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > > Notes: > rfc->v0: > - dropped separate bindings description, left only one with all groups, > functions and etc... > - added Alexander Sverdlin to maintainers > - added Linus Reviwed-by tags, through i shoudn't =) too many changes > - fixed warning and added seq_file header > > .../pinctrl/cirrus,ep93xx-pinctrl.yaml | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.example.dtb: /example-0/syscon@80930000: failed to match any schema with compatible: ['cirrus,ep9301-syscon', 'syscon', 'simple-mfd'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230424123522.18302-4-nikita.shubin@maquefel.me The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 25/04/2023 00:29, Jakub Kicinski wrote: > On Mon, 24 Apr 2023 13:31:25 +0200 Arnd Bergmann wrote: >> Thanks a lot for your continued work. I can't merge any of this at >> the moment since the upstream merge window just opened, but I'm >> happy to take this all through the soc tree for 6.5, provided we >> get the sufficient Acks from the subsystem maintainers. Merging >> it through each individual tree would take a lot longer, so I >> hope we can avoid that. > > Is there a dependency between the patches? I didn't get entire patchset and cover letter does not mention dependencies, but usually there shouldn't be such. Maybe for the next versions this should be split per subsystem? Best regards, Krzysztof
On 24/04/2023 14:34, Nikita Shubin wrote: > Add YAML bindings ep93xx SoC. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > > Notes: > rfc->v0: > - dropped separate bindings description, left only one with all groups, > functions and etc... > - added Alexander Sverdlin to maintainers > - added Linus Reviwed-by tags, through i shoudn't =) too many changes > - fixed warning and added seq_file header > > .../pinctrl/cirrus,ep93xx-pinctrl.yaml | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml > new file mode 100644 > index 000000000000..cba4be7c5994 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml > @@ -0,0 +1,66 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/cirrus,ep93xx-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus ep93xx pins mux controller > + > +maintainers: > + - Nikita Shubin <nikita.shubin@maquefel.me> > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + > +properties: > + compatible: > + enum: > + - cirrus,ep9301-pinctrl > + - cirrus,ep9307-pinctrl > + - cirrus,ep9312-pinctrl Blank line > + regmap: > + description: phandle to syscon This should be specific - vendor prefix, property name, explanation what do you need here in description. "phandle to syscon" is redundant. You also miss type. Anyway, your example contradicts this. You do not have regmap. > + > +patternProperties: > + '^pinctrl-': '^pins-' instead, because pinctrl is the entire device. > + type: object > + description: pin node > + $ref: pinmux-node.yaml# > + > + properties: > + function: > + enum: [ spi, ac97, i2s, pwm, keypad, pata, lcd, gpio1, gpio2, gpio3, > + gpio4, gpio6, gpio7 ] Why gpio has different versions? gpio should be one function. > + groups: > + minItems: 1 > + maxItems: 2 > + items: > + enum: [ ssp, ac97, i2s_on_ssp, i2s_on_ac97, pwm1, gpio1agrp, > + gpio2agrp, gpio3agrp, gpio4agrp, gpio6agrp, gpio7agrp, > + rasteronsdram0grp, rasteronsdram3grp, keypadgrp, idegrp] > + > + required: > + - function > + - groups > + > +required: > + - compatible > + - regmap > + > +additionalProperties: false > + > +examples: > + - | > + syscon: syscon@80930000 { > + compatible = "cirrus,ep9301-syscon", > + "syscon", "simple-mfd"; You created now dependency on this binding. Drop the parent node. If this is supposed to be always part of syscon, then example could be in parent's node... but I have doubts that is a part of syscon in the first place. > + reg = <0x80930000 0x1000>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + pinctrl: pinctrl { > + compatible = "cirrus,ep9312-pinctrl"; Mixed indentation. Actually before also looks odd... Use 4 spaces for example indentation. > + regmap = <&syscon>; > + spi_default_pins: pinctrl-spi { > + function = "spi"; > + groups = "ssp"; > + }; > + }; > + }; Best regards, Krzysztof
On Tue, Apr 25, 2023, at 10:20, Krzysztof Kozlowski wrote: > On 25/04/2023 00:29, Jakub Kicinski wrote: >> On Mon, 24 Apr 2023 13:31:25 +0200 Arnd Bergmann wrote: >>> Thanks a lot for your continued work. I can't merge any of this at >>> the moment since the upstream merge window just opened, but I'm >>> happy to take this all through the soc tree for 6.5, provided we >>> get the sufficient Acks from the subsystem maintainers. Merging >>> it through each individual tree would take a lot longer, so I >>> hope we can avoid that. >> >> Is there a dependency between the patches? > > I didn't get entire patchset and cover letter does not mention > dependencies, but usually there shouldn't be such. Maybe for the next > versions this should be split per subsystem? Clearly the last patch that removes the board files depends on all the previous patches, but I assume that the other ones are all independent. We don't do complete conversions from boardfiles to DT that often any more, but in the past we tended to do this through a cross- subsystem branch in the soc tree, which helps do it more quickly and is less work for Nikita. In this case, I would make it a separate top-level branch in the soc tree. If anyone strongly feels that the patches should go through the subsystem trees here, we'll take the longer path and do the changes separately, with the boardfile removal coming a release later. Arnd
On Mon, Apr 24, 2023 at 11:35 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > This series aims to convert ep93xx from platform to full device tree support. > > Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302. Neat, I'd say let's merge this for 6.5 once the final rough edges are off. The DT bindings should be easy to fix. This is a big patch set and the improvement to the ARM kernel it brings is great, so I am a bit worried about over-review stalling the merged. If there start to be nitpicky comments I would prefer that we merge it and let minor comments and "nice-to-haves" be addressed in-tree during the development cycle. I encourage you to use b4 to manage the patch series if you have time to learn it, it could help you: https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1 Yours, Linus Walleij
On Wed, Apr 26, 2023 at 11:02 PM Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 26, 2023 at 10:56:53PM +0200, Linus Walleij wrote: > > > This is a big patch set and the improvement to the ARM kernel it > > brings is great, so I am a bit worried about over-review stalling the > > merged. If there start to be nitpicky comments I would prefer that > > we merge it and let minor comments and "nice-to-haves" be > > addressed in-tree during the development cycle. > > I'm really not enthusiastic about the SPI bindings being merged as-is. Agree, the bindings are more important than the code IMO, they tend to get written in stone. Yours, Linus Walleij
On 4/24/2023 5:34 AM, Nikita Shubin wrote: > This series aims to convert ep93xx from platform to full device tree support. > > Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302. > > Thank you Linus and Arnd for your support, review and comments, sorry if i missed something - > these series are quite big for me. > > Big thanks to Alexander Sverdlin for his testing, support, review, fixes and patches. If anyone is interested I still have a TS-7300 board [1] that is fully functional and could be sent out to a new home. https://www.embeddedts.com/products/TS-7300
Hello Florian! On Mon, 2023-05-15 at 20:47 -0700, Florian Fainelli wrote: > > > On 4/24/2023 5:34 AM, Nikita Shubin wrote: > > This series aims to convert ep93xx from platform to full device > > tree support. > > > > Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302. > > > > Thank you Linus and Arnd for your support, review and comments, > > sorry if i missed something - > > these series are quite big for me. > > > > Big thanks to Alexander Sverdlin for his testing, support, review, > > fixes and patches. > > If anyone is interested I still have a TS-7300 board [1] that is > fully > functional and could be sent out to a new home. Thank you kindly, i'll keep this in mind ! > > https://www.embeddedts.com/products/TS-7300
On 01/06/2023 07:33, Nikita Shubin wrote: > Add YAML bindings for ep93xx SoC pinctrl. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > > Notes: > v0 -> v1: > > Krzysztof Kozlowski: > - removed wildcards > - use fallback compatible and list all possible compatibles > - fix ident > - dropped bindings in title > > .../pinctrl/cirrus,ep9301-pinctrl.yaml | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml > new file mode 100644 > index 000000000000..ff7b30a11bab > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml > @@ -0,0 +1,66 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/cirrus,ep9301-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus ep93xx pins mux controller > + > +maintainers: > + - Nikita Shubin <nikita.shubin@maquefel.me> > + - Alexander Sverdlin <alexander.sverdlin@gmail.com> > + > +properties: > + compatible: > + oneOf: > + - const: cirrus,ep9301-pinctrl > + - items: > + - enum: > + - cirrus,ep9302-pinctrl > + - cirrus,ep9307-pinctrl > + - cirrus,ep9312-pinctrl > + - cirrus,ep9315-pinctrl > + - const: cirrus,ep9301-pinctrl > + > +patternProperties: > + '^pins-': > + type: object > + description: pin node > + $ref: pinmux-node.yaml# > + > + properties: > + function: > + enum: [ spi, ac97, i2s, pwm, keypad, pata, lcd, gpio ] Blank line. > + groups: > + minItems: 1 > + maxItems: 2 How one pin can belong to two groups? What does it mean? > + items: > + enum: [ ssp, ac97, i2s_on_ssp, i2s_on_ac97, pwm1, gpio1agrp, > + gpio2agrp, gpio3agrp, gpio4agrp, gpio6agrp, gpio7agrp, > + rasteronsdram0grp, rasteronsdram3grp, keypadgrp, idegrp] > + > + required: > + - function > + - groups > + > +required: > + - compatible > + > +additionalProperties: false > + > +examples: > + - | > + syscon@80930000 { > + compatible = "cirrus,ep9301-syscon", > + "syscon", "simple-mfd"; Weird wrapping. > + reg = <0x80930000 0x1000>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + pinctrl { > + compatible = "cirrus,ep9312-pinctrl", "cirrus,ep9301-pinctrl"; > + spi_default_pins: pins-spi { > + function = "spi"; > + groups = "ssp"; > + }; > + }; > + }; Best regards, Krzysztof
Thu, Jun 01, 2023 at 08:33:52AM +0300, Nikita Shubin kirjoitti: > This prepares ep93xx SOC gpio to convert into device tree driver: > - dropped banks and legacy defines > - split AB IRQ and make it shared > > We are relying on IRQ number information A, B ports have single shared > IRQ, while F port have dedicated IRQ for each line. > > Also we had to split single ep93xx platform_device into multiple, one > for each port, without this we can't do a full working transition from > legacy platform code into device tree capable. All GPIO_LOOKUP were > change to match new chip namings. First of all, check if you added In-Reply-to email header to the previous thread, at least `b4` downloaded 188 messages in this one so far. Second, the previous was kinda v0, while we usually assume that non-versioned series is v1. This is a bit ambiguous. ... > + GPIO_LOOKUP_IDX("gpio-ep93xx.4", 1, NULL, 1, GPIO_ACTIVE_HIGH), TAB used instead of space. ... > struct device __init *ep93xx_init_devices(void) > { > struct device *parent; > + int i; It's unsigned, right? > + for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_device); i++) > + platform_device_register(ep93xx_gpio_device[i]); ... > writeb(eic->int_debounce, > - epg->base + eic->irq_offset + EP93XX_INT_DEBOUNCE_OFFSET); > + eic->base + EP93XX_INT_DEBOUNCE_OFFSET); Now this can be a single line. Also some other cases may be optimized. ... > + void __iomem *intr = devm_platform_ioremap_resource_byname(pdev, "intr"); It's less error prone if the assignment is split from definition and moved closer to its (first) user... > + ...here. > + if (IS_ERR(intr)) > + return PTR_ERR(intr); ... > + egc->eic = devm_kcalloc(dev, 1, > + sizeof(*egc->eic), > + GFP_KERNEL); Why kcalloc(1), is this a part that will be (slightly) modified in the next patches in the series? > + if (!egc->eic) > + return -ENOMEM; > ... > + irq = platform_get_irq(pdev, 0); No return value check? > + ret = devm_request_irq(dev, irq, > + ep93xx_ab_irq_handler, > + IRQF_SHARED, gc->label, gc); > + if (ret) { > + dev_err(dev, "error requesting IRQ : %d\n", irq); > + return ret; If it's soslely part of the ->probe() flow, you may use dev_err_probe(). > + } > > + girq->parents[0] = irq; ... > for (i = 0; i < girq->num_parents; i++) { > + irq = platform_get_irq(pdev, i); > + if (irq <= 0) == 0 is never happen case. Why? > + continue; > + > + girq->parents[i] = irq; > } > + ret = bgpio_init(gc, &pdev->dev, 1, data, NULL, NULL, dir, NULL, 0); > + if (ret) { > + dev_err(&pdev->dev, "unable to init generic GPIO\n"); > + return ret; return dev_err_probe(...); > } ... > + if (platform_irq_count(pdev) > 0) { Do you need this check? > + dev_dbg(&pdev->dev, "setting up irqs for %s\n", dev_name(&pdev->dev)); > + ret = ep93xx_setup_irqs(pdev, egc); > + if (ret) > + dev_err(&pdev->dev, "setup irqs failed for %s\n", dev_name(&pdev->dev)); If it's an error, why continuing? > + }
On Thu, Jun 1, 2023 at 10:20 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > +title: EP93xx GPIO controller > > + > > +maintainers: > > + - Linus Walleij <linus.walleij@linaro.org> > > + - Bartosz Golaszewski <brgl@bgdev.pl> > > Did you choose correct maintainers? Bartosz, Linus, do you take care > about EP93xx platform? I'm fine with it (I have a platform). Yours, Linus Walleij
On Fri, Jun 2, 2023 at 9:41 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Jun 1, 2023 at 10:20 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > +title: EP93xx GPIO controller > > > + > > > +maintainers: > > > + - Linus Walleij <linus.walleij@linaro.org> > > > + - Bartosz Golaszewski <brgl@bgdev.pl> > > > > Did you choose correct maintainers? Bartosz, Linus, do you take care > > about EP93xx platform? > > I'm fine with it (I have a platform). I don't but I'm actually not sure how DT bindings maintainership works - do GPIO bindings all fall under the GPIO jurisdiction automatically? Bart
On Tue, Jun 13, 2023 at 4:55 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Fri, Jun 2, 2023 at 9:41 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Jun 1, 2023 at 10:20 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > Did you choose correct maintainers? Bartosz, Linus, do you take care > > > about EP93xx platform? > > > > I'm fine with it (I have a platform). > > I don't but I'm actually not sure how DT bindings maintainership works > - do GPIO bindings all fall under the GPIO jurisdiction automatically? Not really, more on the people selected by the person writing the bindings, confirmed by them being merged. Traditionally, Linux as the biggest software project with the most active subsystem maintainers do the reviewing and take the responsibility for them. Technically, e.g. BSD people could appear on the devicetree mailinglist and review patches and suggest maintainership from their side, but I haven't seen them much around. Neither Apple Computer or anyone else who ought to be there but isn't. Yours, Linus Walleij
Hello Andy ! On Fri, 2023-06-02 at 04:50 +0300, andy.shevchenko@gmail.com wrote: > Thu, Jun 01, 2023 at 08:33:52AM +0300, Nikita Shubin kirjoitti: > > This prepares ep93xx SOC gpio to convert into device tree driver: > > - dropped banks and legacy defines > > - split AB IRQ and make it shared > > > > We are relying on IRQ number information A, B ports have single > > shared > > IRQ, while F port have dedicated IRQ for each line. > > > > Also we had to split single ep93xx platform_device into multiple, > > one > > for each port, without this we can't do a full working transition > > from > > legacy platform code into device tree capable. All GPIO_LOOKUP were > > change to match new chip namings. > > First of all, check if you added In-Reply-to email header to the > previous > thread, at least `b4` downloaded 188 messages in this one so far. > Second, > the previous was kinda v0, while we usually assume that non-versioned > series > is v1. This is a bit ambiguous. > > ... > > > + GPIO_LOOKUP_IDX("gpio-ep93xx.4", 1, NULL, 1, > > GPIO_ACTIVE_HIGH), > > TAB used instead of space. > > ... > > > struct device __init *ep93xx_init_devices(void) > > { > > struct device *parent; > > + int i; > > It's unsigned, right? > > > + for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_device); i++) > > + platform_device_register(ep93xx_gpio_device[i]); > > ... > > > writeb(eic->int_debounce, > > - epg->base + eic->irq_offset + > > EP93XX_INT_DEBOUNCE_OFFSET); > > + eic->base + EP93XX_INT_DEBOUNCE_OFFSET); > > Now this can be a single line. Also some other cases may be > optimized. > > ... > > > + void __iomem *intr = > > devm_platform_ioremap_resource_byname(pdev, "intr"); > > It's less error prone if the assignment is split from definition and > moved > closer to its (first) user... > > > + > > ...here. > > > + if (IS_ERR(intr)) > > + return PTR_ERR(intr); > > ... > > > + egc->eic = devm_kcalloc(dev, 1, > > + sizeof(*egc->eic), > > + GFP_KERNEL); > > Why kcalloc(1), is this a part that will be (slightly) modified in > the next > patches in the series? > > > + if (!egc->eic) > > + return -ENOMEM; > > > ... > > > + irq = platform_get_irq(pdev, 0); > > No return value check? > > > + ret = devm_request_irq(dev, irq, > > + ep93xx_ab_irq_handler, > > + IRQF_SHARED, gc->label, gc); > > + if (ret) { > > + dev_err(dev, "error requesting IRQ : %d\n", > > irq); > > + return ret; > > If it's soslely part of the ->probe() flow, you may use > dev_err_probe(). > > > + } > > > > + girq->parents[0] = irq; > > ... > > > for (i = 0; i < girq->num_parents; i++) { > > + irq = platform_get_irq(pdev, i); > > + if (irq <= 0) > > == 0 is never happen case. Why? > > > + continue; > > + > > + girq->parents[i] = irq; > > } > > > + ret = bgpio_init(gc, &pdev->dev, 1, data, NULL, NULL, dir, > > NULL, 0); > > + if (ret) { > > + dev_err(&pdev->dev, "unable to init generic > > GPIO\n"); > > + return ret; > > return dev_err_probe(...); > > > } > > ... > > > + if (platform_irq_count(pdev) > 0) { > > Do you need this check? Only A/B/F ports have irq's so we don't bother setting up for other ports. > > > + dev_dbg(&pdev->dev, "setting up irqs for %s\n", > > dev_name(&pdev->dev)); > > + ret = ep93xx_setup_irqs(pdev, egc); > > + if (ret) > > + dev_err(&pdev->dev, "setup irqs failed for > > %s\n", dev_name(&pdev->dev)); > > If it's an error, why continuing? Well - it's not fatal, we can still use gpios even without irq's but we should warn. All other comments acknowledged and fixed, thank you. > > > + } >