From: Arnd Bergmann <arnd@arndb.de> To: Linus Walleij <linus.walleij@linaro.org> Cc: Arnd Bergmann <arnd@arndb.de>, "Hawkins, Nick" <nick.hawkins@hpe.com>, "Verdun, Jean-Marie" <verdun@hpe.com>, Joel Stanley <joel@jms.id.au>, OpenBMC Maillist <openbmc@lists.ozlabs.org>, Daniel Lezcano <daniel.lezcano@linaro.org>, Thomas Gleixner <tglx@linutronix.de>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v5 04/11] clocksource/drivers: Add HPE GXP timer Date: Tue, 26 Apr 2022 08:00:20 +0200 [thread overview] Message-ID: <CAK8P3a2ie8kFYstCYr6FO6+yFw7VxyJjWYyy9b+rUHu_u0YXPg@mail.gmail.com> (raw) In-Reply-To: <CACRpkdbJ1N7VOgoBzGS+cOaErFcYiOVDeWRmhPyHA7=9W0CGhA@mail.gmail.com> On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Apr 22, 2022 at 3:16 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Apr 21, 2022 at 9:21 PM <nick.hawkins@hpe.com> wrote: > > > > > + > > > +static struct platform_device gxp_watchdog_device = { > > > + .name = "gxp-wdt", > > > + .id = -1, > > > +}; > > > +/* > > > + * This probe gets called after the timer is already up and running. This will create > > > + * the watchdog device as a child since the registers are shared. > > > + */ > > > + > > > +static int gxp_timer_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + > > > + /* Pass the base address (counter) as platform data and nothing else */ > > > + gxp_watchdog_device.dev.platform_data = local_gxp_timer->counter; > > > + gxp_watchdog_device.dev.parent = dev; > > > + return platform_device_register(&gxp_watchdog_device); > > > +} > > > > I don't understand what this is about: the device should be created from > > DT, not defined statically in the code. There are multiple ways of creating > > a platform_device from a DT node, or you can allocate one here, but static > > definitions are generally a mistake. > > > > I see that you copied this from the ixp4xx driver, so I think we should fix this > > there as well. > > The ixp4xx driver looks like that because the register range used for > the timer and the watchdog is combined, i.e. it is a single IP block: > > timer@c8005000 { > compatible = "intel,ixp4xx-timer"; > reg = <0xc8005000 0x100>; > interrupts = <5 IRQ_TYPE_LEVEL_HIGH>; > }; > > Device tree probing does not allow two devices to probe from the same > DT node, so this was solved by letting the (less important) watchdog > be spawn as a platform device from the timer. > > I don't know if double-probing for the same register range can be fixed, > but I was assuming that the one-compatible-to-one-driver assumption > was pretty hard-coded into the abstractions. Maybe it isn't? Having a child device is fine, my objection was about the way the device is created from a 'static platform_device ...' definition rather than having the device structure allocated at probe time. > Another way is of course to introduce an MFD. That becomes > problematic in another way: MFD abstractions are supposed to > be inbetween the resource and the devices it spawns, and with > timers/clocksources this creates a horrible special-casing since the > MFD bus (the parent may be providing e.g. an MMIO regmap) > then need to be early-populated and searched by the timer core > from TIMER_OF_DECLARE() early in boot. > > So this solution was the lesser evil that I could think about. There are multiple ways of doing this that we already discussed in the thread. The easiest is probably to have a child node without custom registers in the DT and then use the DT helpers to populate the linux devices with the correct data. Arnd
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de> To: Linus Walleij <linus.walleij@linaro.org> Cc: Arnd Bergmann <arnd@arndb.de>, OpenBMC Maillist <openbmc@lists.ozlabs.org>, Daniel Lezcano <daniel.lezcano@linaro.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Joel Stanley <joel@jms.id.au>, "Hawkins, Nick" <nick.hawkins@hpe.com>, Thomas Gleixner <tglx@linutronix.de>, "Verdun, Jean-Marie" <verdun@hpe.com> Subject: Re: [PATCH v5 04/11] clocksource/drivers: Add HPE GXP timer Date: Tue, 26 Apr 2022 08:00:20 +0200 [thread overview] Message-ID: <CAK8P3a2ie8kFYstCYr6FO6+yFw7VxyJjWYyy9b+rUHu_u0YXPg@mail.gmail.com> (raw) In-Reply-To: <CACRpkdbJ1N7VOgoBzGS+cOaErFcYiOVDeWRmhPyHA7=9W0CGhA@mail.gmail.com> On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Apr 22, 2022 at 3:16 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Apr 21, 2022 at 9:21 PM <nick.hawkins@hpe.com> wrote: > > > > > + > > > +static struct platform_device gxp_watchdog_device = { > > > + .name = "gxp-wdt", > > > + .id = -1, > > > +}; > > > +/* > > > + * This probe gets called after the timer is already up and running. This will create > > > + * the watchdog device as a child since the registers are shared. > > > + */ > > > + > > > +static int gxp_timer_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + > > > + /* Pass the base address (counter) as platform data and nothing else */ > > > + gxp_watchdog_device.dev.platform_data = local_gxp_timer->counter; > > > + gxp_watchdog_device.dev.parent = dev; > > > + return platform_device_register(&gxp_watchdog_device); > > > +} > > > > I don't understand what this is about: the device should be created from > > DT, not defined statically in the code. There are multiple ways of creating > > a platform_device from a DT node, or you can allocate one here, but static > > definitions are generally a mistake. > > > > I see that you copied this from the ixp4xx driver, so I think we should fix this > > there as well. > > The ixp4xx driver looks like that because the register range used for > the timer and the watchdog is combined, i.e. it is a single IP block: > > timer@c8005000 { > compatible = "intel,ixp4xx-timer"; > reg = <0xc8005000 0x100>; > interrupts = <5 IRQ_TYPE_LEVEL_HIGH>; > }; > > Device tree probing does not allow two devices to probe from the same > DT node, so this was solved by letting the (less important) watchdog > be spawn as a platform device from the timer. > > I don't know if double-probing for the same register range can be fixed, > but I was assuming that the one-compatible-to-one-driver assumption > was pretty hard-coded into the abstractions. Maybe it isn't? Having a child device is fine, my objection was about the way the device is created from a 'static platform_device ...' definition rather than having the device structure allocated at probe time. > Another way is of course to introduce an MFD. That becomes > problematic in another way: MFD abstractions are supposed to > be inbetween the resource and the devices it spawns, and with > timers/clocksources this creates a horrible special-casing since the > MFD bus (the parent may be providing e.g. an MMIO regmap) > then need to be early-populated and searched by the timer core > from TIMER_OF_DECLARE() early in boot. > > So this solution was the lesser evil that I could think about. There are multiple ways of doing this that we already discussed in the thread. The easiest is probably to have a child node without custom registers in the DT and then use the DT helpers to populate the linux devices with the correct data. Arnd
next prev parent reply other threads:[~2022-04-26 6:00 UTC|newest] Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-21 19:21 [PATCH v5 01/11] aach: arm: mach-hpe: Introduce the HPE GXP architecture nick.hawkins 2022-04-21 19:21 ` nick.hawkins 2022-04-21 19:21 ` [PATCH v5 01/11] archh: " nick.hawkins 2022-04-21 19:21 ` nick.hawkins 2022-04-22 13:20 ` Arnd Bergmann 2022-04-22 13:20 ` Arnd Bergmann 2022-04-22 13:20 ` Arnd Bergmann 2022-04-21 19:21 ` [PATCH v5 02/11] arch: arm: configs: multi_v7_defconfig nick.hawkins 2022-04-21 19:21 ` nick.hawkins 2022-04-23 11:06 ` Krzysztof Kozlowski 2022-04-23 11:06 ` Krzysztof Kozlowski 2022-04-29 20:34 ` Hawkins, Nick 2022-04-29 20:34 ` Hawkins, Nick 2022-04-30 11:40 ` Krzysztof Kozlowski 2022-04-30 11:40 ` Krzysztof Kozlowski 2022-04-23 11:08 ` Krzysztof Kozlowski 2022-04-23 11:08 ` Krzysztof Kozlowski 2022-04-21 19:21 ` [PATCH v5 03/11] drivers: wdt: Introduce HPE GXP SoC Watchdog nick.hawkins 2022-04-21 19:21 ` [PATCH v5 04/11] clocksource/drivers: Add HPE GXP timer nick.hawkins 2022-04-22 13:16 ` Arnd Bergmann 2022-04-22 13:16 ` Arnd Bergmann 2022-04-25 20:38 ` Linus Walleij 2022-04-25 20:38 ` Linus Walleij 2022-04-25 21:05 ` Jonathan Neuschäfer 2022-04-26 6:00 ` Arnd Bergmann [this message] 2022-04-26 6:00 ` Arnd Bergmann 2022-04-26 21:38 ` Rob Herring 2022-04-26 21:38 ` Rob Herring 2022-04-26 21:55 ` Arnd Bergmann 2022-04-26 21:55 ` Arnd Bergmann 2022-04-26 22:04 ` Rob Herring 2022-04-26 22:04 ` Rob Herring 2022-04-26 22:26 ` Arnd Bergmann 2022-04-26 22:26 ` Arnd Bergmann 2022-04-22 14:56 ` Thomas Gleixner 2022-04-21 19:21 ` [PATCH v5 05/11] dt-bindings: timer: Add HPE GXP Timer Binding nick.hawkins 2022-04-23 10:50 ` Krzysztof Kozlowski 2022-04-21 19:21 ` [PATCH v5 06/11] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding nick.hawkins 2022-04-23 10:52 ` Krzysztof Kozlowski 2022-04-25 22:04 ` Rob Herring 2022-04-25 22:04 ` Rob Herring 2022-04-26 13:21 ` Hawkins, Nick 2022-04-26 13:34 ` Krzysztof Kozlowski 2022-04-26 13:34 ` Krzysztof Kozlowski 2022-04-26 13:52 ` Hawkins, Nick 2022-04-26 15:44 ` Krzysztof Kozlowski 2022-04-26 15:44 ` Krzysztof Kozlowski 2022-04-21 19:21 ` [PATCH v5 07/11] dt-bindings: arm: Add HPE GXP Binding nick.hawkins 2022-04-23 10:58 ` Krzysztof Kozlowski 2022-04-21 19:21 ` [PATCH v5 08/11] dt-bindings: usb: generic-ehci: Add HPE GXP ehci binding nick.hawkins 2022-04-23 10:52 ` Krzysztof Kozlowski 2022-04-21 19:21 ` [PATCH v5 09/11] dt-bindings: usb: generic-ohci: Add HPE GXP ohci binding nick.hawkins 2022-04-23 10:53 ` Krzysztof Kozlowski 2022-04-21 19:21 ` [PATCH v5 10/11] arch: arm: boot: dts: Introduce HPE GXP Device tree nick.hawkins 2022-04-21 19:21 ` nick.hawkins 2022-04-21 19:21 ` nick.hawkins 2022-04-22 13:06 ` Arnd Bergmann 2022-04-22 13:06 ` Arnd Bergmann 2022-04-22 13:06 ` Arnd Bergmann 2022-04-23 11:01 ` Krzysztof Kozlowski 2022-04-23 11:01 ` Krzysztof Kozlowski 2022-04-26 7:22 ` Krzysztof Kozlowski 2022-04-26 7:22 ` Krzysztof Kozlowski 2022-04-26 7:22 ` Krzysztof Kozlowski 2022-04-21 19:21 ` [PATCH v5 11/11] maintainers: Introduce HPE GXP Architecture nick.hawkins 2022-04-23 11:04 ` [PATCH v5 01/11] aach: arm: mach-hpe: Introduce the HPE GXP architecture Krzysztof Kozlowski 2022-04-23 11:04 ` Krzysztof Kozlowski 2022-04-25 15:00 ` Hawkins, Nick 2022-04-25 15:00 ` Hawkins, Nick 2022-04-26 8:25 ` Paul Menzel 2022-04-26 8:25 ` Paul Menzel 2022-04-26 8:25 ` Paul Menzel 2022-04-26 17:28 ` Hawkins, Nick 2022-04-26 17:28 ` Hawkins, Nick 2022-04-26 17:50 ` Paul Menzel 2022-04-26 17:50 ` Paul Menzel 2022-04-26 17:50 ` Paul Menzel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAK8P3a2ie8kFYstCYr6FO6+yFw7VxyJjWYyy9b+rUHu_u0YXPg@mail.gmail.com \ --to=arnd@arndb.de \ --cc=daniel.lezcano@linaro.org \ --cc=joel@jms.id.au \ --cc=linus.walleij@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=nick.hawkins@hpe.com \ --cc=openbmc@lists.ozlabs.org \ --cc=tglx@linutronix.de \ --cc=verdun@hpe.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.