[PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Remi Pommarel]

PCIe device probing failures have been seen on some AXG platforms and were
due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
solved the problem. After being contacted about this, vendor reported that
this bit was linked to PCIe PLL CML output.

This serie adds a way to set this bit through AXG clock gating logic.
Platforms having this kind of issue could make use of this gating by
applying a patch to their devicetree similar to:

                clocks = <&clkc CLKID_USB
                        &clkc CLKID_MIPI_ENABLE
                        &clkc CLKID_PCIE_A
-                       &clkc CLKID_PCIE_CML_EN0>;
+                       &clkc CLKID_PCIE_CML_EN0
+                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
                clock-names = "pcie_general",
                                "pcie_mipi_en",
                                "pcie",
-                               "port";
+                               "port",
+                               "pll_cml_en";
                resets = <&reset RESET_PCIE_PHY>,
                        <&reset RESET_PCIE_A>,
                        <&reset RESET_PCIE_APB>;


Remi Pommarel (2):
  clk: meson: axg: add pcie pll cml gating
  PCI: amlogic: Use PCIe pll gate when available

 drivers/clk/meson/axg.c                | 3 +++
 drivers/clk/meson/axg.h                | 2 +-
 drivers/pci/controller/dwc/pci-meson.c | 5 +++++
 include/dt-bindings/clock/axg-clkc.h   | 1 +
 4 files changed, 10 insertions(+), 1 deletion(-)

-- 
2.24.0

[PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Remi Pommarel]

PCIe device probing failures have been seen on some AXG platforms and were
due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
solved the problem. After being contacted about this, vendor reported that
this bit was linked to PCIe PLL CML output.

This serie adds a way to set this bit through AXG clock gating logic.
Platforms having this kind of issue could make use of this gating by
applying a patch to their devicetree similar to:

                clocks = <&clkc CLKID_USB
                        &clkc CLKID_MIPI_ENABLE
                        &clkc CLKID_PCIE_A
-                       &clkc CLKID_PCIE_CML_EN0>;
+                       &clkc CLKID_PCIE_CML_EN0
+                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
                clock-names = "pcie_general",
                                "pcie_mipi_en",
                                "pcie",
-                               "port";
+                               "port",
+                               "pll_cml_en";
                resets = <&reset RESET_PCIE_PHY>,
                        <&reset RESET_PCIE_A>,
                        <&reset RESET_PCIE_APB>;


Remi Pommarel (2):
  clk: meson: axg: add pcie pll cml gating
  PCI: amlogic: Use PCIe pll gate when available

 drivers/clk/meson/axg.c                | 3 +++
 drivers/clk/meson/axg.h                | 2 +-
 drivers/pci/controller/dwc/pci-meson.c | 5 +++++
 include/dt-bindings/clock/axg-clkc.h   | 1 +
 4 files changed, 10 insertions(+), 1 deletion(-)

-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Remi Pommarel]

PCIe device probing failures have been seen on some AXG platforms and were
due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
solved the problem. After being contacted about this, vendor reported that
this bit was linked to PCIe PLL CML output.

This serie adds a way to set this bit through AXG clock gating logic.
Platforms having this kind of issue could make use of this gating by
applying a patch to their devicetree similar to:

                clocks = <&clkc CLKID_USB
                        &clkc CLKID_MIPI_ENABLE
                        &clkc CLKID_PCIE_A
-                       &clkc CLKID_PCIE_CML_EN0>;
+                       &clkc CLKID_PCIE_CML_EN0
+                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
                clock-names = "pcie_general",
                                "pcie_mipi_en",
                                "pcie",
-                               "port";
+                               "port",
+                               "pll_cml_en";
                resets = <&reset RESET_PCIE_PHY>,
                        <&reset RESET_PCIE_A>,
                        <&reset RESET_PCIE_APB>;


Remi Pommarel (2):
  clk: meson: axg: add pcie pll cml gating
  PCI: amlogic: Use PCIe pll gate when available

 drivers/clk/meson/axg.c                | 3 +++
 drivers/clk/meson/axg.h                | 2 +-
 drivers/pci/controller/dwc/pci-meson.c | 5 +++++
 include/dt-bindings/clock/axg-clkc.h   | 1 +
 4 files changed, 10 insertions(+), 1 deletion(-)

-- 
2.24.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 1/2] clk: meson: axg: add pcie pll cml gating

[Remi Pommarel]

PCIE_PLL_CML_ENABLE is used to enable or disable pcie clock PAD
output reliably on AXG platforms.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 drivers/clk/meson/axg.c              | 3 +++
 drivers/clk/meson/axg.h              | 2 +-
 include/dt-bindings/clock/axg-clkc.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
index 13fc0006f63d..ac9ab7f75ee8 100644
--- a/drivers/clk/meson/axg.c
+++ b/drivers/clk/meson/axg.c
@@ -1142,6 +1142,7 @@ static MESON_GATE(axg_vpu_intr, HHI_GCLK_MPEG2, 25);
 static MESON_GATE(axg_sec_ahb_ahb3_bridge, HHI_GCLK_MPEG2, 26);
 static MESON_GATE(axg_gic, HHI_GCLK_MPEG2, 30);
 static MESON_GATE(axg_mipi_enable, HHI_MIPI_CNTL0, 29);
+static MESON_GATE(axg_pcie_pll_cml_enable, HHI_MIPI_CNTL0, 26);
 
 /* Always On (AO) domain gates */
 
@@ -1246,6 +1247,7 @@ static struct clk_hw_onecell_data axg_hw_onecell_data = {
 		[CLKID_HIFI_PLL_DCO]		= &axg_hifi_pll_dco.hw,
 		[CLKID_PCIE_PLL_DCO]		= &axg_pcie_pll_dco.hw,
 		[CLKID_PCIE_PLL_OD]		= &axg_pcie_pll_od.hw,
+		[CLKID_PCIE_PLL_CML_ENABLE]	= &axg_pcie_pll_cml_enable.hw,
 		[NR_CLKS]			= NULL,
 	},
 	.num = NR_CLKS,
