[v7,17/20] PCI: dwc: Introduce generic resources getter

  Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
the separate parts of the DW PCIe core driver. It doesn't really make
sense since the both controller types have identical set of the core CSR
regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
and EP initialization methods by moving the platform-specific registers
space getting and mapping into a common method. It gets to be even more
justified seeing the CSRs base address pointers are preserved in the
common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
initialization will be moved to the new method too in order to have a
single function for all the generic platform properties handling in single
place.

A nice side-effect of this change is that the pcie-designware-host.c and
pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
storage modification, which makes the DW PCIe core, Root Port and Endpoint
modules more coherent.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Rob Herring <robh@kernel.org>

---

Changelog v3:
- This is a new patch created on v3 lap of the series.

Changelog v4:
- Convert the method name from dw_pcie_get_res() to
  dw_pcie_get_resources(). (@Bjorn)

Changelog v7:
- Get back device.of_node pointer to the dw_pcie_ep_init() method.
  (@Yoshihiro)
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 25 +------
 .../pci/controller/dwc/pcie-designware-host.c | 15 +---
 drivers/pci/controller/dwc/pcie-designware.c  | 75 ++++++++++++++-----
 drivers/pci/controller/dwc/pcie-designware.h  |  3 +
 4 files changed, 65 insertions(+), 53 deletions(-)
  

On Sun, Nov 13, 2022 at 10:12:58PM +0300, Serge Semin wrote:
> Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
> the separate parts of the DW PCIe core driver. It doesn't really make
> sense since the both controller types have identical set of the core CSR
> regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
> and EP initialization methods by moving the platform-specific registers
> space getting and mapping into a common method. It gets to be even more
> justified seeing the CSRs base address pointers are preserved in the
> common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
> initialization will be moved to the new method too in order to have a
> single function for all the generic platform properties handling in single
> place.
> 
> A nice side-effect of this change is that the pcie-designware-host.c and
> pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
> storage modification, which makes the DW PCIe core, Root Port and Endpoint
> modules more coherent.
> 

You have clubbed both generic resource API and introducing CDM_CHECK flag.
Please split them into separate patches.

