[linux-sunxi] Re: [PATCH 1/4] brcmfmac: Add brcm, nvram_file_name dt property

[Hans de Goede]

Hi,

On 30-06-16 12:18, Jonas Gorski wrote:
> Hi,
>
> On 30 June 2016 at 12:04, Hans de Goede <hdegoede at redhat.com> wrote:
>> Hi,
>>
>>
>> On 30-06-16 11:58, Kalle Valo wrote:
>>>
>>> Hans de Goede <hdegoede at redhat.com> writes:
>>>
>>>> Hi,
>>>>
>>>> On 30-06-16 11:02, Kalle Valo wrote:
>>>>>
>>>>> Priit Laes <plaes at plaes.org> writes:
>>>>>
>>>>>>> What is the size of this nvram file? As it's board specific, I wonder
>>>>>>> if we can simply include it inside of the DT verbatim. I remember
>>>>>>> doing that (in the pre-dtb days, on real open firmware) for the
>>>>>>> "spidernet"
>>>>>>> ethernet driver.
>>>>>>
>>>>>>
>>>>>> It contains a bit too much info:
>>>>>>
>>>>>> This is what CubieTruck requires:
>>>>>>
>>>>>>
>>>>>> http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_ap6210.txt
>>>>>
>>>>>
>>>>> In the nvram file I see lots of identifiers:
>>>>>
>>>>> manfid=0x2d0
>>>>> prodid=0x492
>>>>> vendid=0x14e4
>>>>> devid=0x4343
>>>>> boardtype=0x0598
>>>>> boardrev=0x1307
>>>>> boardnum=777
>>>>>
>>>>> Are any of these (or a combination of them) unique for each board type?
>>>>> What if one of these is provided through DT and then the driver could
>>>>> choose the nvram file based on the board id? Just another idea...
>>>>
>>>>
>>>> That would require updating a table in the driver every time a new
>>>> board comes out, I do not believe that that is a good solution.
>>>
>>>
>>> You don't necessarily need to have a table in the driver if you embed
>>> the id to the filename, for example something like this:
>>>
>>> nvram-0598-1307.txt
>>> nvram-<boardtype>-<boardrev>.txt
>>
>>
>> Downside of this is, that as Arend already said, the nvram files are not
>> readily available.
>>
>> Typically the boards we are talking about are shipped with android,
>> and the mainline kernel bringup is done by a user, not the manufacturer.
>>
>> So the nvram file needs to be extracted from e.g. an android image,
>> having ap6210 in the filename allows the user to see that his module
>> (which is clearly labelled ap6210 is already supported, where as the
>> boardtype/boardrev magic numbers are a big unknown.
>>
>> So I believe that using a human readable string is better here.
>
> So then how about making use of a more specific compatible string?
>
> e.g.
>
> brcmf {
>         compatible = "foo,ap6210", "brcm,bcm4329-fmac";
>         ...
> };
>
> and if the compatible has more than one element you request
> FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
> lastly no suffix) until you get a match. (AFAICT, this is what the
> "model" property was originally intended for anyway, but almost nobody
> did it right, and everyone put a user readable string into "model" for
> boards instead of the ePAPR defined compatible string).

Hmm, interesting idea. Not sure how easy / hard it will be to implement
this, but from a dt binding point of view it seems elegant.

Kalle, Arend, what do you think of this ?

Regards,

Hans