[PATCH 0/2] PCI: mediatek-gen3: Set PBUS_CSR regs for Airoha EN7581 SoC.

[PATCH 0/2] PCI: mediatek-gen3: Set PBUS_CSR regs for Airoha EN7581 SoC.

[Lorenzo Bianconi]

Configure PBus base address and address mask in order to allow the hw
detecting if a given address is on PCIE0, PCIE1 or PCIE2.
Introduce binding for PBUS_CSR node available on EN7581 SoC.

---
Lorenzo Bianconi (2):
      dt-bindings: arm: airoha: Add the pbus-csr node for EN7581 SoC
      PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC

 .../bindings/arm/airoha,en7581-pbus-csr.yaml       | 41 ++++++++++++++++++++++
 drivers/pci/controller/pcie-mediatek-gen3.c        | 29 ++++++++++++++-
 2 files changed, 69 insertions(+), 1 deletion(-)
---
base-commit: d02e16e4e05d5d2530a4836ca92318c6a6b21b01
change-id: 20250115-en7581-pcie-pbus-csr-994ab50984d7

Best regards,
-- 
Lorenzo Bianconi <lorenzo@kernel.org>

[PATCH 1/2] dt-bindings: arm: airoha: Add the pbus-csr node for EN7581 SoC

[Lorenzo Bianconi]

This patch adds the pbus-csr document bindings for EN7581 SoC.
The airoha pbus-csr block provides a configuration interface for the
PBUS controller used to detect if a given address is on PCIE0, PCIE1 or
PCIE2.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../bindings/arm/airoha,en7581-pbus-csr.yaml       | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml b/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..80b237e195cd3607645efe3fda1eb6152134481c
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/airoha,en7581-pbus-csr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha Pbus CSR Controller for EN7581 SoC
+
+maintainers:
+  - Lorenzo Bianconi <lorenzo@kernel.org>
+
+description:
+  The airoha pbus-csr block provides a configuration interface for the PBUS
+  controller used to detect if a given address is on PCIE0, PCIE1 or PCIE2.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - airoha,en7581-pbus-csr
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      syscon@1fbe3400 {
+        compatible = "airoha,en7581-pbus-csr", "syscon";
+        reg = <0x0 0x1fbe3400 0x0 0xff>;
+      };
+    };

-- 
2.48.0

Re: [PATCH 1/2] dt-bindings: arm: airoha: Add the pbus-csr node for EN7581 SoC

[Krzysztof Kozlowski]

On Wed, Jan 15, 2025 at 06:32:30PM +0100, Lorenzo Bianconi wrote:
> This patch adds the pbus-csr document bindings for EN7581 SoC.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> The airoha pbus-csr block provides a configuration interface for the
> PBUS controller used to detect if a given address is on PCIE0, PCIE1 or
> PCIE2.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  .../bindings/arm/airoha,en7581-pbus-csr.yaml       | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml b/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..80b237e195cd3607645efe3fda1eb6152134481c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/airoha,en7581-pbus-csr.yaml#

arm is only top level bindings and ARM stuff. This is soc.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Airoha Pbus CSR Controller for EN7581 SoC
> +
> +maintainers:
> +  - Lorenzo Bianconi <lorenzo@kernel.org>
> +
> +description:
> +  The airoha pbus-csr block provides a configuration interface for the PBUS
> +  controller used to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - airoha,en7581-pbus-csr

Does not fit standard syscon bindings?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: arm: airoha: Add the pbus-csr node for EN7581 SoC

[Lorenzo Bianconi]

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

> On Wed, Jan 15, 2025 at 06:32:30PM +0100, Lorenzo Bianconi wrote:
> > This patch adds the pbus-csr document bindings for EN7581 SoC.
> 
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

ack, I will fix it in v2

> 
> > The airoha pbus-csr block provides a configuration interface for the
> > PBUS controller used to detect if a given address is on PCIE0, PCIE1 or
> > PCIE2.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  .../bindings/arm/airoha,en7581-pbus-csr.yaml       | 41 ++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml b/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..80b237e195cd3607645efe3fda1eb6152134481c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/airoha,en7581-pbus-csr.yaml
> > @@ -0,0 +1,41 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/airoha,en7581-pbus-csr.yaml#
> 
> arm is only top level bindings and ARM stuff. This is soc.

in this case we should create an airoha folder in
'Documentation/devicetree/bindings/soc', correct?

> 
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Airoha Pbus CSR Controller for EN7581 SoC
> > +
> > +maintainers:
> > +  - Lorenzo Bianconi <lorenzo@kernel.org>
> > +
> > +description:
> > +  The airoha pbus-csr block provides a configuration interface for the PBUS
> > +  controller used to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - airoha,en7581-pbus-csr
> 
> Does not fit standard syscon bindings?

I think standard syscon is fine. In this case we could drop this patch. Agree?

Regards,
Lorenzo

> 
> Best regards,
> Krzysztof
> 

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

Re: [PATCH 1/2] dt-bindings: arm: airoha: Add the pbus-csr node for EN7581 SoC

[Krzysztof Kozlowski]

On 01/02/2025 13:16, Lorenzo Bianconi wrote:
>>> +maintainers:
>>> +  - Lorenzo Bianconi <lorenzo@kernel.org>
>>> +
>>> +description:
>>> +  The airoha pbus-csr block provides a configuration interface for the PBUS
>>> +  controller used to detect if a given address is on PCIE0, PCIE1 or PCIE2.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - airoha,en7581-pbus-csr
>>
>> Does not fit standard syscon bindings?
> 
> I think standard syscon is fine. In this case we could drop this patch. Agree?

