[PATCH v2 09/10] pci: Add driver for Broadcom STB PCIe controller

[Simon Glass <sjg@chromium.org>]

Hi Sylwester,

On Fri, 8 May 2020 at 05:46, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>
> Hi Simon,
>
> On 06.05.2020 16:47, Simon Glass wrote:
> > On Mon, 4 May 2020 at 06:45, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>
> >> ---
> >>  drivers/pci/Kconfig        |   6 +
> >>  drivers/pci/Makefile       |   1 +
> >>  drivers/pci/pcie_brcmstb.c | 594 +++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 601 insertions(+)
> >>  create mode 100644 drivers/pci/pcie_brcmstb.c
> >
> > A few small comments.
>
> Thank you for time and a valuable review.
>
> >> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>
> >> +config PCI_BRCMSTB
> >> +       bool "Broadcom STB PCIe controller"
> >> +       depends on DM_PCI
> >> +       depends on ARCH_BCM283X
> >> +       help
> >> +         Say Y here if you want to enable Broadcom STB PCIe controller support.
> >
> > What is STB? What features does this support? You should get a warning
> > here to write at least three lines.
>
> I'm going to change that help text to something along the lines of:
>
>   Say Y here if you want to enable support for PCIe controller
>   on Broadcom set-top-box (STB) SoCs.
>   This driver currently supports only BCM2711 SoC and RC mode
>   of the controller.
>
> >> diff --git a/drivers/pci/pcie_brcmstb.c b/drivers/pci/pcie_brcmstb.c
> >> new file mode 100644
> >> index 0000000..c6ddf92
> >> --- /dev/null
> >> +++ b/drivers/pci/pcie_brcmstb.c
> >> @@ -0,0 +1,594 @@
>
> >> +#include <asm/io.h>
> >> +#include <common.h>
> >> +#include <dm.h>
> >> +#include <dm/ofnode.h>
> >> +#include <errno.h>
> >> +#include <linux/bitfield.h>
> >> +#include <linux/log2.h>
> >> +#include <pci.h>
> >
> > Check ordering of include files:
> >
> > https://protect2.fireeye.com/url?k=c3a0292d-9e737093-c3a1a262-0cc47a31ba82-690f9f10b3970f9d&q=1&u=https%3A%2F%2Fwww.denx.de%2Fwiki%2FU-Boot%2FCodingStyle
>
> Thanks for the hint, it felt there was something wrong with the ordering.
>
> >> +#define PCIE_MISC_MISC_CTRL                            0x4008
> >> +#define  PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK                0x1000
> >> +#define  PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK     0x2000
> >> +#define  PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK       0x300000
> >> +#define  PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_128                0x0
> >> +#define  PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK            0xf8000000
> >
> > If you have a _MASK, don't you need a _SHIFT to allow you to read from
> > the field?
>
> I had shift definitions originally but these got removed when we started to
> use the FIELD_GET macro and similar. Shifts are retrieved there from a mask
> by ffs() call (find first bit set in a word). If that's not preferred in
> u-boot I will switch back to using explicit shift definitions.

+Tom Rini

We've had a policy of avoiding these for some time, but now that Linux
has them, should we pull this in and start using it? It is not that
widely used in Linux but has been there for a few years.

Regards,
Simon