[PATCH 0/4] Introduce clock support for Airoha EN7581 SoC

[PATCH 0/4] Introduce clock support for Airoha EN7581 SoC

[Lorenzo Bianconi]

This series is based on the following series:
https://patchwork.kernel.org/project/linux-arm-kernel/cover/cover.1709975956.git.lorenzo@kernel.org/

Lorenzo Bianconi (4):
  dt-bindings: clock: airoha: add EN7581 binding
  arm64: dts: airoha: Add EN7581 clock node
  clk: en7523: make pcie clk_ops accessible through of_device_id struct
  clk: en7523: add EN7581 support

 .../bindings/clock/airoha,en7523-scu.yaml     |  26 ++-
 arch/arm64/boot/dts/airoha/en7581.dtsi        |   9 +
 drivers/clk/clk-en7523.c                      | 155 ++++++++++++++++--
 3 files changed, 171 insertions(+), 19 deletions(-)

-- 
2.44.0

[PATCH 1/4] dt-bindings: clock: airoha: add EN7581 binding

[Lorenzo Bianconi]

Introduce Airoha EN7581 entry in Airoha EN7523 clock binding

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../bindings/clock/airoha,en7523-scu.yaml     | 26 +++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
index 79b0752faa91..cf893d4c74cd 100644
--- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
+++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
@@ -29,10 +29,13 @@ description: |
 properties:
   compatible:
     items:
-      - const: airoha,en7523-scu
+      - enum:
+          - airoha,en7523-scu
+          - airoha,en7581-scu
 
   reg:
-    maxItems: 2
+    minItems: 2
+    maxItems: 3
 
   "#clock-cells":
     description:
@@ -45,6 +48,25 @@ required:
   - reg
   - '#clock-cells'
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          const: airoha,en7523-scu
+    then:
+      properties:
+        reg:
+          maxItems: 2
+
+  - if:
+      properties:
+        compatible:
+          const: airoha,en7581-scu
+    then:
+      properties:
+        reg:
+          maxItems: 3
+
 additionalProperties: false
 
 examples:
-- 
2.44.0

[PATCH 2/4] arm64: dts: airoha: Add EN7581 clock node

[Lorenzo Bianconi]

Introduce the Airoha EN7581 clock node in Airoha EN7581 dtsi

Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 arch/arm64/boot/dts/airoha/en7581.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/airoha/en7581.dtsi b/arch/arm64/boot/dts/airoha/en7581.dtsi
index 55eb1762fb11..a1daaaef0de0 100644
--- a/arch/arm64/boot/dts/airoha/en7581.dtsi
+++ b/arch/arm64/boot/dts/airoha/en7581.dtsi
@@ -2,6 +2,7 @@
 
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/clock/en7523-clk.h>
 
 / {
 	interrupt-parent = <&gic>;
@@ -150,5 +151,13 @@ uart1: serial@1fbf0000 {
 			interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
 			clock-frequency = <1843200>;
 		};
+
+		scu: system-controller@1fa20000 {
+			compatible = "airoha,en7581-scu";
+			reg = <0x0 0x1fa20000 0x0 0x400>,
+			      <0x0 0x1fb00000 0x0 0x1000>,
+			      <0x0 0x1fbe3400 0x0 0xfc>;
+			#clock-cells = <1>;
+		};
 	};
 };
-- 
2.44.0

[PATCH 3/4] clk: en7523: make pcie clk_ops accessible through of_device_id struct

[Lorenzo Bianconi]

Make pcie clk_ops structure accessible through of_device_id data
pointer in order to define multiple clk_ops for each supported SoC.
This is a preliminary patch to introduce EN7581 clock support.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/clk/clk-en7523.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
index 7cde328495e2..c7def87b74c6 100644
--- a/drivers/clk/clk-en7523.c
+++ b/drivers/clk/clk-en7523.c
@@ -145,11 +145,6 @@ static const struct en_clk_desc en7523_base_clks[] = {
 	}
 };
 
