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

[Simon Glass]

Hi Jim,

On Fri, 8 May 2020 at 08:25, Jim Quinlan <james.quinlan at broadcom.com> wrote:
>
> On Fri, May 8, 2020 at 5:50 AM Nicolas Saenz Julienne
> <nsaenzjulienne at suse.de> wrote:
> >
> > Hi, I'll try to add my two cents.
> >
> > On Wed, 2020-05-06 at 08:47 -0600, Simon Glass wrote:
> > > > +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.
> >
> > STB is short for set top box, which is the wider family of SoCs bcm2711 is part
> > of[1].
> >
> > This implementation is, for now, tailor made for bcm2711. The bus not being
> > exposed, only the relevant bits to enabling xHCI compatibility, which is
> > hardwired into the RPi4 board, have been tested. For example there is so far no
> > support for IO memory mappings. Note that some work is already being done on
> > the Linux version of this driver to incorporate other devices, which will be
> > easily ported here, as for now, both are pretty much the same (minus interrupt
> > handling).
> >
> > [1] https://www.broadcom.com/products/broadband/set-top-box
> >
> > [...]
> >
> > > > +#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?
> >
> > See patch #7, where we import some bitfield helpers from Linux. The shift
> > calculation happens there.
> >
> > > Can you drop the PCIE_MISC prefix? These are very long and local to this file.
> >
> > This was also discussed when upstreaming this in Linux, Broadcom engineers
> > asked to keep the naming scheme as is, convoluted as it is, to help with
> > debugging in the future. I added Jim and Florian in to the conversation in case
> > they want to clarify something.
> >
> > [...]
> >
> > > > +
> > > > +       return !!FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK, val);
> > >
> > > What is FIELD_GET? We have this sort of thing rejected in the past.
> >
> > This is also part of the bitfield helpers introduced in patch #7
> >
> > [...]
> >
> > > > +static int brcm_pcie_probe(struct udevice *dev)
> > > > +{
> > > > +       struct udevice *ctlr = pci_get_controller(dev);
> > > > +       struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> > > > +       struct brcm_pcie *pcie = dev_get_priv(dev);
> > > > +       void __iomem *base = pcie->base;
> > > > +       bool ssc_good = false;
> > > > +       int num_out_wins = 0;
> > > > +       u64 rc_bar2_offset, rc_bar2_size;
> > > > +       unsigned int scb_size_val;
> > > > +       int i, ret;
> > > > +       u16 nlw, cls, lnksta;
> > > > +       u32 tmp;
> > > > +
> > > > +       /* Reset the bridge */
> > > > +       brcm_pcie_bridge_sw_init_set(pcie, 1);
> > > > +
> > > > +       udelay(150);
> > >
> > > Please add a comment as to how you chose the value, and below.
> >
> > This was picked from Jim Quinlan's original code submission:
> > https://lkml.org/lkml/2018/9/19/642
> >
> > Sadly there isn't any comment there.
>
> The bridge is being reset and then un-reset.  The delay is a safety
> precaution to preclude the reset signal from looking like a glitch.

OK well that is a reasonable comment. It is important to put comments
on delays since people coming later may make changes and not know what
the delays are for.

> BTW, what is the context here?  Aren't these patches already upstream?

Not yet.

Regards,
Simon