Re: [PATCH 1/2] brcmfmac: Use separate struct to declare firmware names for Apple OTP chips

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

[-- Attachment #1: Type: text/plain, Size: 5640 bytes --]

On 1/3/2023 2:46 PM, Hector Martin wrote:
> On 03/01/2023 22.30, Arend van Spriel wrote:
>> On 1/3/2023 4:55 AM, Hector Martin wrote:
>>> On 2023/01/03 3:27, Arend Van Spriel wrote:
>>>> On January 2, 2023 4:15:41 PM Hector Martin <marcan@marcan.st> wrote:
>>>>
>>>>> On 02/01/2023 23.40, Aditya Garg wrote:
>>>>>> From: Aditya Garg <gargaditya08@live.com>
>>>>>>
>>>>>> Commit 'dce45ded7619' added support for 89459 chip pcie device. It uses the
>>>>>> BRCM4355 chip which is also found in Apple hardware. However this commit
>>>>>> causes conflicts in the firmware naming between Apple hardware, which
>>>>>> supports OTP and other non-Apple hardwares. So, this patch makes these
>>>>>> Apple chips use their own firmware table so as to avoid possible conflicts
>>>>>> like these in the future.
>>>>>
>>>>> I think my reply to Arend flew over your head.
>>>>>
>>>>> My point was that I'd rather have the Broadcom/Cypress people actually
>>>>> answer my question so we can figure out how to do this *properly*,
>>>>> instead of doing "safer-but-dumb" things (like this patch) because we
>>>>> just don't have the information to do it properly.
>>>>
>>>> Fair enough. Can you accurately (re)state your question and I will try to
>>>> answer it.
>>>
>>> As per my original email: Is the CYW89459 just a rebrand of the BCM4355,
>>> or just a subset? Can we consider them equivalent, and equivalent to the
>>> Apple part (BCM4355C1 / revision 12)?
>>
>> There is probably no easy answer. Mainly because Cypress is a separate
>> entity. However, they use the same/similar technology and code base. So
>> let me first start with the chip naming. The wifi chip primarily has a
>> number and revision. The chip number is straighforward and can be read
>> from the device. The chip revision comes in two variants: 1) simple
>> increasing number as read from the device, and 2) a <letter><digit>
>> format. The latter start at a0, which you almost never see in the wild
>> unless we do it "first time right". Whenever spinning a new chip we
>> either increase the digit or the letter depending on type/amount of
>> changes. There is not predictable mapping between the revision variants.
>> Depending on the hurdles in a chip project we may move from a0 to b0, or
>> from b0 to b1 or whatever.
> 
> Right, this is standard chip spin numbering, that much I know.
> 
>> If CYW89459 chip reads chip number 0x4355 than it is a BCM4355. If it is
>> a different revision it may require different firmware. A different
>> letter will always require different firmware. A different digit may
>> work although the firmware can have code paths for a specific revision.
> 
> So is it always correct to have the same firmware (in a generic
> situation, not a customized OEM build) for, say, a BCM4355 rev 12,
> regardless of what the PCI ID programmed into the OTP is (and what the
> marketing device name is)?

Yes.

> If so, then my conclusion is that the original patch I replied to is
> incorrect, all the defines should've been called BCM4355 (not the
> Cypress part number), and we probably need two firmware table entries
> since (judging by the revision check elsewhere in that patch) there are
> other revisions in the wild than the one Apple uses, and therefore there
> should at the very least be a firmware name split at C1. It would then
> be very helpful to know what revisions *do* exist and their correct naming.

I can only track down what we have in Broadcom. For the 4355 the 
revisions B1 (=6), B3 (=8), C0 (=10) and C1 are mentioned as released. 
Here things get weird, because you mentioned BCM4355 rev12, which would 
be a C2. So without asking around I can only assume this C2 variant is 
not different from firmware perspective and can happily run the C1 firmware.

> If different PCI device IDs might need different firmware, then the
> exiting firmware selection/table mechanism is insufficient.
> 
>> Happy New year to you. Thanks for clearly marking the rant. Makes it
>> easier to ignore, but let me get into this. I would not call bcmdhd the
>> downstream driver. It is a separate out-of-tree driver. Indeed resources
>> were pulled from brcm80211 development, but there always have been only
>> 2 or 3 people working on it. Me being the constant working mule and
>> these days only for 20% of my working hours to do the job. So you are
>> not really doing our job as we are not assigned to do so. I guess there
>> is no ROI for Broadcom or so it is perceived and there is no customer
>> pushing for it. That said I am always happy to help and clarify whatever
>> I can.
> 
> Is there any chance you can provide a list of chips/shipping revisions
> and revision IDs, so we can stop guessing at the mappings in the
> firmware table? Because this is effectively breaking userspace ABI every
> time we make a change to an existing chip, as it can change the firmware
> file name that userspace loads. This already happened with BCM4364,
> where (at least) B2 and B3 revisions exist in the wild and we need
> separate firmwares, yet it was added with a full mask, resulting in
> people copying "the right firmware for them" manually and my patch to
> split it into properly named firmwares will break those users.

Userspace is not loading anything these days. AFAIK the kernel is 
directly accessing the firmware file. Anyway, I never considered this as 
being a big issue. If people change their installed os to get things 
working, they can expect the reverse can happen anytime and deal with it 
once more. If this is considered a real issue we should only set the 
revmask for the revision we know to be working.

Regards,
Arend

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]