Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Bjorn Helgaas <bhelgaas@google.com>]

On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote:
> From: gabriele paoloni <gabriele.paoloni@huawei.com>
> 
> This patch changes the implementation of dw_pcie_cfg_read() and
> dw_pcie_cfg_write() to improve the function usage from the callers
> perspective.
> Currently the callers are obliged to pass the 32bit aligned address
> of the register that contains the field of the PCI header that they
> want to read/write; also they have to pass the offset of the field
> in that register. This is quite tricky to use as the callers are
> obliged to sum the PCI header base address to the field offset
> masked to retrieve the 32b aligned register address.
> 
> With the new API the callers have to pass the base address of the
> PCI header and the offset corresponding to the field they intend to
> read/write.
> 
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  drivers/pci/host/pci-exynos.c      |  5 ++---
>  drivers/pci/host/pci-keystone-dw.c |  4 ++--
>  drivers/pci/host/pcie-designware.c | 28 ++++++++++++++--------------
>  drivers/pci/host/pcie-spear13xx.c  | 26 ++++++++++++--------------
>  4 files changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> index f9f468d..8b0e04b 100644
> --- a/drivers/pci/host/pci-exynos.c
> +++ b/drivers/pci/host/pci-exynos.c
> @@ -454,7 +454,7 @@ static int exynos_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>  	int ret;
>  
>  	exynos_pcie_sideband_dbi_r_mode(pp, true);
> -	ret = dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, size, val);
> +	ret = dw_pcie_cfg_read(pp->dbi_base, where, size, val);
>  	exynos_pcie_sideband_dbi_r_mode(pp, false);
>  	return ret;
>  }

Is there really any value in keeping "addr" and "where" separate?
dw_pcie_cfg_write() clearly doesn't care; it just adds them together.  I
don't think dw_pcie_cfg_read() needs to care either: it could round the
address down to a 32-bit boundary and use the difference to compute the
mask and shift.

So I'm proposing something like this:

  int dw_pcie_cfg_read(void __iomem *addr, int size, u32 *val)
  int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val)

>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>  {
> +	addr += (where & ~0x3);
>  	*val = readl(addr);
> +	where &= 3;
>  
>  	if (size == 1)
> -		*val = (*val >> (8 * (where & 3))) & 0xff;
> +		*val = (*val >> (8 * where)) & 0xff;
>  	else if (size == 2)
> -		*val = (*val >> (8 * (where & 3))) & 0xffff;
> +		*val = (*val >> (8 * where)) & 0xffff;
>  	else if (size != 4)
>  		return PCIBIOS_BAD_REGISTER_NUMBER;
>  
> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>  
>  int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val)
>  {
> +	addr += where;
> +
>  	if (size == 4)
>  		writel(val, addr);
>  	else if (size == 2)
> -		writew(val, addr + (where & 2));
> +		writew(val, addr);
>  	else if (size == 1)
> -		writeb(val, addr + (where & 3));
> +		writeb(val, addr);
>  	else
>  		return PCIBIOS_BAD_REGISTER_NUMBER;