diff mbox series

[v2,16/17] PCI: dwc: Introduce generic platform clocks and resets sets

Message ID 20220503214638.1895-17-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

Commit Message

Serge Semin May 3, 2022, 9:46 p.m. UTC
Currently almost each platform driver uses its own resets and clocks
naming in order to get the corresponding descriptors. It makes the code
harder to maintain and comprehend especially seeing the DWC PCIe core main
resets and clocks signals set hasn't changed much for about at least one
major IP-core release. So in order to organize things around these signals
we suggest to create a generic interface for them in accordance with the
naming introduced in the DWC PCIe IP-core reference manual:

Clocks:
- DBI - data bus interface clock (on some DWC PCIe platforms it's
  referred as "pclk", "pcie", "sys", "ahb", "cfg", "iface", "gio", "reg",
  "pcie_apb_sys");
- MSTR - AXI-bus master interface clock (some DWC PCIe glue drivers refer
  to this clock as "port", "bus", "pcie_bus",
  "bus_master/master_bus/axi_m", "pcie_aclk");
- SLV - AXI-bus slave interface clock (also called as "port", "bus",
  "pcie_bus", "bus_slave/slave_bus/axi_s", "pcie_aclk",
  "pcie_inbound_axi");
- PIPE - Core-PCS PIPE interface clock coming from external PHY (it's
  normally named by the platform drivers as just "pipe")
- CORE - primary clock of the controller (none of the platform drivers
  declare such a clock but in accordance with the ref. manual the devices
  may have it separately specified);
- AUX - Auxiliary PMC domain clock (it is named by some platforms as
  "pcie_aux" and just "aux")
- REF - Generic reference clock (it is a generic clock source, which can
  be used as a signal source for multiple interfaces, some platforms call
  it as "ref", "general", "pcie_phy", "pcie_phy_ref").

Application resets:
- DBI - Data-bus interface reset (it's CSR interface clock and is normally
  called as "apb" though technically it's not APB but DWC PCIe-specific
  interface);
  apb, sys,
- MSTR -AXI-bus master reset (some platforms call it as "port", "apps",
  "bus", "axi_m");
- SLV - ABI-bus slave reset (some platforms call it as "port", "apps",
  "bus", "axi_s").

Core resets:
- NON_STICKY - Non-sticky CSR flags reset;
- STICKY - sticky CSR flags reset;
- PIPE - PIPE-interface (Core-PCS) logic reset (some platforms call it
  just "pipe");
- CORE - controller primary reset (resets everything except PMC module,
  some platforms refer to this signal as "soft", "pci");
- PHY - PCS/PHY block reset (strictly speaking it is normally connected to
  the out of the external block, but the reference manual says it must be
  available for the PMC working correctly, some existing platforms call it
  as "pciephy", "phy", "link");
