| Message ID | 20240816020635.1273911-1-jacobe.zang@wesion.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add AP6275P wireless support | expand |
Hi, 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. 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. Greetings, -- Sebastian
On 8/19/2024 6:42 PM, Sebastian Reichel wrote: > Hi, > > 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 > Greetings, > > -- Sebastian
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
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
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. >
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;-)
These add AP6275P wireless support on Khadas Edge2. Enable 32k clock for Wi-Fi module and extend the hardware IDs table in the brcmfmac driver for it to attach. Changes in v11: - Retain interrupt check in of.c - Split DTS and submit separately - Link to v10: https://lore.kernel.org/all/20240813082007.2625841-1-jacobe.zang@wesion.com/ Changes in v10: - Use ret instead unused probe_attach_result in sdio.c - Link to v9: https://lore.kernel.org/all/20240810035141.439024-1-jacobe.zang@wesion.com/ Changes in v9: - Add return -ENODEV error pointer from brcmf_sdio_probe as the default for the fail path - Add if statement for brcmf_of_probe in common.c - Retain modifications to of.c other than the return values - Link to v8: https://lore.kernel.org/all/20240805073425.3492078-1-jacobe.zang@wesion.com/ Changes in v8: - Add appropriate errno's for return values that will be send to bus when error occurred. - Link to v7: https://lore.kernel.org/all/20240802025715.2360456-1-jacobe.zang@wesion.com/ Changes in v7: - Change brcmf_of_probe prototypes from void to int, add appropriate errno's for return value, move clock check to the end of brcmf_of_probe - Add "brcm,bcm4329-fmac" compatible for wifi node - Link to v6: https://lore.kernel.org/all/20240731061132.703368-1-jacobe.zang@wesion.com/ Changes in v6: - Move "brcm,bcm4329-fmac" check to the top of brcmf_of_probe in of.c - Add return if clk didn't set in DTS -Link to v5: https://lore.kernel.org/all/20240730033053.4092132-1-jacobe.zang@wesion.com/ Changes in v5: - Add more commit message to the clock in bindings - Use IS_ERR_OR_NULL as a judgment condition of clk - Link to v4: https://lore.kernel.org/all/20240729070102.3770318-1-jacobe.zang@wesion.com/ Changes in v4: - Change clock description in dt-bindings - Move enable clk from pcie.c to of.c - Add compatible for wifi node in DTS - Add random seed flag for firmware download - Link to v3: https://lore.kernel.org/all/20240630073605.2164346-1-jacobe.zang@wesion.com/ Changes in v3: - Dropped redundant parts in dt-bindings. - Change driver patch title prefix as 'wifi: brcmfmac:'. - Change DTS Wi-Fi node clock-name as 'lpo'. - Link to v2: https://lore.kernel.org/all/20240624081906.1399447-1-jacobe.zang@wesion.com/ Changes in v2: - Add SoB tags for original developer. - Add dt-bindings for pci14e4,449d and clocks. - Replace dev_info to brcmf_dbg in pcie.c - Link to v1: https://lore.kernel.org/all/20240620020015.4021696-1-jacobe.zang@wesion.com/ Jacobe Zang (4): dt-bindings: net: wireless: brcm4329-fmac: add pci14e4,449d dt-bindings: net: wireless: brcm4329-fmac: add clock description for AP6275P wifi: brcmfmac: Add optional lpo clock enable support wifi: brcmfmac: add flag for random seed during firmware download .../net/wireless/brcm,bcm4329-fmac.yaml | 9 +++ .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 +- .../broadcom/brcm80211/brcmfmac/common.c | 3 +- .../wireless/broadcom/brcm80211/brcmfmac/of.c | 29 +++++++--- .../wireless/broadcom/brcm80211/brcmfmac/of.h | 9 +-- .../broadcom/brcm80211/brcmfmac/pcie.c | 55 ++++++++++++++++--- .../broadcom/brcm80211/brcmfmac/sdio.c | 22 +++++--- .../broadcom/brcm80211/brcmfmac/usb.c | 3 + .../broadcom/brcm80211/include/brcm_hw_ids.h | 2 + 9 files changed, 104 insertions(+), 32 deletions(-)