* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-02 19:34 ` [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC Lorenzo Bianconi
@ 2025-02-14 17:11 ` Manivannan Sadhasivam
2025-02-17 12:19 ` Lorenzo Bianconi
2025-02-19 18:26 ` Manivannan Sadhasivam
2025-02-20 18:20 ` Bjorn Helgaas
2 siblings, 1 reply; 17+ messages in thread
From: Manivannan Sadhasivam @ 2025-02-14 17:11 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, linux-pci, linux-mediatek, devicetree,
linux-arm-kernel
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
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-14 17:11 ` Manivannan Sadhasivam
@ 2025-02-17 12:19 ` Lorenzo Bianconi
0 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Bianconi @ 2025-02-17 12:19 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, linux-pci, linux-mediatek, devicetree,
linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-02 19:34 ` [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC Lorenzo Bianconi
2025-02-14 17:11 ` Manivannan Sadhasivam
@ 2025-02-19 18:26 ` Manivannan Sadhasivam
2025-02-20 20:23 ` Frank Li
2025-02-20 18:20 ` Bjorn Helgaas
2 siblings, 1 reply; 17+ messages in thread
From: Manivannan Sadhasivam @ 2025-02-19 18:26 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, linux-pci,
linux-mediatek, devicetree, linux-arm-kernel
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
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-19 18:26 ` Manivannan Sadhasivam
@ 2025-02-20 20:23 ` Frank Li
2025-02-20 22:39 ` Lorenzo Bianconi
0 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2025-02-20 20:23 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Lorenzo Bianconi, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, linux-pci,
linux-mediatek, devicetree, linux-arm-kernel
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
> >
>
> --
> மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-20 20:23 ` Frank Li
@ 2025-02-20 22:39 ` Lorenzo Bianconi
0 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Bianconi @ 2025-02-20 22:39 UTC (permalink / raw)
To: Frank Li
Cc: Manivannan Sadhasivam, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, linux-pci,
linux-mediatek, devicetree, linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-02 19:34 ` [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC Lorenzo Bianconi
2025-02-14 17:11 ` Manivannan Sadhasivam
2025-02-19 18:26 ` Manivannan Sadhasivam
@ 2025-02-20 18:20 ` Bjorn Helgaas
2025-02-20 19:54 ` Lorenzo Bianconi
2 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2025-02-20 18:20 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, linux-pci,
linux-mediatek, devicetree, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-20 18:20 ` Bjorn Helgaas
@ 2025-02-20 19:54 ` Lorenzo Bianconi
2025-02-20 23:56 ` Bjorn Helgaas
0 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Bianconi @ 2025-02-20 19:54 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, linux-pci,
linux-mediatek, devicetree, linux-arm-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-20 19:54 ` Lorenzo Bianconi
@ 2025-02-20 23:56 ` Bjorn Helgaas
2025-02-21 9:20 ` Lorenzo Bianconi
0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2025-02-20 23:56 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, linux-pci,
linux-mediatek, devicetree, linux-arm-kernel, Frank Li
[+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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-20 23:56 ` Bjorn Helgaas
@ 2025-02-21 9:20 ` Lorenzo Bianconi
2025-02-21 9:30 ` 回复: " Hui Ma (马慧)
0 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Bianconi @ 2025-02-21 9:20 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, linux-pci,
linux-mediatek, devicetree, linux-arm-kernel, Frank Li, Hui.Ma,
upstream
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* 回复: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-21 9:20 ` Lorenzo Bianconi
@ 2025-02-21 9:30 ` Hui Ma (马慧)
2025-02-21 18:31 ` Bjorn Helgaas
0 siblings, 1 reply; 17+ messages in thread
From: Hui Ma (马慧) @ 2025-02-21 9:30 UTC (permalink / raw)
To: Lorenzo Bianconi, Bjorn Helgaas
Cc: Ryder Lee, Jianjun Wang (王建军),
Lorenzo Pieralisi, Krzysztof Wilczy��ski,
Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, linux-pci@vger.kernel.org,
linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Frank Li, upstream
> [+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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: 回复: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-21 9:30 ` 回复: " Hui Ma (马慧)
@ 2025-02-21 18:31 ` Bjorn Helgaas
2025-02-21 23:18 ` Lorenzo Bianconi
0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2025-02-21 18:31 UTC (permalink / raw)
To: Hui Ma (马慧)
Cc: Lorenzo Bianconi, Ryder Lee,
Jianjun Wang (王建军), Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno,
linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Frank Li, upstream
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: 回复: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-21 18:31 ` Bjorn Helgaas
@ 2025-02-21 23:18 ` Lorenzo Bianconi
2025-02-22 0:07 ` Bjorn Helgaas
0 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Bianconi @ 2025-02-21 23:18 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Hui Ma (马慧), Ryder Lee,
Jianjun Wang (王建军), Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno,
linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Frank Li, upstream
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: 回复: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC
2025-02-21 23:18 ` Lorenzo Bianconi
@ 2025-02-22 0:07 ` Bjorn Helgaas
0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Helgaas @ 2025-02-22 0:07 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Hui Ma (马慧), Ryder Lee,
Jianjun Wang (王建军), Lorenzo Pieralisi,
Krzysztof Wilczyński, Manivannan Sadhasivam, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno,
linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Frank Li, upstream
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
^ permalink raw reply [flat|nested] 17+ messages in thread