Re: [PATCH v11 0/4] Add AP6275P wireless support

Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx> · Tue, 20 Aug 2024 11:53:41 +0200

On 8/19/2024 10:33 PM, Sebastian Reichel wrote:
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.

It was introduced simply because the compatible property has a meaning 
that goes beyond informational. It is a claim that the properties of the 
node comply to the bindings specification. I would really want to keep 
the sanity check event though all properties are optional. The 
constraint keeps the compatible matching in the driver relatively simple.

Regards,
Arend