Message-ID: <xc5226th2sifhop3gnwnziok4lfl5s6yqbxq6wx4vygnuf4via@4475aaonnmaz>
Date: Mon, 19 Aug 2024 22:33:07 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Arend van Spriel <arend.vanspriel@...adcom.com>
Cc: jacobe.zang@...ion.com, bhelgaas@...gle.com, 
	brcm80211-dev-list.pdl@...adcom.com, brcm80211@...ts.linux.dev, christophe.jaillet@...adoo.fr, 
	conor+dt@...nel.org, davem@...emloft.net, devicetree@...r.kernel.org, 
	duoming@....edu.cn, edumazet@...gle.com, gregkh@...uxfoundation.org, 
	krzk+dt@...nel.org, kuba@...nel.org, kvalo@...nel.org, linux-kernel@...r.kernel.org, 
	linux-wireless@...r.kernel.org, megi@....cz, minipli@...ecurity.net, netdev@...r.kernel.org, 
	pabeni@...hat.com, robh@...nel.org, saikrishnag@...vell.com, 
	stern@...land.harvard.edu, yajun.deng@...ux.dev
Subject: Re: [PATCH v11 0/4] Add AP6275P wireless support

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists