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

Jacobe Zang <jacobe.zang@xxxxxxxxxx> · Tue, 20 Aug 2024 18:00:19 +0800

On 2024/8/20 4:33, 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

Before these patches, the compatible check in brcmf_of_probe() is only 
used for interrupt configuration in DTS. Wi-Fi compatible in some boards 
without IRO configuration is just a compatible don't have real function 
but required by the binding. So if the compatible check is no longer 
needed, I think it can be dropped.

And also just adjusting the yaml is a good choice, but need to add 
fallback compatible for boards just like Apple.

optional resources are defined.

--
Best Regards
Jacobe