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 85501C433F5 for ; Mon, 25 Apr 2022 20:38:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245595AbiDYUl0 (ORCPT ); Mon, 25 Apr 2022 16:41:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54418 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235623AbiDYUlY (ORCPT ); Mon, 25 Apr 2022 16:41:24 -0400 Received: from mail-yw1-x1130.google.com (mail-yw1-x1130.google.com [IPv6:2607:f8b0:4864:20::1130]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1ADCD3A5F8 for ; Mon, 25 Apr 2022 13:38:20 -0700 (PDT) Received: by mail-yw1-x1130.google.com with SMTP id 00721157ae682-2f7ca2ce255so62365887b3.7 for ; Mon, 25 Apr 2022 13:38:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=EKOHrVYni9xvs7+L4Ks12r0VkZa5fal1q0hxrC8iDII=; b=pI5SEWzs2KSeo7OJ9BaxRVNP6bzCJc0A52cY3RfMiztkV4JnDSd8lOMVKL+/QxdhgW VQZ9Ur7YmuEVpRX2Qr9v5aoG6SzgNBVEpyquqONdmSZeK0bapSmJhDaeHxg/58e9Xyre 949ELWpDNA1uFOf3EVrO+2AZcb/7+stGJkRZWvVkHyiw8EJJv4PWhZvchdZUh4lxwxsT xz6j6jQwiXki8wfOWBuPDst5Mp2p6NTKC9jNSSMKBBiIBjeZNAzM7f5OQ0HrbMW3RdSV gr8L5rDU+mceJ9rUgYT46TYSTJr1LPcCc2FFf9vNdlGvE/t7kYZI0QvCANVEia6AvJB5 4b9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EKOHrVYni9xvs7+L4Ks12r0VkZa5fal1q0hxrC8iDII=; b=V+2/+HAysb1Zy94Pu6MCfN1ZwtngggHDPtUBTjHrBDXj16/D1DVW0bg7q4UQObEI5U sPlE5dkR+v/aREFsT7pzRAy4ZfcX903x6ssUQ+EOd9DjAnQpeuOMJebFnq+psO4XH0Or X3Mt68yMpGxFnDeKUa6bGgfjJ53AqUygETiCiWsJ4o2Die7ssLkvEQ9WzcMD1a5FkYjM gzTEaAjL0kBOyieN//d+n56QTgiMNFCxKijkecnacVSJbOcHJAdW2C8AA7mSZb4F7F9S wxJfYaA15jA7zxQSSqjmiQ1aohLIay4nm8wgYbd9++MEGNf7Ne5vM6UFxrbzgGABExiX G8kg== X-Gm-Message-State: AOAM532R97nj+OTDMWUTm3nacaHQ9YdLeNkNFTOANDyJQNXifby7czet fYkU22OnA27OnoI3swc1Wpdq4mqvMJ/VuVHl6oe4YQ== X-Google-Smtp-Source: ABdhPJx0cRFmnkcRXSnlvAH/s+i67gX4XqlpKmn/ICwjou3ZKFFzhb03fJf+2j/zJFPnDYmeW16iPtutIvSdQow5dNg= X-Received: by 2002:a81:2108:0:b0:2f5:6938:b2b8 with SMTP id h8-20020a812108000000b002f56938b2b8mr17509528ywh.151.1650919099262; Mon, 25 Apr 2022 13:38:19 -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: Linus Walleij Date: Mon, 25 Apr 2022 22:38:08 +0200 Message-ID: Subject: Re: [PATCH v5 04/11] clocksource/drivers: Add HPE GXP timer To: Arnd Bergmann Cc: "Hawkins, Nick" , "Verdun, Jean-Marie" , Joel Stanley , OpenBMC Maillist , Daniel Lezcano , Thomas Gleixner , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? 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. Yours, Linus Walleij 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 00A1EC433EF for ; Mon, 25 Apr 2022 20:39:06 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4KnH1529B0z3bmP for ; Tue, 26 Apr 2022 06:39:05 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=pI5SEWzs; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=linaro.org (client-ip=2607:f8b0:4864:20::112b; helo=mail-yw1-x112b.google.com; envelope-from=linus.walleij@linaro.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=pI5SEWzs; dkim-atps=neutral Received: from mail-yw1-x112b.google.com (mail-yw1-x112b.google.com [IPv6:2607:f8b0:4864:20::112b]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4KnH0K4mkgz2xBF for ; Tue, 26 Apr 2022 06:38:23 +1000 (AEST) Received: by mail-yw1-x112b.google.com with SMTP id 00721157ae682-2f7d621d1caso50350447b3.11 for ; Mon, 25 Apr 2022 13:38:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=EKOHrVYni9xvs7+L4Ks12r0VkZa5fal1q0hxrC8iDII=; b=pI5SEWzs2KSeo7OJ9BaxRVNP6bzCJc0A52cY3RfMiztkV4JnDSd8lOMVKL+/QxdhgW VQZ9Ur7YmuEVpRX2Qr9v5aoG6SzgNBVEpyquqONdmSZeK0bapSmJhDaeHxg/58e9Xyre 949ELWpDNA1uFOf3EVrO+2AZcb/7+stGJkRZWvVkHyiw8EJJv4PWhZvchdZUh4lxwxsT xz6j6jQwiXki8wfOWBuPDst5Mp2p6NTKC9jNSSMKBBiIBjeZNAzM7f5OQ0HrbMW3RdSV gr8L5rDU+mceJ9rUgYT46TYSTJr1LPcCc2FFf9vNdlGvE/t7kYZI0QvCANVEia6AvJB5 4b9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EKOHrVYni9xvs7+L4Ks12r0VkZa5fal1q0hxrC8iDII=; b=s2yeG8IR9nut1hLxV95qrgKi+SgKanX6Ffsei5VglpZ8k8TjQ8GWFmLzQkS4r8Sl7O hFNtSLNv+Pq9Jw3FNgO64WqhjuFGQ/IGqSFhk+D3jGv4G3kwRneU9esU6ITOq4tg52hf lMiWIG7LbTrRwH6Fm0key3YtpL3nCurrEY7RJHazdF9laHFF/Xj9A4tsTSduPteHtOV3 Hgb7HWqjUjBCXIezzY7MOBs9T3RETqdfNn2BUWKbhCU0RxmXv4x6rO6sHRCAk5mwtVg5 SW2pXYdY1ZmpTzwm/sZSk9SKNV4vKqvTFzDBPzupoB+H+mH25boHRKSeQX9OrEOUT41m ZOmQ== X-Gm-Message-State: AOAM533N6LCukJsnrRTucSIpxVJTvUukXWynE9gRbVb7vc/1y7bCHMsn Iseh/pTQnPN4NTeReEab03/7/6LlUcu146hxZThhlg== X-Google-Smtp-Source: ABdhPJx0cRFmnkcRXSnlvAH/s+i67gX4XqlpKmn/ICwjou3ZKFFzhb03fJf+2j/zJFPnDYmeW16iPtutIvSdQow5dNg= X-Received: by 2002:a81:2108:0:b0:2f5:6938:b2b8 with SMTP id h8-20020a812108000000b002f56938b2b8mr17509528ywh.151.1650919099262; Mon, 25 Apr 2022 13:38:19 -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: Linus Walleij Date: Mon, 25 Apr 2022 22:38:08 +0200 Message-ID: Subject: Re: [PATCH v5 04/11] clocksource/drivers: Add HPE GXP timer To: Arnd Bergmann Content-Type: text/plain; charset="UTF-8" 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: "Verdun, Jean-Marie" , Daniel Lezcano , Linux Kernel Mailing List , Joel Stanley , "Hawkins, Nick" , Thomas Gleixner , OpenBMC Maillist Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" 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? 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. Yours, Linus Walleij