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

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

[Lorenzo Bianconi]

Configure PBus base address and address mask to allow the hw to detect
if a given address is on PCIE0, PCIE1 or PCIE2.
Introduce mediatek,pbus-csr phandle property.

Changes in v2:
- Introduce mediatek,pbus-csr phandle property
- Drop patch 1/2 in v1
- Do not hard-code compatible sting in the driver and use phandle
  instead

---
Lorenzo Bianconi (2):
      dt-bindings: PCI: mediatek-gen3: Add mediatek,pbus-csr phandle property
      PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC

 .../bindings/pci/mediatek-pcie-gen3.yaml           | 12 +++++++++
 drivers/pci/controller/pcie-mediatek-gen3.c        | 30 +++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)
---
base-commit: 647d69605c70368d54fc012fce8a43e8e5955b04
change-id: 20250201-en7581-pcie-pbus-csr-f9c4f88ce5b3

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

[PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: Add mediatek,pbus-csr phandle property

[Lorenzo Bianconi]

Introduce the mediatek,pbus-csr property for the pbus-csr syscon node
available on 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>
---
 .../devicetree/bindings/pci/mediatek-pcie-gen3.yaml          | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
index f05aab2b1addcac91d4685d7d94f421814822b92..02f2cd9bcf007c1f16b18b1330a88ce43807a9be 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -109,6 +109,12 @@ properties:
   power-domains:
     maxItems: 1
 
+  mediatek,pbus-csr:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon node used to detect if a given address is on
+      the PCIe ports.
+
   '#interrupt-cells':
     const: 1
 
@@ -168,6 +174,8 @@ allOf:
           minItems: 1
           maxItems: 2
 
+        mediatek,pbus-csr: false
+
   - if:
       properties:
         compatible:
@@ -197,6 +205,8 @@ allOf:
           minItems: 1
           maxItems: 2
 
+        mediatek,pbus-csr: false
+
   - if:
       properties:
         compatible:
@@ -224,6 +234,8 @@ allOf:
           minItems: 1
           maxItems: 2
 
+        mediatek,pbus-csr: false
+
   - if:
       properties:
         compatible:

-- 
2.48.1

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

[Lorenzo Bianconi]

Configure PBus base address and address mask to allow the hw
to detect 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 | 30 ++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..9c2a592cae959de8fbe9ca5c5c2253f8eadf2c76 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,24 @@ 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_phandle(dev->of_node,
+					      "mediatek,pbus-csr");
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	/*
+	 * Configure PBus base address and address mask to allow the
+	 * hw to detect 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.1

Re: [PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: Add mediatek,pbus-csr phandle property

[Krzysztof Kozlowski]

On Sun, Feb 02, 2025 at 08:34:23PM +0100, Lorenzo Bianconi wrote:
> Introduce the mediatek,pbus-csr property for the pbus-csr syscon node
> available on 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>
> ---
>  .../devicetree/bindings/pci/mediatek-pcie-gen3.yaml          | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Manivannan Sadhasivam]

On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> Configure PBus base address and address mask to allow the hw
> to detect 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 | 30 ++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..9c2a592cae959de8fbe9ca5c5c2253f8eadf2c76 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,24 @@ 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_phandle(dev->of_node,
> +					      "mediatek,pbus-csr");
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);

So this is going to regress the devicetree's that do not define this syscon
region? But I do not see any devicetree using this 'airoha,en7581-pcie'
compatible, so not sure if this is going to be an issue. Are the downstream
devicetrees used?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Lorenzo Bianconi]

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

> On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > Configure PBus base address and address mask to allow the hw
> > to detect 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 | 30 ++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..9c2a592cae959de8fbe9ca5c5c2253f8eadf2c76 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,24 @@ 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_phandle(dev->of_node,
> > +					      "mediatek,pbus-csr");
> > +	if (IS_ERR(map))
> > +		return PTR_ERR(map);
> 
> So this is going to regress the devicetree's that do not define this syscon
> region? But I do not see any devicetree using this 'airoha,en7581-pcie'
> compatible, so not sure if this is going to be an issue. Are the downstream
> devicetrees used?

AFAIK there is no upstream or downstream (e.g. OpenWrt) en7581 dts with PCIe
support yet so I do not know if this is an issue or not. If so, I guess we
need to add the proper Fixes tag:

Fixes: f6ab898356dd ("PCI: mediatek-gen3: Add Airoha EN7581 support")

Regards,
Lorenzo

> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

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

[Manivannan Sadhasivam]

On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> Configure PBus base address and address mask to allow the hw
> to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 30 ++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..9c2a592cae959de8fbe9ca5c5c2253f8eadf2c76 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,24 @@ 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_phandle(dev->of_node,
> +					      "mediatek,pbus-csr");
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	/*
> +	 * Configure PBus base address and address mask to allow the
> +	 * hw to detect 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.1
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Bjorn Helgaas]

On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> Configure PBus base address and address mask to allow the hw
> to detect 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 | 30 ++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..9c2a592cae959de8fbe9ca5c5c2253f8eadf2c76 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)

Are these addresses something that should be expressed in devicetree?
It seems unusual to encode addresses directly in a driver.

Bjorn

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

[Lorenzo Bianconi]

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

On Feb 20, Bjorn Helgaas wrote:
> On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > Configure PBus base address and address mask to allow the hw
> > to detect 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 | 30 ++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..9c2a592cae959de8fbe9ca5c5c2253f8eadf2c76 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)
> 
> Are these addresses something that should be expressed in devicetree?

Do you have any example/pointer for it?

> It seems unusual to encode addresses directly in a driver.

AFAIK they are fixed for EN7581 SoC.

Regards,
Lorenzo

> 
> Bjorn

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

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

[Frank Li]

On Wed, Feb 19, 2025 at 11:56:50PM +0530, Manivannan Sadhasivam wrote:
> On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > Configure PBus base address and address mask to allow the hw
> > to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> - Mani
>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 30 ++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..9c2a592cae959de8fbe9ca5c5c2253f8eadf2c76 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)

look like these data should be in dts ?

> > +#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,24 @@ 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_phandle(dev->of_node,
> > +					      "mediatek,pbus-csr");
> > +	if (IS_ERR(map))
> > +		return PTR_ERR(map);
> > +
> > +	/*
> > +	 * Configure PBus base address and address mask to allow the
> > +	 * hw to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> > +	 */
> > +	slot = of_get_pci_domain_nr(dev->of_node);

I am not sure if too much abuse domain_id here.

> > +	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);