-static const struct of_device_id of_match_clk_en7523[] = {
-	{ .compatible = "airoha,en7523-scu", },
-	{ /* sentinel */ }
-};
-
 static unsigned int en7523_get_base_rate(void __iomem *base, unsigned int i)
 {
 	const struct en_clk_desc *desc = &en7523_base_clks[i];
@@ -247,14 +242,9 @@ static void en7523_pci_unprepare(struct clk_hw *hw)
 static struct clk_hw *en7523_register_pcie_clk(struct device *dev,
 					       void __iomem *np_base)
 {
-	static const struct clk_ops pcie_gate_ops = {
-		.is_enabled = en7523_pci_is_enabled,
-		.prepare = en7523_pci_prepare,
-		.unprepare = en7523_pci_unprepare,
-	};
 	struct clk_init_data init = {
 		.name = "pcie",
-		.ops = &pcie_gate_ops,
+		.ops = of_device_get_match_data(dev),
 	};
 	struct en_clk_gate *cg;
 
@@ -264,7 +254,7 @@ static struct clk_hw *en7523_register_pcie_clk(struct device *dev,
 
 	cg->base = np_base;
 	cg->hw.init = &init;
-	en7523_pci_unprepare(&cg->hw);
+	init.ops->unprepare(&cg->hw);
 
 	if (clk_hw_register(dev, &cg->hw))
 		return NULL;
@@ -333,6 +323,17 @@ static int en7523_clk_probe(struct platform_device *pdev)
 	return r;
 }
 
+static const struct clk_ops en7523_pcie_ops = {
+	.is_enabled = en7523_pci_is_enabled,
+	.prepare = en7523_pci_prepare,
+	.unprepare = en7523_pci_unprepare,
+};
+
+static const struct of_device_id of_match_clk_en7523[] = {
+	{ .compatible = "airoha,en7523-scu", .data = &en7523_pcie_ops },
+	{ /* sentinel */ }
+};
+
 static struct platform_driver clk_en7523_drv = {
 	.probe = en7523_clk_probe,
 	.driver = {
-- 
2.44.0

[PATCH 4/4] clk: en7523: add EN7581 support

[Lorenzo Bianconi]

Introduce EN7581 clock support to clk-en7523 driver.

Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/clk/clk-en7523.c | 130 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 125 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
index c7def87b74c6..51a6c0cc7f58 100644
--- a/drivers/clk/clk-en7523.c
+++ b/drivers/clk/clk-en7523.c
@@ -4,13 +4,16 @@
 #include <linux/clk-provider.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <dt-bindings/clock/en7523-clk.h>
 
 #define REG_PCI_CONTROL			0x88
 #define   REG_PCI_CONTROL_PERSTOUT	BIT(29)
 #define   REG_PCI_CONTROL_PERSTOUT1	BIT(26)
+#define   REG_PCI_CONTROL_REFCLK_EN0	BIT(23)
 #define   REG_PCI_CONTROL_REFCLK_EN1	BIT(22)
+#define   REG_PCI_CONTROL_PERSTOUT2	BIT(16)
 #define REG_GSW_CLK_DIV_SEL		0x1b4
 #define REG_EMI_CLK_DIV_SEL		0x1b8
 #define REG_BUS_CLK_DIV_SEL		0x1bc
@@ -18,10 +21,25 @@
 #define REG_SPI_CLK_FREQ_SEL		0x1c8
 #define REG_NPU_CLK_DIV_SEL		0x1fc
 #define REG_CRYPTO_CLKSRC		0x200
-#define REG_RESET_CONTROL		0x834
+#define REG_RESET_CONTROL2		0x830
+#define   REG_RESET2_CONTROL_PCIE2	BIT(27)
+#define REG_RESET_CONTROL1		0x834
 #define   REG_RESET_CONTROL_PCIEHB	BIT(29)
 #define   REG_RESET_CONTROL_PCIE1	BIT(27)
 #define   REG_RESET_CONTROL_PCIE2	BIT(26)
+/* EN7581 */
+#define REG_PCIE0_MEM			0x00
+#define REG_PCIE0_MEM_MASK		0x04
+#define REG_PCIE1_MEM			0x08
+#define REG_PCIE1_MEM_MASK		0x0c
+#define REG_PCIE2_MEM			0x10
+#define REG_PCIE2_MEM_MASK		0x14
+#define REG_PCIE_RESET_OPEN_DRAIN	0x018c
+#define REG_PCIE_RESET_OPEN_DRAIN_MASK	GENMASK(2, 0)
+#define REG_NP_SCU_PCIC			0x88
+#define REG_NP_SCU_SSTR			0x9c
+#define REG_PCIE_XSI0_SEL_MASK		GENMASK(14, 13)
+#define REG_PCIE_XSI1_SEL_MASK		GENMASK(12, 11)
 
 struct en_clk_desc {
 	int id;
@@ -207,14 +225,14 @@ static int en7523_pci_prepare(struct clk_hw *hw)
 	usleep_range(1000, 2000);
 
 	/* Reset to default */
-	val = readl(np_base + REG_RESET_CONTROL);
+	val = readl(np_base + REG_RESET_CONTROL1);
 	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
 	       REG_RESET_CONTROL_PCIEHB;
-	writel(val & ~mask, np_base + REG_RESET_CONTROL);
+	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
 	usleep_range(1000, 2000);
-	writel(val | mask, np_base + REG_RESET_CONTROL);
+	writel(val | mask, np_base + REG_RESET_CONTROL1);
 	msleep(100);
-	writel(val & ~mask, np_base + REG_RESET_CONTROL);
+	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
 	usleep_range(5000, 10000);
 
 	/* Release device */
@@ -262,6 +280,64 @@ static struct clk_hw *en7523_register_pcie_clk(struct device *dev,
 	return &cg->hw;
 }
 
+static int en7581_pci_is_enabled(struct clk_hw *hw)
+{
+	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
+	u32 val, mask;
+
+	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
+	val = readl(cg->base + REG_PCI_CONTROL);
+	return (val & mask) == mask;
+}
+
+static int en7581_pci_prepare(struct clk_hw *hw)
+{
+	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
+	void __iomem *np_base = cg->base;
+	u32 val, mask;
+
+	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
+	       REG_RESET_CONTROL_PCIEHB;
+	val = readl(np_base + REG_RESET_CONTROL1);
+	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
+	val = readl(np_base + REG_RESET_CONTROL2);
+	writel(val & ~REG_RESET2_CONTROL_PCIE2, np_base + REG_RESET_CONTROL2);
+	usleep_range(5000, 10000);
+
+	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
+	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
+	       REG_PCI_CONTROL_PERSTOUT;
+	val = readl(np_base + REG_PCI_CONTROL);
+	writel(val | mask, np_base + REG_PCI_CONTROL);
+	msleep(250);
+
+	return 0;
+}
+
+static void en7581_pci_unprepare(struct clk_hw *hw)
+{
+	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
+	void __iomem *np_base = cg->base;
+	u32 val, mask;
+
+	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
+	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
+	       REG_PCI_CONTROL_PERSTOUT;
+	val = readl(np_base + REG_PCI_CONTROL);
+	writel(val & ~mask, np_base + REG_PCI_CONTROL);
+	usleep_range(1000, 2000);
+
+	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
+	       REG_RESET_CONTROL_PCIEHB;
+	val = readl(np_base + REG_RESET_CONTROL1);
+	writel(val | mask, np_base + REG_RESET_CONTROL1);
+	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2;
+	writel(val | mask, np_base + REG_RESET_CONTROL1);
+	val = readl(np_base + REG_RESET_CONTROL2);
+	writel(val | REG_RESET_CONTROL_PCIE2, np_base + REG_RESET_CONTROL2);
+	msleep(100);
+}
+
 static void en7523_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
 				   void __iomem *base, void __iomem *np_base)
 {
@@ -291,6 +367,37 @@ static void en7523_register_clocks(struct device *dev, struct clk_hw_onecell_dat
 	clk_data->num = EN7523_NUM_CLOCKS;
 }
 
+static int en7581_clk_hw_init(struct platform_device *pdev,
+			      void __iomem *base,
+			      void __iomem *np_base)
+{
+	void __iomem *pb_base;
+	u32 val;
+
+	pb_base = devm_platform_ioremap_resource(pdev, 2);
+	if (IS_ERR(pb_base))
+		return PTR_ERR(pb_base);
+
+	val = readl(np_base + REG_NP_SCU_SSTR);
+	val &= ~(REG_PCIE_XSI0_SEL_MASK | REG_PCIE_XSI1_SEL_MASK);
+	writel(val, np_base + REG_NP_SCU_SSTR);
+	val = readl(np_base + REG_NP_SCU_PCIC);
+	writel(val | 3, np_base + REG_NP_SCU_PCIC);
+
+	writel(0x20000000, pb_base + REG_PCIE0_MEM);
+	writel(0xfc000000, pb_base + REG_PCIE0_MEM_MASK);
+	writel(0x24000000, pb_base + REG_PCIE1_MEM);
+	writel(0xfc000000, pb_base + REG_PCIE1_MEM_MASK);
+	writel(0x28000000, pb_base + REG_PCIE2_MEM);
+	writel(0xfc000000, pb_base + REG_PCIE2_MEM_MASK);
+
+	val = readl(base + REG_PCIE_RESET_OPEN_DRAIN);
+	writel(val | REG_PCIE_RESET_OPEN_DRAIN_MASK,
+	       base + REG_PCIE_RESET_OPEN_DRAIN);
+
+	return 0;
+}
+
 static int en7523_clk_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
@@ -306,6 +413,12 @@ static int en7523_clk_probe(struct platform_device *pdev)
 	if (IS_ERR(np_base))
 		return PTR_ERR(np_base);
 
+	if (of_device_is_compatible(node, "airoha,en7581-scu")) {
+		r = en7581_clk_hw_init(pdev, base, np_base);
+		if (r)
+			return r;
+	}
+
 	clk_data = devm_kzalloc(&pdev->dev,
 				struct_size(clk_data, hws, EN7523_NUM_CLOCKS),
 				GFP_KERNEL);
@@ -329,8 +442,15 @@ static const struct clk_ops en7523_pcie_ops = {
 	.unprepare = en7523_pci_unprepare,
 };
 
+static const struct clk_ops en7581_pcie_ops = {
+	.is_enabled = en7581_pci_is_enabled,
+	.prepare = en7581_pci_prepare,
+	.unprepare = en7581_pci_unprepare,
+};
+
 static const struct of_device_id of_match_clk_en7523[] = {
 	{ .compatible = "airoha,en7523-scu", .data = &en7523_pcie_ops },
+	{ .compatible = "airoha,en7581-scu", .data = &en7581_pcie_ops },
 	{ /* sentinel */ }
 };
 
-- 
2.44.0

Re: [PATCH 1/4] dt-bindings: clock: airoha: add EN7581 binding

[Krzysztof Kozlowski]

On 03/04/2024 18:20, Lorenzo Bianconi wrote:
> Introduce Airoha EN7581 entry in Airoha EN7523 clock binding
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  .../bindings/clock/airoha,en7523-scu.yaml     | 26 +++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> index 79b0752faa91..cf893d4c74cd 100644
> --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> @@ -29,10 +29,13 @@ description: |
>  properties:
>    compatible:
>      items:
> -      - const: airoha,en7523-scu
> +      - enum:
> +          - airoha,en7523-scu
> +          - airoha,en7581-scu
>  
>    reg:
> -    maxItems: 2
> +    minItems: 2
> +    maxItems: 3
>  
>    "#clock-cells":
>      description:
> @@ -45,6 +48,25 @@ required:
>    - reg
>    - '#clock-cells'
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: airoha,en7523-scu
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 2
> +
> +  - if:
> +      properties:
> +        compatible:
> +          const: airoha,en7581-scu
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 3

Original code had here issue - lack of description of the items. You are
now growing it. Please instead list the items (items: - description: foo
bar .....).

Best regards,
Krzysztof

Re: [PATCH 4/4] clk: en7523: add EN7581 support

[Krzysztof Kozlowski]

On 03/04/2024 18:20, Lorenzo Bianconi wrote:
> Introduce EN7581 clock support to clk-en7523 driver.
> 
> Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>


> +	return 0;
> +}
> +
>  static int en7523_clk_probe(struct platform_device *pdev)
>  {
>  	struct device_node *node = pdev->dev.of_node;
> @@ -306,6 +413,12 @@ static int en7523_clk_probe(struct platform_device *pdev)
>  	if (IS_ERR(np_base))
>  		return PTR_ERR(np_base);
>  
> +	if (of_device_is_compatible(node, "airoha,en7581-scu")) {

Having matching and compatible comparisons inside various code is
discouraged. Does not scale. Use driver/match data to store some sort of
flags and check for the flag or some other parameter. The best if
compatible appears once and only once: in of_device_id.

> +		r = en7581_clk_hw_init(pdev, base, np_base);
> +		if (r)
> +			return r;
> +	}
> +
>  	clk_data = devm_kzalloc(&pdev->dev,
>  				struct_size(clk_data, hws, EN7523_NUM_CLOCKS),
>  				GFP_KERNEL);
> @@ -329,8 +442,15 @@ static const struct clk_ops en7523_pcie_ops = {
>  	.unprepare = en7523_pci_unprepare,
>  };
>  
> +static const struct clk_ops en7581_pcie_ops = {
> +	.is_enabled = en7581_pci_is_enabled,
> +	.prepare = en7581_pci_prepare,
> +	.unprepare = en7581_pci_unprepare,
> +};
> +
>  static const struct of_device_id of_match_clk_en7523[] = {
>  	{ .compatible = "airoha,en7523-scu", .data = &en7523_pcie_ops },
> +	{ .compatible = "airoha,en7581-scu", .data = &en7581_pcie_ops },
>  	{ /* sentinel */ }
>  };
>  

Best regards,
Krzysztof

Re: [PATCH 4/4] clk: en7523: add EN7581 support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

> On 03/04/2024 18:20, Lorenzo Bianconi wrote:
> > Introduce EN7581 clock support to clk-en7523 driver.
> > 
> > Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> 
> > +	return 0;
> > +}
> > +
> >  static int en7523_clk_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node *node = pdev->dev.of_node;
> > @@ -306,6 +413,12 @@ static int en7523_clk_probe(struct platform_device *pdev)
> >  	if (IS_ERR(np_base))
> >  		return PTR_ERR(np_base);
> >  
> > +	if (of_device_is_compatible(node, "airoha,en7581-scu")) {
> 
> Having matching and compatible comparisons inside various code is
> discouraged. Does not scale. Use driver/match data to store some sort of
> flags and check for the flag or some other parameter. The best if
> compatible appears once and only once: in of_device_id.

ack, I will fix it.

Regards,
Lorenzo

> 
> > +		r = en7581_clk_hw_init(pdev, base, np_base);
> > +		if (r)
> > +			return r;
> > +	}
> > +
> >  	clk_data = devm_kzalloc(&pdev->dev,
> >  				struct_size(clk_data, hws, EN7523_NUM_CLOCKS),
> >  				GFP_KERNEL);
> > @@ -329,8 +442,15 @@ static const struct clk_ops en7523_pcie_ops = {
> >  	.unprepare = en7523_pci_unprepare,
> >  };
> >  
> > +static const struct clk_ops en7581_pcie_ops = {
> > +	.is_enabled = en7581_pci_is_enabled,
> > +	.prepare = en7581_pci_prepare,
> > +	.unprepare = en7581_pci_unprepare,
> > +};
> > +
> >  static const struct of_device_id of_match_clk_en7523[] = {
> >  	{ .compatible = "airoha,en7523-scu", .data = &en7523_pcie_ops },
> > +	{ .compatible = "airoha,en7581-scu", .data = &en7581_pcie_ops },
> >  	{ /* sentinel */ }
> >  };
> >  
> 
> Best regards,
> Krzysztof
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] dt-bindings: clock: airoha: add EN7581 binding

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

> On 03/04/2024 18:20, Lorenzo Bianconi wrote:
> > Introduce Airoha EN7581 entry in Airoha EN7523 clock binding
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  .../bindings/clock/airoha,en7523-scu.yaml     | 26 +++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> > index 79b0752faa91..cf893d4c74cd 100644
> > --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> > +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> > @@ -29,10 +29,13 @@ description: |
> >  properties:
> >    compatible:
> >      items:
> > -      - const: airoha,en7523-scu
> > +      - enum:
> > +          - airoha,en7523-scu
> > +          - airoha,en7581-scu
> >  
> >    reg:
> > -    maxItems: 2
> > +    minItems: 2
> > +    maxItems: 3
> >  
> >    "#clock-cells":
> >      description:
> > @@ -45,6 +48,25 @@ required:
> >    - reg
> >    - '#clock-cells'
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: airoha,en7523-scu
> > +    then:
> > +      properties:
> > +        reg:
> > +          maxItems: 2
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: airoha,en7581-scu
> > +    then:
> > +      properties:
> > +        reg:
> > +          maxItems: 3
> 
> Original code had here issue - lack of description of the items. You are
> now growing it. Please instead list the items (items: - description: foo
> bar .....).

ack, I will fix it.

Regards,
Lorenzo

> 
> Best regards,
> Krzysztof
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] arm64: dts: airoha: Add EN7581 clock node

[AngeloGioacchino Del Regno]

Il 03/04/24 18:20, Lorenzo Bianconi ha scritto:
> Introduce the Airoha EN7581 clock node in Airoha EN7581 dtsi
> 
> Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>   arch/arm64/boot/dts/airoha/en7581.dtsi | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/airoha/en7581.dtsi b/arch/arm64/boot/dts/airoha/en7581.dtsi
> index 55eb1762fb11..a1daaaef0de0 100644
> --- a/arch/arm64/boot/dts/airoha/en7581.dtsi
> +++ b/arch/arm64/boot/dts/airoha/en7581.dtsi
> @@ -2,6 +2,7 @@
>   
>   #include <dt-bindings/interrupt-controller/irq.h>
>   #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/en7523-clk.h>
>   
>   / {
>   	interrupt-parent = <&gic>;
> @@ -150,5 +151,13 @@ uart1: serial@1fbf0000 {
>   			interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
>   			clock-frequency = <1843200>;
>   		};
> +
> +		scu: system-controller@1fa20000 {

Uhm, why is this not a clock-controller but a system-controller?

Cheers,
Angelo

> +			compatible = "airoha,en7581-scu";
> +			reg = <0x0 0x1fa20000 0x0 0x400>,
> +			      <0x0 0x1fb00000 0x0 0x1000>,
> +			      <0x0 0x1fbe3400 0x0 0xfc>;
> +			#clock-cells = <1>;
> +		};
>   	};
>   };

Re: [PATCH 2/4] arm64: dts: airoha: Add EN7581 clock node

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]

