Re: [PATCH 1/2] dt-bindings: wireless: restore constraint for brcm,bcm4329-fmac compatible property

[Arend Van Spriel <arend.vanspriel@broadcom.com>]

On August 22, 2024 2:26:27 AM Julian Calaby <julian.calaby@gmail.com> wrote:

> Hi Krzysztof, Arend,
>
> On Wed, Aug 21, 2024 at 4:46 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 20/08/2024 21:29, Arend van Spriel wrote:
>>> On August 20, 2024 5:51:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>
>>>> On 20/08/2024 17:36, Krzysztof Kozlowski wrote:
>>>>> On 20/08/2024 14:50, Arend van Spriel wrote:
>>>>>> On 8/20/2024 1:39 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 20/08/2024 13:27, Krzysztof Kozlowski wrote:
>>>>>>>> On Tue, Aug 20, 2024 at 12:12:15PM +0200, Arend van Spriel wrote:
>>>>>>>>> When extending the bindings for Apple PCIe devices the compatible property
>>>>>>>>> specification was changed. However, it was changed such that for these
>>>>>>>>> devices it was no longer necessary to have "brcm,bcm4329-fmac" listed as
>>>>>>>>> string in the compatible list property as it was before that extension.
>>>>>>>>
>>>>>>>> Apart that this was never tested... That statement is not true. Look at
>>>>>>>> "fixed" commit - it is not doing like that at all.
>>>>>>>>
>>>>>>>> I don't understand the reasoning.
>>>>>>>>
>>>>>>>>> This patch restores that constraint.
>>>>>>>>>
>>>>>>>>> Fixes: e2e37224e8b3 ("dt-bindings: net: bcm4329-fmac: Add Apple properties
>>>>>>>>> & chips")
>>>>>>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>>>>>>>> ---
>>>>>>>>> .../net/wireless/brcm,bcm4329-fmac.yaml       | 19 ++++++++++---------
>>>>>>>>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
>>>>>>>>> b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
>>>>>>>>> index e564f20d8f41..47f90446322f 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
>>>>>>>>> +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
>>>>>>>>> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>> title: Broadcom BCM4329 family fullmac wireless SDIO/PCIE devices
>>>>>>>>>
>>>>>>>>> maintainers:
>>>>>>>>> -  - Arend van Spriel <arend@broadcom.com>
>>>>>>>>> +  - Arend van Spriel <arend.vanspriel@broadcom.com>
>>>>>>>>>
>>>>>>>>> description:
>>>>>>>>> The Broadcom Single chip MAC part for the BCM4329 family and
>>>>>>>>> @@ -27,7 +27,6 @@ properties:
>>>>>>>>>           - brcm,bcm4341b0-fmac
>>>>>>>>>           - brcm,bcm4341b4-fmac
>>>>>>>>>           - brcm,bcm4341b5-fmac
>>>>>>>>> -              - brcm,bcm4329-fmac
>>>>>>>>>           - brcm,bcm4330-fmac
>>>>>>>>>           - brcm,bcm4334-fmac
>>>>>>>>>           - brcm,bcm43340-fmac
>>>>>>>>> @@ -46,13 +45,15 @@ properties:
>>>>>>>>>           - cypress,cyw43012-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
>>>>>>>>> +    - items:
>>>>>>>>> +          - enum:
>>>>>>>>> +              - pci14e4,43dc  # BCM4355
>>>>>>>>> +              - pci14e4,4464  # BCM4364
>>>>>>>>> +              - pci14e4,4488  # BCM4377
>>>>>>>>> +              - pci14e4,4425  # BCM4378
>>>>>>>>> +              - pci14e4,4433  # BCM4387
>>>>>>>>> +          - const: brcm,bcm4329-fmac
>>>>>>>>> +    - const: brcm,bcm4329-fmac
>>>>>>>>
>>>>>>>> And this does not make sense... You claim that some constrained was
>>>>>>>> droppped and you re-add it, but in fact you still add the same code as
>>>>>>>> it was before.
>>>>>>>>
>>>>>>>> NAK.
>>>>>>>
>>>>>>> Ah, the last "const" actually makes sense, I missed that.
>>>>>>>
>>>>>>> Commit still however lacks rationale why these devices are compatible.
>>>>>>> Plus existing rationale that e2e37224e8b3 changed something is entirely
>>>>>>> WRONG. It changed nothing. ZERO. It only added new devices, which was
>>>>>>> claimed are not compatible with brcm,bcm4329-fmac.
>>>>>>
>>>>>> So is that claim true? What does it mean that these new devices are not
>>>>>> compatible. If they are they should be in a separate binding or the
>>>>>
>>>>> Whether binding is separate or not, is just way of organizing things.
>>>>>
>>>>>> applicable properties for these devices should be made conditional.
>>>>>
>>>>> Could be if they are not applicable.
>>>>>
>>>>>>
>>>>>>> Now if you claim that original commit which said "these devices are not
>>>>>>> compatible with brcm,bcm4329-fmac", then please provide arguments, not
>>>>>>> just say "other commit did something". It did nothing...
>>>>>>
>>>>>> Not entirely true. Indeed new devices were added for which no
>>>>>> "brcm,bcm4329-fmac" string is required in the compatible property. Also
>>>>>> the commit added new properties for these new devices. Now in my opinion
>>>>>> a driver should not use these properties without a "compatible" check.
>>>>>> Hope we can agree to that. However, the driver patch for supporting the
>>>>>
>>>>> Sorry, I don't follow. Why the driver would need to check for compatible?
>>>>>
>>>>>> binding change does no such thing. So if we leave the binding as it
>>>>>> currently is the driver will have to check if compatible has any of the
>>>>>> listed PCI IDs before processing the properties. As all properties old
>>>>>
>>>>> Why driver needs to check it? Are these properties not valid?
>>>
>>> How would the driver know other than the compatible property? The node
>>> with properties is delivered by the bus driver. If that comes with
>>> guarantees about validity than that's great.
>>
>> I still do not follow what is the problem being addressed by driver
>> needing to check.
>>
>>>
>>>>>
>>>>>
>>>>>> and new are marked as optional I can not come up with an argument that
>>>>>> these new devices are *not* compatible with brcm,bcm4329-fmac.
>>>>>
>>>>> Compatibility is expressed by implementing same programming interfasce
>>>>> (or its subset) thus being able to bind via fallback and correctly
>>>>> operate in given SW.
>>>
>>> This exactly what I mean to say (and apparently fail to do so ;-s ).
>>>
>>>>> I don't know whether that's the case here, so rephrasing my earlier
>>>>> comments - the commit msg should focus on this aspect and tell that
>>>>> devices are fully compatible, thus they should use fallback.
>>>>>
>>>>> Quick look at drivers told me that not - they are not compatible...
>>>
>>> Okay. That puts use in different corner of the arena. Can you elaborate
>>> how you come to that assessment? Is that based on the fact that some of
>>> the properties are SDIO-only?
>>
>> The simplest: because they do not use the same match/bind code. Plus PCI
>> devices never used half of brcmf_of_probe(). Although that's more of a
>> reason these are significantly different.
>>
>>>
>>>>>
>>>> Another thing is that calling SDIO and PCI devices compatible is quite a
>>>> stretch... Clearly hardware-wise they are very different and Linux does
>>>> not use the same interfaces to match/bind them.
>>>
>>> These are wifi devices which hardware-wise are 95% the same. If you find
>>> the block diagram with IP cores for these devices (enough google results
>>> to find some) you can see they sometimes even have both PCI and SDIO
>>> block on-chip although only one is used so they can be considered 100%
>>> the same. In both cases the bus driver will attach the DT node to the
>>> binding device.
>>
>> I understand they are similar, but it does not matter if that is 95% or
>> even 99% if the interface is different. Linux cannot use these devices
>> through the same interface. However if you claim it can, then please
>> write appropriate commit msg.
>>
>> My entire objection hare started not because I believe these are not
>> compatible (although based on different buses I believe they are not
>> compatible), but because the argument was about that other commit. That
>> argument is not correct to make the change. Correct argument to make the
>> change could be: These devices are compatible, because of foo and bar.
>
> A way to put it might be something like:
>
> Almost all Broadcom wireless chipsets, some Broadcom ethernet chipsets
> and most Broadcom-derived Infineon and Cypress wireless chipsets share
> the same rough layout:
>
> System bus -> bus glue logic -> internal bus -> RF chipset(s)
>
> Previous generations had separate drivers for the bus glue logic and
> then probed the internal bus for the RF chipset(s), however in the
> current generation that separation is mostly just a historical
> artifact and abstraction layer.
>
> The firmwares for all WiFi chipsets in this generation also provide
> fundamentally identical interfaces with any differences either probed
> at runtime or derived from data in the firmware. As such, once the
> glue logic is abstracted away, all chipsets of this generation are
> fundamentally identical from a software perspective. Therefore the
> only data needed to use one of these chipsets is the type of bus to
> select the glue logic driver and the firmware to load which is derived
> from the vendor and product IDs.
>
> The only real hardware differences between different chipsets are that
> some have external interrupt lines or additional clocks and I'm not
> sure if hooking those up is actually mandatory.

Thanks, Julian

I will respin and see what to make of it. Your input is (always) 
appreciated although your clown avatar always scares the shit out of me.

Regards,
Arend