@@ -1341,6 +1343,7 @@ static struct clk_regmap *const axg_clk_regmaps[] = {
 	&axg_hifi_pll_dco,
 	&axg_pcie_pll_dco,
 	&axg_pcie_pll_od,
+	&axg_pcie_pll_cml_enable,
 };
 
 static const struct meson_eeclkc_data axg_clkc_data = {
diff --git a/drivers/clk/meson/axg.h b/drivers/clk/meson/axg.h
index 0431dabac629..d65670d6c607 100644
--- a/drivers/clk/meson/axg.h
+++ b/drivers/clk/meson/axg.h
@@ -140,7 +140,7 @@
 #define CLKID_PCIE_PLL_DCO			89
 #define CLKID_PCIE_PLL_OD			90
 
-#define NR_CLKS					91
+#define NR_CLKS					92
 
 /* include the CLKIDs that have been made part of the DT binding */
 #include <dt-bindings/clock/axg-clkc.h>
diff --git a/include/dt-bindings/clock/axg-clkc.h b/include/dt-bindings/clock/axg-clkc.h
index fd1f938c38d1..218a05ff508d 100644
--- a/include/dt-bindings/clock/axg-clkc.h
+++ b/include/dt-bindings/clock/axg-clkc.h
@@ -72,5 +72,6 @@
 #define CLKID_PCIE_CML_EN1			80
 #define CLKID_MIPI_ENABLE			81
 #define CLKID_GEN_CLK				84
+#define CLKID_PCIE_PLL_CML_ENABLE		91
 
 #endif /* __AXG_CLKC_H */
-- 
2.24.0

[PATCH 1/2] clk: meson: axg: add pcie pll cml gating

[Remi Pommarel]

PCIE_PLL_CML_ENABLE is used to enable or disable pcie clock PAD
output reliably on AXG platforms.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 drivers/clk/meson/axg.c              | 3 +++
 drivers/clk/meson/axg.h              | 2 +-
 include/dt-bindings/clock/axg-clkc.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
index 13fc0006f63d..ac9ab7f75ee8 100644
--- a/drivers/clk/meson/axg.c
+++ b/drivers/clk/meson/axg.c
@@ -1142,6 +1142,7 @@ static MESON_GATE(axg_vpu_intr, HHI_GCLK_MPEG2, 25);
 static MESON_GATE(axg_sec_ahb_ahb3_bridge, HHI_GCLK_MPEG2, 26);
 static MESON_GATE(axg_gic, HHI_GCLK_MPEG2, 30);
 static MESON_GATE(axg_mipi_enable, HHI_MIPI_CNTL0, 29);
+static MESON_GATE(axg_pcie_pll_cml_enable, HHI_MIPI_CNTL0, 26);
 
 /* Always On (AO) domain gates */
 
@@ -1246,6 +1247,7 @@ static struct clk_hw_onecell_data axg_hw_onecell_data = {
 		[CLKID_HIFI_PLL_DCO]		= &axg_hifi_pll_dco.hw,
 		[CLKID_PCIE_PLL_DCO]		= &axg_pcie_pll_dco.hw,
 		[CLKID_PCIE_PLL_OD]		= &axg_pcie_pll_od.hw,
+		[CLKID_PCIE_PLL_CML_ENABLE]	= &axg_pcie_pll_cml_enable.hw,
 		[NR_CLKS]			= NULL,
 	},
 	.num = NR_CLKS,
@@ -1341,6 +1343,7 @@ static struct clk_regmap *const axg_clk_regmaps[] = {
 	&axg_hifi_pll_dco,
 	&axg_pcie_pll_dco,
 	&axg_pcie_pll_od,
+	&axg_pcie_pll_cml_enable,
 };
 
 static const struct meson_eeclkc_data axg_clkc_data = {
diff --git a/drivers/clk/meson/axg.h b/drivers/clk/meson/axg.h
index 0431dabac629..d65670d6c607 100644
--- a/drivers/clk/meson/axg.h
+++ b/drivers/clk/meson/axg.h
@@ -140,7 +140,7 @@
 #define CLKID_PCIE_PLL_DCO			89
 #define CLKID_PCIE_PLL_OD			90
 
-#define NR_CLKS					91
+#define NR_CLKS					92
 
 /* include the CLKIDs that have been made part of the DT binding */
 #include <dt-bindings/clock/axg-clkc.h>
diff --git a/include/dt-bindings/clock/axg-clkc.h b/include/dt-bindings/clock/axg-clkc.h
index fd1f938c38d1..218a05ff508d 100644
--- a/include/dt-bindings/clock/axg-clkc.h
+++ b/include/dt-bindings/clock/axg-clkc.h
@@ -72,5 +72,6 @@
 #define CLKID_PCIE_CML_EN1			80
 #define CLKID_MIPI_ENABLE			81
 #define CLKID_GEN_CLK				84
+#define CLKID_PCIE_PLL_CML_ENABLE		91
 
 #endif /* __AXG_CLKC_H */
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] clk: meson: axg: add pcie pll cml gating

[Remi Pommarel]

PCIE_PLL_CML_ENABLE is used to enable or disable pcie clock PAD
output reliably on AXG platforms.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 drivers/clk/meson/axg.c              | 3 +++
 drivers/clk/meson/axg.h              | 2 +-
 include/dt-bindings/clock/axg-clkc.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
index 13fc0006f63d..ac9ab7f75ee8 100644
--- a/drivers/clk/meson/axg.c
+++ b/drivers/clk/meson/axg.c
@@ -1142,6 +1142,7 @@ static MESON_GATE(axg_vpu_intr, HHI_GCLK_MPEG2, 25);
 static MESON_GATE(axg_sec_ahb_ahb3_bridge, HHI_GCLK_MPEG2, 26);
 static MESON_GATE(axg_gic, HHI_GCLK_MPEG2, 30);
 static MESON_GATE(axg_mipi_enable, HHI_MIPI_CNTL0, 29);
+static MESON_GATE(axg_pcie_pll_cml_enable, HHI_MIPI_CNTL0, 26);
 
 /* Always On (AO) domain gates */
 
@@ -1246,6 +1247,7 @@ static struct clk_hw_onecell_data axg_hw_onecell_data = {
 		[CLKID_HIFI_PLL_DCO]		= &axg_hifi_pll_dco.hw,
 		[CLKID_PCIE_PLL_DCO]		= &axg_pcie_pll_dco.hw,
 		[CLKID_PCIE_PLL_OD]		= &axg_pcie_pll_od.hw,
+		[CLKID_PCIE_PLL_CML_ENABLE]	= &axg_pcie_pll_cml_enable.hw,
 		[NR_CLKS]			= NULL,
 	},
 	.num = NR_CLKS,
@@ -1341,6 +1343,7 @@ static struct clk_regmap *const axg_clk_regmaps[] = {
 	&axg_hifi_pll_dco,
 	&axg_pcie_pll_dco,
 	&axg_pcie_pll_od,
+	&axg_pcie_pll_cml_enable,
 };
 
 static const struct meson_eeclkc_data axg_clkc_data = {
diff --git a/drivers/clk/meson/axg.h b/drivers/clk/meson/axg.h
index 0431dabac629..d65670d6c607 100644
--- a/drivers/clk/meson/axg.h
+++ b/drivers/clk/meson/axg.h
@@ -140,7 +140,7 @@
 #define CLKID_PCIE_PLL_DCO			89
 #define CLKID_PCIE_PLL_OD			90
 
-#define NR_CLKS					91
+#define NR_CLKS					92
 
 /* include the CLKIDs that have been made part of the DT binding */
 #include <dt-bindings/clock/axg-clkc.h>
diff --git a/include/dt-bindings/clock/axg-clkc.h b/include/dt-bindings/clock/axg-clkc.h
index fd1f938c38d1..218a05ff508d 100644
--- a/include/dt-bindings/clock/axg-clkc.h
+++ b/include/dt-bindings/clock/axg-clkc.h
@@ -72,5 +72,6 @@
 #define CLKID_PCIE_CML_EN1			80
 #define CLKID_MIPI_ENABLE			81
 #define CLKID_GEN_CLK				84
+#define CLKID_PCIE_PLL_CML_ENABLE		91
 
 #endif /* __AXG_CLKC_H */
-- 
2.24.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Remi Pommarel]

In order to get PCIe working reliably on some AXG platforms, PCIe pll
cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
clock gate.

This clock gate is optional, so do not fail if it is missing in the
devicetree.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 drivers/pci/controller/dwc/pci-meson.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 3772b02a5c55..32b70ea9a426 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
 	struct clk *mipi_gate;
 	struct clk *port_clk;
 	struct clk *general_clk;
+	struct clk *pll_cml_gate;
 };
 
 struct meson_pcie_rc_reset {
@@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
 	if (IS_ERR(res->clk))
 		return PTR_ERR(res->clk);
 
+	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
+	if (IS_ERR(res->pll_cml_gate))
+		res->pll_cml_gate = NULL;
+
 	return 0;
 }
 
-- 
2.24.0

[PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Remi Pommarel]

In order to get PCIe working reliably on some AXG platforms, PCIe pll
cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
clock gate.

