[PATCH v6 6/7] PCI: aspeed: Add ASPEED PCIe RC driver

[Jacky Chou]

Hi Bjorn,

Thank you for your reply.

> > +	u8 tx_tag;
> > +	int host_bus_num;
> 
> Only needs a u8.
> 

Agreed.

> > +static int aspeed_pcie_port_init(struct aspeed_pcie_port *port) {
> > +	struct aspeed_pcie *pcie = port->pcie;
> > +	struct device *dev = pcie->dev;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(port->clk);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to set clock for slot (%d)\n",
> > +				     port->slot);
> > +
> > +	ret = phy_init(port->phy);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to init phy pcie for slot (%d)\n",
> > +				     port->slot);
> > +
> > +	ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE,
> PHY_MODE_PCIE_RC);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to set phy mode for slot (%d)\n",
> > +				     port->slot);
> > +
> > +	reset_control_deassert(port->perst);
> > +	mdelay(PCIE_RESET_CONFIG_WAIT_MS);
> 
> I think this should use msleep().
> 

Agreed.

> > +static int aspeed_ast2600_setup(struct platform_device *pdev) {
> > +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> > +	struct device *dev = pcie->dev;
> > +
> > +	pcie->ahbc = syscon_regmap_lookup_by_phandle(dev->of_node,
> > +						     "aspeed,ahbc");
> > +	if (IS_ERR(pcie->ahbc))
> > +		return dev_err_probe(dev, PTR_ERR(pcie->ahbc),
> > +				     "failed to map ahbc base\n");
> > +
> > +	aspeed_host_reset(pcie);
> > +
> > +	regmap_write(pcie->ahbc, ASPEED_AHBC_KEY,
> ASPEED_AHBC_UNLOCK_KEY);
> > +	regmap_update_bits(pcie->ahbc, ASPEED_AHBC_ADDR_MAPPING,
> > +			   ASPEED_PCIE_RC_MEMORY_EN,
> ASPEED_PCIE_RC_MEMORY_EN);
> > +	regmap_write(pcie->ahbc, ASPEED_AHBC_KEY, ASPEED_AHBC_UNLOCK);
> > +
> > +	/* Due to the BAR assignment is fixed mapping on 0x6000_0000.*/
> > +	writel(ASPEED_AHB_REMAP_LO_ADDR(0x600) |
> ASPEED_AHB_MASK_LO_ADDR(0xe00),
> > +	       pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG0);
> > +	writel(ASPEED_AHB_REMAP_HI_ADDR(0),
> > +	       pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG1);
> 
> I assume this ASPEED_H2X_AHB_ADDR_CONFIG is doing basically the same
> thing as aspeed_ast2700_remap_pci_addr() below, so see the comments
> there.
> 

Yes.

> > +	writel(ASPEED_AHB_MASK_HI_ADDR(~0),
> > +	       pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG2);
> > +	writel(ASPEED_H2X_BRIDGE_EN, pcie->reg + ASPEED_H2X_CTRL);
> > +
> > +	writel(ASPEED_PCIE_RX_DMA_EN | ASPEED_PCIE_RX_LINEAR |
> > +	       ASPEED_PCIE_RX_MSI_SEL | ASPEED_PCIE_RX_MSI_EN |
> > +	       ASPEED_PCIE_WAIT_RX_TLP_CLR |
> ASPEED_PCIE_RC_RX_ENABLE |
> > +	       ASPEED_PCIE_RC_ENABLE,
> > +	       pcie->reg + ASPEED_H2X_DEV_CTRL);
> > +
> > +	writel(ASPEED_RC_TLP_TX_TAG_NUM, pcie->reg +
> ASPEED_H2X_DEV_TX_TAG);
> > +
> > +	pcie->host->ops = &aspeed_ast2600_pcie_ops;
> > +	pcie->host->child_ops = &aspeed_ast2600_pcie_child_ops;
> > +
> > +	return 0;
> > +}
> > +
> > +static int aspeed_ast2700_remap_pci_addr(struct aspeed_pcie *pcie) {
> > +	struct device_node *dev_node = pcie->dev->of_node;
> > +	struct of_pci_range range;
> > +	struct of_pci_range_parser parser;
> > +	int ret;
> > +
> > +	ret = of_pci_range_parser_init(&parser, dev_node);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for_each_of_pci_range(&parser, &range) {
> > +		if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) {
> > +			writel(ASPEED_REMAP_PCI_ADDR_31_12(range.pci_addr),
> > +			       pcie->reg + ASPEED_H2X_REMAP_PCI_ADDR_LO);
> > +			writel(ASPEED_REMAP_PCI_ADDR_63_32(range.pci_addr),
> > +			       pcie->reg + ASPEED_H2X_REMAP_PCI_ADDR_HI);
> > +			return 0;
> 
> It looks like this is essentially hardcoding the offset between the
> parent-bus-address and the child-bus-address in the DT 'ranges'
> property.  Since ASPEED_REMAP_PCI_ADDR_31_12() and
> ASPEED_REMAP_PCI_ADDR_63_32() do nothing except mask out the low 12
> bits, I assume that offset is zero.
> 
> But this should not be hard-coded at all; it should be extracted from 'ranges'.
> I don't think we really have a consistent way of doing this, but you can take a
> look at how these other drivers program "outbound" mappings like this using
> bridge->windows:
> 
>   cdns_pcie_hpa_host_init_address_translation()
>   dw_pcie_iatu_setup()
>   mobiveil_host_init()
>   xgene_pcie_map_ranges()
>   iproc_pcie_map_ranges()
>   rzg3s_pcie_parse_map_ranges()
> 

Thank you for providing the information.
We will refer to this information to integrate the PCIe mapping range.

> > +static int aspeed_pcie_parse_dt(struct aspeed_pcie *pcie) {
> > +	struct device *dev = pcie->dev;
> > +	struct device_node *node = dev->of_node;
> > +	int ret;
> > +
> > +	for_each_available_child_of_node_scoped(node, child) {
> > +		int slot;
> > +		const char *type;
> > +
> > +		ret = of_property_read_string(child, "device_type", &type);
> > +		if (ret || strcmp(type, "pci"))
> > +			continue;
> > +
> > +		ret = of_pci_get_devfn(child);
> > +		if (ret < 0)
> > +			return dev_err_probe(dev, ret,
> > +					     "failed to parse devfn\n");
> > +
> > +		slot = PCI_SLOT(ret);
> > +
> > +		ret = aspeed_pcie_parse_port(pcie, child, slot);
> > +		if (ret)
> > +			return ret;
> 
> It looks unnecessarily complicated to put each port on a list in
> aspeed_pcie_parse_port() and then iterate over that list in
> aspeed_pcie_init_ports().
> 
> I think you could just do something like:
> 
>   aspeed_pcie_parse_port();
>   aspeed_pcie_port_init();
> 
> and get rid of the list completely.
> 

Agreed.
We will remove the list and initialize the port directly.

> > +	}
> > +
> > +	if (list_empty(&pcie->ports))
> > +		return dev_err_probe(dev, -ENODEV,
> > +				     "No PCIe port found in DT\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int aspeed_pcie_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct pci_host_bridge *host;
> > +	struct aspeed_pcie *pcie;
> > +	struct resource_entry *entry;
> > +	const struct aspeed_pcie_rc_platform *md;
> > +	int irq, ret;
> > +
> > +	md = of_device_get_match_data(dev);
> > +	if (!md)
> > +		return -ENODEV;
> > +
> > +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> > +	if (!host)
> > +		return -ENOMEM;
> > +
> > +	pcie = pci_host_bridge_priv(host);
> > +	pcie->dev = dev;
> > +	pcie->tx_tag = 0;
> > +	platform_set_drvdata(pdev, pcie);
> > +
> > +	pcie->platform = md;
> > +	pcie->host = host;
> > +	INIT_LIST_HEAD(&pcie->ports);
> > +
> > +	/* Get root bus num for cfg command to decide tlp type 0 or type 1 */
> > +	entry = resource_list_first_type(&host->windows, IORESOURCE_BUS);
> > +	if (entry)
> > +		pcie->host_bus_num = entry->res->start;
> 
> s/host_bus_num/root_bus_nr/   to match struct altera_pcie

Agreed.

Thanks,
Jacky