look like
	syscon
	{
		csr1 : csr1 =
		{
			reg = <0x20000000, >; //or other property
		}

		csr2: csr2 =
		{
			....
		}
	}

	pcie1 {
		mediatek,pbus-csr = <&csr1>;
	}

	pcie2 {
                mediatek,pbus-csr = <&csr2>;
        }

	...

Or
	pcie1 {
                mediatek,pbus-csr = <&csr1 0x20000000>;
        }
	...

	you can use syscon_regmap_lookup_by_phandle_args() to get address.
Frank


> > +
> >  	/*
> >  	 * Unlike the other MediaTek Gen3 controllers, the Airoha EN7581
> >  	 * requires PHY initialization and power-on before PHY reset deassert.
> >
> > --
> > 2.48.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்

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

[Lorenzo Bianconi]

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

> On Wed, Feb 19, 2025 at 11:56:50PM +0530, Manivannan Sadhasivam wrote:
> > On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > > Configure PBus base address and address mask to allow the hw
> > > to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> >
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >
> > - Mani
> >
> > > ---
> > >  drivers/pci/controller/pcie-mediatek-gen3.c | 30 ++++++++++++++++++++++++++++-
> > >  1 file changed, 29 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > index aa24ac9aaecc749b53cfc4faf6399913d20cdbf2..9c2a592cae959de8fbe9ca5c5c2253f8eadf2c76 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)
> 
> look like these data should be in dts ?
> 
> > > +#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,24 @@ 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_phandle(dev->of_node,
> > > +					      "mediatek,pbus-csr");
> > > +	if (IS_ERR(map))
> > > +		return PTR_ERR(map);
> > > +
> > > +	/*
> > > +	 * Configure PBus base address and address mask to allow the
> > > +	 * hw to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> > > +	 */
> > > +	slot = of_get_pci_domain_nr(dev->of_node);
> 
> I am not sure if too much abuse domain_id here.
> 
> > > +	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);
> 
> look like
> 	syscon
> 	{
> 		csr1 : csr1 =
> 		{
> 			reg = <0x20000000, >; //or other property
> 		}
> 
> 		csr2: csr2 =
> 		{
> 			....
> 		}
> 	}
> 
> 	pcie1 {
> 		mediatek,pbus-csr = <&csr1>;
> 	}
> 
> 	pcie2 {
>                 mediatek,pbus-csr = <&csr2>;
>         }
> 
> 	...
> 
> Or
> 	pcie1 {
>                 mediatek,pbus-csr = <&csr1 0x20000000>;
>         }
> 	...
> 
> 	you can use syscon_regmap_lookup_by_phandle_args() to get address.
> Frank