This clock gate is optional, so do not fail if it is missing in the
devicetree.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 drivers/pci/controller/dwc/pci-meson.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 3772b02a5c55..32b70ea9a426 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
 	struct clk *mipi_gate;
 	struct clk *port_clk;
 	struct clk *general_clk;
+	struct clk *pll_cml_gate;
 };
 
 struct meson_pcie_rc_reset {
@@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
 	if (IS_ERR(res->clk))
 		return PTR_ERR(res->clk);
 
+	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
+	if (IS_ERR(res->pll_cml_gate))
+		res->pll_cml_gate = NULL;
+
 	return 0;
 }
 
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Remi Pommarel]

In order to get PCIe working reliably on some AXG platforms, PCIe pll
cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
clock gate.

This clock gate is optional, so do not fail if it is missing in the
devicetree.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 drivers/pci/controller/dwc/pci-meson.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 3772b02a5c55..32b70ea9a426 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
 	struct clk *mipi_gate;
 	struct clk *port_clk;
 	struct clk *general_clk;
+	struct clk *pll_cml_gate;
 };
 
 struct meson_pcie_rc_reset {
@@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
 	if (IS_ERR(res->clk))
 		return PTR_ERR(res->clk);
 
+	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
+	if (IS_ERR(res->pll_cml_gate))
+		res->pll_cml_gate = NULL;
+
 	return 0;
 }
 
-- 
2.24.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/2] clk: meson: axg: add pcie pll cml gating

[Martin Blumenstingl]

Hi Remi,

On Sun, Dec 8, 2019 at 9:56 PM Remi Pommarel <repk@triplefau.lt> wrote:
[...]
> +static MESON_GATE(axg_pcie_pll_cml_enable, HHI_MIPI_CNTL0, 26);
we already have CLKID_PCIE_CML_EN0
do you know how this new one is related (in terms of clock hierarchy)
to the existing one?

[...]
> --- a/include/dt-bindings/clock/axg-clkc.h
> +++ b/include/dt-bindings/clock/axg-clkc.h
> @@ -72,5 +72,6 @@
>  #define CLKID_PCIE_CML_EN1                     80
>  #define CLKID_MIPI_ENABLE                      81
>  #define CLKID_GEN_CLK                          84
> +#define CLKID_PCIE_PLL_CML_ENABLE              91
this has to be a separate patch if you want the .dts patch to go into
the same cycle
the .dts change depends on this one. what we typically do is to apply
the dt-bindings patches to a separate clock branch, create an
immutable tag and then Kevin pulls that into his dt64 branch.
the clock controller changes go into a separate patch in the
clk-meson/drivers branch to avoid conflicts with other driver changes


Martin

Re: [PATCH 1/2] clk: meson: axg: add pcie pll cml gating

[Martin Blumenstingl]

Hi Remi,

On Sun, Dec 8, 2019 at 9:56 PM Remi Pommarel <repk@triplefau.lt> wrote:
[...]
> +static MESON_GATE(axg_pcie_pll_cml_enable, HHI_MIPI_CNTL0, 26);
we already have CLKID_PCIE_CML_EN0
do you know how this new one is related (in terms of clock hierarchy)
to the existing one?

[...]
> --- a/include/dt-bindings/clock/axg-clkc.h
> +++ b/include/dt-bindings/clock/axg-clkc.h
> @@ -72,5 +72,6 @@
>  #define CLKID_PCIE_CML_EN1                     80
>  #define CLKID_MIPI_ENABLE                      81
>  #define CLKID_GEN_CLK                          84
> +#define CLKID_PCIE_PLL_CML_ENABLE              91
this has to be a separate patch if you want the .dts patch to go into
the same cycle
the .dts change depends on this one. what we typically do is to apply
the dt-bindings patches to a separate clock branch, create an
immutable tag and then Kevin pulls that into his dt64 branch.
the clock controller changes go into a separate patch in the
clk-meson/drivers branch to avoid conflicts with other driver changes


Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] clk: meson: axg: add pcie pll cml gating

[Martin Blumenstingl]

Hi Remi,

On Sun, Dec 8, 2019 at 9:56 PM Remi Pommarel <repk@triplefau.lt> wrote:
[...]
> +static MESON_GATE(axg_pcie_pll_cml_enable, HHI_MIPI_CNTL0, 26);
we already have CLKID_PCIE_CML_EN0
do you know how this new one is related (in terms of clock hierarchy)
to the existing one?

[...]
> --- a/include/dt-bindings/clock/axg-clkc.h
> +++ b/include/dt-bindings/clock/axg-clkc.h
> @@ -72,5 +72,6 @@
>  #define CLKID_PCIE_CML_EN1                     80
>  #define CLKID_MIPI_ENABLE                      81
>  #define CLKID_GEN_CLK                          84
> +#define CLKID_PCIE_PLL_CML_ENABLE              91
this has to be a separate patch if you want the .dts patch to go into
the same cycle
the .dts change depends on this one. what we typically do is to apply
the dt-bindings patches to a separate clock branch, create an
immutable tag and then Kevin pulls that into his dt64 branch.
the clock controller changes go into a separate patch in the
clk-meson/drivers branch to avoid conflicts with other driver changes


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Jerome Brunet]

On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:

> PCIe device probing failures have been seen on some AXG platforms and were
> due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> solved the problem. After being contacted about this, vendor reported that
> this bit was linked to PCIe PLL CML output.

Thanks for reporting the problem.

As Martin pointed out, the CML outputs already exist in the AXG clock
controller but are handled using HHI_PCIE_PLL_CNTL6. Although
incomplete, it seems to be aligned with the datasheet I have (v0.9)

According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
Unfortunately bit 26 is not documented

AFAICT, the clock controller is not appropriate driver to deal with this
register/bit

>
> This serie adds a way to set this bit through AXG clock gating logic.
> Platforms having this kind of issue could make use of this gating by
> applying a patch to their devicetree similar to:
>
>                 clocks = <&clkc CLKID_USB
>                         &clkc CLKID_MIPI_ENABLE
>                         &clkc CLKID_PCIE_A
> -                       &clkc CLKID_PCIE_CML_EN0>;
> +                       &clkc CLKID_PCIE_CML_EN0
> +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
>                 clock-names = "pcie_general",
>                                 "pcie_mipi_en",
>                                 "pcie",
> -                               "port";
> +                               "port",
> +                               "pll_cml_en";
>                 resets = <&reset RESET_PCIE_PHY>,
>                         <&reset RESET_PCIE_A>,
>                         <&reset RESET_PCIE_APB>;

A few remarks for your future patches:

* You need to document any need binding you introduce:
  It means that there should have been a patch in
  Documentation/devicetree/... before using your newclock name in the
  pcie driver. As Martin pointed out, dt-bindings should be dealt with
  in their own patches

>
>
> Remi Pommarel (2):
>   clk: meson: axg: add pcie pll cml gating

Whenever possible, patches intended for different maintainers should be
sent separately (different series)

>   PCI: amlogic: Use PCIe pll gate when available
>
>  drivers/clk/meson/axg.c                | 3 +++
>  drivers/clk/meson/axg.h                | 2 +-
>  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>  include/dt-bindings/clock/axg-clkc.h   | 1 +
>  4 files changed, 10 insertions(+), 1 deletion(-)

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Jerome Brunet]

On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:

> PCIe device probing failures have been seen on some AXG platforms and were
> due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> solved the problem. After being contacted about this, vendor reported that
> this bit was linked to PCIe PLL CML output.

Thanks for reporting the problem.

As Martin pointed out, the CML outputs already exist in the AXG clock
controller but are handled using HHI_PCIE_PLL_CNTL6. Although
incomplete, it seems to be aligned with the datasheet I have (v0.9)

According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
Unfortunately bit 26 is not documented