> Il 03/04/24 18:20, Lorenzo Bianconi ha scritto:
> > Introduce the Airoha EN7581 clock node in Airoha EN7581 dtsi
> > 
> > Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >   arch/arm64/boot/dts/airoha/en7581.dtsi | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/airoha/en7581.dtsi b/arch/arm64/boot/dts/airoha/en7581.dtsi
> > index 55eb1762fb11..a1daaaef0de0 100644
> > --- a/arch/arm64/boot/dts/airoha/en7581.dtsi
> > +++ b/arch/arm64/boot/dts/airoha/en7581.dtsi
> > @@ -2,6 +2,7 @@
> >   #include <dt-bindings/interrupt-controller/irq.h>
> >   #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/clock/en7523-clk.h>
> >   / {
> >   	interrupt-parent = <&gic>;
> > @@ -150,5 +151,13 @@ uart1: serial@1fbf0000 {
> >   			interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> >   			clock-frequency = <1843200>;
> >   		};
> > +
> > +		scu: system-controller@1fa20000 {
> 
> Uhm, why is this not a clock-controller but a system-controller?

I used the same approach used for en7523.dtsi. I guess it is done
that way because the registers come from scu (system control unit)
regmap, but I guess we can use clock-controller instead.

Regards,
Lorenzo

> 
> Cheers,
> Angelo
> 
> > +			compatible = "airoha,en7581-scu";
> > +			reg = <0x0 0x1fa20000 0x0 0x400>,
> > +			      <0x0 0x1fb00000 0x0 0x1000>,
> > +			      <0x0 0x1fbe3400 0x0 0xfc>;
> > +			#clock-cells = <1>;
> > +		};
> >   	};
> >   };
> 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 4/4] clk: en7523: add EN7581 support

