Re: [PATCH v2] brcmfmac: Add support for BCM43596 PCIe Wi-Fi

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/11/2022 15:40, Konrad Dybcio wrote:
> 
> 
> On 26.11.2022 22:45, Linus Walleij wrote:
>> On Fri, Nov 25, 2022 at 1:25 PM Kalle Valo <kvalo@kernel.org> wrote:
>>> Konrad Dybcio <konrad.dybcio@linaro.org> writes:
>>>
>>>> On 25.11.2022 12:53, Kalle Valo wrote:
>>>>> Konrad Dybcio <konrad.dybcio@linaro.org> writes:
>>>>>
>>>>>> On 21.11.2022 14:56, Linus Walleij wrote:
>>>>>>> On Fri, Nov 18, 2022 at 5:47 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>>>>>
>>>>>>>> I can think of a couple of hacky ways to force use of 43596 fw, but I
>>>>>>>> don't think any would be really upstreamable..
>>>>>>>
>>>>>>> If it is only known to affect the Sony Xperias mentioned then
>>>>>>> a thing such as:
>>>>>>>
>>>>>>> if (of_machine_is_compatible("sony,xyz") ||
>>>>>>>     of_machine_is_compatible("sony,zzz")... ) {
>>>>>>>    // Enforce FW version
>>>>>>> }
>>>>>>>
>>>>>>> would be completely acceptable in my book. It hammers the
>>>>>>> problem from the top instead of trying to figure out itsy witsy
>>>>>>> details about firmware revisions.
>>>>>>>
>>>>>>> Yours,
>>>>>>> Linus Walleij
>>>>>>
>>>>>> Actually, I think I came up with a better approach by pulling a page
>>>>>> out of Asahi folks' book - please take a look and tell me what you
>>>>>> think about this:
>>>>>>
>>>>>> [1]
>>>>>> https://github.com/SoMainline/linux/commit/4b6fccc995cd79109b0dae4e4ab2e48db97695e7
>>>>>> [2]
>>>>>> https://github.com/SoMainline/linux/commit/e3ea1dc739634f734104f37fdbed046873921af7
>>
>> Something in this direction works too.
>>
>> The upside is that it tells all operating systems how to deal
>> with the firmware for this hardware.
>>
>>>>> Instead of a directory path ("brcm/brcmfmac43596-pcie") why not provide
>>>>> just the chipset name ("brcmfmac43596-pcie")? IMHO it's unnecessary to
>>>>> have directory names in Device Tree.
>>>>
>>>> I think it's common practice to include a full $FIRMWARE_DIR-relative
>>>> path when specifying firmware in DT, though here I left out the board
>>>> name bit as that's assigned dynamically anyway. That said, if you don't
>>>> like it, I can change it.
>>>
>>> It's just that I have understood that Device Tree is supposed to
>>> describe hardware and to me a firmware directory "brcm/" is a software
>>> property, not a hardware property. But this is really for the Device
>>> Tree maintainers to decide, they know this best :)
>>
>> I would personally just minimize the amount of information
>> put into the device tree to be exactly what is needed to find
>> the right firmware.
>>
>> brcm,firmware-compatible = "43596";
>>
>> since the code already knows how to conjure the rest of the string.
>>
>> But check with Rob/Krzysztof.
>>
>> Yours,
>> Linus Walleij
> 
> Krzysztof, Rob [added to CC] - can I have your opinions?

I just got here bunch of quotes and no original message, so my response
probably won't be complete.

Devicetree also describes the system integration properties because we
need to know how the things are glued together.

We have firmware-name property which is a form of path. The
"directories" in the firmware-name are actually good because they allow
to create a hierarchy based on a vendor/SoC/model/board. I don't think
it is worth adding new properties replacing firmware-name.


Best regards,
Krzysztof