Yes.


Best regards,
Krzysztof

[PATCH 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC

[Lorenzo Bianconi]

Configure PBus base address and address mask in order to allow the hw
detecting if a given address is on PCIE0, PCIE1 or PCIE2.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..b172a46cf95a9c728291c5b7a88457d3b725681a 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -15,6 +15,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/of_device.h>
@@ -24,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 #include <linux/reset.h>
 
 #include "../pci.h"
@@ -127,6 +129,13 @@
 
 #define PCIE_MTK_RESET_TIME_US		10
 
+#define PCIE_EN7581_PBUS_ADDR(_n)	(0x00 + ((_n) << 3))
+#define PCIE_EN7581_PBUS_ADDR_MASK(_n)	(0x04 + ((_n) << 3))
+#define PCIE_EN7581_PBUS_BASE_ADDR(_n)	\
+	((_n) == 2 ? 0x28000000 :	\
+	 (_n) == 1 ? 0x24000000 : 0x20000000)
+#define PCIE_EN7581_PBUS_BASE_ADDR_MASK	GENMASK(31, 26)
+
 /* Time in ms needed to complete PCIe reset on EN7581 SoC */
 #define PCIE_EN7581_RESET_TIME_MS	100
 
@@ -931,7 +940,8 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
-	int err;
+	struct regmap *map;
+	int err, slot;
 	u32 val;
 
 	/*
@@ -945,6 +955,23 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
 	/* Wait for the time needed to complete the reset lines assert. */
 	msleep(PCIE_EN7581_RESET_TIME_MS);
 
+	map = syscon_regmap_lookup_by_compatible("airoha,en7581-pbus-csr");
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	/*
+	 * Configure PBus base address and address mask in order to allow the
+	 * hw detecting if a given address is on PCIE0, PCIE1 or PCIE2.
+	 */
+	slot = of_get_pci_domain_nr(dev->of_node);
+	if (slot < 0)
+		return slot;
+
+	regmap_write(map, PCIE_EN7581_PBUS_ADDR(slot),
+		     PCIE_EN7581_PBUS_BASE_ADDR(slot));
+	regmap_write(map, PCIE_EN7581_PBUS_ADDR_MASK(slot),
+		     PCIE_EN7581_PBUS_BASE_ADDR_MASK);
+
 	/*
 	 * Unlike the other MediaTek Gen3 controllers, the Airoha EN7581
 	 * requires PHY initialization and power-on before PHY reset deassert.

-- 
2.48.0

Re: [PATCH 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC

[Krzysztof Kozlowski]

On Wed, Jan 15, 2025 at 06:32:31PM +0100, Lorenzo Bianconi wrote:
> Configure PBus base address and address mask in order to allow the hw
> detecting if a given address is on PCIE0, PCIE1 or PCIE2.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..b172a46cf95a9c728291c5b7a88457d3b725681a 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -15,6 +15,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/msi.h>
>  #include <linux/of_device.h>
> @@ -24,6 +25,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>  #include <linux/reset.h>
>  
>  #include "../pci.h"
> @@ -127,6 +129,13 @@
>  
>  #define PCIE_MTK_RESET_TIME_US		10
>  
> +#define PCIE_EN7581_PBUS_ADDR(_n)	(0x00 + ((_n) << 3))
> +#define PCIE_EN7581_PBUS_ADDR_MASK(_n)	(0x04 + ((_n) << 3))
> +#define PCIE_EN7581_PBUS_BASE_ADDR(_n)	\
> +	((_n) == 2 ? 0x28000000 :	\
> +	 (_n) == 1 ? 0x24000000 : 0x20000000)
> +#define PCIE_EN7581_PBUS_BASE_ADDR_MASK	GENMASK(31, 26)
> +
>  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
>  #define PCIE_EN7581_RESET_TIME_MS	100
>  
> @@ -931,7 +940,8 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
>  static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> -	int err;
> +	struct regmap *map;
> +	int err, slot;
>  	u32 val;
>  
>  	/*
> @@ -945,6 +955,23 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  	/* Wait for the time needed to complete the reset lines assert. */
>  	msleep(PCIE_EN7581_RESET_TIME_MS);
>  
> +	map = syscon_regmap_lookup_by_compatible("airoha,en7581-pbus-csr");

No, don't sprinkle compatibles in other drivers. It does not scale, does
not allow reuse and you kind of try to escape ABI break, but you won't.
This is still clear ABI break without any statement in commit msg and
without explanation.

NAK.

Relationship between devices is expressed with phandles. There are
plenty of examples how to do that with syscon.

Best regards,
Krzysztof

Re: [PATCH 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC

[Lorenzo Bianconi]

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

> On Wed, Jan 15, 2025 at 06:32:31PM +0100, Lorenzo Bianconi wrote:

[./.]

> 
> No, don't sprinkle compatibles in other drivers. It does not scale, does
> not allow reuse and you kind of try to escape ABI break, but you won't.
> This is still clear ABI break without any statement in commit msg and
> without explanation.
> 
> NAK.
> 
> Relationship between devices is expressed with phandles. There are
> plenty of examples how to do that with syscon.

ack, I will fix it in v2.

Regards,
Lorenzo

> 
> Best regards,
> Krzysztof
> 

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