- HOT - PMC hot reset signal (also called as sleep");
- PWR - cold reset signal (can be referred as "pwr", "turnoff").

As you can see each platform uses it's own naming for basically the same
set of the signals. In the framework of this commit we suggest to add a
set of the clocks and signals identifiers and corresponding names for each
denoted entity. The platforms will be able to use them to define local
mapping tables between the generic identifiers and the available set of
the clocks and resets. The tables can be then utilized to create the
corresponding bulk-arrays, which in its turn can be passed to the
clock/reset-bulk API methods to easily get/enable/disable/put,
get/reset/assert/deassert/put all the handlers at once or, if it's
required, manipulate with the handlers individually.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/pci/controller/dwc/pcie-designware.h | 79 ++++++++++++++++++++
 1 file changed, 79 insertions(+)

Comments

Rob Herring (Arm) May 16, 2022, 10:29 p.m. UTC | #1
On Wed, May 04, 2022 at 12:46:37AM +0300, Serge Semin wrote:
> Currently almost each platform driver uses its own resets and clocks
> naming in order to get the corresponding descriptors. It makes the code
> harder to maintain and comprehend especially seeing the DWC PCIe core main
> resets and clocks signals set hasn't changed much for about at least one
> major IP-core release. So in order to organize things around these signals
> we suggest to create a generic interface for them in accordance with the
> naming introduced in the DWC PCIe IP-core reference manual:
> 
> Clocks:
> - DBI - data bus interface clock (on some DWC PCIe platforms it's
>   referred as "pclk", "pcie", "sys", "ahb", "cfg", "iface", "gio", "reg",
>   "pcie_apb_sys");
> - MSTR - AXI-bus master interface clock (some DWC PCIe glue drivers refer
>   to this clock as "port", "bus", "pcie_bus",
>   "bus_master/master_bus/axi_m", "pcie_aclk");
> - SLV - AXI-bus slave interface clock (also called as "port", "bus",
>   "pcie_bus", "bus_slave/slave_bus/axi_s", "pcie_aclk",
>   "pcie_inbound_axi");
> - PIPE - Core-PCS PIPE interface clock coming from external PHY (it's
>   normally named by the platform drivers as just "pipe")
> - CORE - primary clock of the controller (none of the platform drivers
>   declare such a clock but in accordance with the ref. manual the devices
>   may have it separately specified);
> - AUX - Auxiliary PMC domain clock (it is named by some platforms as
>   "pcie_aux" and just "aux")
> - REF - Generic reference clock (it is a generic clock source, which can
>   be used as a signal source for multiple interfaces, some platforms call
>   it as "ref", "general", "pcie_phy", "pcie_phy_ref").
> 
> Application resets:
> - DBI - Data-bus interface reset (it's CSR interface clock and is normally
>   called as "apb" though technically it's not APB but DWC PCIe-specific
>   interface);
>   apb, sys,
> - MSTR -AXI-bus master reset (some platforms call it as "port", "apps",
>   "bus", "axi_m");
> - SLV - ABI-bus slave reset (some platforms call it as "port", "apps",
>   "bus", "axi_s").
> 
> Core resets:
> - NON_STICKY - Non-sticky CSR flags reset;
> - STICKY - sticky CSR flags reset;
> - PIPE - PIPE-interface (Core-PCS) logic reset (some platforms call it
>   just "pipe");
> - CORE - controller primary reset (resets everything except PMC module,
>   some platforms refer to this signal as "soft", "pci");
> - PHY - PCS/PHY block reset (strictly speaking it is normally connected to
>   the out of the external block, but the reference manual says it must be
>   available for the PMC working correctly, some existing platforms call it
>   as "pciephy", "phy", "link");
> - HOT - PMC hot reset signal (also called as sleep");
> - PWR - cold reset signal (can be referred as "pwr", "turnoff").
> 
> As you can see each platform uses it's own naming for basically the same
> set of the signals. In the framework of this commit we suggest to add a
> set of the clocks and signals identifiers and corresponding names for each
> denoted entity. The platforms will be able to use them to define local
> mapping tables between the generic identifiers and the available set of
> the clocks and resets. The tables can be then utilized to create the
> corresponding bulk-arrays, which in its turn can be passed to the
> clock/reset-bulk API methods to easily get/enable/disable/put,
> get/reset/assert/deassert/put all the handlers at once or, if it's
> required, manipulate with the handlers individually.

No doubt there is way to much variation here (ummm, Qcom!). Some 
standardization of names in (new) bindings would be good. That's where 
we should be defining names IMO.

On the driver side, I'd like to see the DW core handle clocks/resets/phys 
at least for the easy cases of just turn on/off all the clocks and 
toggle all resets. Perhaps even more minimally, move the clk/reset 
struct pointers to the DWC core.

IOW, I'm not sure this patch is really helpful without some of the above 
happening. 

Rob
Serge Semin May 20, 2022, 4:02 p.m. UTC | #2
On Mon, May 16, 2022 at 05:29:20PM -0500, Rob Herring wrote:
> On Wed, May 04, 2022 at 12:46:37AM +0300, Serge Semin wrote:
> > Currently almost each platform driver uses its own resets and clocks
> > naming in order to get the corresponding descriptors. It makes the code
> > harder to maintain and comprehend especially seeing the DWC PCIe core main
> > resets and clocks signals set hasn't changed much for about at least one
> > major IP-core release. So in order to organize things around these signals
> > we suggest to create a generic interface for them in accordance with the
> > naming introduced in the DWC PCIe IP-core reference manual:
> > 
> > Clocks:
> > - DBI - data bus interface clock (on some DWC PCIe platforms it's
> >   referred as "pclk", "pcie", "sys", "ahb", "cfg", "iface", "gio", "reg",
> >   "pcie_apb_sys");
> > - MSTR - AXI-bus master interface clock (some DWC PCIe glue drivers refer
> >   to this clock as "port", "bus", "pcie_bus",
> >   "bus_master/master_bus/axi_m", "pcie_aclk");
> > - SLV - AXI-bus slave interface clock (also called as "port", "bus",
> >   "pcie_bus", "bus_slave/slave_bus/axi_s", "pcie_aclk",
> >   "pcie_inbound_axi");
> > - PIPE - Core-PCS PIPE interface clock coming from external PHY (it's
> >   normally named by the platform drivers as just "pipe")
> > - CORE - primary clock of the controller (none of the platform drivers
> >   declare such a clock but in accordance with the ref. manual the devices
> >   may have it separately specified);
> > - AUX - Auxiliary PMC domain clock (it is named by some platforms as
> >   "pcie_aux" and just "aux")
> > - REF - Generic reference clock (it is a generic clock source, which can
> >   be used as a signal source for multiple interfaces, some platforms call
> >   it as "ref", "general", "pcie_phy", "pcie_phy_ref").
> > 
> > Application resets:
> > - DBI - Data-bus interface reset (it's CSR interface clock and is normally
> >   called as "apb" though technically it's not APB but DWC PCIe-specific
> >   interface);
> >   apb, sys,
> > - MSTR -AXI-bus master reset (some platforms call it as "port", "apps",
> >   "bus", "axi_m");
> > - SLV - ABI-bus slave reset (some platforms call it as "port", "apps",
> >   "bus", "axi_s").
> > 
> > Core resets:
> > - NON_STICKY - Non-sticky CSR flags reset;
> > - STICKY - sticky CSR flags reset;
> > - PIPE - PIPE-interface (Core-PCS) logic reset (some platforms call it
> >   just "pipe");
> > - CORE - controller primary reset (resets everything except PMC module,
> >   some platforms refer to this signal as "soft", "pci");
> > - PHY - PCS/PHY block reset (strictly speaking it is normally connected to
> >   the out of the external block, but the reference manual says it must be
> >   available for the PMC working correctly, some existing platforms call it
> >   as "pciephy", "phy", "link");
> > - HOT - PMC hot reset signal (also called as sleep");
> > - PWR - cold reset signal (can be referred as "pwr", "turnoff").
> > 
> > As you can see each platform uses it's own naming for basically the same
> > set of the signals. In the framework of this commit we suggest to add a
> > set of the clocks and signals identifiers and corresponding names for each
> > denoted entity. The platforms will be able to use them to define local
> > mapping tables between the generic identifiers and the available set of
> > the clocks and resets. The tables can be then utilized to create the
> > corresponding bulk-arrays, which in its turn can be passed to the
> > clock/reset-bulk API methods to easily get/enable/disable/put,
> > get/reset/assert/deassert/put all the handlers at once or, if it's
> > required, manipulate with the handlers individually.
> 

> No doubt there is way to much variation here (ummm, Qcom!). Some 
> standardization of names in (new) bindings would be good. That's where 
> we should be defining names IMO.

That's what my patchset starts from. See the {reset,clock,reg}-names
properties definitions in the snps,dw-pcie-common.yaml schema being added
in the framework of the patch
[PATCH v2 01/17] dt-bindings: PCI: dwc: Define common and native DT bindings
in this series.

> 
> On the driver side, I'd like to see the DW core handle clocks/resets/phys 
> at least for the easy cases of just turn on/off all the clocks and 
> toggle all resets. Perhaps even more minimally, move the clk/reset 
> struct pointers to the DWC core.


If it was that easy I would have done that in the first place.) Even
though there is well defined set of the clocks and resets a normal DW
PCIe RP/EP controller can have (see the list in the patch log), the
way they are toggled and the signal sources due to the controller
complexity in the most of the cases is platform-dependent.  Just
enabling and de-asserting all of them likely will never work. So such
function would be just useless. In addition to that there are signals
like app_ltssm_enable or phy/link reset request which needs to be
checked at some point of the reset procedure. The way these signals
are available in the system is also platform-dependent (syscon,
additional CSRs blocks, etc). Thirdly there can be PHY viewport CSRs
available in the DW PCIe Port Logic CSRs which can be used on the
platforms to tune the PHY settings up during the cold reset process.
Finally the platform-specific timings between the reset signals
assertion/de-assertion need to be preserved. Taking all of that into
account would cause having a very complicated generic clock/reset
handling procedure.

Synopsys provides a Verilog module called DWC_pcie_clkrst.v/CLK_RST.v
which can be used by the platform engineers to implement the clocks
generation and cold/hot reset without much software interference. But
it isn't always possible to predict the timings being required on the
real silicon. So the hw engineers mainly omit such module and rely on the
software to implement the proper reset procedure, which due to many
reasons (PHY settings, timings, etc) mostly differs from SoC-to-SoC.

So to speak I don't think that we can (should?) create some truly
generic at least cold start procedure due to too many variables at
stake (though the one implemented in my Baikal-T1 PCIe driver, last
patch in the series, follows the cold procedure described in the hw
reference manual). At least IMO it won't be possible without many
hooks inside such method with platform-dependent signals checking and
delays. So the only generic thing that we can indeed implement is to
have the clock and reset structure pointers inside the DWC core and
create a generic platform resources request/release methods. Even in
that case these methods can't be called from the generic
host/end-point probe procedure and need to be invoked from the
platform-specific host_init/host_deinit methods by the platforms
themself. It's because there are so many platforms already
implemented. If I try to consolidate all their resource
request/release parts in a single method, it most likely cause the
regression.

What do you think if I would just create the arrays with pointers to the
generic clk/reset structures in the DWC private data, implement their
request (release won't be required due to devm-methods utilization)
procedure and use it in the framework of my driver only for now?

> 
> IOW, I'm not sure this patch is really helpful without some of the above 
> happening.

Well, I've provided a driver (the last patch in this series) which
design can be used as a reference to implement the clocks/reset
request procedure with a help of the submitted in this patch tables.
But if you are agree with creating a generic resource request method,
the clocks/reset request part of it can be re-designed, simplified
and moved to that method.

-Sergey

> 
> Rob
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 03de8f20a2cc..765d99d5bfaa 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -183,6 +183,35 @@  enum dw_pcie_device_mode {
 	DW_PCIE_RC_TYPE,
 };
 
+enum dw_pcie_clk {
+	DW_PCIE_DBI_CLK,
+	DW_PCIE_MSTR_CLK,
+	DW_PCIE_SLV_CLK,
+	DW_PCIE_PIPE_CLK,
+	DW_PCIE_CORE_CLK,
+	DW_PCIE_AUX_CLK,
+	DW_PCIE_REF_CLK,
+	DW_PCIE_NUM_CLKS
+};
+
+enum dw_pcie_app_rst {
+	DW_PCIE_DBI_RST,
+	DW_PCIE_MSTR_RST,
+	DW_PCIE_SLV_RST,
+	DW_PCIE_NUM_APP_RSTS
+};
+
+enum dw_pcie_core_rst {
+	DW_PCIE_NON_STICKY_RST,
+	DW_PCIE_STICKY_RST,
+	DW_PCIE_CORE_RST,
+	DW_PCIE_PIPE_RST,
+	DW_PCIE_PHY_RST,
+	DW_PCIE_HOT_RST,
+	DW_PCIE_PWR_RST,
+	DW_PCIE_NUM_CORE_RSTS
+};
+
 struct dw_pcie_host_ops {
 	int (*host_init)(struct pcie_port *pp);
 	void (*host_deinit)(struct pcie_port *pp);
@@ -373,6 +402,56 @@  static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
 	dw_pcie_writel_dbi(pci, reg, val);
 }
 
+static inline const char *dw_pcie_clk_name(enum dw_pcie_clk id)
+{
+	static const char *names[DW_PCIE_NUM_CLKS] = {
+		[DW_PCIE_DBI_CLK] = "dbi",
+		[DW_PCIE_MSTR_CLK] = "mstr",
+		[DW_PCIE_SLV_CLK] = "slv",
+		[DW_PCIE_PIPE_CLK] = "pipe",
+		[DW_PCIE_CORE_CLK] = "core",
+		[DW_PCIE_AUX_CLK] = "aux",
+		[DW_PCIE_REF_CLK] = "ref",
+	};
+
+	if (id >= DW_PCIE_NUM_CLKS)
+		return NULL;
+
+	return names[id];
+}
+
+static inline const char *dw_pcie_app_rst_name(enum dw_pcie_app_rst id)
+{
+	static const char *names[DW_PCIE_NUM_APP_RSTS] = {
+		[DW_PCIE_DBI_RST] = "dbi",
+		[DW_PCIE_MSTR_RST] = "mstr",
+		[DW_PCIE_SLV_RST] = "slv",
+	};
+
+	if (id >= DW_PCIE_NUM_APP_RSTS)
+		return NULL;
+
+	return names[id];
+}
+
+static inline const char *dw_pcie_core_rst_name(enum dw_pcie_core_rst id)
+{
+	static const char *names[DW_PCIE_NUM_CORE_RSTS] = {
+		[DW_PCIE_NON_STICKY_RST] = "non-sticky",
+		[DW_PCIE_STICKY_RST] = "sticky",
+		[DW_PCIE_CORE_RST] = "core",
+		[DW_PCIE_PIPE_RST] = "pipe",
+		[DW_PCIE_PHY_RST] = "phy",
+		[DW_PCIE_HOT_RST] = "hot",
+		[DW_PCIE_PWR_RST] = "pwr",
+	};
+
+	if (id >= DW_PCIE_NUM_CORE_RSTS)
+		return NULL;
+
+	return names[id];
+}
+
 #ifdef CONFIG_PCIE_DW_HOST
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp);
 int dw_pcie_setup_rc(struct pcie_port *pp);