[AngeloGioacchino Del Regno]

Il 03/04/24 18:20, Lorenzo Bianconi ha scritto:
> Introduce EN7581 clock support to clk-en7523 driver.
> 
> Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>   drivers/clk/clk-en7523.c | 130 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 125 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index c7def87b74c6..51a6c0cc7f58 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -4,13 +4,16 @@
>   #include <linux/clk-provider.h>
>   #include <linux/io.h>
>   #include <linux/of.h>
> +#include <linux/of_device.h>
>   #include <linux/platform_device.h>
>   #include <dt-bindings/clock/en7523-clk.h>
>   
>   #define REG_PCI_CONTROL			0x88
>   #define   REG_PCI_CONTROL_PERSTOUT	BIT(29)
>   #define   REG_PCI_CONTROL_PERSTOUT1	BIT(26)
> +#define   REG_PCI_CONTROL_REFCLK_EN0	BIT(23)
>   #define   REG_PCI_CONTROL_REFCLK_EN1	BIT(22)
> +#define   REG_PCI_CONTROL_PERSTOUT2	BIT(16)
>   #define REG_GSW_CLK_DIV_SEL		0x1b4
>   #define REG_EMI_CLK_DIV_SEL		0x1b8
>   #define REG_BUS_CLK_DIV_SEL		0x1bc
> @@ -18,10 +21,25 @@
>   #define REG_SPI_CLK_FREQ_SEL		0x1c8
>   #define REG_NPU_CLK_DIV_SEL		0x1fc
>   #define REG_CRYPTO_CLKSRC		0x200
> -#define REG_RESET_CONTROL		0x834
> +#define REG_RESET_CONTROL2		0x830

