Re: [PATCH 00/11] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Andrea della Porta <andrea.porta@suse.com>]

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
>