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

[Jim Quinlan]

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.
BTW, what is the context here?  Aren't these patches already upstream?
Regards,
Jim

>
> Regards,
> Nicolas
>