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 AF40EC83F15 for ; Thu, 29 Aug 2024 13:13:31 +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=6HSLQ8KvULAlJol+4PZpsMQiwSkPlENMupYF/HR2lik=; b=vfemdt7NuhozGL+X5pCkAoyfuX x0rLHhdrPGMF73+M403o6W74kaBeP6EROtwi5ZMlSuseRkYeaFoY5DBj7owNKhWXvpM43Hrmw7ght Sxp5VADZjbNcG2fy4pmM9kySgPfh9LeJzrpfG7ZfIxSOA8esBsSo1dF1i10YS0KRyp+ponVT3V5qn gHm0IcTPJvSQWwTYuyM5CmBnhfNJZUUqCKNjv9j1Tyyojcb27V8c7X1HRNzWSHyDYd6fWkZugX4IJ wRiLLc0Odv5btf86l9NL2E/VZVLKhhJFPdW+o/qco1LpoH35DLFdwPD7wvQeyhgK1UPCQC4vLgphf d+EPCyeQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sjexh-000000026Hj-2Pa6; Thu, 29 Aug 2024 13:13:17 +0000 Received: from mail-ed1-x544.google.com ([2a00:1450:4864:20::544]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sjew2-000000025eA-2Qzd for linux-arm-kernel@lists.infradead.org; Thu, 29 Aug 2024 13:11:38 +0000 Received: by mail-ed1-x544.google.com with SMTP id 4fb4d7f45d1cf-5befd2f35bfso630922a12.2 for ; Thu, 29 Aug 2024 06:11:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1724937093; x=1725541893; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:date:from:from:to:cc :subject:date:message-id:reply-to; bh=6HSLQ8KvULAlJol+4PZpsMQiwSkPlENMupYF/HR2lik=; b=PYaAPGrz94W/AKPcx740uOb2T6kJXnLTICKcLR4qBmFZBuFpcNQdSGmFjVXtil4SEv g9OeF/Rp4Dd5npwKZFLpN8/M/g/Fc3K5BqEuYC08xT6xZlosvURpJ9KqhITDT+oikZFz qo5JnZDxC0dfE6IxB6FQUbh1TWt8O7LkeOaNiSUmZLoIepkObO7Eu3JAeftIckWCPiFU Kq5e1oQKKSq+LyxJCdrxuEkM0b5RzVhKpBa3zB/3cSpAvC4ng+wUM5Ae6WdmHe8a0Gn2 fft1b85Ny3j7rOFInQjpzKvV4AYkCTxqNCAhoaBS08sMGEuT0Y/YtzOv27Qpmvg8qzWU ofuQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724937093; x=1725541893; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:date:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=6HSLQ8KvULAlJol+4PZpsMQiwSkPlENMupYF/HR2lik=; b=VnQaCcJd+U1uK+gVy2miWsA/AaPR9yt8vQMTVazXQ9YJ6MPNtpzUZVKDDlT1Ua7URF ctNKk6D9B+/NdbkSqms5oy4RBkknBg3FhVVxrEjc4Zfbnd0wEwAfpkfXPY5aU9svL1kh HODDfXWVXx3aIkjitAa1xf836FR9fIn0tMAgg55+aCVisKUF7+Ajmwp8rnW5YKr9kBi1 /eMOPBtGnB2bYl4yuKuXQAs2+DvXmtfuoItbfMTWZqM3zUCSbthd9kXM0Qqz5kSClnYb jLLfv1Dnn6ZSaukQTOIJfeKVxFCLDTEHGgkOCQicfKyKpmDieOfOzrw6BA3/YHZI6RRf bwWQ== X-Forwarded-Encrypted: i=1; AJvYcCXi065AnJLAtAHZkee0PecGP1v2j5fWddciJ2BdaAizWevFrPvC83VGVHJwAk73P1UoeXsPn04DK7AQqVfNhQRA@lists.infradead.org X-Gm-Message-State: AOJu0YzIlm9/J+AP0WzY0DnzYnFTz1uWxR/k1S/QXELGnELO8ivjv/tI yoYHoxTR+QvB338O2WiNR872wx7K0R8faHJ+qWpJkZB4yHsyldfQoOB4NngOOog= X-Google-Smtp-Source: AGHT+IEH/zJZio4NTe6eEsV2DoExlez2oCgJWKVnp5yu1Qv76cYjCAYlwUS9Kb57z8iTsLMFbn1guw== X-Received: by 2002:a05:6402:268e:b0:5bf:dd0:93ad with SMTP id 4fb4d7f45d1cf-5c21ed8e66bmr2407157a12.27.1724937091825; Thu, 29 Aug 2024 06:11:31 -0700 (PDT) Received: from localhost (host-80-182-198-72.retail.telecomitalia.it. [80.182.198.72]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a8988fea68dsm77550266b.26.2024.08.29.06.11.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Aug 2024 06:11:31 -0700 (PDT) From: Andrea della Porta X-Google-Original-From: Andrea della Porta Date: Thu, 29 Aug 2024 15:11:38 +0200 To: Krzysztof Kozlowski Subject: Re: [PATCH 00/11] Add support for RaspberryPi RP1 PCI device using a DT overlay Message-ID: Mail-Followup-To: Krzysztof Kozlowski , Andrea della Porta , Michael Turquette , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Florian Fainelli , Broadcom internal kernel review list , Linus Walleij , Catalin Marinas , Will Deacon , Derek Kiernan , Dragan Cvetic , Arnd Bergmann , Greg Kroah-Hartman , Nicolas Ferre , Claudiu Beznea , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Saravana Kannan , Bjorn Helgaas , linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, netdev@vger.kernel.org, linux-pci@vger.kernel.org, linux-arch@vger.kernel.org, Lee Jones , Andrew Lunn , Stefan Wahren References: <14990d25-40a2-46c0-bf94-25800f379a30@kernel.org> 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-20240829_061134_775253_EA05EC66 X-CRM114-Status: GOOD ( 65.88 ) 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 11:50 Thu 22 Aug , Krzysztof Kozlowski wrote: > On 22/08/2024 11:05, Andrea della Porta wrote: > > Hi Krzysztof, > > > > On 15:42 Wed 21 Aug , Krzysztof Kozlowski wrote: > >> On 20/08/2024 16:36, Andrea della Porta wrote: > >>> RP1 is an MFD chipset that acts as a south-bridge PCIe endpoint sporting > >>> a pletora of subdevices (i.e. Ethernet, USB host controller, I2C, PWM, > >>> etc.) whose registers are all reachable starting from an offset from the > >>> BAR address. The main point here is that while the RP1 as an endpoint > >>> itself is discoverable via usual PCI enumeraiton, the devices it contains > >>> are not discoverable and must be declared e.g. via the devicetree. > >>> > >>> This patchset is an attempt to provide a minimum infrastructure to allow > >>> the RP1 chipset to be discovered and perpherals it contains to be added > >>> from a devictree overlay loaded during RP1 PCI endpoint enumeration. > >>> Followup patches should add support for the several peripherals contained > >>> in RP1. > >>> > >>> This work is based upon dowstream drivers code and the proposal from RH > >>> et al. (see [1] and [2]). A similar approach is also pursued in [3]. > >> > >> Looking briefly at findings it seems this was not really tested by > >> automation and you expect reviewers to find issues which are pointed out > >> by tools. That's not nice approach. Reviewer's time is limited, while > >> tools do it for free. And the tools are free - you can use them without > >> any effort. > > > > Sorry if I gave you that impression, but this is not obviously the case. > > Just look at number of reports... so many sparse reports that I wonder > how it is not the case. > > And many kbuild reports. Ack. > > > I've spent quite a bit of time in trying to deliver a patchset that ease > > your and others work, at least to the best I can. In fact, I've used many > > of the checking facilities you mentioned before sending it, solving all > > of the reported issues, except the ones for which there are strong reasons > > to leave untouched, as explained below. > > > >> > >> It does not look like you tested the DTS against bindings. Please run > >> `make dtbs_check W=1` (see > >> Documentation/devicetree/bindings/writing-schema.rst or > >> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > >> for instructions). > > > > #> make W=1 dt_binding_check DT_SCHEMA_FILES=raspberrypi,rp1-gpio.yaml > > CHKDT Documentation/devicetree/bindings > > LINT Documentation/devicetree/bindings > > DTEX Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.example.dts > > DTC_CHK Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.example.dtb > > > > #> make W=1 dt_binding_check DT_SCHEMA_FILES=raspberrypi,rp1-clocks.yaml > > CHKDT Documentation/devicetree/bindings > > LINT Documentation/devicetree/bindings > > DTEX Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.example.dts > > DTC_CHK Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.example.dtb > > > > I see no issues here, in case you've found something different, I kindly ask you to post > > the results. > > > > #> make W=1 CHECK_DTBS=y broadcom/rp1.dtbo > > DTC arch/arm64/boot/dts/broadcom/rp1.dtbo > > 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 believe that These warnings are unavoidable, and stem from the fact that this > > is quite a peculiar setup (PCI endpoint which dynamically loads platform driver > > addressable via BAR). > > The missing reg/ranges in the threee clocks are due to the simple-bus of the > > containing node to which I believe they should belong: I did a test to place > > This is not the place where they belong. non-MMIO nodes should not be > under simple-bus. Ack. > > > those clocks in the same dtso under root or /clocks node but AFAIK it doesn't > > seems to work. I could move them in a separate dtso to be loaded before the main > > Well... who instantiates them? If they are in top-level, then > CLK_OF_DECLARE which is not called at your point? > > You must instantiate clocks different way, since they are not part of > "rp1". That's another bogus DT description... external oscilator is not > part of RP1. > Ok, I'll dive into that and see what I can come up with. Many thanks for this feedback. > > > one but this is IMHO even more cumbersome than having a couple of warnings in > > CHECK_DTBS. > > Of course, if you have any suggestion on how to improve it I would be glad to > > discuss. > > About the last warning about the address/size-cells, if I drop those two lines > > in the _overlay_ node it generates even more warning, so again it's a "don't fix" > > one. > > > >> > >> Please run standard kernel tools for static analysis, like coccinelle, > >> smatch and sparse, and fix reported warnings. Also please check for > >> warnings when building with W=1. Most of these commands (checks or W=1 > >> build) can build specific targets, like some directory, to narrow the > >> scope to only your code. The code here looks like it needs a fix. Feel > >> free to get in touch if the warning is not clear. > > > > I didn't run those static analyzers since I've preferred a more "manual" aproach > > by carfeully checking the code, but I agree that something can escape even the > > more carefully executed code inspection so I will add them to my arsenal from > > now on. Thanks for the heads up. > > I don't care if you do not run static analyzers if you produce good > code. But if you produce bugs which could have been easily spotted with > sparser, than it is different thing. > > Start running static checkers instead of asking reviewers to do that. Ack. > > > > >> > >> Please run scripts/checkpatch.pl and fix reported warnings. Then please > >> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. > >> Some warnings can be ignored, especially from --strict run, but the code > >> here looks like it needs a fix. Feel free to get in touch if the warning > >> is not clear. > >> > > > > Again, most of checkpatch's complaints have been addressed, the remaining > > ones I deemed as not worth fixing, for example:> > > #> scripts/checkpatch.pl --strict --codespell tmp/*.patch > > > > WARNING: please write a help paragraph that fully describes the config symbol > > #42: FILE: drivers/clk/Kconfig:91: > > +config COMMON_CLK_RP1 > > + tristate "Raspberry Pi RP1-based clock support" > > + depends on PCI || COMPILE_TEST > > + depends on COMMON_CLK > > + help > > + Enable common clock framework support for Raspberry Pi RP1. > > + This mutli-function device has 3 main PLLs and several clock > > + generators to drive the internal sub-peripherals. > > + > > > > I don't understand this warning, the paragraph is there and is more or less similar > > to many in the same file that are already upstream. Checkpatch bug? > > > > > > CHECK: Alignment should match open parenthesis > > #1541: FILE: drivers/clk/clk-rp1.c:1470: > > + if (WARN_ON_ONCE(clock_data->num_std_parents > AUX_SEL && > > + strcmp("-", clock_data->parents[AUX_SEL]))) > > > > This would have worsen the code readability. > > > > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > #673: FILE: drivers/pinctrl/pinctrl-rp1.c:600: > > + return -ENOTSUPP; > > > > This I must investigate: I've already tried to fix it before sending the patchset > > but for some reason it wouldn't work, so I planned to fix it in the upcoming > > releases. > > > > > > WARNING: externs should be avoided in .c files > > #331: FILE: drivers/misc/rp1/rp1-pci.c:58: > > +extern char __dtbo_rp1_pci_begin[]; > > > > True, but in this case we don't have a symbol that should be exported to other > > translation units, it just needs to be referenced inside the driver and > > consumed locally. Hence it would be better to place the extern in .c file. > > > > > > Apologies for a couple of other warnings that I could have seen in the first > > place, but honestly they don't seems to be a big deal (one typo and on over > > 100 chars comment, that will be fixed in next patch version). > > Again, judging by number of reports from checkers that is a big deal, > because it is your task to run the tools. Ack. Many thanks, Andrea > > Best regards, > Krzysztof >