AFAICT, the clock controller is not appropriate driver to deal with this
register/bit

>
> This serie adds a way to set this bit through AXG clock gating logic.
> Platforms having this kind of issue could make use of this gating by
> applying a patch to their devicetree similar to:
>
>                 clocks = <&clkc CLKID_USB
>                         &clkc CLKID_MIPI_ENABLE
>                         &clkc CLKID_PCIE_A
> -                       &clkc CLKID_PCIE_CML_EN0>;
> +                       &clkc CLKID_PCIE_CML_EN0
> +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
>                 clock-names = "pcie_general",
>                                 "pcie_mipi_en",
>                                 "pcie",
> -                               "port";
> +                               "port",
> +                               "pll_cml_en";
>                 resets = <&reset RESET_PCIE_PHY>,
>                         <&reset RESET_PCIE_A>,
>                         <&reset RESET_PCIE_APB>;

A few remarks for your future patches:

* You need to document any need binding you introduce:
  It means that there should have been a patch in
  Documentation/devicetree/... before using your newclock name in the
  pcie driver. As Martin pointed out, dt-bindings should be dealt with
  in their own patches

>
>
> Remi Pommarel (2):
>   clk: meson: axg: add pcie pll cml gating

Whenever possible, patches intended for different maintainers should be
sent separately (different series)

>   PCI: amlogic: Use PCIe pll gate when available
>
>  drivers/clk/meson/axg.c                | 3 +++
>  drivers/clk/meson/axg.h                | 2 +-
>  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>  include/dt-bindings/clock/axg-clkc.h   | 1 +
>  4 files changed, 10 insertions(+), 1 deletion(-)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Jerome Brunet]

On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:

> PCIe device probing failures have been seen on some AXG platforms and were
> due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> solved the problem. After being contacted about this, vendor reported that
> this bit was linked to PCIe PLL CML output.

Thanks for reporting the problem.

As Martin pointed out, the CML outputs already exist in the AXG clock
controller but are handled using HHI_PCIE_PLL_CNTL6. Although
incomplete, it seems to be aligned with the datasheet I have (v0.9)

According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
Unfortunately bit 26 is not documented

AFAICT, the clock controller is not appropriate driver to deal with this
register/bit

>
> This serie adds a way to set this bit through AXG clock gating logic.
> Platforms having this kind of issue could make use of this gating by
> applying a patch to their devicetree similar to:
>
>                 clocks = <&clkc CLKID_USB
>                         &clkc CLKID_MIPI_ENABLE
>                         &clkc CLKID_PCIE_A
> -                       &clkc CLKID_PCIE_CML_EN0>;
> +                       &clkc CLKID_PCIE_CML_EN0
> +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
>                 clock-names = "pcie_general",
>                                 "pcie_mipi_en",
>                                 "pcie",
> -                               "port";
> +                               "port",
> +                               "pll_cml_en";
>                 resets = <&reset RESET_PCIE_PHY>,
>                         <&reset RESET_PCIE_A>,
>                         <&reset RESET_PCIE_APB>;

A few remarks for your future patches:

* You need to document any need binding you introduce:
  It means that there should have been a patch in
  Documentation/devicetree/... before using your newclock name in the
  pcie driver. As Martin pointed out, dt-bindings should be dealt with
  in their own patches

>
>
> Remi Pommarel (2):
>   clk: meson: axg: add pcie pll cml gating

Whenever possible, patches intended for different maintainers should be
sent separately (different series)

>   PCI: amlogic: Use PCIe pll gate when available
>
>  drivers/clk/meson/axg.c                | 3 +++
>  drivers/clk/meson/axg.h                | 2 +-
>  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>  include/dt-bindings/clock/axg-clkc.h   | 1 +
>  4 files changed, 10 insertions(+), 1 deletion(-)


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Andrew Murray]

On Sun, Dec 08, 2019 at 10:03:20PM +0100, Remi Pommarel wrote:
> In order to get PCIe working reliably on some AXG platforms, PCIe pll
> cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
> clock gate.

s/cml/CML/

In addition to Jerome's feedback - it would also be helpful to explain
when CML outputs should be enabled, i.e. which platforms and why those
ones?

> 
> This clock gate is optional, so do not fail if it is missing in the
> devicetree.

If certain platforms require PCIE_PLL_CML_ENABLE to work reliably and
thus the clock is specified in the device tree - then surely if there
is an error in enabling the clock we should fail? I.e. should you only
ignore -ENOENT here?

Thanks,

Andrew Murray

> 
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 3772b02a5c55..32b70ea9a426 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
>  	struct clk *mipi_gate;
>  	struct clk *port_clk;
>  	struct clk *general_clk;
> +	struct clk *pll_cml_gate;
>  };
>  
>  struct meson_pcie_rc_reset {
> @@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
>  	if (IS_ERR(res->clk))
>  		return PTR_ERR(res->clk);
>  
> +	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
> +	if (IS_ERR(res->pll_cml_gate))
> +		res->pll_cml_gate = NULL;
> +
>  	return 0;
>  }
>  
> -- 
> 2.24.0
> 

Re: [PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Andrew Murray]

On Sun, Dec 08, 2019 at 10:03:20PM +0100, Remi Pommarel wrote:
> In order to get PCIe working reliably on some AXG platforms, PCIe pll
> cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
> clock gate.

s/cml/CML/

In addition to Jerome's feedback - it would also be helpful to explain
when CML outputs should be enabled, i.e. which platforms and why those
ones?

> 
> This clock gate is optional, so do not fail if it is missing in the
> devicetree.

If certain platforms require PCIE_PLL_CML_ENABLE to work reliably and
thus the clock is specified in the device tree - then surely if there
is an error in enabling the clock we should fail? I.e. should you only
ignore -ENOENT here?

Thanks,

Andrew Murray

> 
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 3772b02a5c55..32b70ea9a426 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
>  	struct clk *mipi_gate;
>  	struct clk *port_clk;
>  	struct clk *general_clk;
> +	struct clk *pll_cml_gate;
>  };
>  
>  struct meson_pcie_rc_reset {
> @@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
>  	if (IS_ERR(res->clk))
>  		return PTR_ERR(res->clk);
>  
> +	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
> +	if (IS_ERR(res->pll_cml_gate))
> +		res->pll_cml_gate = NULL;
> +
>  	return 0;
>  }
>  
> -- 
> 2.24.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Andrew Murray]

On Sun, Dec 08, 2019 at 10:03:20PM +0100, Remi Pommarel wrote:
> In order to get PCIe working reliably on some AXG platforms, PCIe pll
> cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
> clock gate.

s/cml/CML/

In addition to Jerome's feedback - it would also be helpful to explain
when CML outputs should be enabled, i.e. which platforms and why those
ones?

> 
> This clock gate is optional, so do not fail if it is missing in the
> devicetree.

If certain platforms require PCIE_PLL_CML_ENABLE to work reliably and
thus the clock is specified in the device tree - then surely if there
is an error in enabling the clock we should fail? I.e. should you only
ignore -ENOENT here?

Thanks,

Andrew Murray

> 
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 3772b02a5c55..32b70ea9a426 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
>  	struct clk *mipi_gate;
>  	struct clk *port_clk;
>  	struct clk *general_clk;
> +	struct clk *pll_cml_gate;
>  };
>  
>  struct meson_pcie_rc_reset {
> @@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
>  	if (IS_ERR(res->clk))
>  		return PTR_ERR(res->clk);
>  
> +	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
> +	if (IS_ERR(res->pll_cml_gate))
> +		res->pll_cml_gate = NULL;
> +
>  	return 0;
>  }
>  
> -- 
> 2.24.0
> 

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Remi Pommarel]

