[GIT PULL 6/6] Broadcom drivers changes for 4.14

[f.fainelli@gmail.com (Florian Fainelli)]

On 08/19/2017 01:34 PM, Arnd Bergmann wrote:
> On Sat, Aug 19, 2017 at 12:35 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 08/18/2017 02:58 PM, Arnd Bergmann wrote:
>>> On Thu, Aug 17, 2017 at 8:37 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> The following changes since commit 5771a8c08880cdca3bfb4a3fc6d309d6bba20877:
>>>>
>>>>   Linux v4.13-rc1 (2017-07-15 15:22:10 -0700)
>>>>
>>>> are available in the git repository at:
>>>>
>>>>   http://github.com/Broadcom/stblinux.git tags/arm-soc/for-4.14/drivers
>>>>
>>>> for you to fetch changes up to 7f95522be533c27a918fe388ae5d733344660ef2:
>>>>
>>>>   soc bcm: brcmstb: Add support for S2/S3/S5 suspend states (MIPS) (2017-07-28 16:57:03 -0700)
>>>>
>>>> ----------------------------------------------------------------
>>>> This pull request contains Broadcom ARM/ARM64 SoC drivers changes for 4.14,
>>>> please pull the following:
>>>>
>>>> - Markus adds support for the Broadcom STB DDR PHY frontend which supports
>>>>   dynamic firmware loading and offers the ability to respond with DRAM refresh
>>>>   rates. He also adds a proper documentation binding document for that peripheral
>>>>
>>>
>>> I had not seen this driver before, but now I looked at it and have two small
>>> comments:
>>>
>>> - I'd rather see this added to drivers/memory than drivers/soc. The distinction
>>>   is not always clear, but I think that's where most of the DDR memory interface
>>>   drivers are at the moment.
>>
>> This driver does not control the SoC's memory interface though it does
>> talk to the frontend processor built into the DDR controller to first
>> load its firmware and then query it to get the DDR refresh rates. If you
>> feel like drivers/memory/broadcom/ is more appropriate I suppose we
>> could relocate the driver there.
> 
> Yes, I still think it makes sense to put it there, although it's not the only
> possible choice. drivers/memory is a very broad category that groups
> all kinds of things that are related to memory interfaces.

Alright, we'll move it there.

> 
>>> - In a function called __write_firmware, I stumbled over this small
>>>   hunk and similar functions elsewhere in the driver:
>>>
>>> +       /* Now copy it. */
>>> +       if (is_big_endian) {
>>> +               for (i = 0; i < size; i++)
>>> +                       writel_relaxed(be32_to_cpu(fw[i]), mem + i);
>>> +       } else {
>>> +               for (i = 0; i < size; i++)
>>> +                       writel_relaxed(fw[i], mem + i);
>>> +       }
>>>
>>> This looks wrong to me, as the behavior is different between
>>> little-endian and big-endian kernels: the former will byteswap
>>> big-endian fw images but not little-endian images, while the
>>> latter will byte-swap both.
>>>
>>> What is the expected behavior here?
>>
>> So the is_big_endian flag actually refers to the endianess of the DPFE
>> CPU/firmware image here, so if I read this code correctly, we have the
>> following happening
>>
>> LE kernel + BE DPFE: swapping from BE to LE during file read but not I/O
>> write (KO because resulting DPFE image is LE)
>> LE kernel + LE DPFE: no-swapping (OK)
>>
>> BE kernel + BE DPFE: swapping to LE during I/O write not file read (KO,
>> because resulting DPFE image is LE)
>> BE kernel + LE DPFE: swapping to LE during I/O write not file read (OK)
>>
>> Markus, does that sound like what is happening?
> 
> I suspect it would be better to never swap while copying the image from
> memory to I/O and always use memcpy_toio there, which performs
> a byte stream copy. The information you then need really is not what
> the endianess of the DPFE CPU is, but rather if it was stored as
> byte-reversed in the file.

I think you are right, it does not look like we need to swap and since
we are actually already checking the endianess of the file it should be
a lot simpler, we'll fix that too. Thanks!
-- 
Florian