Message-ID: <DM6PR12MB40101465C6D032B025473EFADAC40@DM6PR12MB4010.namprd12.prod.outlook.com>
Date:   Mon, 22 Jul 2019 08:54:50 +0000
From:   Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>
To:     "Chocron, Jonathan" <jonnyc@...zon.com>,
        "jingoohan1@...il.com" <jingoohan1@...il.com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "Gustavo.Pimentel@...opsys.com" <Gustavo.Pimentel@...opsys.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        "Hanoch, Uri" <hanochu@...zon.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "Wasserstrom, Barak" <barakw@...zon.com>,
        "Saidi, Ali" <alisaidi@...zon.com>,
        "Hawa, Hanna" <hhhawa@...zon.com>,
        "Shenhar, Talel" <talel@...zon.com>,
        "Krupnik, Ronen" <ronenk@...zon.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "benh@...nel.crashing.org" <benh@...nel.crashing.org>
Subject: RE: [PATCH v2 6/8] PCI: al: Add support for DW based driver type

On Sun, Jul 21, 2019 at 16:8:18, Chocron, Jonathan <jonnyc@...zon.com> 
wrote:

> On Fri, 2019-07-19 at 08:55 +0000, Gustavo Pimentel wrote:
> > On Thu, Jul 18, 2019 at 10:47:16, Jonathan Chocron <jonnyc@...zon.com
> > > 
> > wrote:
> > 
> > > This driver is DT based and utilizes the DesignWare APIs.
> > > It allows using a smaller ECAM range for a larger bus range -
> > > usually an entire bus uses 1MB of address space, but the driver
> > > can use it for a larger number of buses.
> > > 
> > > All link initializations are handled by the boot FW.
> > > 
> > > Signed-off-by: Jonathan Chocron <jonnyc@...zon.com>
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig   |  12 +
> > >  drivers/pci/controller/dwc/pcie-al.c | 373
> > > +++++++++++++++++++++++++++
> > >  2 files changed, 385 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/Kconfig
> > > b/drivers/pci/controller/dwc/Kconfig
> > > index 6ea778ae4877..3c6094cbcc3b 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -230,4 +230,16 @@ config PCIE_UNIPHIER
> > >  	  Say Y here if you want PCIe controller support on UniPhier
> > > SoCs.
> > >  	  This driver supports LD20 and PXs3 SoCs.
> > >  
> > > +config PCIE_AL
> > > +	bool "Amazon Annapurna Labs PCIe controller"
> > > +	depends on OF && (ARM64 || COMPILE_TEST)
> > > +	depends on PCI_MSI_IRQ_DOMAIN
> > > +	select PCIE_DW_HOST
> > > +	help
> > > +	  Say Y here to enable support of the Amazon's Annapurna Labs
> > > PCIe
> > > +	  controller IP on Amazon SoCs. The PCIe controller uses the
> > > DesignWare
> > > +	  core plus Annapurna Labs proprietary hardware wrappers. This
> > > is
> > > +	  required only for DT-based platforms. ACPI platforms with the
> > > +	  Annapurna Labs PCIe controller don't need to enable this.
> > > +
> > >  endmenu
> > > diff --git a/drivers/pci/controller/dwc/pcie-al.c
> > > b/drivers/pci/controller/dwc/pcie-al.c
> > > index 3ab58f0584a8..40555532fb9a 100644
> > > --- a/drivers/pci/controller/dwc/pcie-al.c
> > > +++ b/drivers/pci/controller/dwc/pcie-al.c
> > > @@ -91,3 +91,376 @@ struct pci_ecam_ops al_pcie_ops = {
> > >  };
> > >  
> > >  #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> > > +
> > > +#ifdef CONFIG_PCIE_AL
> > > +
> > > +#include <linux/of_pci.h>
> > > +#include "pcie-designware.h"
> > > +
> > > +#define AL_PCIE_REV_ID_2	2
> > > +#define AL_PCIE_REV_ID_3	3
> > > +#define AL_PCIE_REV_ID_4	4
> > > +
> > > +#define AXI_BASE_OFFSET		0x0
> > > +
> > > +#define DEVICE_ID_OFFSET	0x16c
> > > +
> > > +#define DEVICE_REV_ID			0x0
> > > +#define DEVICE_REV_ID_DEV_ID_MASK	GENMASK(31, 16)
> > > +
> > > +#define DEVICE_REV_ID_DEV_ID_X4		0
> > > +#define DEVICE_REV_ID_DEV_ID_X8		2
> > > +#define DEVICE_REV_ID_DEV_ID_X16	4
> > > +
> > > +#define OB_CTRL_REV1_2_OFFSET	0x0040
> > > +#define OB_CTRL_REV3_5_OFFSET	0x0030
> > > +
> > > +#define CFG_TARGET_BUS			0x0
> > > +#define CFG_TARGET_BUS_MASK_MASK	GENMASK(7, 0)
> > > +#define CFG_TARGET_BUS_BUSNUM_MASK	GENMASK(15, 8)
> > > +
> > > +#define CFG_CONTROL			0x4
> > > +#define CFG_CONTROL_SUBBUS_MASK		GENMASK(15, 8)
> > > +#define CFG_CONTROL_SEC_BUS_MASK	GENMASK(23, 16)
> > > +
> > > +struct al_pcie_reg_offsets {
> > > +	unsigned int ob_ctrl;
> > > +};
> > > +
> > > +struct al_pcie_target_bus_cfg {
> > > +	u8 reg_val;
> > > +	u8 reg_mask;
> > > +	u8 ecam_mask;
> > > +};
> > > +
> > > +struct al_pcie {
> > > +	struct dw_pcie *pci;
> > > +	void __iomem *controller_base; /* base of PCIe unit (not DW
> > > core) */
> > > +	struct device *dev;
> > > +	resource_size_t ecam_size;
> > > +	unsigned int controller_rev_id;
> > > +	struct al_pcie_reg_offsets reg_offsets;
> > > +	struct al_pcie_target_bus_cfg target_bus_cfg;
> > > +};
> > > +
> > > +#define PCIE_ECAM_DEVFN(x)		(((x) & 0xff) << 12)
> > > +
> > > +#define to_al_pcie(x)		dev_get_drvdata((x)->dev)
> > > +
> > > +static int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int
> > > *rev_id)
> > > +{
> > > +	void __iomem *dev_rev_id_addr;
> > > +	u32 dev_rev_id;
> > > +
> > > +	dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie-
> > > >controller_base +
> > > +			  AXI_BASE_OFFSET + DEVICE_ID_OFFSET +
> > > DEVICE_REV_ID);
> > > +
> > > +	dev_rev_id = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK,
> > > +			       readl(dev_rev_id_addr));
> > > +	switch (dev_rev_id) {
> > > +	case DEVICE_REV_ID_DEV_ID_X4:
> > > +		*rev_id = AL_PCIE_REV_ID_2;
> > > +		break;
> > > +	case DEVICE_REV_ID_DEV_ID_X8:
> > > +		*rev_id = AL_PCIE_REV_ID_3;
> > > +		break;
> > > +	case DEVICE_REV_ID_DEV_ID_X16:
> > > +		*rev_id = AL_PCIE_REV_ID_4;
> > > +		break;
> > > +	default:
> > > +		dev_err(pcie->dev, "Unsupported dev_rev_id (0x%x)\n",
> > > +			dev_rev_id);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	dev_dbg(pcie->dev, "dev_rev_id: 0x%x\n", dev_rev_id);
> > 
> > Consider s/dev_dbg/pci_dbg/g
> > 
> There is no struct pci_dev context (the dev belongs to the
> platform_device).

It seems so. It sucks...
Disregard this then.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
> > > +{
> > > +	switch (pcie->controller_rev_id) {
> > > +	case AL_PCIE_REV_ID_2:
> > > +		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
> > > +		break;
> > > +	case AL_PCIE_REV_ID_3:
> > > +	case AL_PCIE_REV_ID_4:
> > > +		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
> > > +		break;
> > > +	default:
> > > +		dev_err(pcie->dev, "Unsupported controller rev_id:
> > > 0x%x\n",
> > > +			pcie->controller_rev_id);
> > 
> > Consider s/dev_err/pci_err/g
> > 
> Same as above.
> 
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
> > > +					  u8 target_bus,
> > > +					  u8 mask_target_bus)
> > > +{
> > > +	void __iomem *cfg_control_addr;
> > > +	u32 reg;
> > > +
> > > +	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
> > > +	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
> > > +
> > > +	cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > > >controller_base +
> > > +			   AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl
> > > +
> > > +			   CFG_TARGET_BUS);
> > > +
> > > +	writel(reg, cfg_control_addr);
> > 
> > From what I'm seeing you commonly use writel() and readl() with a
> > common 
> > base address, such as pcie->controller_base + AXI_BASE_OFFSET.
> > I'd suggest to creating a writel and readl with that offset built-in.
> > 
> I prefer to keep it generic, since in future revisions we might want to
> access regs which are not in the AXI region. You think I should add
> wrappers which simply hide the pcie->controller_base part?

I and other developers typically do that, but it's a suggestion. IMHO it 
helps to keep the code cleaner and more readable.

> 
> > > +}
> > > +
> > > +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> > > +					   unsigned int busnr,
> > > +					   unsigned int devfn)
> > > +{
> > > +	void __iomem *pci_base_addr;
> > 
> > Consider passing this variable declaration to the bottom, following
> > the 
> > reverse tree order.
> > 
> Done. Moved 'struct pcie_port *pp' as well.
> 
> > > +	struct pcie_port *pp = &pcie->pci->pp;
> > > +	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie-
> > > >target_bus_cfg;
> > > +	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
> > > +	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
> > > +
> > > +	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> > > +					 (busnr_ecam << 20) +
> > > +					 PCIE_ECAM_DEVFN(devfn));
> > > +
> > > +	if (busnr_reg != target_bus_cfg->reg_val) {
> > > +		dev_dbg(pcie->pci->dev, "Changing target bus busnum val
> > > from 0x%x to 0x%x\n",
> > > +			target_bus_cfg->reg_val, busnr_reg);
> > > +		target_bus_cfg->reg_val = busnr_reg;
> > > +		al_pcie_target_bus_set(pcie,
> > > +				       target_bus_cfg->reg_val,
> > > +				       target_bus_cfg->reg_mask);
> > > +	}
> > > +
> > > +	return pci_base_addr;
> > > +}
> > > +
> > > +static int al_pcie_rd_other_conf(struct pcie_port *pp, struct
> > > pci_bus *bus,
> > > +				 unsigned int devfn, int where, int
> > > size,
> > > +				 u32 *val)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct al_pcie *pcie = to_al_pcie(pci);
> > > +	unsigned int busnr = bus->number;
> > > +	void __iomem *pci_addr;
> > > +	int rc;
> > > +
> > > +	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> > > +
> > > +	rc = dw_pcie_read(pci_addr + where, size, val);
> > > +
> > > +	dev_dbg(pci->dev, "%d-byte config read from %04x:%02x:%02x.%d
> > > offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
> > > +		size, pci_domain_nr(bus), bus->number,
> > > +		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> > > +		(pci_addr + where), *val);
> > > +
> > > +	return rc;
> > > +}
> > > +
> > > +static int al_pcie_wr_other_conf(struct pcie_port *pp, struct
> > > pci_bus *bus,
> > > +				 unsigned int devfn, int where, int
> > > size,
> > > +				 u32 val)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct al_pcie *pcie = to_al_pcie(pci);
> > > +	unsigned int busnr = bus->number;
> > > +	void __iomem *pci_addr;
> > > +	int rc;
> > > +
> > > +	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> > > +
> > > +	rc = dw_pcie_write(pci_addr + where, size, val);
> > > +
> > > +	dev_err(pci->dev, "%d-byte config write to %04x:%02x:%02x.%d
> > > offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
> > > +		size, pci_domain_nr(bus), bus->number,
> > > +		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> > > +		(pci_addr + where), val);
> > > +
> > > +	return rc;
> > > +}
> > > +
> > > +static int al_pcie_config_prepare(struct al_pcie *pcie)
> > > +{
> > > +	struct al_pcie_target_bus_cfg *target_bus_cfg;
> > > +	struct pcie_port *pp = &pcie->pci->pp;
> > > +	unsigned int ecam_bus_mask;
> > > +	u8 secondary_bus;
> > > +	u8 subordinate_bus;
> > > +	void __iomem *cfg_control_addr;
> > > +	u32 cfg_control;
> > > +	u32 reg;
> > > +
> > > +	target_bus_cfg = &pcie->target_bus_cfg;
> > > +
> > > +	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> > > +	if (ecam_bus_mask > 255) {
> > > +		dev_warn(pcie->dev, "ECAM window size is larger than
> > > 256MB. Cutting off at 256\n");
> > > +		ecam_bus_mask = 255;
> > > +	}
> > > +
> > > +	/* This portion is taken from the transaction address */
> > > +	target_bus_cfg->ecam_mask = ecam_bus_mask;
> > > +	/* This portion is taken from the cfg_target_bus reg */
> > > +	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> > > +	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg-
> > > >reg_mask;
> > > +
> > > +	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
> > > +			       target_bus_cfg->reg_mask);
> > > +
> > > +	secondary_bus = pp->busn->start + 1;
> > > +	subordinate_bus = pp->busn->end;
> > > +
> > > +	/* Set the valid values of secondary and subordinate buses */
> > > +	cfg_control_addr = pcie->controller_base + AXI_BASE_OFFSET +
> > > +			   pcie->reg_offsets.ob_ctrl + CFG_CONTROL;
> > > +
> > > +	cfg_control = readl(cfg_control_addr);
> > > +
> > > +	reg = cfg_control &
> > > +	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
> > > +
> > > +	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
> > > +	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
> > > +
> > > +	writel(reg, cfg_control_addr);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int al_pcie_host_init(struct pcie_port *pp)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct al_pcie *pcie = to_al_pcie(pci);
> > > +	int link_up;
> > > +	int rc;
> > > +
> > > +	link_up = dw_pcie_link_up(pci);
> > > +	if (!link_up) {
> > > +		dev_err(pci->dev, "link is not up!\n");
> > > +		return -ENOLINK;
> > > +	}
> > > +
> > > +	dev_info(pci->dev, "link is up\n");
> > 
> > Consider s/dev_info/pci_info/g
> > 
> Same as the response above.
> 
> > > +
> > > +	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	rc = al_pcie_reg_offsets_set(pcie);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	rc = al_pcie_config_prepare(pcie);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> > > +	.rd_other_conf = al_pcie_rd_other_conf,
> > > +	.wr_other_conf = al_pcie_wr_other_conf,
> > > +	.host_init = al_pcie_host_init,
> > > +};
> > > +
> > > +static int al_add_pcie_port(struct pcie_port *pp,
> > > +			    struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	int ret;
> > > +
> > > +	pp->ops = &al_pcie_host_ops;
> > > +
> > > +	ret = dw_pcie_host_init(pp);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to initialize host\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct dw_pcie_ops dw_pcie_ops = {
> > > +};
> > > +
> > > +static int al_pcie_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct al_pcie *al_pcie;
> > > +	struct dw_pcie *pci;
> > > +	struct resource *dbi_res;
> > > +	struct resource *controller_res;
> > > +	struct resource *ecam_res;
> > > +	int ret;
> > 
> > Please sort the variables following the reverse tree order.
> > 
> Done. 
> 
> I'd think that it would make sense to group variables which have a
> common characteristic (e.g. resources read from the DT), even if it
> mildly breaks the convention (as long as the general frame is longest
> to shortest). Does this sound ok?
> 
> BTW, I couldn't find any documentation regarding the reverse-tree
> convention, do you have a pointer to some?

There isn't as far as I know, but it's a convention normally used in 
several subsystems.
I suppose it's a way to try to keep some organization along with the code 

style. IMHO it doesn't harm, but it's just a suggestion. 😊

> 
> > > +
> > > +	al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> > > +	if (!al_pcie)
> > > +		return -ENOMEM;
> > > +
> > > +	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > +	if (!pci)
> > > +		return -ENOMEM;
> > > +
> > > +	pci->dev = dev;
> > > +	pci->ops = &dw_pcie_ops;
> > > +
> > > +	al_pcie->pci = pci;
> > > +	al_pcie->dev = dev;
> > > +
> > > +	dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > "dbi");
> > > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res);
> > > +	if (IS_ERR(pci->dbi_base)) {
> > > +		dev_err(dev, "couldn't remap dbi base %pR\n", dbi_res);
> > > +		return PTR_ERR(pci->dbi_base);
> > > +	}
> > > +
> > > +	ecam_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > "config");
> > > +	if (!ecam_res) {
> > > +		dev_err(dev, "couldn't find 'config' reg in DT\n");
> > > +		return -ENOENT;
> > > +	}
> > > +	al_pcie->ecam_size = resource_size(ecam_res);
> > > +
> > > +	controller_res = platform_get_resource_byname(pdev,
> > > IORESOURCE_MEM,
> > > +						      "controller");
> > > +	al_pcie->controller_base = devm_ioremap_resource(dev,
> > > controller_res);
> > > +	if (IS_ERR(al_pcie->controller_base)) {
> > > +		dev_err(dev, "couldn't remap controller base %pR\n",
> > > +			controller_res);
> > > +		return PTR_ERR(al_pcie->controller_base);
> > > +	}
> > > +
> > > +	dev_dbg(dev, "From DT: dbi_base: %pR, controller_base: %pR\n",
> > > +		dbi_res, controller_res);
> > > +
> > > +	platform_set_drvdata(pdev, al_pcie);
> > > +
> > > +	ret = al_add_pcie_port(&pci->pp, pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > 
> > Those operations are redundant, aren't they? They can be replaced
> > just 
> > by:
> > 
> > return ret;
> > 
> 
> Ack.

Thanks.

> 
> > > +}
> > > +
> > > +static const struct of_device_id al_pcie_of_match[] = {
> > > +	{ .compatible = "amazon,al-pcie",
> > > +	},
> > > +	{},
> > > +};
> > > +
> > > +static struct platform_driver al_pcie_driver = {
> > > +	.driver = {
> > > +		.name	= "al-pcie",
> > > +		.of_match_table = al_pcie_of_match,
> > > +		.suppress_bind_attrs = true,
> > > +	},
> > > +	.probe = al_pcie_probe,
> > > +};
> > > +builtin_platform_driver(al_pcie_driver);
> > > +
> > > +#endif /* CONFIG_PCIE_AL*/
> > > -- 
> > > 2.17.1
> > 
> > 

Powered by blists - more mailing lists