On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
> 
> On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
> 
> > PCIe device probing failures have been seen on some AXG platforms and were
> > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> > solved the problem. After being contacted about this, vendor reported that
> > this bit was linked to PCIe PLL CML output.
> 
> Thanks for reporting the problem.
> 
> As Martin pointed out, the CML outputs already exist in the AXG clock
> controller but are handled using HHI_PCIE_PLL_CNTL6. Although
> incomplete, it seems to be aligned with the datasheet I have (v0.9)
> 
> According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
> Unfortunately bit 26 is not documented
> 
> AFAICT, the clock controller is not appropriate driver to deal with this
> register/bit
> 

Regarding both @Martin's and your remark.

Unfortunately the documentation I have and vendor feedback are a bit
vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
name for this bit because this register is MIPI related.

Here is the information I got from the vendor [1]. As you can see
HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
HHI_MIPI_CNTL0[29] is implemented in the clock controller as
axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].

So maybe I could rename this bit to something MIPI related ?

> >
> > This serie adds a way to set this bit through AXG clock gating logic.
> > Platforms having this kind of issue could make use of this gating by
> > applying a patch to their devicetree similar to:
> >
> >                 clocks = <&clkc CLKID_USB
> >                         &clkc CLKID_MIPI_ENABLE
> >                         &clkc CLKID_PCIE_A
> > -                       &clkc CLKID_PCIE_CML_EN0>;
> > +                       &clkc CLKID_PCIE_CML_EN0
> > +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
> >                 clock-names = "pcie_general",
> >                                 "pcie_mipi_en",
> >                                 "pcie",
> > -                               "port";
> > +                               "port",
> > +                               "pll_cml_en";
> >                 resets = <&reset RESET_PCIE_PHY>,
> >                         <&reset RESET_PCIE_A>,
> >                         <&reset RESET_PCIE_APB>;
> 
> A few remarks for your future patches:
> 
> * You need to document any need binding you introduce:
>   It means that there should have been a patch in
>   Documentation/devicetree/... before using your newclock name in the
>   pcie driver. As Martin pointed out, dt-bindings should be dealt with
>   in their own patches
> 
> >
> >
> > Remi Pommarel (2):
> >   clk: meson: axg: add pcie pll cml gating
> 
> Whenever possible, patches intended for different maintainers should be
> sent separately (different series)

Thanks, will do both of the above remarks.

> 
> >   PCI: amlogic: Use PCIe pll gate when available
> >
> >  drivers/clk/meson/axg.c                | 3 +++
> >  drivers/clk/meson/axg.h                | 2 +-
> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
> >  include/dt-bindings/clock/axg-clkc.h   | 1 +
> >  4 files changed, 10 insertions(+), 1 deletion(-)
> 

Thanks for reviewing this.

[1] https://i.snipboard.io/bHMPeq.jpg
-- 
Remi

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Remi Pommarel]

On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
> 
> On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
> 
> > PCIe device probing failures have been seen on some AXG platforms and were
> > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> > solved the problem. After being contacted about this, vendor reported that
> > this bit was linked to PCIe PLL CML output.
> 
> Thanks for reporting the problem.
> 
> As Martin pointed out, the CML outputs already exist in the AXG clock
> controller but are handled using HHI_PCIE_PLL_CNTL6. Although
> incomplete, it seems to be aligned with the datasheet I have (v0.9)
> 
> According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
> Unfortunately bit 26 is not documented
> 
> AFAICT, the clock controller is not appropriate driver to deal with this
> register/bit
> 

Regarding both @Martin's and your remark.

Unfortunately the documentation I have and vendor feedback are a bit
vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
name for this bit because this register is MIPI related.

Here is the information I got from the vendor [1]. As you can see
HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
HHI_MIPI_CNTL0[29] is implemented in the clock controller as
axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].

So maybe I could rename this bit to something MIPI related ?

> >
> > This serie adds a way to set this bit through AXG clock gating logic.
> > Platforms having this kind of issue could make use of this gating by
> > applying a patch to their devicetree similar to:
> >
> >                 clocks = <&clkc CLKID_USB
> >                         &clkc CLKID_MIPI_ENABLE
> >                         &clkc CLKID_PCIE_A
> > -                       &clkc CLKID_PCIE_CML_EN0>;
> > +                       &clkc CLKID_PCIE_CML_EN0
> > +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
> >                 clock-names = "pcie_general",
> >                                 "pcie_mipi_en",
> >                                 "pcie",
> > -                               "port";
> > +                               "port",
> > +                               "pll_cml_en";
> >                 resets = <&reset RESET_PCIE_PHY>,
> >                         <&reset RESET_PCIE_A>,
> >                         <&reset RESET_PCIE_APB>;
> 
> A few remarks for your future patches:
> 
> * You need to document any need binding you introduce:
>   It means that there should have been a patch in
>   Documentation/devicetree/... before using your newclock name in the
>   pcie driver. As Martin pointed out, dt-bindings should be dealt with
>   in their own patches
> 
> >
> >
> > Remi Pommarel (2):
> >   clk: meson: axg: add pcie pll cml gating
> 
> Whenever possible, patches intended for different maintainers should be
> sent separately (different series)

Thanks, will do both of the above remarks.

> 
> >   PCI: amlogic: Use PCIe pll gate when available
> >
> >  drivers/clk/meson/axg.c                | 3 +++
> >  drivers/clk/meson/axg.h                | 2 +-
> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
> >  include/dt-bindings/clock/axg-clkc.h   | 1 +
> >  4 files changed, 10 insertions(+), 1 deletion(-)
> 

Thanks for reviewing this.

[1] https://i.snipboard.io/bHMPeq.jpg
-- 
Remi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Remi Pommarel]

On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
> 
> On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
> 
> > PCIe device probing failures have been seen on some AXG platforms and were
> > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> > solved the problem. After being contacted about this, vendor reported that
> > this bit was linked to PCIe PLL CML output.
> 
> Thanks for reporting the problem.
> 
> As Martin pointed out, the CML outputs already exist in the AXG clock
> controller but are handled using HHI_PCIE_PLL_CNTL6. Although
> incomplete, it seems to be aligned with the datasheet I have (v0.9)
> 
> According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
> Unfortunately bit 26 is not documented
> 
> AFAICT, the clock controller is not appropriate driver to deal with this
> register/bit
> 

Regarding both @Martin's and your remark.

Unfortunately the documentation I have and vendor feedback are a bit
vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
name for this bit because this register is MIPI related.

Here is the information I got from the vendor [1]. As you can see
HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
HHI_MIPI_CNTL0[29] is implemented in the clock controller as
axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].

So maybe I could rename this bit to something MIPI related ?

> >
> > This serie adds a way to set this bit through AXG clock gating logic.
> > Platforms having this kind of issue could make use of this gating by
> > applying a patch to their devicetree similar to:
> >
> >                 clocks = <&clkc CLKID_USB
> >                         &clkc CLKID_MIPI_ENABLE
> >                         &clkc CLKID_PCIE_A
> > -                       &clkc CLKID_PCIE_CML_EN0>;
> > +                       &clkc CLKID_PCIE_CML_EN0
> > +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
> >                 clock-names = "pcie_general",
> >                                 "pcie_mipi_en",
> >                                 "pcie",
> > -                               "port";
> > +                               "port",
> > +                               "pll_cml_en";
> >                 resets = <&reset RESET_PCIE_PHY>,
> >                         <&reset RESET_PCIE_A>,
> >                         <&reset RESET_PCIE_APB>;
> 
> A few remarks for your future patches:
> 
> * You need to document any need binding you introduce:
>   It means that there should have been a patch in
>   Documentation/devicetree/... before using your newclock name in the
>   pcie driver. As Martin pointed out, dt-bindings should be dealt with
>   in their own patches
> 
> >
> >
> > Remi Pommarel (2):
> >   clk: meson: axg: add pcie pll cml gating
> 
> Whenever possible, patches intended for different maintainers should be
> sent separately (different series)