Wait what? The RESET2 register comes before RESET1 ?!?!

Is this a typo? :-)

> +#define   REG_RESET2_CONTROL_PCIE2	BIT(27)
> +#define REG_RESET_CONTROL1		0x834
>   #define   REG_RESET_CONTROL_PCIEHB	BIT(29)
>   #define   REG_RESET_CONTROL_PCIE1	BIT(27)
>   #define   REG_RESET_CONTROL_PCIE2	BIT(26)
> +/* EN7581 */
> +#define REG_PCIE0_MEM			0x00
> +#define REG_PCIE0_MEM_MASK		0x04
> +#define REG_PCIE1_MEM			0x08
> +#define REG_PCIE1_MEM_MASK		0x0c
> +#define REG_PCIE2_MEM			0x10
> +#define REG_PCIE2_MEM_MASK		0x14
> +#define REG_PCIE_RESET_OPEN_DRAIN	0x018c
> +#define REG_PCIE_RESET_OPEN_DRAIN_MASK	GENMASK(2, 0)
> +#define REG_NP_SCU_PCIC			0x88
> +#define REG_NP_SCU_SSTR			0x9c
> +#define REG_PCIE_XSI0_SEL_MASK		GENMASK(14, 13)
> +#define REG_PCIE_XSI1_SEL_MASK		GENMASK(12, 11)
>   
>   struct en_clk_desc {
>   	int id;
> @@ -207,14 +225,14 @@ static int en7523_pci_prepare(struct clk_hw *hw)
>   	usleep_range(1000, 2000);
>   
>   	/* Reset to default */
> -	val = readl(np_base + REG_RESET_CONTROL);
> +	val = readl(np_base + REG_RESET_CONTROL1);
>   	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
>   	       REG_RESET_CONTROL_PCIEHB;
> -	writel(val & ~mask, np_base + REG_RESET_CONTROL);
> +	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
>   	usleep_range(1000, 2000);
> -	writel(val | mask, np_base + REG_RESET_CONTROL);
> +	writel(val | mask, np_base + REG_RESET_CONTROL1);
>   	msleep(100);
> -	writel(val & ~mask, np_base + REG_RESET_CONTROL);
> +	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
>   	usleep_range(5000, 10000);
>   
>   	/* Release device */
> @@ -262,6 +280,64 @@ static struct clk_hw *en7523_register_pcie_clk(struct device *dev,
>   	return &cg->hw;
>   }
>   
> +static int en7581_pci_is_enabled(struct clk_hw *hw)
> +{
> +	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> +	u32 val, mask;
> +
> +	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
> +	val = readl(cg->base + REG_PCI_CONTROL);
> +	return (val & mask) == mask;
> +}
> +
> +static int en7581_pci_prepare(struct clk_hw *hw)
> +{
> +	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> +	void __iomem *np_base = cg->base;
> +	u32 val, mask;
> +
> +	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
> +	       REG_RESET_CONTROL_PCIEHB;
> +	val = readl(np_base + REG_RESET_CONTROL1);
> +	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
> +	val = readl(np_base + REG_RESET_CONTROL2);
> +	writel(val & ~REG_RESET2_CONTROL_PCIE2, np_base + REG_RESET_CONTROL2);
> +	usleep_range(5000, 10000);
> +
> +	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
> +	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
> +	       REG_PCI_CONTROL_PERSTOUT;

I'm not sure that this is actually something to control in a clock driver...

the right thing to do would be to add a reset controller to this clock driver
and then assert/deassert reset in the PCIe PHY/MAC driver.

Perhaps REFCLK_EN0/EN1 can be manipulated in a .enable() callback, treating
this really just as what it appears to really be: a gate clock! (hint: check
clk-gate.c)

> +	val = readl(np_base + REG_PCI_CONTROL);
> +	writel(val | mask, np_base + REG_PCI_CONTROL);
> +	msleep(250);
> +
> +	return 0;
> +}
> +
> +static void en7581_pci_unprepare(struct clk_hw *hw)
> +{
> +	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> +	void __iomem *np_base = cg->base;
> +	u32 val, mask;
> +
> +	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |

...and this should be a clk-gate .disable() callback, I guess :-)

> +	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
> +	       REG_PCI_CONTROL_PERSTOUT;
> +	val = readl(np_base + REG_PCI_CONTROL);
> +	writel(val & ~mask, np_base + REG_PCI_CONTROL);
> +	usleep_range(1000, 2000);
> +
> +	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
> +	       REG_RESET_CONTROL_PCIEHB;
> +	val = readl(np_base + REG_RESET_CONTROL1);
> +	writel(val | mask, np_base + REG_RESET_CONTROL1);
> +	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2;
> +	writel(val | mask, np_base + REG_RESET_CONTROL1);
> +	val = readl(np_base + REG_RESET_CONTROL2);
> +	writel(val | REG_RESET_CONTROL_PCIE2, np_base + REG_RESET_CONTROL2);
> +	msleep(100);
> +}
> +
>   static void en7523_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
>   				   void __iomem *base, void __iomem *np_base)
>   {
> @@ -291,6 +367,37 @@ static void en7523_register_clocks(struct device *dev, struct clk_hw_onecell_dat
>   	clk_data->num = EN7523_NUM_CLOCKS;
>   }
>   
> +static int en7581_clk_hw_init(struct platform_device *pdev,
> +			      void __iomem *base,
> +			      void __iomem *np_base)
> +{
> +	void __iomem *pb_base;
> +	u32 val;
> +
> +	pb_base = devm_platform_ioremap_resource(pdev, 2);
> +	if (IS_ERR(pb_base))
> +		return PTR_ERR(pb_base);
> +
> +	val = readl(np_base + REG_NP_SCU_SSTR);
> +	val &= ~(REG_PCIE_XSI0_SEL_MASK | REG_PCIE_XSI1_SEL_MASK);
> +	writel(val, np_base + REG_NP_SCU_SSTR);
> +	val = readl(np_base + REG_NP_SCU_PCIC);
> +	writel(val | 3, np_base + REG_NP_SCU_PCIC);

What is 3?

#define SOMETHING 3 ??

> +
> +	writel(0x20000000, pb_base + REG_PCIE0_MEM);
> +	writel(0xfc000000, pb_base + REG_PCIE0_MEM_MASK);
> +	writel(0x24000000, pb_base + REG_PCIE1_MEM);
> +	writel(0xfc000000, pb_base + REG_PCIE1_MEM_MASK);
> +	writel(0x28000000, pb_base + REG_PCIE2_MEM);
> +	writel(0xfc000000, pb_base + REG_PCIE2_MEM_MASK);

And... this is .. some BIT() and some GENMASK() as far as I understand...
do we have any clue about what you're setting to those registers?

Can MediaTek/Airoha help with this, please?

#define SOMETHING BIT(29) /* this is 0x20000000 */
#define SOME_MASK GENMASK(31, 26) /* this is 0xfc00000 */

> +
> +	val = readl(base + REG_PCIE_RESET_OPEN_DRAIN);
> +	writel(val | REG_PCIE_RESET_OPEN_DRAIN_MASK,
> +	       base + REG_PCIE_RESET_OPEN_DRAIN);
> +
> +	return 0;
> +}
> +
>   static int en7523_clk_probe(struct platform_device *pdev)
>   {
>   	struct device_node *node = pdev->dev.of_node;
> @@ -306,6 +413,12 @@ static int en7523_clk_probe(struct platform_device *pdev)
>   	if (IS_ERR(np_base))
>   		return PTR_ERR(np_base);
>   
> +	if (of_device_is_compatible(node, "airoha,en7581-scu")) {
> +		r = en7581_clk_hw_init(pdev, base, np_base);
> +		if (r)
> +			return r;
> +	}
> +
>   	clk_data = devm_kzalloc(&pdev->dev,
>   				struct_size(clk_data, hws, EN7523_NUM_CLOCKS),
>   				GFP_KERNEL);
> @@ -329,8 +442,15 @@ static const struct clk_ops en7523_pcie_ops = {
>   	.unprepare = en7523_pci_unprepare,
>   };
>   

static const struct clk_en7523_pdata en7581_pdata = {
	.init = en7581_clk_hw_init, /* if (pdata->init) pdata->init(x, y, z) */
	.ops = en7581_pcie_ops,
};

or, alternatively:

static const struct .... = {
	.init = ...,
	.ops = (const struct clk_ops) {
		.is_enabled = en7581_pci_is_enabled,
		.... etc
	}
};

Cheers,
Angelo

> +static const struct clk_ops en7581_pcie_ops = {
> +	.is_enabled = en7581_pci_is_enabled,
> +	.prepare = en7581_pci_prepare,
> +	.unprepare = en7581_pci_unprepare,
> +};
> +
>   static const struct of_device_id of_match_clk_en7523[] = {
>   	{ .compatible = "airoha,en7523-scu", .data = &en7523_pcie_ops },
> +	{ .compatible = "airoha,en7581-scu", .data = &en7581_pcie_ops },
>   	{ /* sentinel */ }
>   };
>   

-

Re: [PATCH 2/4] arm64: dts: airoha: Add EN7581 clock node

[AngeloGioacchino Del Regno]

Il 04/04/24 10:57, Lorenzo Bianconi ha scritto:
>> Il 03/04/24 18:20, Lorenzo Bianconi ha scritto:
>>> Introduce the Airoha EN7581 clock node in Airoha EN7581 dtsi
>>>
>>> Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>> ---
>>>    arch/arm64/boot/dts/airoha/en7581.dtsi | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/airoha/en7581.dtsi b/arch/arm64/boot/dts/airoha/en7581.dtsi
>>> index 55eb1762fb11..a1daaaef0de0 100644
>>> --- a/arch/arm64/boot/dts/airoha/en7581.dtsi
>>> +++ b/arch/arm64/boot/dts/airoha/en7581.dtsi
>>> @@ -2,6 +2,7 @@
>>>    #include <dt-bindings/interrupt-controller/irq.h>
>>>    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +#include <dt-bindings/clock/en7523-clk.h>
>>>    / {
>>>    	interrupt-parent = <&gic>;
>>> @@ -150,5 +151,13 @@ uart1: serial@1fbf0000 {
>>>    			interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
>>>    			clock-frequency = <1843200>;
>>>    		};
>>> +
>>> +		scu: system-controller@1fa20000 {
>>
>> Uhm, why is this not a clock-controller but a system-controller?
> 
> I used the same approach used for en7523.dtsi. I guess it is done
> that way because the registers come from scu (system control unit)
> regmap, but I guess we can use clock-controller instead.
> 

Yeah, comes from there but you're actually defining a node for a clock-controller,
not a system-controller... makes sense to define this as

	scuclk: clock-controller@1fa20000

...or something along that line (for the phandle) so that, if another scu related
node appears for whatever reason, we distinguish between scuxyz and scuclk.

Cheers

> Regards,
> Lorenzo
> 
>>
>> Cheers,
>> Angelo
>>
>>> +			compatible = "airoha,en7581-scu";
>>> +			reg = <0x0 0x1fa20000 0x0 0x400>,
>>> +			      <0x0 0x1fb00000 0x0 0x1000>,
>>> +			      <0x0 0x1fbe3400 0x0 0xfc>;
>>> +			#clock-cells = <1>;
>>> +		};
>>>    	};
>>>    };
>>
>>
>>

Re: [PATCH 2/4] arm64: dts: airoha: Add EN7581 clock node

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2307 bytes --]

