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

[Nicolas Saenz Julienne]

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.

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200508/8e017400/attachment.sig>