[GIT PULL 1/3] Keystone SOC driver updates for 4.4

[m-karicheri2@ti.com (Murali Karicheri)]

On 10/12/2015 11:06 AM, Murali Karicheri wrote:
> On 10/09/2015 02:54 PM, Karicheri, Muralidharan wrote:
>>
>>> -----Original Message-----
>>> From: Arnd Bergmann [mailto:arnd at arndb.de]
>>> Sent: Friday, October 09, 2015 11:21 AM
>>> To: Karicheri, Muralidharan
>>> Cc: Santosh Shilimkar; olof at lixom.net; arm at kernel.org; linux-arm-
>>> kernel at lists.infradead.org; khilman at kernel.org
>>> Subject: Re: [GIT PULL 1/3] Keystone SOC driver updates for 4.4
>>>
>>> On Friday 09 October 2015 15:09:56 Karicheri, Muralidharan wrote:
>>>>> Normally, the firmware name is fixed in the driver. If the API
>>>>> changes in order to support a new feature, the driver of course needs
>>>>> to be aware of that, but it should not require a device tree change
>>>>> to update the file
>>> name.
>>>>>
>>>>> Conversely, if you get a new chip that needs a slightly different
>>>>> blob but has an identical API, the driver should ideally not need
>>>>> to be changed, but still
>>> see  a new file name.
>>>>>
>>>>> The first can be done once you need it, e.g. by appending the number
>>>>> of the API version to the file name inside of the driver, and trying
>>>>> the highest version supported by the driver first, before falling
>>>>> back to older version in reverse order until the oldest version
>>>>> that is supported by the
>>> driver.
>>>>>
>>>> What I gather is the firmware file name is to be part of the driver
>>>> itself instead of adding the same to DT. This way as firmware API
>>>> change, additional filename strings can be added and handled as
>>>> needed. As API is not
>>> expected to change that often, this change will be needed very rarely.
>>>
>>> Right.
>>>
>>>> But how to deal with the case where firmware blob changes, but no API
>>>> changes which is mostly the case for keystone. For example currently
>>>> we have 2.0.0, and 2.0.1 is available, but no API change. Probably I
>>>> could name the file as file_2.0.x in driver and as long as there is
>>>> no API change, one could
>>> provide a soft link in file system to point to different minor versions.
>>>>
>>>> ln -s /lib/firmware/file_2.0.1 /lib/firmware/file_2.0.x
>>>>
>>>> In the case of qmss firmware, it will be named as
>>>> ks2_qmss_pdsp_acc48_k2_le_1_0_0_x.bin in the driver source code as per
>>>> the above suggestion. In the file system, there will be a soft link to
>>> ks2_qmss_pdsp_acc48_k2_le_1_0_0_9.bin.
>>>> ln -s /lib/firmware/ks2_qmss_pdsp_acc48_k2_le_1_0_0_9.bin
>>>> /lib/firmware/ ks2_qmss_pdsp_acc48_k2_le_1_0_0_x.bin
>>>>
>>>> Is this acceptable? or do you see any issue with this approach?
>>>
>>> I don't know what the policy is for the linux-firmware repository. My
>>> first thought would have
>>> been that you only ever put the latest version into that tree and
>>> never change the name.
>>> Symlinks can obviously work as well, and I see some cases of this in
>>> my /lib/firmware
>>> directory:
>>
>> A quick grep under linux-firmware.git provided my some firmware names
>> with multiple versions.
>>
>> ./bnx2x/bnx2x-e2-7.8.17.0.fw
>> ./bnx2x/bnx2x-e2-7.2.16.0.fw
>> ./bnx2x/bnx2x-e1h-7.0.29.0.fw
>> ./bnx2x/bnx2x-e2-7.12.30.0.fw
>> ./bnx2x/bnx2x-e2-7.2.51.0.fw
>> ./bnx2x/bnx2x-e1h-7.8.17.0.fw
>> ./bnx2x/bnx2x-e1h-7.2.16.0.fw
>> ./bnx2x/bnx2x-e2-7.0.29.0.fw
>> ./bnx2x/bnx2x-e2-7.8.2.0.fw
>> ./bnx2x/bnx2x-e1-6.2.5.0.fw
>> ./bnx2x/bnx2x-e1-7.8.2.0.fw
>> ./bnx2x/bnx2x-e1h-7.8.2.0.fw
>> ./bnx2x/bnx2x-e1-7.0.20.0.fw
>> ./bnx2x/bnx2x-e1-7.10.51.0.fw
>> ./bnx2x/bnx2x-e2-7.0.20.0.fw
>> ./bnx2x/bnx2x-e1h-6.0.34.0.fw
>> ./bnx2x/bnx2x-e1-7.8.19.0.fw
>> ./bnx2x/bnx2x-e2-6.0.34.0.fw
>> ./bnx2x/bnx2x-e1-7.2.51.0.fw
>> ./bnx2x/bnx2x-e2-6.2.9.0.fw
>> ./bnx2x/bnx2x-e1-7.8.17.0.fw
>> ./bnx2x/bnx2x-e2-7.0.23.0.fw
>> ./bnx2x/bnx2x-e1h-6.2.9.0.fw
>> ./bnx2x/bnx2x-e1-7.0.29.0.fw
>> ./bnx2x/bnx2x-e2-7.10.51.0.fw
>> ./bnx2x/bnx2x-e1-6.0.34.0.fw
>> ./bnx2x/bnx2x-e2-7.8.19.0.fw
>> ./bnx2x/bnx2x-e1h-7.0.23.0.fw
>> ./bnx2x/bnx2x-e1-7.2.16.0.fw
>> ./bnx2x/bnx2x-e1-7.0.23.0.fw
>> ./bnx2x/bnx2x-e1h-7.2.51.0.fw
>> ./bnx2x/bnx2x-e1-6.2.9.0.fw
>> ./bnx2x/bnx2x-e1h-6.2.5.0.fw
>> ./bnx2x/bnx2x-e1h-7.8.19.0.fw
>> ./bnx2x/bnx2x-e1h-7.12.30.0.fw
>> ./bnx2x/bnx2x-e2-6.2.5.0.fw
>> ./bnx2x/bnx2x-e1h-7.0.20.0.fw
>> ./bnx2x/bnx2x-e1h-7.10.51.0.fw
>> ./bnx2x/bnx2x-e1-7.12.30.0.fw
>>
>> And then
>>
>> ./bnx2/bnx2-rv2p-09ax-5.0.0.j10.fw
>> ./bnx2/bnx2-rv2p-09ax-5.0.0.j3.fw
>> ./bnx2/bnx2-rv2p-09-6.0.17.fw
>> ./bnx2/bnx2-rv2p-06-5.0.0.j3.fw
>> ./bnx2/bnx2-rv2p-09ax-6.0.17.fw
>> ./bnx2/bnx2-mips-09-6.2.1.fw
>> ./bnx2/bnx2-mips-06-6.2.1.fw
>> ./bnx2/bnx2-mips-09-5.0.0.j3.fw
>> ./bnx2/bnx2-rv2p-09-5.0.0.j10.fw
>> ./bnx2/bnx2-mips-09-6.2.1b.fw
>> ./bnx2/bnx2-mips-09-5.0.0.j9.fw
>> ./bnx2/bnx2-mips-09-6.0.17.fw
>> ./bnx2/bnx2-mips-09-6.2.1a.fw
>> ./bnx2/bnx2-mips-06-6.0.15.fw
>> ./bnx2/bnx2-mips-06-4.6.16.fw
>> ./bnx2/bnx2-mips-06-5.0.0.j6.fw
>> ./bnx2/bnx2-mips-09-5.0.0.j15.fw
>> ./bnx2/bnx2-mips-09-4.6.17.fw
>> ./bnx2/bnx2-rv2p-09-5.0.0.j3.fw
>> ./bnx2/bnx2-mips-06-6.2.3.fw
>> ./bnx2/bnx2-rv2p-09-4.6.15.fw
>> ./bnx2/bnx2-mips-06-5.0.0.j3.fw
>> ./bnx2/bnx2-rv2p-06-6.0.15.fw
>> ./bnx2/bnx2-rv2p-06-4.6.16.fw
>>
>> So multiple versions are possible in the repo.
>>
>>>
>>> $ find /lib/firmware/ -type l | xargs ls -l
>>> find: `/lib/firmware/b43': Permission denied lrwxrwxrwx 1 root root
>>> 16 Jan 31  2014
>>> /lib/firmware/cxgb4/t4fw.bin -> t4fw-1.6.2.0.bin lrwxrwxrwx 1 root
>>> root 18 Jan 31  2014
>>> /lib/firmware/libertas/sd8688.bin -> ../mrvl/sd8688.bin lrwxrwxrwx 1
>>> root root 25 Jan 31
>>> 2014 /lib/firmware/libertas/sd8688_helper.bin ->
>>> ../mrvl/sd8688_helper.bin lrwxrwxrwx 1
>>> root root 10 Jan 31  2014 /lib/firmware/rt3070.bin -> rt2870.bin
>>> lrwxrwxrwx 1 root root 10
>>> Jan 31  2014 /lib/firmware/rt3090.bin -> rt2860.bin lrwxrwxrwx 1 root
>>> root 17 Jan 31  2014
>>> /lib/firmware/s2250.fw -> go7007/s2250-2.fw lrwxrwxrwx 1 root root 17
>>> Jan 31  2014
>>> /lib/firmware/s2250_loader.fw -> go7007/s2250-1.fw lrwxrwxrwx 1 root
>>> root 14 Jan 31
>>> 2014 /lib/firmware/ti-connectivity/wl1271-nvs.bin -> wl127x-nvs.bin
>>> lrwxrwxrwx 1 root root
>>> 14 Jan 31  2014 /lib/firmware/ti-connectivity/wl12xx-nvs.bin ->
>>> wl127x-nvs.bin
>>>
>>>>> The second one basically depends on the "compatible" string of the
>>>>> device, which identifies which device you have. The driver can then
>>>>> look up the file name for each device it supports based on this
>>>>> string, and by using multiple compatible strings in DT, you can
>>>>> provide the specific version of the hardware that is used for the
>>>>> file name without having to match that hardware name in the driver
>>>>>
>>>>
>>>> This make sense.
>>>>
>>>>>> W.r.t to the patch for documentation update, can I send an
>>>>>> incremental patch to the update the linux-firmware.git reference
>>>>>> as well?
>>>>>
>>>>> I'd rather skip that documentation change until we have decided on
>>>>> how to handle the firmware loading in the future.
>>>>
>>>> So if the above is acceptable, I will add the
>>>> ks2_qmss_pdsp_acc48_k2_le_1_0_0_x.bin name to the Kconfig
>>>> description as you have
>>> proposed.
>>>
>>> Maybe something shorter for the symlink? I don't know what all the
>>> parts of the name mean,
>>> but I assume that the _1_0_0_x can go, and the _le presumably refers
>>> to little-endian and
>>> can be removed as well, as the kernel expects the firmware to have a
>>> fixed endianess,
>>> independent of how the kernel is built.
>>
>> A generic name I can suggest is ks2_qmss_pdsp_acc48.bin in the code.
>> It describes, soc (ks2),
>> sub system (qmss), pdsp (CPU running the firmware), channel
>> configuration (acc48). Since there
>> can be other configuration (acc32) or firmware can be on a dsp as
>> well, this doesn't cause any
>> conflict in the future. I will go with this name if no one has
>> objections.
>>
>> In the file system, this can be a symbolic link which can point to the
>> actual firmware which is
>> obtained from the linux-firmware.git repo.
>>
>> If API change in future, then a new file name can be added at that
>> point and driver may be modified
>> to work with this new firmware. So this would accommodate future
>> upgrades as well.
>>
>> const char *qmss_acc_firmware[] = "ks2_qmss_pdsp_acc48.bin";
>>
>> This can be expanded in future if API change. The driver can start
>> searching for the firmware starting
>> with latest to oldest. If specific firmware requires customization of
>> the interface, this can be handled
>> by using additional firmware specific data in a struct instead of char
>> ptr array.
>>
>> In the file system, we could add a link to the real firmware file.
>>
>> ln -s /lib/firmware/ks2_qmss_pdsp_acc48.bin
>> /lib/firmware/ks2_qmss_pdsp_acc48_k2_le_1_0_0_x.bin
>>
>> Another thing I have noticed is that some of the firmware are named as
>> *.fw, while others are *.bin. Not sure
>> what the difference is and both suffixes are currently in use. I just
>> picked a bin suffix as this a binary blob for
>> the software.
>>
>> W.r.t documentation, Santosh has reservation on adding firmware name
>> to Kconfig description.
>> IMO, a better option is to add a Documentation for driver. Currently
>> driver design is also captured as part of the
>> DT bindings. We could just keep the DT bindings description in the DT
>> documentation and move the Design to
>> Documentation/arm/keystone/qmss.txt and provide a link to refer each
>> other. The firmware detail can be added
>> to this. Does it work?
>>
> Arnd, Santosh,
>
> Can I go ahead and send a patch based on my last response? Here is the
> summary of what we discussed.
>
> 1) Remove the firmware filename from DT to driver. Use a name
> ks2_qmss_pdsp_acc48.bin. The idea is this can be a soft link pointing to
> the real firmware file in file system
> 2. For future upgrade, if there is no interface change, the user may
> copy the new firmware to file system and rename the soft link to point
> to the new firmware
> 3) For future upgrade if there is a API change, add a new firmware file
> name in the driver and adapt the driver to the new API. Also provide
> backward compatibility to older firmware. i.e start search with new
> firmware and if not found, go back to the older firmwares until a file
> is found.
> 4) Move the description of the driver design from DT document to one
> under Documentation/arm/keystone/knav-qmss.txt. Update the this document
> with location of acc firmware in linux-firmware.git.
>
> I will work on this and send an updated patch series today assuming you
> are fine with the above changes. I would like this to be reviewed and
> merged to v4.4 next branch ASAP.
>
> Thanks and regards,
>
> Murali
>> Murali
>>
>>>
>>>     Arnd
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
>
Santosh, Arnd,

I have posted [PATCH v2 0/4] soc: ti: knav_qmss: enable accumulator 
queue support

Please review and merge if looks good. This is based on what we had 
discussed above.

-- 
Murali Karicheri
Linux Kernel, Keystone