| Message ID | 20170817183748.1450-7-f.fainelli@gmail.com |
|---|---|
| State | New |
| Headers | show |
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. - 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? 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
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. > > - 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 >
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. >> - 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. Arnd
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!
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 - Brian adds support for S2/S3/S5 system suspend/resume modes on ARM-based SoCs which is not new but had been lingering for a long time - Justin adds S2/S3 system suspend/resume modes on MIPS-based SoCs which is a bit new newer and builds on top of the ARM-based support - Florian adds Device Tree binding documents for both ARM and MIPS based systems describing the necessary nodes for S2/S3/S5 on these SoCs ---------------------------------------------------------------- Brian Norris (1): soc: bcm: brcmstb: Add support for S2/S3/S5 suspend states (ARM) Florian Fainelli (2): dt-bindings: ARM: brcmstb: Update Broadcom STB Power Management binding dt-bindings: Document MIPS Broadcom STB power management nodes Justin Chen (1): soc bcm: brcmstb: Add support for S2/S3/S5 suspend states (MIPS) Markus Mayer (2): dt/bindings: Add bindings for Broadcom STB DRAM Sensors soc: brcmstb: Add driver for DPFE .../devicetree/bindings/arm/bcm/brcm,brcmstb.txt | 6 +- .../devicetree/bindings/mips/brcm/soc.txt | 153 ++++ .../devicetree/bindings/soc/bcm/brcm,dpfe-cpu.txt | 27 + MAINTAINERS | 8 + drivers/soc/bcm/Kconfig | 2 + drivers/soc/bcm/brcmstb/Kconfig | 9 + drivers/soc/bcm/brcmstb/Makefile | 3 +- drivers/soc/bcm/brcmstb/dpfe.c | 689 +++++++++++++++++ drivers/soc/bcm/brcmstb/pm/Makefile | 3 + drivers/soc/bcm/brcmstb/pm/aon_defs.h | 113 +++ drivers/soc/bcm/brcmstb/pm/pm-arm.c | 822 +++++++++++++++++++++ drivers/soc/bcm/brcmstb/pm/pm-mips.c | 461 ++++++++++++ drivers/soc/bcm/brcmstb/pm/pm.h | 89 +++ drivers/soc/bcm/brcmstb/pm/s2-arm.S | 76 ++ drivers/soc/bcm/brcmstb/pm/s2-mips.S | 200 +++++ drivers/soc/bcm/brcmstb/pm/s3-mips.S | 146 ++++ 16 files changed, 2805 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,dpfe-cpu.txt create mode 100644 drivers/soc/bcm/brcmstb/Kconfig create mode 100644 drivers/soc/bcm/brcmstb/dpfe.c create mode 100644 drivers/soc/bcm/brcmstb/pm/Makefile create mode 100644 drivers/soc/bcm/brcmstb/pm/aon_defs.h create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-arm.c create mode 100644 drivers/soc/bcm/brcmstb/pm/pm-mips.c create mode 100644 drivers/soc/bcm/brcmstb/pm/pm.h create mode 100644 drivers/soc/bcm/brcmstb/pm/s2-arm.S create mode 100644 drivers/soc/bcm/brcmstb/pm/s2-mips.S create mode 100644 drivers/soc/bcm/brcmstb/pm/s3-mips.S