Re: [PATCH RESEND v4 10/15] PCI: dwc: Drop iATU regions enumeration - dw_pcie_region_type

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Fri, Jun 24, 2022 at 05:39:42PM +0300, Serge Semin wrote:
> There is no point in having the dw_pcie_region_type enumeration for almost
> the same reasons as it was stated for dw_pcie_as_type. First of all it's
> redundant since the driver already has a set of the macros declared which
> describe the possible inbound and outbound iATU regions. Having an
> addition abstraction just needlessly complicates the code. Secondly
> checking the region type passed to the dw_pcie_disable_atu() method for
> validity is pointless since the erroneous situation is just ignored in the
> current method implementation. So to speak let's drop the redundant
> dw_pcie_region_type enumeration replacing it with the direct iATU
> direction macro usage.
> 
> Since the dw_pcie_disable_atu() method now directly accepts the
> in-/outbound iATU region direction instead of the abstract region type we
> need to change the argument name and the arguments order. The later change
> makes the function prototype looking more logical since the passed index
> indicates an iATU window within the regions with the corresponding
> direction.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> ---
> 
> Changelog v2:
> - Move this patch to being applied before the IB/OB iATU windows setup
>   simplification patch (@Rob).
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   |  4 +--
>  .../pci/controller/dwc/pcie-designware-host.c |  2 +-
>  drivers/pci/controller/dwc/pcie-designware.c  | 28 +++++--------------
>  drivers/pci/controller/dwc/pcie-designware.h  | 13 ++-------
>  4 files changed, 13 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 5a158813f687..2e91222f7c98 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -212,7 +212,7 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  
>  	__dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags);
>  
> -	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
> +	dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index);
>  	clear_bit(atu_index, ep->ib_window_map);
>  	ep->epf_bar[bar] = NULL;
>  }
> @@ -286,7 +286,7 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	if (ret < 0)
>  		return;
>  
> -	dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_OUTBOUND);
> +	dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, atu_index);
>  	clear_bit(atu_index, ep->ob_window_map);
>  }
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 4f984c845b59..e0a2819608c6 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -618,7 +618,7 @@ void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  		 * multiple matches
>  		 */
>  		for (i = 0; i < pci->num_ob_windows; i++)
> -			dw_pcie_disable_atu(pci, i, DW_PCIE_REGION_OUTBOUND);
> +			dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, i);
>  
>  		/* Get last memory resource entry */
>  		resource_list_for_each_entry(entry, &pp->bridge->windows) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 330575182712..a90d3f6ce50c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -353,7 +353,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  	limit_addr = cpu_addr + size - 1;
>  
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT,
> -			   PCIE_ATU_REGION_OUTBOUND | index);
> +			   PCIE_ATU_REGION_DIR_OB | index);
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_BASE,
>  			   lower_32_bits(cpu_addr));
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_BASE,
> @@ -464,7 +464,7 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  		return dw_pcie_prog_inbound_atu_unroll(pci, func_no, index, type,
>  						       cpu_addr, bar);
>  
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_INBOUND |
> +	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_IB |
>  			   index);
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, lower_32_bits(cpu_addr));
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, upper_32_bits(cpu_addr));
> @@ -491,24 +491,10 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  	return -EBUSY;
>  }
>  
> -void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> -			 enum dw_pcie_region_type type)
> +void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
>  {
> -	u32 region;
> -
> -	switch (type) {
> -	case DW_PCIE_REGION_INBOUND:
> -		region = PCIE_ATU_REGION_INBOUND;
> -		break;
> -	case DW_PCIE_REGION_OUTBOUND:
> -		region = PCIE_ATU_REGION_OUTBOUND;
> -		break;
> -	default:
> -		return;
> -	}
> -
>  	if (pci->iatu_unroll_enabled) {
> -		if (region == PCIE_ATU_REGION_INBOUND) {
> +		if (dir == PCIE_ATU_REGION_DIR_IB) {
>  			dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
>  						 ~(u32)PCIE_ATU_ENABLE);
>  		} else {
> @@ -516,7 +502,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
>  						 ~(u32)PCIE_ATU_ENABLE);
>  		}
>  	} else {
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> +		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, dir | index);
>  		dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
>  	}
>  }
> @@ -661,7 +647,7 @@ static void dw_pcie_iatu_detect_regions(struct dw_pcie *pci)
>  	max_region = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT) + 1;
>  
>  	for (i = 0; i < max_region; i++) {
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_OUTBOUND | i);
> +		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_OB | i);
>  		dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, 0x11110000);
>  		val = dw_pcie_readl_dbi(pci, PCIE_ATU_LOWER_TARGET);
>  		if (val == 0x11110000)
> @@ -671,7 +657,7 @@ static void dw_pcie_iatu_detect_regions(struct dw_pcie *pci)
>  	}
>  
>  	for (i = 0; i < max_region; i++) {
> -		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_INBOUND | i);
> +		dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, PCIE_ATU_REGION_DIR_IB | i);
>  		dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET, 0x11110000);
>  		val = dw_pcie_readl_dbi(pci, PCIE_ATU_LOWER_TARGET);
>  		if (val == 0x11110000)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index c63ace3c3f25..72d185ff72f3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -104,8 +104,8 @@
>  #define PCIE_VERSION_TYPE		0x8FC
>  
>  #define PCIE_ATU_VIEWPORT		0x900
> -#define PCIE_ATU_REGION_INBOUND		BIT(31)
> -#define PCIE_ATU_REGION_OUTBOUND	0
> +#define PCIE_ATU_REGION_DIR_IB		BIT(31)
> +#define PCIE_ATU_REGION_DIR_OB		0
>  #define PCIE_ATU_CR1			0x904
>  #define PCIE_ATU_INCREASE_REGION_SIZE	BIT(13)
>  #define PCIE_ATU_TYPE_MEM		0x0
> @@ -185,12 +185,6 @@ struct dw_pcie;
>  struct dw_pcie_rp;
>  struct dw_pcie_ep;
>  
> -enum dw_pcie_region_type {
> -	DW_PCIE_REGION_UNKNOWN,
> -	DW_PCIE_REGION_INBOUND,
> -	DW_PCIE_REGION_OUTBOUND,
> -};
> -
>  enum dw_pcie_device_mode {
>  	DW_PCIE_UNKNOWN_TYPE,
>  	DW_PCIE_EP_TYPE,
> @@ -326,8 +320,7 @@ void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  				  u64 size);
>  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  			     int type, u64 cpu_addr, u8 bar);
> -void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> -			 enum dw_pcie_region_type type);
> +void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
>  void dw_pcie_setup(struct dw_pcie *pci);
>  void dw_pcie_iatu_detect(struct dw_pcie *pci);
>  
> -- 
> 2.35.1
> 

-- 
மணிவண்ணன் சதாசிவம்