Thanks,
Mani

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> ---
> 
> Changelog v3:
> - This is a new patch created on v3 lap of the series.
> 
> Changelog v4:
> - Convert the method name from dw_pcie_get_res() to
>   dw_pcie_get_resources(). (@Bjorn)
> 
> Changelog v7:
> - Get back device.of_node pointer to the dw_pcie_ep_init() method.
>   (@Yoshihiro)
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 25 +------
>  .../pci/controller/dwc/pcie-designware-host.c | 15 +---
>  drivers/pci/controller/dwc/pcie-designware.c  | 75 ++++++++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.h  |  3 +
>  4 files changed, 65 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 237bb01d7852..f68d1ab83bb3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -13,8 +13,6 @@
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
>  
> -#include "../../pci.h"
> -
>  void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
>  {
>  	struct pci_epc *epc = ep->epc;
> @@ -694,23 +692,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  
>  	INIT_LIST_HEAD(&ep->func_list);
>  
> -	if (!pci->dbi_base) {
> -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> -		pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> -		if (IS_ERR(pci->dbi_base))
> -			return PTR_ERR(pci->dbi_base);
> -	}
> -
> -	if (!pci->dbi_base2) {
> -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> -		if (!res) {
> -			pci->dbi_base2 = pci->dbi_base + SZ_4K;
> -		} else {
> -			pci->dbi_base2 = devm_pci_remap_cfg_resource(dev, res);
> -			if (IS_ERR(pci->dbi_base2))
> -				return PTR_ERR(pci->dbi_base2);
> -		}
> -	}
> +	ret = dw_pcie_get_resources(pci);
> +	if (ret)
> +		return ret;
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>  	if (!res)
> @@ -739,9 +723,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		return -ENOMEM;
>  	ep->outbound_addr = addr;
>  
> -	if (pci->link_gen < 1)
> -		pci->link_gen = of_pci_get_max_link_speed(np);
> -
>  	epc = devm_pci_epc_create(dev, &epc_ops);
>  	if (IS_ERR(epc)) {
>  		dev_err(dev, "Failed to create epc device\n");
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index ea923c25e12d..3ab6ae3712c4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -16,7 +16,6 @@
>  #include <linux/pci_regs.h>
>  #include <linux/platform_device.h>
>  
> -#include "../../pci.h"
>  #include "pcie-designware.h"
>  
>  static struct pci_ops dw_pcie_ops;
> @@ -395,6 +394,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	raw_spin_lock_init(&pp->lock);
>  
> +	ret = dw_pcie_get_resources(pci);
> +	if (ret)
> +		return ret;
> +
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
>  	if (res) {
>  		pp->cfg0_size = resource_size(res);
> @@ -408,13 +411,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  		return -ENODEV;
>  	}
>  
> -	if (!pci->dbi_base) {
> -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> -		pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> -		if (IS_ERR(pci->dbi_base))
> -			return PTR_ERR(pci->dbi_base);
> -	}
> -
>  	bridge = devm_pci_alloc_host_bridge(dev, 0);
>  	if (!bridge)
>  		return -ENOMEM;
> @@ -429,9 +425,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  		pp->io_base = pci_pio_to_address(win->res->start);
>  	}
>  
> -	if (pci->link_gen < 1)
> -		pci->link_gen = of_pci_get_max_link_speed(np);
> -
>  	/* Set default bus ops */
>  	bridge->ops = &dw_pcie_ops;
>  	bridge->child_ops = &dw_child_pcie_ops;
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 9d78e7ca61e1..a8436027434d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -11,6 +11,7 @@
>  #include <linux/align.h>
>  #include <linux/bitops.h>
>  #include <linux/delay.h>
> +#include <linux/ioport.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/sizes.h>
> @@ -19,6 +20,59 @@
>  #include "../../pci.h"
>  #include "pcie-designware.h"
>  
> +int dw_pcie_get_resources(struct dw_pcie *pci)
> +{
> +	struct platform_device *pdev = to_platform_device(pci->dev);
> +	struct device_node *np = dev_of_node(pci->dev);
> +	struct resource *res;
> +
> +	if (!pci->dbi_base) {
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> +		if (IS_ERR(pci->dbi_base))
> +			return PTR_ERR(pci->dbi_base);
> +	}
> +
> +	/* DBI2 is mainly useful for the endpoint controller */
> +	if (!pci->dbi_base2) {
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> +		if (res) {
> +			pci->dbi_base2 = devm_pci_remap_cfg_resource(pci->dev, res);
> +			if (IS_ERR(pci->dbi_base2))
> +				return PTR_ERR(pci->dbi_base2);
> +		} else {
> +			pci->dbi_base2 = pci->dbi_base + SZ_4K;
> +		}
> +	}
> +
> +	/* For non-unrolled iATU/eDMA platforms this range will be ignored */
> +	if (!pci->atu_base) {
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> +		if (res) {
> +			pci->atu_size = resource_size(res);
> +			pci->atu_base = devm_ioremap_resource(pci->dev, res);
> +			if (IS_ERR(pci->atu_base))
> +				return PTR_ERR(pci->atu_base);
> +		} else {
> +			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> +		}
> +	}
> +
> +	/* Set a default value suitable for at most 8 in and 8 out windows */
> +	if (!pci->atu_size)
> +		pci->atu_size = SZ_4K;
> +
> +	if (pci->link_gen < 1)
> +		pci->link_gen = of_pci_get_max_link_speed(np);
> +
> +	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
> +
> +	if (of_property_read_bool(np, "snps,enable-cdm-check"))
> +		dw_pcie_cap_set(pci, CDM_CHECK);
> +
> +	return 0;
> +}
> +
>  void dw_pcie_version_detect(struct dw_pcie *pci)
>  {
>  	u32 ver;
> @@ -639,25 +693,8 @@ static void dw_pcie_iatu_detect_regions(struct dw_pcie *pci)
>  
>  void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  {
> -	struct platform_device *pdev = to_platform_device(pci->dev);
> -
>  	if (dw_pcie_iatu_unroll_enabled(pci)) {
>  		dw_pcie_cap_set(pci, IATU_UNROLL);
> -
> -		if (!pci->atu_base) {
> -			struct resource *res =
> -				platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> -			if (res) {
> -				pci->atu_size = resource_size(res);
> -				pci->atu_base = devm_ioremap_resource(pci->dev, res);
> -			}
> -			if (!pci->atu_base || IS_ERR(pci->atu_base))
> -				pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> -		}
> -
> -		if (!pci->atu_size)
> -			/* Pick a minimal default, enough for 8 in and 8 out windows */
> -			pci->atu_size = SZ_4K;
>  	} else {
>  		pci->atu_base = pci->dbi_base + PCIE_ATU_VIEWPORT_BASE;
>  		pci->atu_size = PCIE_ATU_VIEWPORT_SIZE;
> @@ -675,7 +712,6 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  
>  void dw_pcie_setup(struct dw_pcie *pci)
>  {
> -	struct device_node *np = pci->dev->of_node;
>  	u32 val;
>  
>  	if (pci->link_gen > 0)
> @@ -703,14 +739,13 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  	val |= PORT_LINK_DLL_LINK_EN;
>  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
>  
> -	if (of_property_read_bool(np, "snps,enable-cdm-check")) {
> +	if (dw_pcie_cap_is(pci, CDM_CHECK)) {
>  		val = dw_pcie_readl_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS);
>  		val |= PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS |
>  		       PCIE_PL_CHK_REG_CHK_REG_START;
>  		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
>  	}
>  
> -	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
>  	if (!pci->num_lanes) {
>  		dev_dbg(pci->dev, "Using h/w default number of lanes\n");
>  		return;
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index c6dddacee3b1..081f169e6021 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -46,6 +46,7 @@
>  
>  /* DWC PCIe controller capabilities */
>  #define DW_PCIE_CAP_IATU_UNROLL		1
> +#define DW_PCIE_CAP_CDM_CHECK		2
>  
>  #define dw_pcie_cap_is(_pci, _cap) \
>  	test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
> @@ -338,6 +339,8 @@ struct dw_pcie {
>  #define to_dw_pcie_from_ep(endpoint)   \
>  		container_of((endpoint), struct dw_pcie, ep)
>  
> +int dw_pcie_get_resources(struct dw_pcie *pci);
> +
>  void dw_pcie_version_detect(struct dw_pcie *pci);
>  
>  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> -- 
> 2.38.1
> 
>
  

On Mon, Nov 14, 2022 at 12:16:54PM +0530, Manivannan Sadhasivam wrote:
> On Sun, Nov 13, 2022 at 10:12:58PM +0300, Serge Semin wrote:
> > Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
> > the separate parts of the DW PCIe core driver. It doesn't really make
> > sense since the both controller types have identical set of the core CSR
> > regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
> > and EP initialization methods by moving the platform-specific registers
> > space getting and mapping into a common method. It gets to be even more
> > justified seeing the CSRs base address pointers are preserved in the
> > common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
> > initialization will be moved to the new method too in order to have a
> > single function for all the generic platform properties handling in single
> > place.
> > 
> > A nice side-effect of this change is that the pcie-designware-host.c and
> > pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
> > storage modification, which makes the DW PCIe core, Root Port and Endpoint
> > modules more coherent.
> > 
> 

> You have clubbed both generic resource API and introducing CDM_CHECK flag.
> Please split them into separate patches.

This modification is a part of the new method dw_pcie_get_resources().
Without that method there is no point in adding the new flag. So no.
It's better to have all of it in a single patch as a part of creating
a coherent resources getter method.

-Sergey

> 
> Thanks,
> Mani
> 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > 
> > ---
> > 
> > Changelog v3:
> > - This is a new patch created on v3 lap of the series.
> > 
> > Changelog v4:
> > - Convert the method name from dw_pcie_get_res() to
> >   dw_pcie_get_resources(). (@Bjorn)
> > 
> > Changelog v7:
> > - Get back device.of_node pointer to the dw_pcie_ep_init() method.
> >   (@Yoshihiro)
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 25 +------
> >  .../pci/controller/dwc/pcie-designware-host.c | 15 +---
> >  drivers/pci/controller/dwc/pcie-designware.c  | 75 ++++++++++++++-----
> >  drivers/pci/controller/dwc/pcie-designware.h  |  3 +
> >  4 files changed, 65 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 237bb01d7852..f68d1ab83bb3 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -13,8 +13,6 @@
> >  #include <linux/pci-epc.h>
> >  #include <linux/pci-epf.h>
> >  
> > -#include "../../pci.h"
> > -
> >  void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> >  {
> >  	struct pci_epc *epc = ep->epc;
> > @@ -694,23 +692,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  
> >  	INIT_LIST_HEAD(&ep->func_list);
> >  
> > -	if (!pci->dbi_base) {
> > -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > -		pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> > -		if (IS_ERR(pci->dbi_base))
> > -			return PTR_ERR(pci->dbi_base);
> > -	}
> > -
> > -	if (!pci->dbi_base2) {
> > -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> > -		if (!res) {
> > -			pci->dbi_base2 = pci->dbi_base + SZ_4K;
> > -		} else {
> > -			pci->dbi_base2 = devm_pci_remap_cfg_resource(dev, res);
> > -			if (IS_ERR(pci->dbi_base2))
> > -				return PTR_ERR(pci->dbi_base2);
> > -		}
> > -	}
> > +	ret = dw_pcie_get_resources(pci);
> > +	if (ret)
> > +		return ret;
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> >  	if (!res)
> > @@ -739,9 +723,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		return -ENOMEM;
> >  	ep->outbound_addr = addr;
> >  
> > -	if (pci->link_gen < 1)
> > -		pci->link_gen = of_pci_get_max_link_speed(np);
> > -
> >  	epc = devm_pci_epc_create(dev, &epc_ops);
> >  	if (IS_ERR(epc)) {
> >  		dev_err(dev, "Failed to create epc device\n");
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index ea923c25e12d..3ab6ae3712c4 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -16,7 +16,6 @@
> >  #include <linux/pci_regs.h>
> >  #include <linux/platform_device.h>
> >  
> > -#include "../../pci.h"
> >  #include "pcie-designware.h"
> >  
> >  static struct pci_ops dw_pcie_ops;
> > @@ -395,6 +394,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  
> >  	raw_spin_lock_init(&pp->lock);
> >  
> > +	ret = dw_pcie_get_resources(pci);
> > +	if (ret)
> > +		return ret;
> > +
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> >  	if (res) {
> >  		pp->cfg0_size = resource_size(res);
> > @@ -408,13 +411,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  		return -ENODEV;
> >  	}
> >  
> > -	if (!pci->dbi_base) {
> > -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > -		pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> > -		if (IS_ERR(pci->dbi_base))
> > -			return PTR_ERR(pci->dbi_base);
> > -	}
> > -
> >  	bridge = devm_pci_alloc_host_bridge(dev, 0);
> >  	if (!bridge)
> >  		return -ENOMEM;
> > @@ -429,9 +425,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  		pp->io_base = pci_pio_to_address(win->res->start);
> >  	}
> >  
> > -	if (pci->link_gen < 1)
> > -		pci->link_gen = of_pci_get_max_link_speed(np);
> > -
> >  	/* Set default bus ops */
> >  	bridge->ops = &dw_pcie_ops;
> >  	bridge->child_ops = &dw_child_pcie_ops;
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 9d78e7ca61e1..a8436027434d 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/align.h>
> >  #include <linux/bitops.h>
> >  #include <linux/delay.h>
> > +#include <linux/ioport.h>
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/sizes.h>
> > @@ -19,6 +20,59 @@
> >  #include "../../pci.h"
> >  #include "pcie-designware.h"
> >  
> > +int dw_pcie_get_resources(struct dw_pcie *pci)
> > +{
> > +	struct platform_device *pdev = to_platform_device(pci->dev);
> > +	struct device_node *np = dev_of_node(pci->dev);
> > +	struct resource *res;
> > +
> > +	if (!pci->dbi_base) {
> > +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > +		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> > +		if (IS_ERR(pci->dbi_base))
> > +			return PTR_ERR(pci->dbi_base);
> > +	}
> > +
> > +	/* DBI2 is mainly useful for the endpoint controller */
> > +	if (!pci->dbi_base2) {
> > +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> > +		if (res) {
> > +			pci->dbi_base2 = devm_pci_remap_cfg_resource(pci->dev, res);
> > +			if (IS_ERR(pci->dbi_base2))
> > +				return PTR_ERR(pci->dbi_base2);
> > +		} else {
> > +			pci->dbi_base2 = pci->dbi_base + SZ_4K;
> > +		}
> > +	}
> > +
> > +	/* For non-unrolled iATU/eDMA platforms this range will be ignored */
> > +	if (!pci->atu_base) {
> > +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> > +		if (res) {
> > +			pci->atu_size = resource_size(res);
> > +			pci->atu_base = devm_ioremap_resource(pci->dev, res);
> > +			if (IS_ERR(pci->atu_base))
> > +				return PTR_ERR(pci->atu_base);
> > +		} else {
> > +			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> > +		}
> > +	}
> > +
> > +	/* Set a default value suitable for at most 8 in and 8 out windows */
> > +	if (!pci->atu_size)
> > +		pci->atu_size = SZ_4K;
> > +
> > +	if (pci->link_gen < 1)
> > +		pci->link_gen = of_pci_get_max_link_speed(np);
> > +
> > +	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
> > +
> > +	if (of_property_read_bool(np, "snps,enable-cdm-check"))
> > +		dw_pcie_cap_set(pci, CDM_CHECK);
> > +
> > +	return 0;
> > +}
> > +
> >  void dw_pcie_version_detect(struct dw_pcie *pci)
> >  {
> >  	u32 ver;
> > @@ -639,25 +693,8 @@ static void dw_pcie_iatu_detect_regions(struct dw_pcie *pci)
> >  
> >  void dw_pcie_iatu_detect(struct dw_pcie *pci)
> >  {
> > -	struct platform_device *pdev = to_platform_device(pci->dev);
> > -
> >  	if (dw_pcie_iatu_unroll_enabled(pci)) {
> >  		dw_pcie_cap_set(pci, IATU_UNROLL);
> > -
> > -		if (!pci->atu_base) {
> > -			struct resource *res =
> > -				platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> > -			if (res) {
> > -				pci->atu_size = resource_size(res);
> > -				pci->atu_base = devm_ioremap_resource(pci->dev, res);
> > -			}
> > -			if (!pci->atu_base || IS_ERR(pci->atu_base))
> > -				pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> > -		}
> > -
> > -		if (!pci->atu_size)
> > -			/* Pick a minimal default, enough for 8 in and 8 out windows */
> > -			pci->atu_size = SZ_4K;
> >  	} else {
> >  		pci->atu_base = pci->dbi_base + PCIE_ATU_VIEWPORT_BASE;
> >  		pci->atu_size = PCIE_ATU_VIEWPORT_SIZE;
> > @@ -675,7 +712,6 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
> >  
> >  void dw_pcie_setup(struct dw_pcie *pci)
> >  {
> > -	struct device_node *np = pci->dev->of_node;
> >  	u32 val;
> >  
> >  	if (pci->link_gen > 0)
> > @@ -703,14 +739,13 @@ void dw_pcie_setup(struct dw_pcie *pci)
> >  	val |= PORT_LINK_DLL_LINK_EN;
> >  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> >  
> > -	if (of_property_read_bool(np, "snps,enable-cdm-check")) {
> > +	if (dw_pcie_cap_is(pci, CDM_CHECK)) {
> >  		val = dw_pcie_readl_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS);
> >  		val |= PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS |
> >  		       PCIE_PL_CHK_REG_CHK_REG_START;
> >  		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
> >  	}
> >  
> > -	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
> >  	if (!pci->num_lanes) {
> >  		dev_dbg(pci->dev, "Using h/w default number of lanes\n");
> >  		return;
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index c6dddacee3b1..081f169e6021 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -46,6 +46,7 @@
> >  
> >  /* DWC PCIe controller capabilities */
> >  #define DW_PCIE_CAP_IATU_UNROLL		1
> > +#define DW_PCIE_CAP_CDM_CHECK		2
> >  
> >  #define dw_pcie_cap_is(_pci, _cap) \
> >  	test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
> > @@ -338,6 +339,8 @@ struct dw_pcie {
> >  #define to_dw_pcie_from_ep(endpoint)   \
> >  		container_of((endpoint), struct dw_pcie, ep)
> >  
> > +int dw_pcie_get_resources(struct dw_pcie *pci);
> > +
> >  void dw_pcie_version_detect(struct dw_pcie *pci);
> >  
> >  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> > -- 
> > 2.38.1
> > 
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
  

On Mon, Nov 14, 2022 at 11:39:03AM +0300, Serge Semin wrote:
> On Mon, Nov 14, 2022 at 12:16:54PM +0530, Manivannan Sadhasivam wrote:
> > On Sun, Nov 13, 2022 at 10:12:58PM +0300, Serge Semin wrote:
> > > Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
> > > the separate parts of the DW PCIe core driver. It doesn't really make
> > > sense since the both controller types have identical set of the core CSR
> > > regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
> > > and EP initialization methods by moving the platform-specific registers
> > > space getting and mapping into a common method. It gets to be even more
> > > justified seeing the CSRs base address pointers are preserved in the
> > > common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
> > > initialization will be moved to the new method too in order to have a
> > > single function for all the generic platform properties handling in single
> > > place.
> > > 
> > > A nice side-effect of this change is that the pcie-designware-host.c and
> > > pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
> > > storage modification, which makes the DW PCIe core, Root Port and Endpoint
> > > modules more coherent.
> > > 
> > 
> 
> > You have clubbed both generic resource API and introducing CDM_CHECK flag.
> > Please split them into separate patches.
> 
> This modification is a part of the new method dw_pcie_get_resources().
> Without that method there is no point in adding the new flag. So no.
> It's better to have all of it in a single patch as a part of creating
> a coherent resources getter method.
> 

Same comment as previous patch. I'll defer it to you.

Thanks,
Mani

> -Sergey
> 
> > 
> > Thanks,
> > Mani
> > 
> > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > 
> > > ---
> > > 
> > > Changelog v3:
> > > - This is a new patch created on v3 lap of the series.
> > > 
> > > Changelog v4:
> > > - Convert the method name from dw_pcie_get_res() to
> > >   dw_pcie_get_resources(). (@Bjorn)
> > > 
> > > Changelog v7:
> > > - Get back device.of_node pointer to the dw_pcie_ep_init() method.
> > >   (@Yoshihiro)
> > > ---
> > >  .../pci/controller/dwc/pcie-designware-ep.c   | 25 +------
> > >  .../pci/controller/dwc/pcie-designware-host.c | 15 +---
> > >  drivers/pci/controller/dwc/pcie-designware.c  | 75 ++++++++++++++-----
> > >  drivers/pci/controller/dwc/pcie-designware.h  |  3 +
> > >  4 files changed, 65 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index 237bb01d7852..f68d1ab83bb3 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -13,8 +13,6 @@
> > >  #include <linux/pci-epc.h>
> > >  #include <linux/pci-epf.h>
> > >  
> > > -#include "../../pci.h"
> > > -
> > >  void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> > >  {
> > >  	struct pci_epc *epc = ep->epc;
> > > @@ -694,23 +692,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > >  
> > >  	INIT_LIST_HEAD(&ep->func_list);
> > >  
> > > -	if (!pci->dbi_base) {
> > > -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > > -		pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> > > -		if (IS_ERR(pci->dbi_base))
> > > -			return PTR_ERR(pci->dbi_base);
> > > -	}
> > > -
> > > -	if (!pci->dbi_base2) {
> > > -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> > > -		if (!res) {
> > > -			pci->dbi_base2 = pci->dbi_base + SZ_4K;
> > > -		} else {
> > > -			pci->dbi_base2 = devm_pci_remap_cfg_resource(dev, res);
> > > -			if (IS_ERR(pci->dbi_base2))
> > > -				return PTR_ERR(pci->dbi_base2);
> > > -		}
> > > -	}
> > > +	ret = dw_pcie_get_resources(pci);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> > >  	if (!res)
> > > @@ -739,9 +723,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > >  		return -ENOMEM;
> > >  	ep->outbound_addr = addr;
> > >  
> > > -	if (pci->link_gen < 1)
> > > -		pci->link_gen = of_pci_get_max_link_speed(np);
> > > -
> > >  	epc = devm_pci_epc_create(dev, &epc_ops);
> > >  	if (IS_ERR(epc)) {
> > >  		dev_err(dev, "Failed to create epc device\n");
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index ea923c25e12d..3ab6ae3712c4 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -16,7 +16,6 @@
> > >  #include <linux/pci_regs.h>
> > >  #include <linux/platform_device.h>
> > >  
> > > -#include "../../pci.h"
> > >  #include "pcie-designware.h"
> > >  
> > >  static struct pci_ops dw_pcie_ops;
> > > @@ -395,6 +394,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  
> > >  	raw_spin_lock_init(&pp->lock);
> > >  
> > > +	ret = dw_pcie_get_resources(pci);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> > >  	if (res) {
> > >  		pp->cfg0_size = resource_size(res);
> > > @@ -408,13 +411,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  		return -ENODEV;
> > >  	}
> > >  
> > > -	if (!pci->dbi_base) {
> > > -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > > -		pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> > > -		if (IS_ERR(pci->dbi_base))
> > > -			return PTR_ERR(pci->dbi_base);
> > > -	}
> > > -
> > >  	bridge = devm_pci_alloc_host_bridge(dev, 0);
> > >  	if (!bridge)
> > >  		return -ENOMEM;
> > > @@ -429,9 +425,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  		pp->io_base = pci_pio_to_address(win->res->start);
> > >  	}
> > >  
> > > -	if (pci->link_gen < 1)
> > > -		pci->link_gen = of_pci_get_max_link_speed(np);
> > > -
> > >  	/* Set default bus ops */
> > >  	bridge->ops = &dw_pcie_ops;
> > >  	bridge->child_ops = &dw_child_pcie_ops;
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 9d78e7ca61e1..a8436027434d 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/align.h>
> > >  #include <linux/bitops.h>
> > >  #include <linux/delay.h>
> > > +#include <linux/ioport.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_platform.h>
> > >  #include <linux/sizes.h>
> > > @@ -19,6 +20,59 @@
> > >  #include "../../pci.h"
> > >  #include "pcie-designware.h"
> > >  
> > > +int dw_pcie_get_resources(struct dw_pcie *pci)
> > > +{
> > > +	struct platform_device *pdev = to_platform_device(pci->dev);
> > > +	struct device_node *np = dev_of_node(pci->dev);
> > > +	struct resource *res;
> > > +
> > > +	if (!pci->dbi_base) {
> > > +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > > +		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> > > +		if (IS_ERR(pci->dbi_base))
> > > +			return PTR_ERR(pci->dbi_base);
> > > +	}
> > > +
> > > +	/* DBI2 is mainly useful for the endpoint controller */
> > > +	if (!pci->dbi_base2) {
> > > +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> > > +		if (res) {
> > > +			pci->dbi_base2 = devm_pci_remap_cfg_resource(pci->dev, res);
> > > +			if (IS_ERR(pci->dbi_base2))
> > > +				return PTR_ERR(pci->dbi_base2);
> > > +		} else {
> > > +			pci->dbi_base2 = pci->dbi_base + SZ_4K;
> > > +		}
> > > +	}
> > > +
> > > +	/* For non-unrolled iATU/eDMA platforms this range will be ignored */
> > > +	if (!pci->atu_base) {
> > > +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> > > +		if (res) {
> > > +			pci->atu_size = resource_size(res);
> > > +			pci->atu_base = devm_ioremap_resource(pci->dev, res);
> > > +			if (IS_ERR(pci->atu_base))
> > > +				return PTR_ERR(pci->atu_base);
> > > +		} else {
> > > +			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> > > +		}
> > > +	}
> > > +
> > > +	/* Set a default value suitable for at most 8 in and 8 out windows */
> > > +	if (!pci->atu_size)
> > > +		pci->atu_size = SZ_4K;
> > > +
> > > +	if (pci->link_gen < 1)
> > > +		pci->link_gen = of_pci_get_max_link_speed(np);
> > > +
> > > +	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
> > > +
> > > +	if (of_property_read_bool(np, "snps,enable-cdm-check"))
> > > +		dw_pcie_cap_set(pci, CDM_CHECK);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  void dw_pcie_version_detect(struct dw_pcie *pci)
> > >  {
> > >  	u32 ver;
> > > @@ -639,25 +693,8 @@ static void dw_pcie_iatu_detect_regions(struct dw_pcie *pci)
> > >  
> > >  void dw_pcie_iatu_detect(struct dw_pcie *pci)
> > >  {
> > > -	struct platform_device *pdev = to_platform_device(pci->dev);
> > > -
> > >  	if (dw_pcie_iatu_unroll_enabled(pci)) {
> > >  		dw_pcie_cap_set(pci, IATU_UNROLL);
> > > -
> > > -		if (!pci->atu_base) {
> > > -			struct resource *res =
> > > -				platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> > > -			if (res) {
> > > -				pci->atu_size = resource_size(res);
> > > -				pci->atu_base = devm_ioremap_resource(pci->dev, res);
> > > -			}
> > > -			if (!pci->atu_base || IS_ERR(pci->atu_base))
> > > -				pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> > > -		}
> > > -
> > > -		if (!pci->atu_size)
> > > -			/* Pick a minimal default, enough for 8 in and 8 out windows */
> > > -			pci->atu_size = SZ_4K;
> > >  	} else {
> > >  		pci->atu_base = pci->dbi_base + PCIE_ATU_VIEWPORT_BASE;
> > >  		pci->atu_size = PCIE_ATU_VIEWPORT_SIZE;
> > > @@ -675,7 +712,6 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
> > >  
> > >  void dw_pcie_setup(struct dw_pcie *pci)
> > >  {
> > > -	struct device_node *np = pci->dev->of_node;
> > >  	u32 val;
> > >  
> > >  	if (pci->link_gen > 0)
> > > @@ -703,14 +739,13 @@ void dw_pcie_setup(struct dw_pcie *pci)
> > >  	val |= PORT_LINK_DLL_LINK_EN;
> > >  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> > >  
> > > -	if (of_property_read_bool(np, "snps,enable-cdm-check")) {
> > > +	if (dw_pcie_cap_is(pci, CDM_CHECK)) {
> > >  		val = dw_pcie_readl_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS);
> > >  		val |= PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS |
> > >  		       PCIE_PL_CHK_REG_CHK_REG_START;
> > >  		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
> > >  	}
> > >  
> > > -	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
> > >  	if (!pci->num_lanes) {
> > >  		dev_dbg(pci->dev, "Using h/w default number of lanes\n");
> > >  		return;
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index c6dddacee3b1..081f169e6021 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -46,6 +46,7 @@
> > >  
> > >  /* DWC PCIe controller capabilities */
> > >  #define DW_PCIE_CAP_IATU_UNROLL		1
> > > +#define DW_PCIE_CAP_CDM_CHECK		2
> > >  
> > >  #define dw_pcie_cap_is(_pci, _cap) \
> > >  	test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
> > > @@ -338,6 +339,8 @@ struct dw_pcie {
> > >  #define to_dw_pcie_from_ep(endpoint)   \
> > >  		container_of((endpoint), struct dw_pcie, ep)
> > >  
> > > +int dw_pcie_get_resources(struct dw_pcie *pci);
> > > +
> > >  void dw_pcie_version_detect(struct dw_pcie *pci);
> > >  
> > >  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> > > -- 
> > > 2.38.1
> > > 
> > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்
  

Hi Serge,

On Sun, Nov 13, 2022 at 10:12:58PM +0300, Serge Semin wrote:
> Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
> the separate parts of the DW PCIe core driver. It doesn't really make
> sense since the both controller types have identical set of the core CSR
> regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
> and EP initialization methods by moving the platform-specific registers
> space getting and mapping into a common method. It gets to be even more
> justified seeing the CSRs base address pointers are preserved in the
> common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
> initialization will be moved to the new method too in order to have a
> single function for all the generic platform properties handling in single
> place.
> 
> A nice side-effect of this change is that the pcie-designware-host.c and
> pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
> storage modification, which makes the DW PCIe core, Root Port and Endpoint
> modules more coherent.

Thanks for these new generic interfaces in the DWC core!  And thanks
for the changes in this patch to take advantage of them in the
pcie-designware drivers.

Do you plan similar changes to other drivers to take advantage of
these DWC-generic data and interfaces?  If you add generic things to
the DWC core but only take advantage of them in your driver, I don't
think they are really usefully generic.

Bjorn
  

Hi Bjorn,

On Wed, Nov 23, 2022 at 01:44:36PM -0600, Bjorn Helgaas wrote:
> Hi Serge,
> 
> On Sun, Nov 13, 2022 at 10:12:58PM +0300, Serge Semin wrote:
> > Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
> > the separate parts of the DW PCIe core driver. It doesn't really make
> > sense since the both controller types have identical set of the core CSR
> > regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
> > and EP initialization methods by moving the platform-specific registers
> > space getting and mapping into a common method. It gets to be even more
> > justified seeing the CSRs base address pointers are preserved in the
> > common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
> > initialization will be moved to the new method too in order to have a
> > single function for all the generic platform properties handling in single
> > place.
> > 
> > A nice side-effect of this change is that the pcie-designware-host.c and
> > pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
> > storage modification, which makes the DW PCIe core, Root Port and Endpoint
> > modules more coherent.
> 

> Thanks for these new generic interfaces in the DWC core!  And thanks
> for the changes in this patch to take advantage of them in the
> pcie-designware drivers.
> 
> Do you plan similar changes to other drivers to take advantage of
> these DWC-generic data and interfaces?  If you add generic things to
> the DWC core but only take advantage of them in your driver, I don't
> think they are really usefully generic.

Could you be more specific what generic things you are referring to? I
am asking because the only part of the changes which is used in my
low-level driver only is introduced in another patch of this series.
It's
< [PATCH v7 19/20] PCI: dwc: Introduce generic platform clocks and resets
The new clock/reset request interface has been implemented the way it
is due to reasons I in details described to Rob here:
Link: https://lore.kernel.org/linux-pci/20220520160246.guczq52v2ycfgc6c@mobilestation
To cut it short it can't be used by the most of the already available
low-level drivers since they already have their own versions of the
names for the clock and reset resources (or don't have any name
defined at all). The only driver for which the interface could be
utilized is Toshiba Visconti PCIe host controller driver. The device
DT-bindings defines the clock names matching the generic names
introduced in the patches of this series. If you find it appropriate
enough I can provide a patch for that driver.

Note the main goal of the patch
[PATCH v7 19/20] PCI: dwc: Introduce generic platform clocks and resets
was to create some interface to stop the developers of the new drivers
from creating the platform-specific DT-bindings to the same clock and
reset resources. Since the already defined DT-bindings can't be
changed anyway I don't think it would worth risking to catch
regressions on an attempt to provide a more complicated interface
utilized in the old drivers too.

-Serge(y)

> 
> Bjorn
  

On Sun, Nov 27, 2022 at 04:10:05AM +0300, Serge Semin wrote:
> On Wed, Nov 23, 2022 at 01:44:36PM -0600, Bjorn Helgaas wrote:
> > On Sun, Nov 13, 2022 at 10:12:58PM +0300, Serge Semin wrote:

> > Thanks for these new generic interfaces in the DWC core!  And thanks
> > for the changes in this patch to take advantage of them in the
> > pcie-designware drivers.
> > 
> > Do you plan similar changes to other drivers to take advantage of
> > these DWC-generic data and interfaces?  If you add generic things to
> > the DWC core but only take advantage of them in your driver, I don't
> > think they are really usefully generic.
> 
> Could you be more specific what generic things you are referring to? I
> am asking because the only part of the changes which is used in my
> low-level driver only is introduced in another patch of this series.

I asked because three of your patches mention "generic" things, but I
didn't see any changes to drivers except pcie-designware:

  PCI: dwc: Introduce generic platform clocks and reset
  PCI: dwc: Introduce generic resources getter
  PCI: dwc: Introduce generic controller capabilities interface

I hoped that we would be able to use these to remove some code from
existing drivers, but if they only improve maintainability of future
drivers, that's useful, too.

Bjorn
  

On Tue, Nov 29, 2022 at 12:35:43PM -0600, Bjorn Helgaas wrote:
> On Sun, Nov 27, 2022 at 04:10:05AM +0300, Serge Semin wrote:
> > On Wed, Nov 23, 2022 at 01:44:36PM -0600, Bjorn Helgaas wrote:
> > > On Sun, Nov 13, 2022 at 10:12:58PM +0300, Serge Semin wrote:
> 
> > > Thanks for these new generic interfaces in the DWC core!  And thanks
> > > for the changes in this patch to take advantage of them in the
> > > pcie-designware drivers.
> > > 
> > > Do you plan similar changes to other drivers to take advantage of
> > > these DWC-generic data and interfaces?  If you add generic things to
> > > the DWC core but only take advantage of them in your driver, I don't
> > > think they are really usefully generic.
> > 
> > Could you be more specific what generic things you are referring to? I
> > am asking because the only part of the changes which is used in my
> > low-level driver only is introduced in another patch of this series.
> 
> I asked because three of your patches mention "generic" things, but I
> didn't see any changes to drivers except pcie-designware:
> 

>   PCI: dwc: Introduce generic platform clocks and reset

This patch introduces a method to request a generic platform clocks
and resets by their names. As I already said these names are defined
by the DT-bindings, which are platform-specific. That's why the most
of the currently available drivers can't be converted to using it.
Instead the new drivers are supposed to be encouraged to use the
generic names (in accordance with the generic DW PCIe DT-schema) and
the resources request interface (based on the generic DT-bindings) if
it suits their design.

Anyway I honestly tried to come up with an even more generic
interface, which could be used by all the low-level drivers. But due
to too much variations of the resource names and their sometimes too
complex utilization in the drivers any solution looked too complex.
After all of thoughts I decided to keep things simpler.

>   PCI: dwc: Introduce generic resources getter

This patch defines a generic resource getter for the DW PCIe host and
end-point drivers. That's why it's called generic. 

>   PCI: dwc: Introduce generic controller capabilities interface

This patch introduces an interface to set the device-specific
capabilities. Since these capabilities can be marked as available by
both the core driver (at least two of them already defined within this
patchset) and low-level platform drivers the interface is called
as generic.

> 
> I hoped that we would be able to use these to remove some code from
> existing drivers, but if they only improve maintainability of future
> drivers, that's useful, too.

Removing some code is possible for instance from the pcie-visconti.c
driver by using the new generic clocks and resets request interface.
I've scheduled to create a small patchset which would do that after
the rest of my patches pass the review process.

-Serge(y)

> 
> Bjorn