> Il 04/04/24 10:57, Lorenzo Bianconi ha scritto:
> > > Il 03/04/24 18:20, Lorenzo Bianconi ha scritto:
> > > > Introduce the Airoha EN7581 clock node in Airoha EN7581 dtsi
> > > > 
> > > > Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >    arch/arm64/boot/dts/airoha/en7581.dtsi | 9 +++++++++
> > > >    1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/airoha/en7581.dtsi b/arch/arm64/boot/dts/airoha/en7581.dtsi
> > > > index 55eb1762fb11..a1daaaef0de0 100644
> > > > --- a/arch/arm64/boot/dts/airoha/en7581.dtsi
> > > > +++ b/arch/arm64/boot/dts/airoha/en7581.dtsi
> > > > @@ -2,6 +2,7 @@
> > > >    #include <dt-bindings/interrupt-controller/irq.h>
> > > >    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > +#include <dt-bindings/clock/en7523-clk.h>
> > > >    / {
> > > >    	interrupt-parent = <&gic>;
> > > > @@ -150,5 +151,13 @@ uart1: serial@1fbf0000 {
> > > >    			interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> > > >    			clock-frequency = <1843200>;
> > > >    		};
> > > > +
> > > > +		scu: system-controller@1fa20000 {
> > > 
> > > Uhm, why is this not a clock-controller but a system-controller?
> > 
> > I used the same approach used for en7523.dtsi. I guess it is done
> > that way because the registers come from scu (system control unit)
> > regmap, but I guess we can use clock-controller instead.
> > 
> 
> Yeah, comes from there but you're actually defining a node for a clock-controller,
> not a system-controller... makes sense to define this as
> 
> 	scuclk: clock-controller@1fa20000
> 
> ...or something along that line (for the phandle) so that, if another scu related
> node appears for whatever reason, we distinguish between scuxyz and scuclk.

ack, I agree. I will fix it.

Regards,
Lorenzo

> 
> Cheers
> 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > Cheers,
> > > Angelo
> > > 
> > > > +			compatible = "airoha,en7581-scu";
> > > > +			reg = <0x0 0x1fa20000 0x0 0x400>,
> > > > +			      <0x0 0x1fb00000 0x0 0x1000>,
> > > > +			      <0x0 0x1fbe3400 0x0 0xfc>;
> > > > +			#clock-cells = <1>;
> > > > +		};
> > > >    	};
> > > >    };
> > > 
> > > 
> > > 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 4/4] clk: en7523: add EN7581 support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 9826 bytes --]

