| Message ID | 20220324013734.18234-11-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support | expand |
On Thu, Mar 24, 2022 at 04:37:28AM +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 macro declared which > describe the possible inbound and outbound iATU regions. Having an > addition abstraction just needlessly complicates the code. Secondly > checking the region index passed to the dw_pcie_disable_atu() method for > validity is pointless since the erroneous situation will be just > ignored in the current code implementation. So to speak let's drop the > redundant dw_pcie_region_type enumeration replacing it with the direct > iATU direction macro usage. > > While at it we suggest to convert the dw_pcie_disable_atu() method to > being more consistent with the dw_pcie_readl_atu{_ib}() and > dw_pcie_readl_atu{_ob}() functions by having the direction parameter > specified ahead of the region index. Thus the code will be a little bit > more pleasant to read. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/pci/controller/dwc/pcie-designware-ep.c | 4 ++-- > .../pci/controller/dwc/pcie-designware-host.c | 2 +- > drivers/pci/controller/dwc/pcie-designware.c | 16 +--------------- > drivers/pci/controller/dwc/pcie-designware.h | 9 +-------- > 4 files changed, 5 insertions(+), 26 deletions(-) This answers my question. I would have expected this to come before the previous patch, but if it's easier to do it this way it's fine. Reviewed-by: Rob Herring <robh@kernel.org>
On Tue, Mar 29, 2022 at 10:31:18AM -0500, Rob Herring wrote: > On Thu, Mar 24, 2022 at 04:37:28AM +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 macro declared which > > describe the possible inbound and outbound iATU regions. Having an > > addition abstraction just needlessly complicates the code. Secondly > > checking the region index passed to the dw_pcie_disable_atu() method for > > validity is pointless since the erroneous situation will be just > > ignored in the current code implementation. So to speak let's drop the > > redundant dw_pcie_region_type enumeration replacing it with the direct > > iATU direction macro usage. > > > > While at it we suggest to convert the dw_pcie_disable_atu() method to > > being more consistent with the dw_pcie_readl_atu{_ib}() and > > dw_pcie_readl_atu{_ob}() functions by having the direction parameter > > specified ahead of the region index. Thus the code will be a little bit > > more pleasant to read. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > drivers/pci/controller/dwc/pcie-designware-ep.c | 4 ++-- > > .../pci/controller/dwc/pcie-designware-host.c | 2 +- > > drivers/pci/controller/dwc/pcie-designware.c | 16 +--------------- > > drivers/pci/controller/dwc/pcie-designware.h | 9 +-------- > > 4 files changed, 5 insertions(+), 26 deletions(-) > > This answers my question. I would have expected this to come before the > previous patch, but if it's easier to do it this way it's fine. > > Reviewed-by: Rob Herring <robh@kernel.org> I thought about placing this patch before the previous one, but it turned a bit easier for me to split the changes in the reverse order. It has made this patch a bit smaller and more coherent. But seeing you weren't happy with too many changes in the previous patch I'll do as you suggest and change the patches order. Since the patch content will be changed I won't add your reviewed-by tag there on v2. So please consider re-reviewing it one more time. -Sergey
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 3bd9026071e8..83ceba84b79d 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 602cf4fe502b..e9aa3d8539d8 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -644,7 +644,7 @@ void dw_pcie_setup_rc(struct pcie_port *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 f1aa6e2e85fe..ce360986609f 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -421,22 +421,8 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index, return -ETIMEDOUT; } -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 dir; - - switch (type) { - case DW_PCIE_REGION_INBOUND: - dir = PCIE_ATU_REGION_DIR_IB; - break; - case DW_PCIE_REGION_OUTBOUND: - dir = PCIE_ATU_REGION_DIR_OB; - break; - default: - return; - } - dw_pcie_writel_atu(pci, dir, index, PCIE_ATU_REGION_CTRL2, 0); } diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 6adf0c957c3b..203f9dfb9048 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -175,12 +175,6 @@ struct pcie_port; struct dw_pcie; 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, @@ -316,8 +310,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);
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 macro declared which describe the possible inbound and outbound iATU regions. Having an addition abstraction just needlessly complicates the code. Secondly checking the region index passed to the dw_pcie_disable_atu() method for validity is pointless since the erroneous situation will be just ignored in the current code implementation. So to speak let's drop the redundant dw_pcie_region_type enumeration replacing it with the direct iATU direction macro usage. While at it we suggest to convert the dw_pcie_disable_atu() method to being more consistent with the dw_pcie_readl_atu{_ib}() and dw_pcie_readl_atu{_ob}() functions by having the direction parameter specified ahead of the region index. Thus the code will be a little bit more pleasant to read. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- drivers/pci/controller/dwc/pcie-designware-ep.c | 4 ++-- .../pci/controller/dwc/pcie-designware-host.c | 2 +- drivers/pci/controller/dwc/pcie-designware.c | 16 +--------------- drivers/pci/controller/dwc/pcie-designware.h | 9 +-------- 4 files changed, 5 insertions(+), 26 deletions(-)