ack, thx for the pointer. I will fix in v3.

Regards,
Lorenzo

> 
> 
> > > +
> > >  	/*
> > >  	 * Unlike the other MediaTek Gen3 controllers, the Airoha EN7581
> > >  	 * requires PHY initialization and power-on before PHY reset deassert.
> > >
> > > --
> > > 2.48.1
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்

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

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

[Bjorn Helgaas]

[+cc Frank, who asked the same question about DT]

On Thu, Feb 20, 2025 at 08:54:06PM +0100, Lorenzo Bianconi wrote:
> On Feb 20, Bjorn Helgaas wrote:
> > On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > > Configure PBus base address and address mask to allow the hw
> > > to detect if a given address is on PCIE0, PCIE1 or PCIE2.

> > > +#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)
> > 
> > Are these addresses something that should be expressed in devicetree?
> 
> Do you have any example/pointer for it?
> 
> > It seems unusual to encode addresses directly in a driver.
> 
> AFAIK they are fixed for EN7581 SoC.

So this is used to detect if a given address is on PCIE0, PCIE1 or
PCIE2.  What does that mean?  There are no other mentions of PCIE0 etc
in the driver, but maybe they match up to "pcie0/1/2" in
arch/arm64/boot/dts/mediatek/mt7988a.dtsi?

It looks like you use PCIE_EN7581_PBUS_ADDR(slot), where "slot" came
from of_get_pci_domain_nr(), which suggests that these might be three
separate Root Ports?

Are we talking about an MMIO address that an endpoint driver uses for
readw() etc, and this code configures the hardware apertures through
the host bridge?  Seems like that would be related to the "ranges"
properties in DT.

Bjorn

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

[Lorenzo Bianconi]

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

> [+cc Frank, who asked the same question about DT]
> 
> On Thu, Feb 20, 2025 at 08:54:06PM +0100, Lorenzo Bianconi wrote:
> > On Feb 20, Bjorn Helgaas wrote:
> > > On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > > > Configure PBus base address and address mask to allow the hw
> > > > to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> 
> > > > +#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)
> > > 
> > > Are these addresses something that should be expressed in devicetree?
> > 
> > Do you have any example/pointer for it?
> > 
> > > It seems unusual to encode addresses directly in a driver.
> > 
> > AFAIK they are fixed for EN7581 SoC.
> 
> So this is used to detect if a given address is on PCIE0, PCIE1 or
> PCIE2.  What does that mean?  There are no other mentions of PCIE0 etc
> in the driver, but maybe they match up to "pcie0/1/2" in
> arch/arm64/boot/dts/mediatek/mt7988a.dtsi?
> 
> It looks like you use PCIE_EN7581_PBUS_ADDR(slot), where "slot" came
> from of_get_pci_domain_nr(), which suggests that these might be three
> separate Root Ports?

I was using pci_domain to detect the specific PCIe controller
(something similar to what is done here [0]) but I agree with Frank, it does
not seem completely correct.

[0] https://github.com/torvalds/linux/blob/master/drivers/pci/controller/pcie-mediatek.c#L1048

> 
> Are we talking about an MMIO address that an endpoint driver uses for
> readw() etc, and this code configures the hardware apertures through
> the host bridge?  Seems like that would be related to the "ranges"
> properties in DT.

I guess so, but I do not have any documentation about pbus-csr (adding Hui in
the loop).

As pointed out by Frank, do you agree to add these info in the dts? Something
like:

pcie0: pcie@1fc00000 {
	....
	mediatek,pbus-csr = <&pbus_csr 0x0 0x20000000 0x4 0xfc000000>;
	....
};

pcie1: pcie@1fc20000 {
	....
	mediatek,pbus-csr = <&pbus_csr 0x8 0x24000000 0xc 0xfc000000>;
	....
};

@Hui: can you please provide a better explanation about pbus-csr usage?

Regards,
Lorenzo

> 
> Bjorn

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

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

[Hui Ma (马慧)]

> [+cc Frank, who asked the same question about DT]
> 
> On Thu, Feb 20, 2025 at 08:54:06PM +0100, Lorenzo Bianconi wrote:
> > On Feb 20, Bjorn Helgaas wrote:
> > > On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > > > Configure PBus base address and address mask to allow the hw to 
> > > > detect if a given address is on PCIE0, PCIE1 or PCIE2.
> 
> > > > +#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)
> > > 
> > > Are these addresses something that should be expressed in devicetree?
> > 
> > Do you have any example/pointer for it?
> > 
> > > It seems unusual to encode addresses directly in a driver.
> > 
> > AFAIK they are fixed for EN7581 SoC.
> 
> So this is used to detect if a given address is on PCIE0, PCIE1 or 
> PCIE2.  What does that mean?  There are no other mentions of PCIE0 etc 
> in the driver, but maybe they match up to "pcie0/1/2" in 
> arch/arm64/boot/dts/mediatek/mt7988a.dtsi?
> 
> It looks like you use PCIE_EN7581_PBUS_ADDR(slot), where "slot" came 
> from of_get_pci_domain_nr(), which suggests that these might be three 
> separate Root Ports?

>I was using pci_domain to detect the specific PCIe controller (something similar to what is done here [0]) but I agree with Frank, it does not seem completely correct.

> [0] https://github.com/torvalds/linux/blob/master/drivers/pci/controller/pcie-mediatek.c#L1048

> 
> Are we talking about an MMIO address that an endpoint driver uses for
> readw() etc, and this code configures the hardware apertures through 
> the host bridge?  Seems like that would be related to the "ranges"
> properties in DT.

>I guess so, but I do not have any documentation about pbus-csr (adding Hui in the loop).

>As pointed out by Frank, do you agree to add these info in the dts? Something
>like:

>pcie0: pcie@1fc00000 {
>	....
>	mediatek,pbus-csr = <&pbus_csr 0x0 0x20000000 0x4 0xfc000000>;
>	....
>};
>
>pcie1: pcie@1fc20000 {
>	....
>	mediatek,pbus-csr = <&pbus_csr 0x8 0x24000000 0xc 0xfc000000>;
>	....
>};

>@Hui: can you please provide a better explanation about pbus-csr usage?

Hi Lorenzo，
	Pbus-csr (base and mask) is used to determine the address range can be access by PCIe bus.

1FBE3400 PCIE0_MEM_BASE 32 PCIE0 base address
1FBE3404 PCIE0_MEM_MASK 32 PCIE0 base address mask
1FBE3408 PCIE1_MEM_BASE 32 PCIE1 base address
1FBE340C PCIE1_MEM_MASK 32 PCIE1 base address mask
1FBE3410 PCIE2_MEM_BASE 32 PCIE2 base address
1FBE3414 PCIE2_MEM_MASK 32 PCIE2 base address mask

>Regards,
>Lorenzo

> 
> Bjorn

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

[Bjorn Helgaas]

On Fri, Feb 21, 2025 at 09:30:40AM +0000, Hui Ma (马慧) wrote:
> > On Thu, Feb 20, 2025 at 08:54:06PM +0100, Lorenzo Bianconi wrote:
> > > On Feb 20, Bjorn Helgaas wrote:
> > > > On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > > > > Configure PBus base address and address mask to allow the hw to 
> > > > > detect if a given address is on PCIE0, PCIE1 or PCIE2.
> > 
> > > > > +#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)
> > > > 
> > > > Are these addresses something that should be expressed in devicetree?
> > > 
> > > Do you have any example/pointer for it?
> > > 
> > > > It seems unusual to encode addresses directly in a driver.
> > > 
> > > AFAIK they are fixed for EN7581 SoC.
> > 
> > So this is used to detect if a given address is on PCIE0, PCIE1 or
> > PCIE2.  What does that mean?  There are no other mentions of PCIE0
> > etc in the driver, but maybe they match up to "pcie0/1/2" in
> > arch/arm64/boot/dts/mediatek/mt7988a.dtsi?
> > 
> > It looks like you use PCIE_EN7581_PBUS_ADDR(slot), where "slot"
> > came from of_get_pci_domain_nr(), which suggests that these might
> > be three separate Root Ports?
> 
> >I was using pci_domain to detect the specific PCIe controller
> >(something similar to what is done here [0]) but I agree with
> >Frank, it does not seem completely correct.
> 
> > [0] https://github.com/torvalds/linux/blob/master/drivers/pci/controller/pcie-mediatek.c#L1048
> 
> > Are we talking about an MMIO address that an endpoint driver uses
> > for readw() etc, and this code configures the hardware apertures
> > through the host bridge?  Seems like that would be related to the
> > "ranges" properties in DT.
> 
> >I guess so, but I do not have any documentation about pbus-csr
> >(adding Hui in the loop).
> 
> >As pointed out by Frank, do you agree to add these info in the dts?
> >Something like:
> 
> >pcie0: pcie@1fc00000 {
> >	....
> >	mediatek,pbus-csr = <&pbus_csr 0x0 0x20000000 0x4 0xfc000000>;
> >	....
> >};
> >
> >pcie1: pcie@1fc20000 {
> >	....
> >	mediatek,pbus-csr = <&pbus_csr 0x8 0x24000000 0xc 0xfc000000>;
> >	....
> >};
> 
> >@Hui: can you please provide a better explanation about pbus-csr usage?
>
> 	Pbus-csr (base and mask) is used to determine the address
> 	range can be access by PCIe bus.
> 
> 1FBE3400 PCIE0_MEM_BASE 32 PCIE0 base address
> 1FBE3404 PCIE0_MEM_MASK 32 PCIE0 base address mask
> 1FBE3408 PCIE1_MEM_BASE 32 PCIE1 base address
> 1FBE340C PCIE1_MEM_MASK 32 PCIE1 base address mask
> 1FBE3410 PCIE2_MEM_BASE 32 PCIE2 base address
> 1FBE3414 PCIE2_MEM_MASK 32 PCIE2 base address mask

"Can be accessed by PCIe bus" sounds like DMA.  Is that what you mean?

I doubt it, because if you have multiple host bridges, I assume they
would all be able to handle DMA to all of system memory.

It would make more sense if this is some sort of description of host
bridge apertures, e.g., something like this to allow CPU MMIO accesses
to reach the first 2GB of PCI memory space below any of the pcie0,
pcie1, pcie2 host bridges:

  pcie0 0000:00: root bus resource [mem 0x84000000000-0x8407fffffff] (bus address [0x00000000-0x7fffffff])
  pcie1 0001:00: root bus resource [mem 0x84100000000-0x8417fffffff] (bus address [0x00000000-0x7fffffff])
  pcie2 0002:00: root bus resource [mem 0x84200000000-0x8427fffffff] (bus address [0x00000000-0x7fffffff])

But I think this would be described via 'ranges' properties.  And I
think it would make sense if the driver had to learn this address map
from devicetree and program it into the hardware, so maybe that's
what Pbus-csr is for?  Total speculation on my part.

Bjorn

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

[Lorenzo Bianconi]

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

[...]

> >
> > 	Pbus-csr (base and mask) is used to determine the address
> > 	range can be access by PCIe bus.
> > 
> > 1FBE3400 PCIE0_MEM_BASE 32 PCIE0 base address
> > 1FBE3404 PCIE0_MEM_MASK 32 PCIE0 base address mask
> > 1FBE3408 PCIE1_MEM_BASE 32 PCIE1 base address
> > 1FBE340C PCIE1_MEM_MASK 32 PCIE1 base address mask
> > 1FBE3410 PCIE2_MEM_BASE 32 PCIE2 base address
> > 1FBE3414 PCIE2_MEM_MASK 32 PCIE2 base address mask
> 
> "Can be accessed by PCIe bus" sounds like DMA.  Is that what you mean?
> 
> I doubt it, because if you have multiple host bridges, I assume they
> would all be able to handle DMA to all of system memory.
> 
> It would make more sense if this is some sort of description of host
> bridge apertures, e.g., something like this to allow CPU MMIO accesses
> to reach the first 2GB of PCI memory space below any of the pcie0,
> pcie1, pcie2 host bridges:
> 
>   pcie0 0000:00: root bus resource [mem 0x84000000000-0x8407fffffff] (bus address [0x00000000-0x7fffffff])
>   pcie1 0001:00: root bus resource [mem 0x84100000000-0x8417fffffff] (bus address [0x00000000-0x7fffffff])
>   pcie2 0002:00: root bus resource [mem 0x84200000000-0x8427fffffff] (bus address [0x00000000-0x7fffffff])
> 
> But I think this would be described via 'ranges' properties.  And I
> think it would make sense if the driver had to learn this address map
> from devicetree and program it into the hardware, so maybe that's
> what Pbus-csr is for?  Total speculation on my part.

I agree we should provide these info to the driver via the dts.

Do you agree to pass the register offsets, base address and base mask values
in the 'mediatek,pbus-csr' phandle array? Something like:

pcie0: pcie@1fc00000 {
	...
	mediatek,pbus-csr = <&pbus_csr 0x0 0x20000000 0x4 0xfc000000>;
	...
}

where:
- reg offset for base address:	0x0
- base address value:		0x20000000
- reg offset for base mask:	0x4
- base mask value:		0xfc000000

Or do you prefer to just pass register offsets in mediatek,pbus-csr phandle
array and get base address values reading ranges property? Something like:

pcie0: pcie@1fc00000 {
	...
	ranges = <0x02000000 0 0x20000000 0x0 0x20000000 0 0x4000000>;
	...
	mediatek,pbus-csr = <&pbus_csr 0x0 0x4>;
	...
}

Considering the latter, even if it is not a real problem for EN7581 since we
have just a single range, what if we have multiple ranges?

Regards,
Lorenzo

> 
> Bjorn

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

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

[Bjorn Helgaas]

On Sat, Feb 22, 2025 at 12:18:52AM +0100, Lorenzo Bianconi wrote:
> [...]
> 
> > >
> > > 	Pbus-csr (base and mask) is used to determine the address
> > > 	range can be access by PCIe bus.
> > > 
> > > 1FBE3400 PCIE0_MEM_BASE 32 PCIE0 base address
> > > 1FBE3404 PCIE0_MEM_MASK 32 PCIE0 base address mask
> > > 1FBE3408 PCIE1_MEM_BASE 32 PCIE1 base address
> > > 1FBE340C PCIE1_MEM_MASK 32 PCIE1 base address mask
> > > 1FBE3410 PCIE2_MEM_BASE 32 PCIE2 base address
> > > 1FBE3414 PCIE2_MEM_MASK 32 PCIE2 base address mask
> > 
> > "Can be accessed by PCIe bus" sounds like DMA.  Is that what you mean?
> > 
> > I doubt it, because if you have multiple host bridges, I assume they
> > would all be able to handle DMA to all of system memory.
> > 
> > It would make more sense if this is some sort of description of host
> > bridge apertures, e.g., something like this to allow CPU MMIO accesses
> > to reach the first 2GB of PCI memory space below any of the pcie0,
> > pcie1, pcie2 host bridges:
> > 
> >   pcie0 0000:00: root bus resource [mem 0x84000000000-0x8407fffffff] (bus address [0x00000000-0x7fffffff])
> >   pcie1 0001:00: root bus resource [mem 0x84100000000-0x8417fffffff] (bus address [0x00000000-0x7fffffff])
> >   pcie2 0002:00: root bus resource [mem 0x84200000000-0x8427fffffff] (bus address [0x00000000-0x7fffffff])
> > 
> > But I think this would be described via 'ranges' properties.  And I
> > think it would make sense if the driver had to learn this address map
> > from devicetree and program it into the hardware, so maybe that's
> > what Pbus-csr is for?  Total speculation on my part.
> 
> I agree we should provide these info to the driver via the dts.
> 
> Do you agree to pass the register offsets, base address and base mask values
> in the 'mediatek,pbus-csr' phandle array? Something like:
> 
> pcie0: pcie@1fc00000 {
> 	...
> 	mediatek,pbus-csr = <&pbus_csr 0x0 0x20000000 0x4 0xfc000000>;
> 	...
> }
> 
> where:
> - reg offset for base address:	0x0
> - base address value:		0x20000000
> - reg offset for base mask:	0x4
> - base mask value:		0xfc000000
> 
> Or do you prefer to just pass register offsets in mediatek,pbus-csr phandle
> array and get base address values reading ranges property? Something like:
> 
> pcie0: pcie@1fc00000 {
> 	...
> 	ranges = <0x02000000 0 0x20000000 0x0 0x20000000 0 0x4000000>;
> 	...
> 	mediatek,pbus-csr = <&pbus_csr 0x0 0x4>;
> 	...
> }
> 
> Considering the latter, even if it is not a real problem for EN7581 since we
> have just a single range, what if we have multiple ranges?

I'm really hesitant about giving DT advice because I don't understand
it well, but I do think it's important to specify host bridge aperture
addresses in 'ranges' because there's lots of generic code that
expects them there.

If you have to program the aperture addresses into the hardware, those
register addresses should be described separately elsewhere.  I assume
the aperture size is configurable since you have to write a mask
value, so the driver would have to compute the mask value based on the
aperture size.

Bjorn