From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E5FDBC433F5 for ; Tue, 26 Apr 2022 06:00:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241707AbiDZGDt (ORCPT ); Tue, 26 Apr 2022 02:03:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39050 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236019AbiDZGDq (ORCPT ); Tue, 26 Apr 2022 02:03:46 -0400 Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.133]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7DD461403F for ; Mon, 25 Apr 2022 23:00:39 -0700 (PDT) Received: from mail-wm1-f54.google.com ([209.85.128.54]) by mrelayeu.kundenserver.de (mreue011 [213.165.67.97]) with ESMTPSA (Nemesis) id 1MhlbM-1oMhnx2qzA-00dpqY for ; Tue, 26 Apr 2022 08:00:37 +0200 Received: by mail-wm1-f54.google.com with SMTP id a14-20020a7bc1ce000000b00393fb52a386so189630wmj.1 for ; Mon, 25 Apr 2022 23:00:37 -0700 (PDT) X-Gm-Message-State: AOAM5332PPBj6JFqHeTpTpw1AGbZ9BDpRHt9Zvo2CsOt/ukxypXO0+zI Xgs3UKa0jupYQFn8dX+foPCGfDYdEgr9KsM+DdY= X-Google-Smtp-Source: ABdhPJy3c5lcSeSluq1OFXX5XYB6LISDtFDlKiWa0Rg3d6M03Gmt+hhEqCW75iUtmXQ0VFTk/w8CPqLe8gy80hIGYlQ= X-Received: by 2002:a1c:f219:0:b0:38c:782c:3bb with SMTP id s25-20020a1cf219000000b0038c782c03bbmr28016089wmc.94.1650952837280; Mon, 25 Apr 2022 23:00:37 -0700 (PDT) MIME-Version: 1.0 References: <20220421192132.109954-1-nick.hawkins@hpe.com> <20220421192132.109954-5-nick.hawkins@hpe.com> In-Reply-To: From: Arnd Bergmann Date: Tue, 26 Apr 2022 08:00:20 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 04/11] clocksource/drivers: Add HPE GXP timer To: Linus Walleij Cc: Arnd Bergmann , "Hawkins, Nick" , "Verdun, Jean-Marie" , Joel Stanley , OpenBMC Maillist , Daniel Lezcano , Thomas Gleixner , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K1:F+NFwuLXS6I9UmDvbjVRMD/vYazHkoSCiI07zytWSzv2FW5FnNs J5xWhK4YMKsukdmB90kv9kx7ua49jPlWwVYFgmiMctVWhuYGuMqxM9PV9nt1Zi0aEXvcxWh 6ybDUNu2tkne+TT8sK4CTQFFUQmm1836hzv78LRzELDwGEcY6jnS+iUEkRfsWBKJfhW5C8X qxa8AUxgE1AsDc6rWxrMg== X-UI-Out-Filterresults: notjunk:1;V03:K0:6nR1b0EwYGw=:5cvFw57fTOrLA9Y6AofL7V A9Hlrhr78PkASx0E4550Ij0rv75xbDEckyrOvOpP3nLYHzj9LDm+u8451R57GiwcyxrxokjEX axVwEjKEdYGeH/JDm3EXT2dwZrfKHCrJ3gVgQsZPQOOEQEQ2FsfllH+keoIbzoX7afFeATfBd yBTHxQ3w70bkmDcVAGH3WlgFziNnGDxD0jsyl8w1nG1h6X6KsBZ1Cv9GstJXRb9KuwrRDx1DC +9eUmEP3SSoyLN9VUBcvFVlvSPPGmYg0gk3zk+v/bujlCYsG4/PBFCBtsXdPBNOJBTUREgsZ9 IbYv25gim4n+UcR5op3l7Y2E0kqNsQuNk3yJI5HG7PUPM5el6SQyOD4y8GYVw/R9D1bixv3Eq jAKK7rfgmAOvr/Bgj7dFbi2xvoNYfETXcd2vVKil/1CMkegbq6oJYHJWFb3IZ8E6DUfx+GsLx 0Hr3hn1be5wS3bLISCDoc9yWdrOj2O5yvKeULsQe5YZssuIYARSNmu34hwPoCbj2xEHjzbxU0 2zSbZAPfPYcY4WkTKQKPUtuo3rnQUxjpRZJ+NE77XppqLg7ByCDRi/s2oq4d+tll7dniaK24n eLLDm9Crg5ZYemwpxRuWVuT2QwZGtgWZBAWO9iz5xPbS7BCN+2FmHEU2M1LZF+WAayJYwo5xF C1MQ7kNZzNw8xbsJ8mwUyQnbO3vVbX0c8hc0MkdS9Xo3SdY3CkApCP1RAWsDMCWq7tJk= Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij wrote: > On Fri, Apr 22, 2022 at 3:16 PM Arnd Bergmann wrote: > > On Thu, Apr 21, 2022 at 9:21 PM 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 58367C433F5 for ; Tue, 26 Apr 2022 06:06:25 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4KnWbg5mJsz3bc8 for ; Tue, 26 Apr 2022 16:06:23 +1000 (AEST) Authentication-Results: lists.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=arndb.de (client-ip=212.227.126.133; helo=mout.kundenserver.de; envelope-from=arnd@arndb.de; receiver=) X-Greylist: delayed 308 seconds by postgrey-1.36 at boromir; Tue, 26 Apr 2022 16:05:53 AEST Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4KnWb50YL7z2xmV for ; Tue, 26 Apr 2022 16:05:52 +1000 (AEST) Received: from mail-wm1-f51.google.com ([209.85.128.51]) by mrelayeu.kundenserver.de (mreue011 [213.165.67.97]) with ESMTPSA (Nemesis) id 1MLzSD-1nRQPv1r61-00Ht8W for ; Tue, 26 Apr 2022 08:00:38 +0200 Received: by mail-wm1-f51.google.com with SMTP id l62-20020a1c2541000000b0038e4570af2fso820813wml.5 for ; Mon, 25 Apr 2022 23:00:37 -0700 (PDT) X-Gm-Message-State: AOAM533afRg8bSuo4COZYgdZIH03ElX44eEcQnEvs3P0E811uaPuOrQq ZT7lpirqF6mpzh9sioevowMFiXSx8Pxesb+OfTY= X-Google-Smtp-Source: ABdhPJy3c5lcSeSluq1OFXX5XYB6LISDtFDlKiWa0Rg3d6M03Gmt+hhEqCW75iUtmXQ0VFTk/w8CPqLe8gy80hIGYlQ= X-Received: by 2002:a1c:f219:0:b0:38c:782c:3bb with SMTP id s25-20020a1cf219000000b0038c782c03bbmr28016089wmc.94.1650952837280; Mon, 25 Apr 2022 23:00:37 -0700 (PDT) MIME-Version: 1.0 References: <20220421192132.109954-1-nick.hawkins@hpe.com> <20220421192132.109954-5-nick.hawkins@hpe.com> In-Reply-To: From: Arnd Bergmann Date: Tue, 26 Apr 2022 08:00:20 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 04/11] clocksource/drivers: Add HPE GXP timer To: Linus Walleij Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K1:0Xp2b+ydEZjipneBlHsBb/iwRFrpjaBxqKGqXQYnfDCrFM1Aeck bhYwdsxvZ9a7Ag9/ZmxzSKDBd1ENUUSkCAVP9FbgcJLjfrzh+Pk94HUbk3odrK1UMmS3RkU aI9yb2kcZdC/vxgYnorl56iY1WodrnzdsfJpfFtdN6u8wkksLDdl4E1v9o1ZVSNZXv4iC4P 8x4llVYQzyoBEg7diYM7g== X-UI-Out-Filterresults: notjunk:1;V03:K0:Ll27IeIfWlo=:keMzcZrXM8omnwwV1rbwQt wimf1yhf4uEIJRe7AkwSSt6LxQMgvViWJFKgUDeS2FbD+RvWYmvy844+UEtX5JTNAbm6YADoL qt5yM7WjbEILK1Woetu62UtuVgDG7xBL5L+068vzXkVviJqcf56s13xNUhp7RtWPhgRJAAG8E lHgsdZqIWvjorM0Y7QPoT38c6djkhlEu5CSgusseHyPECa2qmDuCovfn0UFFV6tgJnhIKiykM XWiD6WldJq/OrGT6Rq2x286AfTwvZshVPEka0993ittAMYedYyjq3ceKzFVaRKugoWkko75S1 IBt0Jf3xkaAoT3RJm7RljM23LLuC2ltkuXX7PrtXUGk9PFX1mjyf9fowmCE3baeGECTYuBikK 6RJzX4HJHrEJ5kYnM76NTpBYGYpFlxdtQ18QY37jpgWx//pIf3H0mj/TH2CERa6G9Gk5zPnCR fnJ1Ohyw6YjkfCeztpLSYL1jVEBqMJUR0ERNrKudUmp2ZWzA+Z48b/i05alH6jY/9A23vqp9B eBsGSQIYKsuJT96EDCeyWsHz+44pI2zfox0TVjltCrjUCFHaZTYQn18BiMlHWbezj19hZu/gs VvC1LnvaUJd6De8L4LyOh9I5kEAKAZF7FAHMjT5dxl6rm7YPGiRm5xLh/sFY3D6oJE9pzdasU ZwKY6aLbTtiLq9egjBX2z+oBBOjOtwNQI+DqQgsUEuv2AtfVdIXDuIO7L4MQWhHhuAJM= X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Arnd Bergmann , OpenBMC Maillist , Daniel Lezcano , Linux Kernel Mailing List , Joel Stanley , "Hawkins, Nick" , Thomas Gleixner , "Verdun, Jean-Marie" Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij wrote: > On Fri, Apr 22, 2022 at 3:16 PM Arnd Bergmann wrote: > > On Thu, Apr 21, 2022 at 9:21 PM 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