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 >
