Re: [PATCH 08/21] PCI: designware: Share code for dw_pcie_rd/wr_other_conf()

[Gustavo Pimentel <gustavo.pimentel@synopsys.com>]

Hi,

On 21/12/2018 07:27, Andrey Smirnov wrote:
> Default implementation of pcie_rd_other_conf() and
> dw_pcie_wd_other_conf() share more than 80% of their code. Move shared
> code into a dedicated subroutine and convert pcie_rd_other_conf() and
> dw_pcie_wd_other_conf() to use it. No functional change intended.
> 
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: linux-imx@nxp.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 61 +++++++------------
>  1 file changed, 23 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 721d60a5d9e4..8f957cd6901b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -512,8 +512,9 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	return ret;
>  }
>  
> -static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> -				 u32 devfn, int where, int size, u32 *val)
> +static int dw_pcie_access_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				     u32 devfn, int where, int size, u32 *val,
> +				     bool write)
>  {
>  	int ret, type;
>  	u32 busdev, cfg_size;
> @@ -521,9 +522,6 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  	void __iomem *va_cfg_base;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  
> -	if (pp->ops->rd_other_conf)
> -		return pp->ops->rd_other_conf(pp, bus, devfn, where, size, val);
> -
>  	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
>  		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
>  
> @@ -542,7 +540,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
>  				  type, cpu_addr,
>  				  busdev, cfg_size);
> -	ret = dw_pcie_read(va_cfg_base + where, size, val);
> +	if (write)
> +		ret = dw_pcie_write(va_cfg_base + where, size, *val);
> +	else
> +		ret = dw_pcie_read(va_cfg_base + where, size, val);
> +
>  	if (pci->num_viewport <= 2)
>  		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
>  					  PCIE_ATU_TYPE_IO, pp->io_base,
> @@ -551,43 +553,26 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  	return ret;
>  }
>  
> +static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				 u32 devfn, int where, int size, u32 *val)
> +{
> +	if (pp->ops->rd_other_conf)
> +		return pp->ops->rd_other_conf(pp, bus, devfn, where,
> +					      size, val);
> +
> +	return dw_pcie_access_other_conf(pp, bus, devfn, where, size, val,
> +					 false);
> +}
> +
>  static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>  				 u32 devfn, int where, int size, u32 val)
>  {
> -	int ret, type;
> -	u32 busdev, cfg_size;
> -	u64 cpu_addr;
> -	void __iomem *va_cfg_base;
> -	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> -
>  	if (pp->ops->wr_other_conf)
> -		return pp->ops->wr_other_conf(pp, bus, devfn, where, size, val);
> -
> -	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> -		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
> +		return pp->ops->wr_other_conf(pp, bus, devfn, where,
> +					      size, val);
>  
> -	if (bus->parent->number == pp->root_bus_nr) {
> -		type = PCIE_ATU_TYPE_CFG0;
> -		cpu_addr = pp->cfg0_base;
> -		cfg_size = pp->cfg0_size;
> -		va_cfg_base = pp->va_cfg0_base;
> -	} else {
> -		type = PCIE_ATU_TYPE_CFG1;
> -		cpu_addr = pp->cfg1_base;
> -		cfg_size = pp->cfg1_size;
> -		va_cfg_base = pp->va_cfg1_base;
> -	}
> -
> -	dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> -				  type, cpu_addr,
> -				  busdev, cfg_size);
> -	ret = dw_pcie_write(va_cfg_base + where, size, val);
> -	if (pci->num_viewport <= 2)
> -		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX1,
> -					  PCIE_ATU_TYPE_IO, pp->io_base,
> -					  pp->io_bus_addr, pp->io_size);
> -
> -	return ret;
> +	return dw_pcie_access_other_conf(pp, bus, devfn, where, size, &val,
> +					 true);
>  }
>  
>  static int dw_pcie_valid_device(struct pcie_port *pp, struct pci_bus *bus,
> 

Nice!

Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

Regards,
Gustavo