Thanks, will do both of the above remarks.

> 
> >   PCI: amlogic: Use PCIe pll gate when available
> >
> >  drivers/clk/meson/axg.c                | 3 +++
> >  drivers/clk/meson/axg.h                | 2 +-
> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
> >  include/dt-bindings/clock/axg-clkc.h   | 1 +
> >  4 files changed, 10 insertions(+), 1 deletion(-)
> 

Thanks for reviewing this.

[1] https://i.snipboard.io/bHMPeq.jpg
-- 
Remi


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Remi Pommarel]

On Mon, Dec 09, 2019 at 11:03:15AM +0000, Andrew Murray wrote:
> On Sun, Dec 08, 2019 at 10:03:20PM +0100, Remi Pommarel wrote:
> > In order to get PCIe working reliably on some AXG platforms, PCIe pll
> > cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
> > clock gate.
> 
> s/cml/CML/
> 
> In addition to Jerome's feedback - it would also be helpful to explain
> when CML outputs should be enabled, i.e. which platforms and why those
> ones?
> 
> > 
> > This clock gate is optional, so do not fail if it is missing in the
> > devicetree.
> 
> If certain platforms require PCIE_PLL_CML_ENABLE to work reliably and
> thus the clock is specified in the device tree - then surely if there
> is an error in enabling the clock we should fail? I.e. should you only
> ignore -ENOENT here?

Good point. Will do.

Thanks

-- 
Remi

> 
> Thanks,
> 
> Andrew Murray
> 
> > 
> > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> > ---
> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> > index 3772b02a5c55..32b70ea9a426 100644
> > --- a/drivers/pci/controller/dwc/pci-meson.c
> > +++ b/drivers/pci/controller/dwc/pci-meson.c
> > @@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
> >  	struct clk *mipi_gate;
> >  	struct clk *port_clk;
> >  	struct clk *general_clk;
> > +	struct clk *pll_cml_gate;
> >  };
> >  
> >  struct meson_pcie_rc_reset {
> > @@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
> >  	if (IS_ERR(res->clk))
> >  		return PTR_ERR(res->clk);
> >  
> > +	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
> > +	if (IS_ERR(res->pll_cml_gate))
> > +		res->pll_cml_gate = NULL;
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.24.0
> > 

Re: [PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Remi Pommarel]

On Mon, Dec 09, 2019 at 11:03:15AM +0000, Andrew Murray wrote:
> On Sun, Dec 08, 2019 at 10:03:20PM +0100, Remi Pommarel wrote:
> > In order to get PCIe working reliably on some AXG platforms, PCIe pll
> > cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
> > clock gate.
> 
> s/cml/CML/
> 
> In addition to Jerome's feedback - it would also be helpful to explain
> when CML outputs should be enabled, i.e. which platforms and why those
> ones?
> 
> > 
> > This clock gate is optional, so do not fail if it is missing in the
> > devicetree.
> 
> If certain platforms require PCIE_PLL_CML_ENABLE to work reliably and
> thus the clock is specified in the device tree - then surely if there
> is an error in enabling the clock we should fail? I.e. should you only
> ignore -ENOENT here?

Good point. Will do.

Thanks

-- 
Remi

> 
> Thanks,
> 
> Andrew Murray
> 
> > 
> > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> > ---
> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> > index 3772b02a5c55..32b70ea9a426 100644
> > --- a/drivers/pci/controller/dwc/pci-meson.c
> > +++ b/drivers/pci/controller/dwc/pci-meson.c
> > @@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
> >  	struct clk *mipi_gate;
> >  	struct clk *port_clk;
> >  	struct clk *general_clk;
> > +	struct clk *pll_cml_gate;
> >  };
> >  
> >  struct meson_pcie_rc_reset {
> > @@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
> >  	if (IS_ERR(res->clk))
> >  		return PTR_ERR(res->clk);
> >  
> > +	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
> > +	if (IS_ERR(res->pll_cml_gate))
> > +		res->pll_cml_gate = NULL;
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.24.0
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: amlogic: Use PCIe pll gate when available

[Remi Pommarel]

On Mon, Dec 09, 2019 at 11:03:15AM +0000, Andrew Murray wrote:
> On Sun, Dec 08, 2019 at 10:03:20PM +0100, Remi Pommarel wrote:
> > In order to get PCIe working reliably on some AXG platforms, PCIe pll
> > cml needs to be enabled. This is done by using the PCIE_PLL_CML_ENABLE
> > clock gate.
> 
> s/cml/CML/
> 
> In addition to Jerome's feedback - it would also be helpful to explain
> when CML outputs should be enabled, i.e. which platforms and why those
> ones?
> 
> > 
> > This clock gate is optional, so do not fail if it is missing in the
> > devicetree.
> 
> If certain platforms require PCIE_PLL_CML_ENABLE to work reliably and
> thus the clock is specified in the device tree - then surely if there
> is an error in enabling the clock we should fail? I.e. should you only
> ignore -ENOENT here?

Good point. Will do.

Thanks

-- 
Remi

> 
> Thanks,
> 
> Andrew Murray
> 
> > 
> > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> > ---
> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> > index 3772b02a5c55..32b70ea9a426 100644
> > --- a/drivers/pci/controller/dwc/pci-meson.c
> > +++ b/drivers/pci/controller/dwc/pci-meson.c
> > @@ -89,6 +89,7 @@ struct meson_pcie_clk_res {
> >  	struct clk *mipi_gate;
> >  	struct clk *port_clk;
> >  	struct clk *general_clk;
> > +	struct clk *pll_cml_gate;
> >  };
> >  
> >  struct meson_pcie_rc_reset {
> > @@ -300,6 +301,10 @@ static int meson_pcie_probe_clocks(struct meson_pcie *mp)
> >  	if (IS_ERR(res->clk))
> >  		return PTR_ERR(res->clk);
> >  
> > +	res->pll_cml_gate = meson_pcie_probe_clock(dev, "pll_cml_en", 0);
> > +	if (IS_ERR(res->pll_cml_gate))
> > +		res->pll_cml_gate = NULL;
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.24.0
> > 

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Martin Blumenstingl]

Hi Remi,

On Sun, Dec 15, 2019 at 12:28 PM Remi Pommarel <repk@triplefau.lt> wrote:
>
> On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
> >
> > On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
> >
> > > PCIe device probing failures have been seen on some AXG platforms and were
> > > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> > > solved the problem. After being contacted about this, vendor reported that
> > > this bit was linked to PCIe PLL CML output.
> >
> > Thanks for reporting the problem.
> >
> > As Martin pointed out, the CML outputs already exist in the AXG clock
> > controller but are handled using HHI_PCIE_PLL_CNTL6. Although
> > incomplete, it seems to be aligned with the datasheet I have (v0.9)
> >
> > According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
> > Unfortunately bit 26 is not documented
> >
> > AFAICT, the clock controller is not appropriate driver to deal with this
> > register/bit
> >
>
> Regarding both @Martin's and your remark.
>
> Unfortunately the documentation I have and vendor feedback are a bit
> vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
> name for this bit because this register is MIPI related.
>
> Here is the information I got from the vendor [1]. As you can see
> HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
> HHI_MIPI_CNTL0[29] is implemented in the clock controller as
> axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].
I agree, the details you got so far are unfortunately pretty vague
(with my knowledge at least)
from my experience Amlogic has very good documentation internally, so
I'm sure that more details are available.

