Hello, On Mon, Aug 19, 2024 at 09:35:12PM GMT, Arend van Spriel wrote: > On 8/19/2024 6:42 PM, Sebastian Reichel wrote: > > I tested this on RK3588 EVB1 and the driver is working fine. The DT > > bindings are not correct, though: > > > > linux/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dtb: wifi@0,0: > > compatible: 'oneOf' conditional failed, one must be fixed: > > > > ['pci14e4,449d', 'brcm,bcm4329-fmac'] is too long > > 'pci14e4,449d' is not one of ['brcm,bcm43143-fmac', 'brcm,bcm4341b0-fmac', > > 'brcm,bcm4341b4-fmac', 'brcm,bcm4341b5-fmac', 'brcm,bcm4329-fmac', > > 'brcm,bcm4330-fmac', 'brcm,bcm4334-fmac', 'brcm,bcm43340-fmac', > > 'brcm,bcm4335-fmac', 'brcm,bcm43362-fmac', 'brcm,bcm4339-fmac', > > 'brcm,bcm43430a0-fmac', 'brcm,bcm43430a1-fmac', 'brcm,bcm43455-fmac', > > 'brcm,bcm43456-fmac', 'brcm,bcm4354-fmac', 'brcm,bcm4356-fmac', > > 'brcm,bcm4359-fmac', 'brcm,bcm4366-fmac', 'cypress,cyw4373-fmac', > > 'cypress,cyw43012-fmac', 'infineon,cyw43439-fmac'] > > from schema $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml# > > > > It's easy to see the problem in the binding. It does not expect a > > fallback string after the PCI ID based compatible. Either the > > pci14e4,449d entry must be added to the first enum in the binding, > > which has the fallback compatible, or the fallback compatible > > should not be added to DTS. > > Never understood why we ended up with such a large list. When the binding > was introduced there was one compatible, ie. brcm,bcm4329-fmac. People > wanted all the other flavors because it described a specific wifi chip and > no other reason whatsoever. The PCI ID based compatible do obfuscate that > info so those are even less useful in my opinion. > > > If the fallback compatible is missing in DTS, the compatible check in > > brcmf_of_probe() fails and the lpo clock is not requested resulting > > in the firmware startup failing. So that would require further > > driver changes. > > Right. The text based bindings file in 5.12 kernel clearly says: > > Required properties: > > - compatible : Should be "brcm,bcm4329-fmac". > > In 5.13 kernel this was replaced by the json-schema yaml file. The PCI ID > based enum which was added later does also list brcm,bcm4329-fmac so why > does that not work for the compatible list ['pci14e4,449d', > 'brcm,bcm4329-fmac']? Looking at the compatible property in yaml which I > stripped a bit for brevity: > > properties: > compatible: > oneOf: > - items: > - enum: > - brcm,bcm43143-fmac > - brcm,bcm4329-fmac > - infineon,cyw43439-fmac > - const: brcm,bcm4329-fmac > - enum: > - brcm,bcm4329-fmac > - pci14e4,43dc # BCM4355 > - pci14e4,4464 # BCM4364 > - pci14e4,4488 # BCM4377 > - pci14e4,4425 # BCM4378 > - pci14e4,4433 # BCM4387 > > So how should I read this. Searching for some sort of syntax description I > found [1] which has an example schema with description that has a similarly > complicated compatible property. From that I think the above should be > changed to: > > properties: > compatible: > oneOf: > - items: > - enum: > - brcm,bcm43143-fmac > - - brcm,bcm4329-fmac > - infineon,cyw43439-fmac > - const: brcm,bcm4329-fmac > + - items: > - enum: > - - brcm,bcm4329-fmac > - pci14e4,43dc # BCM4355 > - pci14e4,4464 # BCM4364 > - pci14e4,4488 # BCM4377 > - pci14e4,4425 # BCM4378 > - pci14e4,4433 # BCM4387 > + - const: brcm,bcm4329-fmac > + - const: brcm,bcm4329-fmac > > This poses a constraint in which the last string in the compatible list is > always 'brcm,bcm4329-fmac' even if it is the only string. At least that is > my understanding so if my understanding is wrong feel free to correct me on > this. > > [1] https://docs.kernel.org/devicetree/bindings/writing-schema.html Your proposed change should work as you describe. But it will result in DT check errors for some Apple devices, which followed the current binding and do not have the "brcm,bcm4329-fmac" fallback compatible: $ git grep -E "(pci14e4,43dc)|(pci14e4,4464)|(pci14e4,4488)|(pci14e4,4425)|(pci14e4,4433)" arch/ arch/arm64/boot/dts/apple/t8103-jxxx.dtsi: compatible = "pci14e4,4425"; arch/arm64/boot/dts/apple/t8112-j413.dts: compatible = "pci14e4,4433"; arch/arm64/boot/dts/apple/t8112-j493.dts: compatible = "pci14e4,4425"; I guess patch 3/4 from this series will also introduce some regressions for these devices by moving the check. What is the purpose of the compatible check in brcmf_of_probe() in the first place? Can it just be dropped? I see it was introduced 10 years ago in 61f663dfc1a09, probably to avoid a spurious error message for systems not having the IRQ described in DT? The current code exits quietly when none of the optional resources are defined. -- Sebastian
Attachment:
signature.asc
Description: PGP signature