> Il 03/04/24 18:20, Lorenzo Bianconi ha scritto:
> > Introduce EN7581 clock support to clk-en7523 driver.
> > 
> > Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >   drivers/clk/clk-en7523.c | 130 +++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 125 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> > index c7def87b74c6..51a6c0cc7f58 100644
> > --- a/drivers/clk/clk-en7523.c
> > +++ b/drivers/clk/clk-en7523.c
> > @@ -4,13 +4,16 @@
> >   #include <linux/clk-provider.h>
> >   #include <linux/io.h>
> >   #include <linux/of.h>
> > +#include <linux/of_device.h>
> >   #include <linux/platform_device.h>
> >   #include <dt-bindings/clock/en7523-clk.h>
> >   #define REG_PCI_CONTROL			0x88
> >   #define   REG_PCI_CONTROL_PERSTOUT	BIT(29)
> >   #define   REG_PCI_CONTROL_PERSTOUT1	BIT(26)
> > +#define   REG_PCI_CONTROL_REFCLK_EN0	BIT(23)
> >   #define   REG_PCI_CONTROL_REFCLK_EN1	BIT(22)
> > +#define   REG_PCI_CONTROL_PERSTOUT2	BIT(16)
> >   #define REG_GSW_CLK_DIV_SEL		0x1b4
> >   #define REG_EMI_CLK_DIV_SEL		0x1b8
> >   #define REG_BUS_CLK_DIV_SEL		0x1bc
> > @@ -18,10 +21,25 @@
> >   #define REG_SPI_CLK_FREQ_SEL		0x1c8
> >   #define REG_NPU_CLK_DIV_SEL		0x1fc
> >   #define REG_CRYPTO_CLKSRC		0x200
> > -#define REG_RESET_CONTROL		0x834
> > +#define REG_RESET_CONTROL2		0x830
> 
> Wait what? The RESET2 register comes before RESET1 ?!?!
> 
> Is this a typo? :-)

actually not :)

