Re: [PATCH v2 2/4] PCI: iproc: Add Broadcom iProc PCIe driver

[Arnd Bergmann]

On Friday 12 December 2014 09:08:48 Ray Jui wrote:
> 
> On 12/12/2014 4:29 AM, Arnd Bergmann wrote:
> > On Thursday 11 December 2014 18:36:55 Ray Jui wrote:
> >> Add initial version of the Broadcom iProc PCIe driver. This driver
> >> has been tested on NSP and Cygnus and is expected to work on all iProc
> >> family of SoCs that deploys the same PCIe host controller
> >>
> >> The driver also supports MSI
> >
> > Overall, I'm not convinced it's worth continuing this driver. While it
> > supports more features, Hauke's version seemed much cleaner, and I'd
> > rather see his driver merged and have you add the additional features.
> >
> > Why did you drop him from Cc again?
> >
> In fact, Hauke is on the "to" list of all v2 patchset that I sent out. I 
> added him to the "to" list because he mentioned during v1 patchset 
> review that he'll help to test this on North Star.

Sorry, my mistake. I looked through the Cc list multiple times but failed
to see him there.
 
> Doesn't Hauke's driver depends on BCMA? In that case, how does it work 
> on the SoCs that do not have the IP block to support BCMA?

I hadn't realized that there are some SoCs that are not BCMA based.
As the host controller implementation is closely related, we will
have to come up with some solution.

One way to solve this would be by turning the driver into a library
the same way as the pcie-dw driver, and have separate front-ends
for it for platform_device and bcma_device.

As I mentioned in my other reply, we will likely have the same problem
in a number of other drivers too, so we could try to come up with
a way to make bcma_device fit better into the platform_device
infrastructure, but I wouldn't know how to do that without giving
it more thought.

> >> +static void iproc_pcie_reset(struct iproc_pcie *pcie)
> >> +{
> >> +	u32 val;
> >> +
> >> +	/* send a downstream reset */
> >> +	val = EP_MODE_SURVIVE_PERST | RC_PCIE_RST_OUTPUT;
> >> +	writel(val, pcie->reg + CLK_CONTROL_OFFSET);
> >> +	udelay(250);
> >> +	val &= ~EP_MODE_SURVIVE_PERST;
> >> +	writel(val, pcie->reg + CLK_CONTROL_OFFSET);
> >> +	mdelay(250);
> >> +}
> >
> > 250ms delay is not acceptable. Please find a way to call this function
> > from a context in which you can sleep.
> >
> This is called from driver probe, where runs in the process context 
> where you can sleep. Is my understanding correct? I can change mdelay to 
> msleep so the CPU does not waste time spinning.

Right, that sounds good.

> >> +	if (nlw == 0) {
> >> +		/* try GEN 1 link speed */
> >> +#define PCI_LINK_STATUS_CTRL_2_OFFSET 0xDC
> >> +#define PCI_TARGET_LINK_SPEED_MASK    0xF
> >> +#define PCI_TARGET_LINK_SPEED_GEN2    0x2
> >> +#define PCI_TARGET_LINK_SPEED_GEN1    0x1
> >> +		pci_bus_read_config_dword(&bus, 0,
> >> +				PCI_LINK_STATUS_CTRL_2_OFFSET, &val);
> >> +		if ((val & PCI_TARGET_LINK_SPEED_MASK) ==
> >> +				PCI_TARGET_LINK_SPEED_GEN2) {
> >> +			val &= ~PCI_TARGET_LINK_SPEED_MASK;
> >> +			val |= PCI_TARGET_LINK_SPEED_GEN1;
> >> +			pci_bus_write_config_dword(&bus, 0,
> >> +					PCI_LINK_STATUS_CTRL_2_OFFSET, val);
> >> +			pci_bus_read_config_dword(&bus, 0,
> >> +					PCI_LINK_STATUS_CTRL_2_OFFSET, &val);
> >> +			mdelay(100);
> >
> > Too much delay.
> >
> Need to confirm with the ASIC engineer. The delay may be what's needed.

If you can turn it into msleep here, it's fine. Just don't waste 100
million CPU cycles for no reason.

> >> +static int __init iproc_pcie_init(void)
> >> +{
> >> +	return platform_driver_probe(&iproc_pcie_driver,
> >> +			iproc_pcie_probe);
> >> +}
> >> +subsys_initcall(iproc_pcie_init);
> >
> > module_init()? Doesn't seem necessary to have this early.
> >
> > 	Arnd
> >
> Will look into this. Forgot why we use subsys_initcall here...

Probably copied from other drivers. It used to be required.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/