回覆: [PATCH v4 8/9] PCI: aspeed: Add ASPEED PCIe RC driver

[Jacky Chou]

Hi Bjorn,

Thank you for your reply.

> > +config PCIE_ASPEED
> > +	bool "ASPEED PCIe controller"
> > +	depends on ARCH_ASPEED || COMPILE_TEST
> > +	depends on OF
> > +	depends on PCI_MSI
> > +	select IRQ_MSI_LIB
> > +	help
> > +	  Enable this option to support the PCIe controller found on ASPEED
> > +	  SoCs.
> > +
> > +	  This driver provides initialization and management for PCIe
> > +	  Root Complex functionality, including interrupt and MSI support.
> 
> Maybe "INTx and MSI support", since MSI is an interrupt?
> 

Agreed.

> > +/* Complete status */
> 
> "Completion"
> 

Agreed.

> > +static int aspeed_ast2700_ahb_remap_to_bar(struct aspeed_pcie *pcie)
> > +{
> > +	struct resource_entry *win, *tmp;
> > +	struct pci_host_bridge *bridge = pcie->host;
> > +
> > +	/* Configure AHB remapping to BAR on AST27x0.
> > +	 * The BAR region is HW-fixed in AST27x0, these BARs will be filled
> > +	 * in the ranges of pcie node in DT.
> > +	 */
> 
> I don't understand what "HW-fixed" means here.  It looks like you're writing
> host bridge window addresses (that came from DT) to the hardware.  That
> sounds like they're not actually "fixed" but programmable.
> 
> Host bridge windows are not BARs themselves.  Mem space for devices below
> the host bridge is allocated from the windows, and the addresses are
> programmed into BARs of those downstream devices.
> 

I checked our design, and there were errors in my description and usage.
I want to configure the "PCI address" from the range property of dts.
Therefore, I also get the wrong address, here will get the soc view address.
I will modify our code and my description in next version.

> Multi-line comment style:
> 
>   /*
>    * Configure ...
>    */
> 
> Wrap to fill 78 columns, or add blank lines between paragraphs.
> 

Agreed.

> > +	resource_list_for_each_entry_safe(win, tmp, &bridge->windows) {
> > +		struct resource *res = win->res;
> > +
> > +		if (resource_type(res) == IORESOURCE_MEM &&
> > +		    !(res->flags & IORESOURCE_MEM_64)) {
> > +			writel(ASPEED_REMAP_BAR_BASE(res->start),
> > +			       pcie->reg + ASPEED_H2X_REMAP_DIRECT_ADDR);
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +
> > +static int aspeed_ast2700_setup(struct platform_device *pdev) {
> > +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> > +	struct device *dev = pcie->dev;
> > +	int ret;
> > +
> > +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > +						    "aspeed,pciecfg");
> > +	if (IS_ERR(pcie->cfg))
> > +		return dev_err_probe(dev, PTR_ERR(pcie->cfg),
> > +				     "failed to map pciecfg base\n");
> > +
> > +	regmap_update_bits(pcie->cfg, ASPEED_SCU_60,
> > +			   ASPEED_RC_E2M_PATH_EN | ASPEED_RC_H2XS_PATH_EN |
> > +			   ASPEED_RC_H2XD_PATH_EN | ASPEED_RC_H2XX_PATH_EN
> |
> > +			   ASPEED_RC_UPSTREAM_MEM_EN,
> > +			   ASPEED_RC_E2M_PATH_EN | ASPEED_RC_H2XS_PATH_EN |
> > +			   ASPEED_RC_H2XD_PATH_EN | ASPEED_RC_H2XX_PATH_EN
> |
> > +			   ASPEED_RC_UPSTREAM_MEM_EN);
> > +	regmap_write(pcie->cfg, ASPEED_SCU_64,
> > +		     ASPEED_RC0_DECODE_DMA_BASE(0) |
> > +		     ASPEED_RC0_DECODE_DMA_LIMIT(0xff) |
> > +		     ASPEED_RC1_DECODE_DMA_BASE(0) |
> > +		     ASPEED_RC1_DECODE_DMA_LIMIT(0xff));
> > +	regmap_write(pcie->cfg, ASPEED_SCU_70, ASPEED_DISABLE_EP_FUNC);
> > +
> > +	aspeed_host_reset(pcie);
> > +
> > +	writel(0, pcie->reg + ASPEED_H2X_CTRL);
> > +	writel(ASPEED_H2X_BRIDGE_EN | ASPEED_H2X_BRIDGE_DIRECT_EN,
> > +	       pcie->reg + ASPEED_H2X_CTRL);
> > +
> > +	ret = aspeed_ast2700_ahb_remap_to_bar(pcie);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to assign BAR\n");
> 
> This is not assigning *BARs*.  A host bridge doesn't have BARs in the PCI spec
> sense.  It might have programmable address ranges, but the host bridge is not
> itself a PCI device, so its programmability is device specific.

As you said, host bridge doesn't have BARs.
Here I want to configure the "PCI address" to our design.
Will modify this in next version.

Thanks,
Jacky