Message-ID: <9f1b9b00-00c6-4bd9-b255-aa59a00f826c@wesion.com>
Date: Tue, 20 Aug 2024 18:04:44 +0800
From: Jacobe Zang <jacobe.zang@...ion.com>
To: Arend van Spriel <arend.vanspriel@...adcom.com>,
 Sebastian Reichel <sebastian.reichel@...labora.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 17:53, Arend van Spriel wrote:
> 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.
> 

I see;-)

-- 
Best Regards
Jacobe

Powered by blists - more mailing lists