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
signature.asc
Description: This is a digitally signed message part