> 
> > +#define   REG_RESET2_CONTROL_PCIE2	BIT(27)
> > +#define REG_RESET_CONTROL1		0x834
> >   #define   REG_RESET_CONTROL_PCIEHB	BIT(29)
> >   #define   REG_RESET_CONTROL_PCIE1	BIT(27)
> >   #define   REG_RESET_CONTROL_PCIE2	BIT(26)
> > +/* EN7581 */
> > +#define REG_PCIE0_MEM			0x00
> > +#define REG_PCIE0_MEM_MASK		0x04
> > +#define REG_PCIE1_MEM			0x08
> > +#define REG_PCIE1_MEM_MASK		0x0c
> > +#define REG_PCIE2_MEM			0x10
> > +#define REG_PCIE2_MEM_MASK		0x14
> > +#define REG_PCIE_RESET_OPEN_DRAIN	0x018c
> > +#define REG_PCIE_RESET_OPEN_DRAIN_MASK	GENMASK(2, 0)
> > +#define REG_NP_SCU_PCIC			0x88
> > +#define REG_NP_SCU_SSTR			0x9c
> > +#define REG_PCIE_XSI0_SEL_MASK		GENMASK(14, 13)
> > +#define REG_PCIE_XSI1_SEL_MASK		GENMASK(12, 11)
> >   struct en_clk_desc {
> >   	int id;
> > @@ -207,14 +225,14 @@ static int en7523_pci_prepare(struct clk_hw *hw)
> >   	usleep_range(1000, 2000);
> >   	/* Reset to default */
> > -	val = readl(np_base + REG_RESET_CONTROL);
> > +	val = readl(np_base + REG_RESET_CONTROL1);
> >   	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
> >   	       REG_RESET_CONTROL_PCIEHB;
> > -	writel(val & ~mask, np_base + REG_RESET_CONTROL);
> > +	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
> >   	usleep_range(1000, 2000);
> > -	writel(val | mask, np_base + REG_RESET_CONTROL);
> > +	writel(val | mask, np_base + REG_RESET_CONTROL1);
> >   	msleep(100);
> > -	writel(val & ~mask, np_base + REG_RESET_CONTROL);
> > +	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
> >   	usleep_range(5000, 10000);
> >   	/* Release device */
> > @@ -262,6 +280,64 @@ static struct clk_hw *en7523_register_pcie_clk(struct device *dev,
> >   	return &cg->hw;
> >   }
> > +static int en7581_pci_is_enabled(struct clk_hw *hw)
> > +{
> > +	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> > +	u32 val, mask;
> > +
> > +	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
> > +	val = readl(cg->base + REG_PCI_CONTROL);
> > +	return (val & mask) == mask;
> > +}
> > +
> > +static int en7581_pci_prepare(struct clk_hw *hw)
> > +{
> > +	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> > +	void __iomem *np_base = cg->base;
> > +	u32 val, mask;
> > +
> > +	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
> > +	       REG_RESET_CONTROL_PCIEHB;
> > +	val = readl(np_base + REG_RESET_CONTROL1);
> > +	writel(val & ~mask, np_base + REG_RESET_CONTROL1);
> > +	val = readl(np_base + REG_RESET_CONTROL2);
> > +	writel(val & ~REG_RESET2_CONTROL_PCIE2, np_base + REG_RESET_CONTROL2);
> > +	usleep_range(5000, 10000);
> > +
> > +	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
> > +	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
> > +	       REG_PCI_CONTROL_PERSTOUT;
> 
> I'm not sure that this is actually something to control in a clock driver...
> 
> the right thing to do would be to add a reset controller to this clock driver
> and then assert/deassert reset in the PCIe PHY/MAC driver.
> 
> Perhaps REFCLK_EN0/EN1 can be manipulated in a .enable() callback, treating
> this really just as what it appears to really be: a gate clock! (hint: check
> clk-gate.c)

ack, I will look into it.

> 
> > +	val = readl(np_base + REG_PCI_CONTROL);
> > +	writel(val | mask, np_base + REG_PCI_CONTROL);
> > +	msleep(250);
> > +
> > +	return 0;
> > +}
> > +
> > +static void en7581_pci_unprepare(struct clk_hw *hw)
> > +{
> > +	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> > +	void __iomem *np_base = cg->base;
> > +	u32 val, mask;
> > +
> > +	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
> 
> ...and this should be a clk-gate .disable() callback, I guess :-)

ack, I will look into it.

> 
> > +	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
> > +	       REG_PCI_CONTROL_PERSTOUT;
> > +	val = readl(np_base + REG_PCI_CONTROL);
> > +	writel(val & ~mask, np_base + REG_PCI_CONTROL);
> > +	usleep_range(1000, 2000);
> > +
> > +	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
> > +	       REG_RESET_CONTROL_PCIEHB;
> > +	val = readl(np_base + REG_RESET_CONTROL1);
> > +	writel(val | mask, np_base + REG_RESET_CONTROL1);
> > +	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2;
> > +	writel(val | mask, np_base + REG_RESET_CONTROL1);
> > +	val = readl(np_base + REG_RESET_CONTROL2);
> > +	writel(val | REG_RESET_CONTROL_PCIE2, np_base + REG_RESET_CONTROL2);
> > +	msleep(100);
> > +}
> > +
> >   static void en7523_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
> >   				   void __iomem *base, void __iomem *np_base)
> >   {
> > @@ -291,6 +367,37 @@ static void en7523_register_clocks(struct device *dev, struct clk_hw_onecell_dat
> >   	clk_data->num = EN7523_NUM_CLOCKS;
> >   }
> > +static int en7581_clk_hw_init(struct platform_device *pdev,
> > +			      void __iomem *base,
> > +			      void __iomem *np_base)
> > +{
> > +	void __iomem *pb_base;
> > +	u32 val;
> > +
> > +	pb_base = devm_platform_ioremap_resource(pdev, 2);
> > +	if (IS_ERR(pb_base))
> > +		return PTR_ERR(pb_base);
> > +
> > +	val = readl(np_base + REG_NP_SCU_SSTR);
> > +	val &= ~(REG_PCIE_XSI0_SEL_MASK | REG_PCIE_XSI1_SEL_MASK);
> > +	writel(val, np_base + REG_NP_SCU_SSTR);
> > +	val = readl(np_base + REG_NP_SCU_PCIC);
> > +	writel(val | 3, np_base + REG_NP_SCU_PCIC);
> 
> What is 3?
> 
> #define SOMETHING 3 ??

actullay I do not know what it means since write in the pcie_ctrl subfield of
REG_NP_SCU_PCIC but it is a GENMASK(7, 0) and I do not have any more info
about it.

> 
> > +
> > +	writel(0x20000000, pb_base + REG_PCIE0_MEM);
> > +	writel(0xfc000000, pb_base + REG_PCIE0_MEM_MASK);
> > +	writel(0x24000000, pb_base + REG_PCIE1_MEM);
> > +	writel(0xfc000000, pb_base + REG_PCIE1_MEM_MASK);
> > +	writel(0x28000000, pb_base + REG_PCIE2_MEM);
> > +	writel(0xfc000000, pb_base + REG_PCIE2_MEM_MASK);
> 
> And... this is .. some BIT() and some GENMASK() as far as I understand...
> do we have any clue about what you're setting to those registers?

same as above, they seems undocumented.
@airoha folks: any input about them?

> 
> Can MediaTek/Airoha help with this, please?
> 
> #define SOMETHING BIT(29) /* this is 0x20000000 */
> #define SOME_MASK GENMASK(31, 26) /* this is 0xfc00000 */
> 
> > +
> > +	val = readl(base + REG_PCIE_RESET_OPEN_DRAIN);
> > +	writel(val | REG_PCIE_RESET_OPEN_DRAIN_MASK,
> > +	       base + REG_PCIE_RESET_OPEN_DRAIN);
> > +
> > +	return 0;
> > +}
> > +
> >   static int en7523_clk_probe(struct platform_device *pdev)
> >   {
> >   	struct device_node *node = pdev->dev.of_node;
> > @@ -306,6 +413,12 @@ static int en7523_clk_probe(struct platform_device *pdev)
> >   	if (IS_ERR(np_base))
> >   		return PTR_ERR(np_base);
> > +	if (of_device_is_compatible(node, "airoha,en7581-scu")) {
> > +		r = en7581_clk_hw_init(pdev, base, np_base);
> > +		if (r)
> > +			return r;
> > +	}
> > +
> >   	clk_data = devm_kzalloc(&pdev->dev,
> >   				struct_size(clk_data, hws, EN7523_NUM_CLOCKS),
> >   				GFP_KERNEL);
> > @@ -329,8 +442,15 @@ static const struct clk_ops en7523_pcie_ops = {
> >   	.unprepare = en7523_pci_unprepare,
> >   };
> 
> static const struct clk_en7523_pdata en7581_pdata = {
> 	.init = en7581_clk_hw_init, /* if (pdata->init) pdata->init(x, y, z) */
> 	.ops = en7581_pcie_ops,
> };
> 
> or, alternatively:
> 
> static const struct .... = {
> 	.init = ...,
> 	.ops = (const struct clk_ops) {
> 		.is_enabled = en7581_pci_is_enabled,
> 		.... etc
> 	}

ack, I will fix it.

Regards,
Lorenzo

> };
> 
> Cheers,
> Angelo
> 
> > +static const struct clk_ops en7581_pcie_ops = {
> > +	.is_enabled = en7581_pci_is_enabled,
> > +	.prepare = en7581_pci_prepare,
> > +	.unprepare = en7581_pci_unprepare,
> > +};
> > +
> >   static const struct of_device_id of_match_clk_en7523[] = {
> >   	{ .compatible = "airoha,en7523-scu", .data = &en7523_pcie_ops },
> > +	{ .compatible = "airoha,en7581-scu", .data = &en7581_pcie_ops },
> >   	{ /* sentinel */ }
> >   };
> 
> -

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]