[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <924339e5-4dba-4068-8cea-a64ce0a20eef@wesion.com>
Date: Tue, 20 Aug 2024 18:00:19 +0800
From: Jacobe Zang <jacobe.zang@...ion.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>,
Arend van Spriel <arend.vanspriel@...adcom.com>
Cc: 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
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
Powered by blists - more mailing lists