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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 73E1BCD342A for ; Tue, 3 Sep 2024 15:17:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Cc:List-Subscribe: List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Type:MIME-Version:References:Message-ID:Subject:To:Date:From:Reply-To :Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=OTB7wsQhvCFsxyJgB8CQ5Pa/NBo7hDozebUo452UiWE=; b=LTDBcD0OwXbvbSYRTjG0PixZaC QYjkdAiotUJB9hpNzRHb6jRZUSp1T4cOhynff0u/rapsKvKLhyy6YuAhgx4PCC0iT/thG2KweNyr+ 3myUaon627XM/1und0LyH3+/N4esQWVZeYL8iHT+HAdXbCZapHlR9vkYriBjdxxGIKN5vlBGErtlL sA6pkKTt1haYHfvv/4WYmadj3wkeCJ+x5uRU4UqC7iovLU7JbMDTm2HaaC2Ww3aGFIEeszkf693qB LnbAZCjjpoxOnfZqPt3ZT47BdsLhnwYqDckPuA2FGA4p54BnBnWH4SivDAC+91Yf5unsABvLxUzsM OzE396lQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1slVGx-00000000mqL-0MN2; Tue, 03 Sep 2024 15:16:47 +0000 Received: from mail-ed1-x543.google.com ([2a00:1450:4864:20::543]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1slVFh-00000000mNy-365J for linux-arm-kernel@lists.infradead.org; Tue, 03 Sep 2024 15:15:31 +0000 Received: by mail-ed1-x543.google.com with SMTP id 4fb4d7f45d1cf-5c23f0a9699so3448347a12.1 for ; Tue, 03 Sep 2024 08:15:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1725376527; x=1725981327; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:from:to:cc:subject:date:message-id:reply-to; bh=OTB7wsQhvCFsxyJgB8CQ5Pa/NBo7hDozebUo452UiWE=; b=Izi5mjlaQVSGiUE/y+eenb1irFZH60bnghcbtokb/n3zB0XZMp5WznETM13s4hf0fI wk5UrpmBUfTo9Gzv7tb4ynKYxBArq+BjNj+TIJhl8l310McyXuNGLUgMqrw8ojYhacr8 3Ejhik6KPcb+0EGESkP4rX932usevu2TnSAxPmNx4sWFxeeBjROXWmbS8DiNHy8Vov2J jzLm6kwAdz7D0SGeshfuBeMgFf7/mvcvnaoJaAiT0doptBqmhheABBHTKTG6Tg3ennFs 9ZI1Y4s0Y3gLA1pmF5w8rYm8IKq7LAT3RBctUEAAK1696TS+ITiwUedeQ4q4ocIgg74P PSVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725376527; x=1725981327; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=OTB7wsQhvCFsxyJgB8CQ5Pa/NBo7hDozebUo452UiWE=; b=mtzDNtV6vPQwOOWahAdDl9O5Xhn/v22iS1muHsTxNm0gG7PPnpUXwArxQCYY3s6ezN zrCQ+rloN2OmKIT1dj6JT6/Ru8nzosLpTbJUnolMbYRJVskDBhJUnsdzTjY1XnKa2K6S qgoAgJrUKA1ARLe6fQoYMT53ADoXwe0FmCNb/3zAKbjEhYnWZaUM/ztKnDrS6ZzjuHMY 9N2VFdyuz6gBNsadDyTLTNAh7o++0gMtInCRH6EhYElZhGwGP6RtCiIjXnvi14RDMFyE ARA7WbDsnkks7CB3tLfRnYcQtOo+jq/qkMyklC/VOrCZMdoVqKzCvtwql8o06iH226ms nIJQ== X-Forwarded-Encrypted: i=1; AJvYcCW4uLPinqvfOeQiBlDB3lop6QKkWGWQtUAGjOcd4WoWrfQVpOkoIkzPCoY826q4B5ZJoDwjCmv+Gm5AK3CnMoD8@lists.infradead.org X-Gm-Message-State: AOJu0Yx1yetdMnZWOdYhIgBIWVfOhdK74zwZAnyPBuTT84J1rj8hfmeg 9m1X3JsSgB5Yth5ZyTEW01PfJo27GezwR1COmXC8d7g3ZyN9SAEw06lQJhMpAJY= X-Google-Smtp-Source: AGHT+IFcTte6BIp8pPXY+ojLOKAWveu1EM1jhlSY6FJHdjViAJCeXH8Z3blw4a5BrzCnpyEjcGVtwQ== X-Received: by 2002:a05:6402:3506:b0:5c2:6e5f:3bf9 with SMTP id 4fb4d7f45d1cf-5c26e5f3d09mr1609966a12.28.1725376526784; Tue, 03 Sep 2024 08:15:26 -0700 (PDT) Received: from localhost (host-80-182-198-72.retail.telecomitalia.it. [80.182.198.72]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5c226c7bda6sm6607816a12.41.2024.09.03.08.15.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Sep 2024 08:15:26 -0700 (PDT) From: Andrea della Porta X-Google-Original-From: Andrea della Porta Date: Tue, 3 Sep 2024 17:15:34 +0200 To: Krzysztof Kozlowski Subject: Re: [PATCH 08/11] misc: rp1: RaspberryPi RP1 misc driver Message-ID: References: <5954e4dccc0e158cf434d2c281ad57120538409b.1724159867.git.andrea.porta@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240903_081529_814147_72D260ED X-CRM114-Status: GOOD ( 48.23 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andrew Lunn , Catalin Marinas , Michael Turquette , Claudiu Beznea , Eric Dumazet , Dragan Cvetic , Will Deacon , linux-clk@vger.kernel.org, linux-arch@vger.kernel.org, Rob Herring , Florian Fainelli , Lee Jones , Saravana Kannan , Broadcom internal kernel review list , linux-pci@vger.kernel.org, Jakub Kicinski , Paolo Abeni , Linus Walleij , devicetree@vger.kernel.org, Conor Dooley , Arnd Bergmann , linux-gpio@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, Bjorn Helgaas , Andrea della Porta , linux-arm-kernel@lists.infradead.org, Derek Kiernan , Stephen Boyd , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Stefan Wahren , netdev@vger.kernel.org, Krzysztof Kozlowski , "David S. Miller" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Krzysztof, On 18:52 Fri 30 Aug , Krzysztof Kozlowski wrote: > On 30/08/2024 15:49, Andrea della Porta wrote: > > Hi Krzysztof, > > > > On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: > >> On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > >>> The RaspberryPi RP1 is ia PCI multi function device containing > >>> peripherals ranging from Ethernet to USB controller, I2C, SPI > >>> and others. > >>> Implement a bare minimum driver to operate the RP1, leveraging > >>> actual OF based driver implementations for the on-borad peripherals > >>> by loading a devicetree overlay during driver probe. > >>> The peripherals are accessed by mapping MMIO registers starting > >>> from PCI BAR1 region. > >>> As a minimum driver, the peripherals will not be added to the > >>> dtbo here, but in following patches. > >>> > >>> Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > >>> Signed-off-by: Andrea della Porta > >>> --- > >>> MAINTAINERS | 2 + > >>> arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++ > >> > >> Do not mix DTS with drivers. > >> > >> These MUST be separate. > > > > Separating the dtso from the driver in two different patches would mean > > that the dtso patch would be ordered before the driver one. This is because > > the driver embeds the dtbo binary blob inside itself, at build time. So > > in order to build the driver, the dtso needs to be there also. This is not > > Sure, in such case DTS will have to go through the same tree as driver > as an exception. Please document it in patch changelog (---). Ack. > > > the standard approach used with 'normal' dtb/dtbo, where the dtb patch is > > ordered last wrt the driver it refers to. > > It's not exactly the "ordered last" that matters, but lack of dependency > and going through separate tree and branch - arm-soc/dts. Here there > will be an exception how we handle patch, but still DTS is hardware > description so should not be combined with driver code. Ack. > > > Are you sure you want to proceed in this way? > > > > > >> > >>> drivers/misc/Kconfig | 1 + > >>> drivers/misc/Makefile | 1 + > >>> drivers/misc/rp1/Kconfig | 20 ++ > >>> drivers/misc/rp1/Makefile | 3 + > >>> drivers/misc/rp1/rp1-pci.c | 333 ++++++++++++++++++++++++++ > >>> drivers/misc/rp1/rp1-pci.dtso | 8 + > >>> drivers/pci/quirks.c | 1 + > >>> include/linux/pci_ids.h | 3 + > >>> 10 files changed, 524 insertions(+) > >>> create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso > >>> create mode 100644 drivers/misc/rp1/Kconfig > >>> create mode 100644 drivers/misc/rp1/Makefile > >>> create mode 100644 drivers/misc/rp1/rp1-pci.c > >>> create mode 100644 drivers/misc/rp1/rp1-pci.dtso > >>> > >>> diff --git a/MAINTAINERS b/MAINTAINERS > >>> index 67f460c36ea1..1359538b76e8 100644 > >>> --- a/MAINTAINERS > >>> +++ b/MAINTAINERS > >>> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ > >>> RASPBERRY PI RP1 PCI DRIVER > >>> M: Andrea della Porta > >>> S: Maintained > >>> +F: arch/arm64/boot/dts/broadcom/rp1.dtso > >>> F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml > >>> F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > >>> F: drivers/clk/clk-rp1.c > >>> +F: drivers/misc/rp1/ > >>> F: drivers/pinctrl/pinctrl-rp1.c > >>> F: include/dt-bindings/clock/rp1.h > >>> F: include/dt-bindings/misc/rp1.h > >>> diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > >>> new file mode 100644 > >>> index 000000000000..d80178a278ee > >>> --- /dev/null > >>> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > >>> @@ -0,0 +1,152 @@ > >>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > >>> + > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> + > >>> +/dts-v1/; > >>> +/plugin/; > >>> + > >>> +/ { > >>> + fragment@0 { > >>> + target-path=""; > >>> + __overlay__ { > >>> + #address-cells = <3>; > >>> + #size-cells = <2>; > >>> + > >>> + rp1: rp1@0 { > >>> + compatible = "simple-bus"; > >>> + #address-cells = <2>; > >>> + #size-cells = <2>; > >>> + interrupt-controller; > >>> + interrupt-parent = <&rp1>; > >>> + #interrupt-cells = <2>; > >>> + > >>> + // ranges and dma-ranges must be provided by the includer > >>> + ranges = <0xc0 0x40000000 > >>> + 0x01/*0x02000000*/ 0x00 0x00000000 > >>> + 0x00 0x00400000>; > >> > >> Are you 100% sure you do not have here dtc W=1 warnings? > > > > the W=1 warnings are: > > > > arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property > > arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property > > > > I don't see anything related to the ranges line you mentioned. > > Hm, indeed, but I would expect warning about unit address not matching > ranges/reg. > > > > >> > >>> + > >>> + dma-ranges = > >>> + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx > >>> + <0x10 0x00000000 > >>> + 0x43000000 0x10 0x00000000 > >>> + 0x10 0x00000000>; > >>> + > >>> + clk_xosc: clk_xosc { > >> > >> Nope, switch to DTS coding style. > > > > Ack. > > > >> > >>> + compatible = "fixed-clock"; > >>> + #clock-cells = <0>; > >>> + clock-output-names = "xosc"; > >>> + clock-frequency = <50000000>; > >>> + }; > >>> + > >>> + macb_pclk: macb_pclk { > >>> + compatible = "fixed-clock"; > >>> + #clock-cells = <0>; > >>> + clock-output-names = "pclk"; > >>> + clock-frequency = <200000000>; > >>> + }; > >>> + > >>> + macb_hclk: macb_hclk { > >>> + compatible = "fixed-clock"; > >>> + #clock-cells = <0>; > >>> + clock-output-names = "hclk"; > >>> + clock-frequency = <200000000>; > >>> + }; > >>> + > >>> + rp1_clocks: clocks@c040018000 { > >> > >> Why do you mix MMIO with non-MMIO nodes? This really does not look > >> correct. > >> > > > > Right. This is already under discussion here: > > https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ > > > > IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by > > using CLK_OF_DECLARE. > > Depends. Where are these clocks? Naming suggests they might not be even > part of this device. But if these are part of the device, then why this > is not a clock controller (if they are controllable) or even removed > (because we do not represent internal clock tree in DTS). xosc is a crystal connected to the oscillator input of the RP1, so I would consider it an external fixed-clock. If we were in the entire dts, I would have put it in root under /clocks node, but here we're in the dtbo so I'm not sure where else should I put it. Regarding pclk and hclk, I'm still trying to understand where they come from. If they are external clocks (since they are fixed-clock too), they should be in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because there's no special management of these clocks, so no new clock definition is needed. If they are internal tree, I cannot simply get rid of them because rp1_eth node references these two clocks (see clocks property), so they must be decalred somewhere. Any hint about this?. Many thanks, Andrea > > Best regards, > Krzysztof >