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

[Florian Fainelli]

On 08/18/2017 02:58 PM, Arnd Bergmann wrote:
> On Thu, Aug 17, 2017 at 8:37 PM, Florian Fainelli <f.fainelli at 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.

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

> 
> Both of my points should be easy to address, so I haven't
> pulled the branch yet, but expect that it will make it into 4.14
> without problems.
> 
>        Arnd
> 


-- 
Florian