[RFT PATCH v2 02/42] drivers: pci: host: iproc: Convert link check to raw PCI config accessors

From: ray.jui@broadcom.com (Ray Jui)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFT PATCH v2 02/42] drivers: pci: host: iproc: Convert link check to raw PCI config accessors
Date: Thu, 8 Jun 2017 08:56:05 -0700	[thread overview]
Message-ID: <0eb77019-9945-592c-375d-e2de742b3237@broadcom.com> (raw)
In-Reply-To: <20170608141342.2018-3-lorenzo.pieralisi@arm.com>

Hi Lorenzo,

Thanks, I'll try my best to find time to test this along 15/42 and 33/42 
patches. Hopefully I can get to that some time next week.

I have not yet reviewed these patches in details. Do they have 
dependency on other patches to the generic framework code you changed?

If so, is there a repo I can pull them?

Thanks,

Ray

On 6/8/2017 7:13 AM, Lorenzo Pieralisi wrote:
> Current iproc driver host bridge controller driver requires struct
> pci_bus to be created in order to carry out PCI link checks with standard
> PCI config space accessors.
>
> This struct pci_bus dependency is fictitious and burdens the driver
> with unneeded constraints (eg to use separate APIs to create and scan
> the root bus).
>
> Add PCI raw config space accessors to PCIe iproc driver and remove the
> fictitious struct pci_bus dependency.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Jon Mason <jonmason@broadcom.com>
> ---
>   drivers/pci/host/pcie-iproc.c | 94 ++++++++++++++++++++++++++++++++++---------
>   1 file changed, 74 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 0f39bd2..e48b8e2 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -452,14 +452,13 @@ static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
>    * Note access to the configuration registers are protected at the higher layer
>    * by 'pci_lock' in drivers/pci/access.c
>    */
> -static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
> +static void __iomem *iproc_pcie_map_cfg_bus(struct iproc_pcie *pcie,
> +					    int busno,
>   					    unsigned int devfn,
>   					    int where)
>   {
> -	struct iproc_pcie *pcie = iproc_data(bus);
>   	unsigned slot = PCI_SLOT(devfn);
>   	unsigned fn = PCI_FUNC(devfn);
> -	unsigned busno = bus->number;
>   	u32 val;
>   	u16 offset;
>   
> @@ -499,6 +498,58 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
>   		return (pcie->base + offset);
>   }
>   
> +static void __iomem *iproc_pcie_bus_map_cfg_bus(struct pci_bus *bus,
> +						unsigned int devfn,
> +						int where)
> +{
> +	return iproc_pcie_map_cfg_bus(iproc_data(bus), bus->number, devfn,
> +				      where);
> +}
> +
> +static int iproc_pci_raw_config_read32(struct iproc_pcie *pcie,
> +				       unsigned int devfn, int where,
> +				       int size, u32 *val)
> +{
> +	void __iomem *addr;
> +
> +	addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
> +	if (!addr) {
> +		*val = ~0;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	*val = readl(addr);
> +
> +	if (size <= 2)
> +		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int iproc_pci_raw_config_write32(struct iproc_pcie *pcie,
> +					unsigned int devfn, int where,
> +					int size, u32 val)
> +{
> +	void __iomem *addr;
> +	u32 mask, tmp;
> +
> +	addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
> +	if (!addr)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	if (size == 4) {
> +		writel(val, addr);
> +		return PCIBIOS_SUCCESSFUL;
> +	}
> +
> +	mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8));
> +	tmp = readl(addr) & mask;
> +	tmp |= val << ((where & 0x3) * 8);
> +	writel(tmp, addr);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
>   static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>   				    int where, int size, u32 *val)
>   {
> @@ -524,7 +575,7 @@ static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int devfn,
>   }
>   
>   static struct pci_ops iproc_pcie_ops = {
> -	.map_bus = iproc_pcie_map_cfg_bus,
> +	.map_bus = iproc_pcie_bus_map_cfg_bus,
>   	.read = iproc_pcie_config_read32,
>   	.write = iproc_pcie_config_write32,
>   };
> @@ -556,12 +607,11 @@ static void iproc_pcie_reset(struct iproc_pcie *pcie)
>   	msleep(100);
>   }
>   
> -static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
> +static int iproc_pcie_check_link(struct iproc_pcie *pcie)
>   {
>   	struct device *dev = pcie->dev;
> -	u8 hdr_type;
> -	u32 link_ctrl, class, val;
> -	u16 pos = PCI_EXP_CAP, link_status;
> +	u32 hdr_type, link_ctrl, link_status, class, val;
> +	u16 pos = PCI_EXP_CAP;
>   	bool link_is_active = false;
>   
>   	/*
> @@ -578,7 +628,7 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>   	}
>   
>   	/* make sure we are not in EP mode */
> -	pci_bus_read_config_byte(bus, 0, PCI_HEADER_TYPE, &hdr_type);
> +	iproc_pci_raw_config_read32(pcie,  0, PCI_HEADER_TYPE, 1, &hdr_type);
>   	if ((hdr_type & 0x7f) != PCI_HEADER_TYPE_BRIDGE) {
>   		dev_err(dev, "in EP mode, hdr=%#02x\n", hdr_type);
>   		return -EFAULT;
> @@ -588,13 +638,16 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>   #define PCI_BRIDGE_CTRL_REG_OFFSET 0x43c
>   #define PCI_CLASS_BRIDGE_MASK      0xffff00
>   #define PCI_CLASS_BRIDGE_SHIFT     8
> -	pci_bus_read_config_dword(bus, 0, PCI_BRIDGE_CTRL_REG_OFFSET, &class);
> +	iproc_pci_raw_config_read32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
> +				    4, &class);
>   	class &= ~PCI_CLASS_BRIDGE_MASK;
>   	class |= (PCI_CLASS_BRIDGE_PCI << PCI_CLASS_BRIDGE_SHIFT);
> -	pci_bus_write_config_dword(bus, 0, PCI_BRIDGE_CTRL_REG_OFFSET, class);
> +	iproc_pci_raw_config_write32(pcie, 0, PCI_BRIDGE_CTRL_REG_OFFSET,
> +				     4, class);
>   
>   	/* check link status to see if link is active */
> -	pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA, &link_status);
> +	iproc_pci_raw_config_read32(pcie, 0, pos + PCI_EXP_LNKSTA,
> +				    2, &link_status);
>   	if (link_status & PCI_EXP_LNKSTA_NLW)
>   		link_is_active = true;
>   
> @@ -603,20 +656,21 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
>   #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,
> -					  pos + PCI_EXP_LNKCTL2,
> +		iproc_pci_raw_config_read32(pcie, 0,
> +					  pos + PCI_EXP_LNKCTL2, 4,
>   					  &link_ctrl);
>   		if ((link_ctrl & PCI_TARGET_LINK_SPEED_MASK) ==
>   		    PCI_TARGET_LINK_SPEED_GEN2) {
>   			link_ctrl &= ~PCI_TARGET_LINK_SPEED_MASK;
>   			link_ctrl |= PCI_TARGET_LINK_SPEED_GEN1;
> -			pci_bus_write_config_dword(bus, 0,
> -					   pos + PCI_EXP_LNKCTL2,
> -					   link_ctrl);
> +			iproc_pci_raw_config_write32(pcie, 0,
> +						pos + PCI_EXP_LNKCTL2,
> +						4, link_ctrl);
>   			msleep(100);
>   
> -			pci_bus_read_config_word(bus, 0, pos + PCI_EXP_LNKSTA,
> -						 &link_status);
> +			iproc_pci_raw_config_read32(pcie, 0,
> +						pos + PCI_EXP_LNKSTA,
> +						2, &link_status);
>   			if (link_status & PCI_EXP_LNKSTA_NLW)
>   				link_is_active = true;
>   		}
> @@ -1260,7 +1314,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
>   	}
>   	pcie->root_bus = bus;
>   
> -	ret = iproc_pcie_check_link(pcie, bus);
> +	ret = iproc_pcie_check_link(pcie);
>   	if (ret) {
>   		dev_err(dev, "no PCIe EP device detected\n");
>   		goto err_rm_root_bus;