On Fri, May 8, 2020 at 5:50 AM Nicolas Saenz Julienne <nsaenzjulie...@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 >