Yue Wang (the Amlogic PCIe controller maintainer) is already Cc'ed and
I added Jianxin. I hope that they can explain the meaning of bis 26
and 29 in HHI_MIPI_CNTL0 on the AXG SoCs (assuming Remi's contact at
Amlogic can't) and how they are related to the PCIe controller (even
though they're in a MIPI related register).


Martin

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Martin Blumenstingl]

Hi Remi,

On Sun, Dec 15, 2019 at 12:28 PM Remi Pommarel <repk@triplefau.lt> wrote:
>
> On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
> >
> > On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
> >
> > > PCIe device probing failures have been seen on some AXG platforms and were
> > > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> > > solved the problem. After being contacted about this, vendor reported that
> > > this bit was linked to PCIe PLL CML output.
> >
> > Thanks for reporting the problem.
> >
> > As Martin pointed out, the CML outputs already exist in the AXG clock
> > controller but are handled using HHI_PCIE_PLL_CNTL6. Although
> > incomplete, it seems to be aligned with the datasheet I have (v0.9)
> >
> > According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
> > Unfortunately bit 26 is not documented
> >
> > AFAICT, the clock controller is not appropriate driver to deal with this
> > register/bit
> >
>
> Regarding both @Martin's and your remark.
>
> Unfortunately the documentation I have and vendor feedback are a bit
> vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
> name for this bit because this register is MIPI related.
>
> Here is the information I got from the vendor [1]. As you can see
> HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
> HHI_MIPI_CNTL0[29] is implemented in the clock controller as
> axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].
I agree, the details you got so far are unfortunately pretty vague
(with my knowledge at least)
from my experience Amlogic has very good documentation internally, so
I'm sure that more details are available.

Yue Wang (the Amlogic PCIe controller maintainer) is already Cc'ed and
I added Jianxin. I hope that they can explain the meaning of bis 26
and 29 in HHI_MIPI_CNTL0 on the AXG SoCs (assuming Remi's contact at
Amlogic can't) and how they are related to the PCIe controller (even
though they're in a MIPI related register).


Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Martin Blumenstingl]

Hi Remi,

On Sun, Dec 15, 2019 at 12:28 PM Remi Pommarel <repk@triplefau.lt> wrote:
>
> On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
> >
> > On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
> >
> > > PCIe device probing failures have been seen on some AXG platforms and were
> > > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> > > solved the problem. After being contacted about this, vendor reported that
> > > this bit was linked to PCIe PLL CML output.
> >
> > Thanks for reporting the problem.
> >
> > As Martin pointed out, the CML outputs already exist in the AXG clock
> > controller but are handled using HHI_PCIE_PLL_CNTL6. Although
> > incomplete, it seems to be aligned with the datasheet I have (v0.9)
> >
> > According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
> > Unfortunately bit 26 is not documented
> >
> > AFAICT, the clock controller is not appropriate driver to deal with this
> > register/bit
> >
>
> Regarding both @Martin's and your remark.
>
> Unfortunately the documentation I have and vendor feedback are a bit
> vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
> name for this bit because this register is MIPI related.
>
> Here is the information I got from the vendor [1]. As you can see
> HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
> HHI_MIPI_CNTL0[29] is implemented in the clock controller as
> axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].
I agree, the details you got so far are unfortunately pretty vague
(with my knowledge at least)
from my experience Amlogic has very good documentation internally, so
I'm sure that more details are available.

Yue Wang (the Amlogic PCIe controller maintainer) is already Cc'ed and
I added Jianxin. I hope that they can explain the meaning of bis 26
and 29 in HHI_MIPI_CNTL0 on the AXG SoCs (assuming Remi's contact at
Amlogic can't) and how they are related to the PCIe controller (even
though they're in a MIPI related register).


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Jerome Brunet]

On Sun 15 Dec 2019 at 12:36, Remi Pommarel <repk@triplefau.lt> wrote:

> On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
>> 
>> On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
>> 
>> > PCIe device probing failures have been seen on some AXG platforms and were
>> > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
>> > solved the problem. After being contacted about this, vendor reported that
>> > this bit was linked to PCIe PLL CML output.
>> 
>> Thanks for reporting the problem.
>> 
>> As Martin pointed out, the CML outputs already exist in the AXG clock
>> controller but are handled using HHI_PCIE_PLL_CNTL6. Although
>> incomplete, it seems to be aligned with the datasheet I have (v0.9)
>> 
>> According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
>> Unfortunately bit 26 is not documented
>> 
>> AFAICT, the clock controller is not appropriate driver to deal with this
>> register/bit
>> 
>
> Regarding both @Martin's and your remark.
>
> Unfortunately the documentation I have and vendor feedback are a bit
> vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
> name for this bit because this register is MIPI related.
>
> Here is the information I got from the vendor [1]. As you can see
> HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
> HHI_MIPI_CNTL0[29] is implemented in the clock controller as
> axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].
>

Seems I should have paid more attention when axg_mipi_enable.
Bit 29 is yet another undocumented bit

> So maybe I could rename this bit to something MIPI related ?

This register region is simply not part of the main clock
controller. The bits in it are not related to this controller but the
MIPI PHY. It should not have been mapped in this way to begin with.

I can see how it would be convient to model this with a gate to just
flip the bit when needed but it is just wrong.

The documentation says the register are for the MIPI analog PHY, it
should be implemented as such and used by the PCIe as needed.

Of course, fixing this (remapping the region and removing
axg_mipi_enable) will be a bit messy. If you want to make that MIPI Phy
driver, I can help you with the clock part.

>
>> >
>> > This serie adds a way to set this bit through AXG clock gating logic.
>> > Platforms having this kind of issue could make use of this gating by
>> > applying a patch to their devicetree similar to:
>> >
>> >                 clocks = <&clkc CLKID_USB
>> >                         &clkc CLKID_MIPI_ENABLE
>> >                         &clkc CLKID_PCIE_A
>> > -                       &clkc CLKID_PCIE_CML_EN0>;
>> > +                       &clkc CLKID_PCIE_CML_EN0
>> > +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
>> >                 clock-names = "pcie_general",
>> >                                 "pcie_mipi_en",
>> >                                 "pcie",
>> > -                               "port";
>> > +                               "port",
>> > +                               "pll_cml_en";
>> >                 resets = <&reset RESET_PCIE_PHY>,
>> >                         <&reset RESET_PCIE_A>,
>> >                         <&reset RESET_PCIE_APB>;
>> 
>> A few remarks for your future patches:
>> 
>> * You need to document any need binding you introduce:
>>   It means that there should have been a patch in
>>   Documentation/devicetree/... before using your newclock name in the
>>   pcie driver. As Martin pointed out, dt-bindings should be dealt with
>>   in their own patches
>> 
>> >
>> >
>> > Remi Pommarel (2):
>> >   clk: meson: axg: add pcie pll cml gating
>> 
>> Whenever possible, patches intended for different maintainers should be
>> sent separately (different series)
>
> Thanks, will do both of the above remarks.
>
>> 
>> >   PCI: amlogic: Use PCIe pll gate when available
>> >
>> >  drivers/clk/meson/axg.c                | 3 +++
>> >  drivers/clk/meson/axg.h                | 2 +-
>> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>> >  include/dt-bindings/clock/axg-clkc.h   | 1 +
>> >  4 files changed, 10 insertions(+), 1 deletion(-)
>> 
>
> Thanks for reviewing this.
>
> [1] https://i.snipboard.io/bHMPeq.jpg

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Jerome Brunet]

