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

[Karicheri, Muralidharan]

>-----Original Message-----
>From: Arnd Bergmann [mailto:arnd at arndb.de]
>Sent: Thursday, October 08, 2015 3:17 PM
>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 Thursday 08 October 2015 12:47:52 Murali Karicheri wrote:
>> Arnd,
>>
>> On 10/08/2015 11:41 AM, Arnd Bergmann wrote:
>> > On Tuesday 06 October 2015 10:19:18 Santosh Shilimkar wrote:
>> >> Couple of patches for ARM Keystone SOC drivers
>> >>          - irq affinity bug fix
>> >>          - display the firmware name
>> >>
>> >> ----------------------------------------------------------------
>> >> Murali Karicheri (2):
>> >>        soc: ti: reset irq affinity before freeing irq
>> >>        soc: ti: display firmware file name as part of boot log
>> >>
>> >>   .../bindings/soc/ti/keystone-navigator-qmss.txt      | 20
>+++++++++++++++++++-
>> >>   drivers/soc/ti/knav_qmss_acc.c                       |  4 ++++
>> >>   drivers/soc/ti/knav_qmss_queue.c                     |  3 +++
>> >>   3 files changed, 26 insertions(+), 1 deletion(-)
>> >
>> > The new text you add to the binding document doesn't really seem to
>> > belong in there, so I'm not pulling this until we've discussed how
>> > this should be better handled.
>> >
>> > Ideally, the firmware should just get merged into the
>> > linux-firmware.git tree.
>>
>> I have got the firmware merged to the linux-firmware.git. I will post
>> a patch to change the document to reflect that. Will it suffice?
>
>The binding document isn't really the right place for that. It's certainly good to document
>this, but somewhere else would be more appropriate.
>
>How about the Kconfig text for this driver?
>
>> > Regarding the method of storing the firmware file name in DT, we
>> > recently had a longer discussion about that and basically concluded
>> > that this doesn't work for most devices, in particular when the
>> > communication between the driver and the firmware uses an interface
>> > that is not 100% stable and can change depending on the firmware
>> > blob.
>> >
>> > Can you guarantee that there will never be changes to the interface?
>> This firmware is in use for almost 3 years for now on K2 SoCs, but in
>> a different kernel version. The interface itself is used not only by
>> Linux OS, but other OS as well. So it is not expected to change in the
>> future as backward compatibility will be an issue for other OS as well.
>
>Ok.
>
>> > If not, we should try to come up with a better mechanism here, and
>> > only provide the current method for backwards compatibility.
>> Assuming we need to change interface in the future, How is this
>> handled in other devices? Do they have something like a version to
>> check compatibility between software and firmware? As this firmware
>> has been frozen for almost 3 years, I wouldn't expect any change to
>> it. Could you point me to the thread where this was discussed in the
>> past. Also in my experience, mostly changes are to the firmware
>> itself, but not to the interface. In such cases, do we keep the name
>> of firmware in DT without version information so that in the file
>> system, user could  create a soft link to the firmware matching with
>> the DT name. So this way as long as interface doesn't change, firmware
>> upgrade is possible. If you agree, I can send an incremental patch to
>> rename the firmware to exclude the version information.
>
>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.
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?

>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.

Murali
>
>	Arnd