On Sun 15 Dec 2019 at 12:36, Remi Pommarel <repk@triplefau.lt> wrote:

> On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
>> 
>> On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
>> 
>> > PCIe device probing failures have been seen on some AXG platforms and were
>> > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
>> > solved the problem. After being contacted about this, vendor reported that
>> > this bit was linked to PCIe PLL CML output.
>> 
>> Thanks for reporting the problem.
>> 
>> As Martin pointed out, the CML outputs already exist in the AXG clock
>> controller but are handled using HHI_PCIE_PLL_CNTL6. Although
>> incomplete, it seems to be aligned with the datasheet I have (v0.9)
>> 
>> According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
>> Unfortunately bit 26 is not documented
>> 
>> AFAICT, the clock controller is not appropriate driver to deal with this
>> register/bit
>> 
>
> Regarding both @Martin's and your remark.
>
> Unfortunately the documentation I have and vendor feedback are a bit
> vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
> name for this bit because this register is MIPI related.
>
> Here is the information I got from the vendor [1]. As you can see
> HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
> HHI_MIPI_CNTL0[29] is implemented in the clock controller as
> axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].
>

Seems I should have paid more attention when axg_mipi_enable.
Bit 29 is yet another undocumented bit

> So maybe I could rename this bit to something MIPI related ?

This register region is simply not part of the main clock
controller. The bits in it are not related to this controller but the
MIPI PHY. It should not have been mapped in this way to begin with.

I can see how it would be convient to model this with a gate to just
flip the bit when needed but it is just wrong.

The documentation says the register are for the MIPI analog PHY, it
should be implemented as such and used by the PCIe as needed.

Of course, fixing this (remapping the region and removing
axg_mipi_enable) will be a bit messy. If you want to make that MIPI Phy
driver, I can help you with the clock part.

>
>> >
>> > This serie adds a way to set this bit through AXG clock gating logic.
>> > Platforms having this kind of issue could make use of this gating by
>> > applying a patch to their devicetree similar to:
>> >
>> >                 clocks = <&clkc CLKID_USB
>> >                         &clkc CLKID_MIPI_ENABLE
>> >                         &clkc CLKID_PCIE_A
>> > -                       &clkc CLKID_PCIE_CML_EN0>;
>> > +                       &clkc CLKID_PCIE_CML_EN0
>> > +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
>> >                 clock-names = "pcie_general",
>> >                                 "pcie_mipi_en",
>> >                                 "pcie",
>> > -                               "port";
>> > +                               "port",
>> > +                               "pll_cml_en";
>> >                 resets = <&reset RESET_PCIE_PHY>,
>> >                         <&reset RESET_PCIE_A>,
>> >                         <&reset RESET_PCIE_APB>;
>> 
>> A few remarks for your future patches:
>> 
>> * You need to document any need binding you introduce:
>>   It means that there should have been a patch in
>>   Documentation/devicetree/... before using your newclock name in the
>>   pcie driver. As Martin pointed out, dt-bindings should be dealt with
>>   in their own patches
>> 
>> >
>> >
>> > Remi Pommarel (2):
>> >   clk: meson: axg: add pcie pll cml gating
>> 
>> Whenever possible, patches intended for different maintainers should be
>> sent separately (different series)
>
> Thanks, will do both of the above remarks.
>
>> 
>> >   PCI: amlogic: Use PCIe pll gate when available
>> >
>> >  drivers/clk/meson/axg.c                | 3 +++
>> >  drivers/clk/meson/axg.h                | 2 +-
>> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>> >  include/dt-bindings/clock/axg-clkc.h   | 1 +
>> >  4 files changed, 10 insertions(+), 1 deletion(-)
>> 
>
> Thanks for reviewing this.
>
> [1] https://i.snipboard.io/bHMPeq.jpg


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

[Jerome Brunet]

On Sun 15 Dec 2019 at 12:36, Remi Pommarel <repk@triplefau.lt> wrote:

> On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
>> 
>> On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@triplefau.lt> wrote:
>> 
>> > PCIe device probing failures have been seen on some AXG platforms and were
>> > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
>> > solved the problem. After being contacted about this, vendor reported that
>> > this bit was linked to PCIe PLL CML output.
>> 
>> Thanks for reporting the problem.
>> 
>> As Martin pointed out, the CML outputs already exist in the AXG clock
>> controller but are handled using HHI_PCIE_PLL_CNTL6. Although
>> incomplete, it seems to be aligned with the datasheet I have (v0.9)
>> 
>> According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
>> Unfortunately bit 26 is not documented
>> 
>> AFAICT, the clock controller is not appropriate driver to deal with this
>> register/bit
>> 
>
> Regarding both @Martin's and your remark.
>
> Unfortunately the documentation I have and vendor feedback are a bit
> vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
> name for this bit because this register is MIPI related.
>
> Here is the information I got from the vendor [1]. As you can see
> HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
> HHI_MIPI_CNTL0[29] is implemented in the clock controller as
> axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].
>

Seems I should have paid more attention when axg_mipi_enable.
Bit 29 is yet another undocumented bit

> So maybe I could rename this bit to something MIPI related ?

This register region is simply not part of the main clock
controller. The bits in it are not related to this controller but the
MIPI PHY. It should not have been mapped in this way to begin with.

I can see how it would be convient to model this with a gate to just
flip the bit when needed but it is just wrong.

The documentation says the register are for the MIPI analog PHY, it
should be implemented as such and used by the PCIe as needed.

Of course, fixing this (remapping the region and removing
axg_mipi_enable) will be a bit messy. If you want to make that MIPI Phy
driver, I can help you with the clock part.

>
>> >
>> > This serie adds a way to set this bit through AXG clock gating logic.
>> > Platforms having this kind of issue could make use of this gating by
>> > applying a patch to their devicetree similar to:
>> >
>> >                 clocks = <&clkc CLKID_USB
>> >                         &clkc CLKID_MIPI_ENABLE
>> >                         &clkc CLKID_PCIE_A
>> > -                       &clkc CLKID_PCIE_CML_EN0>;
>> > +                       &clkc CLKID_PCIE_CML_EN0
>> > +                       &clkc CLKID_PCIE_PLL_CML_ENABLE>;
>> >                 clock-names = "pcie_general",
>> >                                 "pcie_mipi_en",
>> >                                 "pcie",
>> > -                               "port";
>> > +                               "port",
>> > +                               "pll_cml_en";
>> >                 resets = <&reset RESET_PCIE_PHY>,
>> >                         <&reset RESET_PCIE_A>,
>> >                         <&reset RESET_PCIE_APB>;
>> 
>> A few remarks for your future patches:
>> 
>> * You need to document any need binding you introduce:
>>   It means that there should have been a patch in
>>   Documentation/devicetree/... before using your newclock name in the
>>   pcie driver. As Martin pointed out, dt-bindings should be dealt with
>>   in their own patches
>> 
>> >
>> >
>> > Remi Pommarel (2):
>> >   clk: meson: axg: add pcie pll cml gating
>> 
>> Whenever possible, patches intended for different maintainers should be
>> sent separately (different series)
>
> Thanks, will do both of the above remarks.
>
>> 
>> >   PCI: amlogic: Use PCIe pll gate when available
>> >
>> >  drivers/clk/meson/axg.c                | 3 +++
>> >  drivers/clk/meson/axg.h                | 2 +-
>> >  drivers/pci/controller/dwc/pci-meson.c | 5 +++++
>> >  include/dt-bindings/clock/axg-clkc.h   | 1 +
>> >  4 files changed, 10 insertions(+), 1 deletion(-)
>> 
>
> Thanks for reviewing this.
>
> [1] https://i.snipboard.io/bHMPeq.jpg


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic