From: Chuanjia Liu <hidden> Date: 2021-08-23 03:28:35
There are two independent PCIe controllers in MT2712 and MT7622 platform.
Each of them should contain an independent MSI domain.
In old dts architecture, MSI domain will be inherited from the root
bridge, and all of the devices will share the same MSI domain.Hence that,
the PCIe devices will not work properly if the irq number which required
is more than 32.
Split the PCIe node for MT2712 and MT7622 platform to comply with
the hardware design and fix MSI issue.
change note:
v12:Add Reviwe by Rob. According to Bjorn's suggestion,
split the driver patch into three parts and rewrite
the commit logs, there is no code change
v11:Rebase for 5.14-rc1 and add "interrupt-names", "linux,pci-domain"
description in binding file. No code change.
v10:Rebase for 5.13-rc1, no code change.
v9:fix kernel-ci bot warning. In the scene of using new dts format,
when mtk_pcie_parse_port fails, of_node_put don't need to be called.
v8:remove slot node and fix yaml warning.
v7:dt-bindings file was modified as suggested by Rob, other file no
change.
v6:Fix yaml error. make sure driver compatible with old and
new DTS format.
v5:rebase for 5.9-rc1, no code change.
v4:change commit message due to bayes statistical bogofilter
considers this series patch SPAM.
v3:rebase for 5.8-rc1. Only collect ack of Ryder, No code change.
v2:change the allocation of MT2712 PCIe MMIO space due to the
allocation size is not right in v1.
Chuanjia Liu (6):
dt-bindings: PCI: mediatek: Update the Device tree bindings
PCI: mediatek: Add new method to get shared pcie-cfg base address
PCI: mediatek: Add new method to get irq number
PCI: mediatek: Get pci domain and decide how to parse node
arm64: dts: mediatek: Split PCIe node for MT2712 and MT7622
ARM: dts: mediatek: Update MT7629 PCIe node for new format
.../bindings/pci/mediatek-pcie-cfg.yaml | 39 ++++
.../devicetree/bindings/pci/mediatek-pcie.txt | 206 ++++++++++--------
arch/arm/boot/dts/mt7629-rfb.dts | 3 +-
arch/arm/boot/dts/mt7629.dtsi | 45 ++--
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 97 +++++----
.../dts/mediatek/mt7622-bananapi-bpi-r64.dts | 16 +-
arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts | 6 +-
arch/arm64/boot/dts/mediatek/mt7622.dtsi | 112 +++++-----
drivers/pci/controller/pcie-mediatek.c | 52 +++--
9 files changed, 330 insertions(+), 246 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-cfg.yaml
--
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
From: Chuanjia Liu <hidden> Date: 2021-08-23 03:28:51
There are two independent PCIe controllers in MT2712 and MT7622
platform. Each of them should contain an independent MSI domain.
In old dts architecture, MSI domain will be inherited from the root
bridge, and all of the devices will share the same MSI domain.
Hence that, the PCIe devices will not work properly if the irq number
which required is more than 32.
Split the PCIe node for MT2712 and MT7622 platform to comply with
the hardware design and fix MSI issue.
Signed-off-by: Chuanjia Liu <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
Reviewed-by: Rob Herring <robh+dt@kernel.org>
---
.../bindings/pci/mediatek-pcie-cfg.yaml | 39 ++++
.../devicetree/bindings/pci/mediatek-pcie.txt | 206 ++++++++++--------
2 files changed, 150 insertions(+), 95 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-cfg.yaml
@@ -8,7 +8,7 @@ Required properties: "mediatek,mt7623-pcie" "mediatek,mt7629-pcie" - device_type: Must be "pci"-- reg: Base addresses and lengths of the PCIe subsys and root ports.+- reg: Base addresses and lengths of the root ports. - reg-names: Names of the above areas to use during resource lookup. - #address-cells: Address representation for root ports (must be 3) - #size-cells: Size representation for root ports (must be 2)
@@ -47,9 +47,12 @@ Required properties for MT7623/MT2701: - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the number of root ports.-Required properties for MT2712/MT7622:+Required properties for MT2712/MT7622/MT7629: -interrupts: A list of interrupt outputs of the controller, must have one entry for each PCIe port+- interrupt-names: Must include the following entries:+ - "pcie_irq": The interrupt that is asserted when an MSI/INTX is received+- linux,pci-domain: PCI domain ID. Should be unique for each host controller In addition, the device tree node must have sub-nodes describing each PCIe port interface, having the following mandatory properties:
From: Chuanjia Liu <hidden> Date: 2021-08-23 03:28:53
Use platform_get_irq_byname() to get the irq number
if the property of "interrupt-names" is defined.
Signed-off-by: Chuanjia Liu <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
drivers/pci/controller/pcie-mediatek.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
@@ -654,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,returnerr;}-port->irq=platform_get_irq(pdev,port->slot);+if(of_find_property(dev->of_node,"interrupt-names",NULL))+port->irq=platform_get_irq_byname(pdev,"pcie_irq");+else+port->irq=platform_get_irq(pdev,port->slot);
This would be the only instance of this pattern, where we look for a
property and use the result to decide how to look for the IRQ.
dw_pcie_host_init() does something like this:
port->irq = platform_get_irq_byname_optional(pdev, "pcie_irq");
if (port->irq < 0) {
port->irq = platform_get_irq(pdev, port->slot);
if (port->irq < 0)
return port->irq;
}
Would that work for you? If not, the commit log should explain why
you can't use the standard pattern.
If you do things differently than other drivers, it makes things
harder to review and slows things down. If you *have* to do something
differently and it adds real value to be different, that's fine. But
we should avoid unnecessary differences.
From: Chuanjia Liu <hidden> Date: 2021-09-02 09:28:55
On Tue, 2021-08-31 at 13:30 -0500, Bjorn Helgaas wrote:
On Mon, Aug 23, 2021 at 11:27:57AM +0800, Chuanjia Liu wrote:
quoted
Use platform_get_irq_byname() to get the irq number
if the property of "interrupt-names" is defined.
From patch 1/6, I have the impression that this patch is part of
fixing an MSI issue. If so, this commit log should mention that as
well.
Hi ,Bjorn
Yes，I will change the commit message as follow
In order to parse the new dts format that conforms to the hardware
design and fixes the MSI issue，add
platform_get_irq_byname_optional to get the irq number.
quoted
Signed-off-by: Chuanjia Liu <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
drivers/pci/controller/pcie-mediatek.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
This would be the only instance of this pattern, where we look for a
property and use the result to decide how to look for the IRQ.
dw_pcie_host_init() does something like this:
port->irq = platform_get_irq_byname_optional(pdev, "pcie_irq");
if (port->irq < 0) {
port->irq = platform_get_irq(pdev, port->slot);
if (port->irq < 0)
return port->irq;
}
Would that work for you? If not, the commit log should explain why
you can't use the standard pattern.
If you do things differently than other drivers, it makes things
harder to review and slows things down. If you *have* to do
something
differently and it adds real value to be different, that's fine. But
we should avoid unnecessary differences.
Thanks for your advice，it is very helpful to me, I will use standard
pattern to avoid unnecessary differences
Thanks again!
Chuanjia
From: Chuanjia Liu <hidden> Date: 2021-08-23 03:29:04
Use of_get_pci_domain_nr() to get the pci domain.
If the property of "linux,pci-domain" is defined in node,
we assume that the PCIe bridge is an individual bridge,
hence that we only need to parse one port.
Signed-off-by: Chuanjia Liu <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
drivers/pci/controller/pcie-mediatek.c | 29 +++++++++++++++-----------
1 file changed, 17 insertions(+), 12 deletions(-)
@@ -1048,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie)structdevice*dev=pcie->dev;structdevice_node*node=dev->of_node,*child;structmtk_pcie_port*port,*tmp;-interr;+interr,slot;++slot=of_get_pci_domain_nr(dev->of_node);+if(slot<0){+for_each_available_child_of_node(node,child){+err=of_pci_get_devfn(child);+if(err<0){+dev_err(dev,"failed to get devfn: %d\n",err);+gotoerror_put_node;+}-for_each_available_child_of_node(node,child){-intslot;+slot=PCI_SLOT(err);-err=of_pci_get_devfn(child);-if(err<0){-dev_err(dev,"failed to parse devfn: %d\n",err);-gotoerror_put_node;+err=mtk_pcie_parse_port(pcie,child,slot);+if(err)+gotoerror_put_node;}--slot=PCI_SLOT(err);--err=mtk_pcie_parse_port(pcie,child,slot);+}else{+err=mtk_pcie_parse_port(pcie,node,slot);if(err)-gotoerror_put_node;+returnerr;}err=mtk_pcie_subsys_powerup(pcie);
--
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
From: Chuanjia Liu <hidden> Date: 2021-08-23 03:29:19
There are two independent PCIe controllers in MT2712 and MT7622
platform. Each of them should contain an independent MSI domain.
In old dts architecture, MSI domain will be inherited from the root
bridge, and all of the devices will share the same MSI domain.
Hence that, the PCIe devices will not work properly if the irq number
which required is more than 32.
Split the PCIe node for MT2712 and MT7622 platform to comply with
the hardware design and fix MSI issue.
Signed-off-by: Chuanjia Liu <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 97 +++++++--------
.../dts/mediatek/mt7622-bananapi-bpi-r64.dts | 16 ++-
arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts | 6 +-
arch/arm64/boot/dts/mediatek/mt7622.dtsi | 112 ++++++++++--------
4 files changed, 118 insertions(+), 113 deletions(-)
There are two independent PCIe controllers in MT2712 and MT7622
platform. Each of them should contain an independent MSI domain.
In old dts architecture, MSI domain will be inherited from the root
bridge, and all of the devices will share the same MSI domain.
Hence that, the PCIe devices will not work properly if the irq number
which required is more than 32.
Split the PCIe node for MT2712 and MT7622 platform to comply with
the hardware design and fix MSI issue.
Signed-off-by: Chuanjia Liu <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
From: Chuanjia Liu <hidden> Date: 2021-08-23 03:29:32
For the new dts format, add a new method to get
shared pcie-cfg base address and use it to configure
the PCIECFG controller
Signed-off-by: Chuanjia Liu <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
drivers/pci/controller/pcie-mediatek.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
@@ -682,6 +686,10 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)val|=PCIE_CSR_LTSSM_EN(port->slot)|PCIE_CSR_ASPM_L1_EN(port->slot);writel(val,pcie->base+PCIE_SYS_CFG_V2);+}elseif(pcie->cfg){+val=PCIE_CSR_LTSSM_EN(port->slot)|+PCIE_CSR_ASPM_L1_EN(port->slot);+regmap_update_bits(pcie->cfg,PCIE_SYS_CFG_V2,val,val);}/* Assert all reset signals */
@@ -985,6 +993,7 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)structdevice*dev=pcie->dev;structplatform_device*pdev=to_platform_device(dev);structresource*regs;+structdevice_node*cfg_node;interr;/* get shared registers, which are optional */
@@ -995,6 +1004,14 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)returnPTR_ERR(pcie->base);}+cfg_node=of_find_compatible_node(NULL,NULL,+"mediatek,generic-pciecfg");+if(cfg_node){+pcie->cfg=syscon_node_to_regmap(cfg_node);+if(IS_ERR(pcie->cfg))+returnPTR_ERR(pcie->cfg);+}+pcie->free_ck=devm_clk_get(dev,"free_ck");if(IS_ERR(pcie->free_ck)){if(PTR_ERR(pcie->free_ck)==-EPROBE_DEFER)
--
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
@@ -682,6 +686,10 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)val|=PCIE_CSR_LTSSM_EN(port->slot)|PCIE_CSR_ASPM_L1_EN(port->slot);writel(val,pcie->base+PCIE_SYS_CFG_V2);+}elseif(pcie->cfg){+val=PCIE_CSR_LTSSM_EN(port->slot)|+PCIE_CSR_ASPM_L1_EN(port->slot);+regmap_update_bits(pcie->cfg,PCIE_SYS_CFG_V2,val,val);}/* Assert all reset signals */
@@ -985,6 +993,7 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)structdevice*dev=pcie->dev;structplatform_device*pdev=to_platform_device(dev);structresource*regs;+structdevice_node*cfg_node;interr;/* get shared registers, which are optional */
@@ -995,6 +1004,14 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)returnPTR_ERR(pcie->base);}+cfg_node=of_find_compatible_node(NULL,NULL,+"mediatek,generic-pciecfg");
This looks wrong to me. IIUC, since we start at NULL, this searches
the entire device tree for any node with
compatible = "mediatek,generic-pciecfg"
but we should only care about the specific device/node this driver
claimed.
Should this be part of the match data, i.e., struct mtk_pcie_soc?
+ if (cfg_node) {
+ pcie->cfg = syscon_node_to_regmap(cfg_node);
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible() (imx6,
kirin, v3-semi).
You should do it the same way unless there's a need to be different.
It's also nice if you can use the same struct member name
("mtk_pcie.cfg") as other drivers. They're not all consistent, but I
don't see any other "cfg".
+ if (IS_ERR(pcie->cfg))
+ return PTR_ERR(pcie->cfg);
+ }
+
pcie->free_ck = devm_clk_get(dev, "free_ck");
if (IS_ERR(pcie->free_ck)) {
if (PTR_ERR(pcie->free_ck) == -EPROBE_DEFER)
--
2.18.0
This looks wrong to me. IIUC, since we start at NULL, this searches
the entire device tree for any node with
compatible = "mediatek,generic-pciecfg"
but we should only care about the specific device/node this driver
claimed.
Should this be part of the match data, i.e., struct mtk_pcie_soc?
Hi Bjorn,
Thanks for your review.
Many drivers in the drivers/ folder use compatible to search directly.
I guess that when different devices need to search for different nodes,
it is necessary to use match data to determine the current node
information that needs to be searched.
But in the devices supported by this driver, I guess it no need
through match data to confirm the compatible information，because it
always search same compatible "mediatek, generic-pciecfg"
quoted
+ if (cfg_node) {
+ pcie->cfg = syscon_node_to_regmap(cfg_node);
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible() (imx6,
kirin, v3-semi).
You should do it the same way unless there's a need to be different.
I have used phandle, but Rob suggested to search for the node by
compatible. The reason why syscon_regmap_lookup_by_compatible() is not
used here is that the pciecfg node is optional, and there is no need to
return error when the node is not searched.
It's also nice if you can use the same struct member name
("mtk_pcie.cfg") as other drivers. They're not all consistent, but I
don't see any other "cfg".
Other drivers struct member name are also different.
For example, scfg(layerscape)，map(v3-semi), crgctrl(kirin),
regmap(artpec6).Or just use local variables, named syscon or regmap.
So I couldn't be consistent with other drivers.
Thanks
Chuanjia
quoted
+ if (IS_ERR(pcie->cfg))
+ return PTR_ERR(pcie->cfg);
+ }
+
pcie->free_ck = devm_clk_get(dev, "free_ck");
if (IS_ERR(pcie->free_ck)) {
if (PTR_ERR(pcie->free_ck) == -EPROBE_DEFER)
--
2.18.0
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible() (imx6,
kirin, v3-semi).
You should do it the same way unless there's a need to be different.
I have used phandle, but Rob suggested to search for the node by
compatible.
The reason why syscon_regmap_lookup_by_compatible() is not
used here is that the pciecfg node is optional, and there is no need to
return error when the node is not searched.
How about this?
regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-pciecfg");
if (!IS_ERR(regmap))
pcie->cfg = regmap;
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible()
(imx6,
kirin, v3-semi).
You should do it the same way unless there's a need to be
different.
I have used phandle, but Rob suggested to search for the node by
compatible.
The reason why syscon_regmap_lookup_by_compatible() is not
used here is that the pciecfg node is optional, and there is no
need to
return error when the node is not searched.
How about this?
regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-
pciecfg");
if (!IS_ERR(regmap))
pcie->cfg = regmap;
Hi Bjorn,
We need to deal with three situations
1) No error
2) The error of the node not found, don't do anything
3) Other errors, return errors
I guess you mean
regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-
pciecfg");
if (!IS_ERR(regmap))
pcie->cfg = regmap;
else if (IS_ERR(regmap) && PTR_ERR(regmap) != -ENODEV)
return PTR_ERR(regmap);
I'm not sure if we need this, it seems a little weird and there are
many drivers in other subsystems that use syscon_node_to_regmap().
Thanks
Chuanjia
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible()
(imx6,
kirin, v3-semi).
You should do it the same way unless there's a need to be
different.
I have used phandle, but Rob suggested to search for the node by
compatible.
The reason why syscon_regmap_lookup_by_compatible() is not
used here is that the pciecfg node is optional, and there is no
need to
return error when the node is not searched.
How about this?
regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-
pciecfg");
if (!IS_ERR(regmap))
pcie->cfg = regmap;
+1
Hi Bjorn,
We need to deal with three situations
1) No error
2) The error of the node not found, don't do anything
3) Other errors, return errors
I guess you mean
regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-
pciecfg");
if (!IS_ERR(regmap))
pcie->cfg = regmap;
else if (IS_ERR(regmap) && PTR_ERR(regmap) != -ENODEV)
You already know IS_ERR is true here.
return PTR_ERR(regmap);
syscon_regmap_lookup_by_compatible_optional is the function you are
looking for. The _optional flavor doesn't exist, so create it. There
is one for the phandle lookup.
I'm not sure if we need this, it seems a little weird and there are
many drivers in other subsystems that use syscon_node_to_regmap().
You are implementing the exact same sequence that
syscon_regmap_lookup_by_compatible() does, so clearly you should be
using it. The one difference is you forgot the of_node_put().
Rob
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible()
(imx6,
kirin, v3-semi).
You should do it the same way unless there's a need to be
different.
I have used phandle, but Rob suggested to search for the node
by
compatible.
The reason why syscon_regmap_lookup_by_compatible() is not
used here is that the pciecfg node is optional, and there is no
need to
return error when the node is not searched.
How about this?
regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-
pciecfg");
if (!IS_ERR(regmap))
pcie->cfg = regmap;
+1
quoted
Hi Bjorn,
We need to deal with three situations
1) No error
2) The error of the node not found, don't do anything
3) Other errors, return errors
I guess you mean
regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-
pciecfg");
if (!IS_ERR(regmap))
pcie->cfg = regmap;
else if (IS_ERR(regmap) && PTR_ERR(regmap) != -ENODEV)
You already know IS_ERR is true here.
quoted
return PTR_ERR(regmap);
syscon_regmap_lookup_by_compatible_optional is the function you are
looking for. The _optional flavor doesn't exist, so create it. There
is one for the phandle lookup.
quoted
I'm not sure if we need this, it seems a little weird and there are
many drivers in other subsystems that use syscon_node_to_regmap().
You are implementing the exact same sequence that
syscon_regmap_lookup_by_compatible() does, so clearly you should be
using it. The one difference is you forgot the of_node_put().
Hi,Rob
Thanks for your explanation. You're right.
Now I understand use syscon_regmap_lookup_by_compatible() is a better
way. I'll follow your advice
regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-
pciecfg");
if (!IS_ERR(regmap))
pcie->cfg = regmap;
else if (PTR_ERR(regmap) != -ENODEV)
return PTR_ERR(regmap);
Best regards
Chuanjia
@@ -682,6 +686,10 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)val|=PCIE_CSR_LTSSM_EN(port->slot)|PCIE_CSR_ASPM_L1_EN(port->slot);writel(val,pcie->base+PCIE_SYS_CFG_V2);+}elseif(pcie->cfg){+val=PCIE_CSR_LTSSM_EN(port->slot)|+PCIE_CSR_ASPM_L1_EN(port->slot);+regmap_update_bits(pcie->cfg,PCIE_SYS_CFG_V2,val,val);}/* Assert all reset signals */
@@ -985,6 +993,7 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)structdevice*dev=pcie->dev;structplatform_device*pdev=to_platform_device(dev);structresource*regs;+structdevice_node*cfg_node;interr;/* get shared registers, which are optional */
@@ -995,6 +1004,14 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)returnPTR_ERR(pcie->base);}+cfg_node=of_find_compatible_node(NULL,NULL,+"mediatek,generic-pciecfg");
This looks wrong to me. IIUC, since we start at NULL, this searches
the entire device tree for any node with
compatible = "mediatek,generic-pciecfg"
but we should only care about the specific device/node this driver
claimed.
Should this be part of the match data, i.e., struct mtk_pcie_soc?
What would you put in match data exactly?
The other way to do this is to have a DT property with the phandle
which people like to do (have everything in the node 'for their
driver'). If there's only 1 possible node (which is almost always the
case), then there is little benefit to having another property. It's
just redundant data. A phandle lookup might be a bit faster with the
caching we do, but on a miss it would still walk all nodes.
The other thing with these 'extra register bits to twiddle' is that
they tend to be SoC specific and change from chip to chip, so either
way is not very portable. The real question to ask is should there be
a standard interface used or created.
quoted
+ if (cfg_node) {
+ pcie->cfg = syscon_node_to_regmap(cfg_node);
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible() (imx6,
kirin, v3-semi).
There's no phandle to use in this case. As above, I'm trying to break
people of this habit.
Rob
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
@@ -682,6 +686,10 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)val|=PCIE_CSR_LTSSM_EN(port->slot)|PCIE_CSR_ASPM_L1_EN(port->slot);writel(val,pcie->base+PCIE_SYS_CFG_V2);+}elseif(pcie->cfg){+val=PCIE_CSR_LTSSM_EN(port->slot)|+PCIE_CSR_ASPM_L1_EN(port->slot);+regmap_update_bits(pcie->cfg,PCIE_SYS_CFG_V2,val,val);}/* Assert all reset signals */
@@ -985,6 +993,7 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)structdevice*dev=pcie->dev;structplatform_device*pdev=to_platform_device(dev);structresource*regs;+structdevice_node*cfg_node;interr;/* get shared registers, which are optional */
@@ -995,6 +1004,14 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)returnPTR_ERR(pcie->base);}+cfg_node=of_find_compatible_node(NULL,NULL,+"mediatek,generic-pciecfg");
This looks wrong to me. IIUC, since we start at NULL, this searches
the entire device tree for any node with
compatible = "mediatek,generic-pciecfg"
but we should only care about the specific device/node this driver
claimed.
Should this be part of the match data, i.e., struct mtk_pcie_soc?
What would you put in match data exactly?
The other way to do this is to have a DT property with the phandle
which people like to do (have everything in the node 'for their
driver'). If there's only 1 possible node (which is almost always the
case), then there is little benefit to having another property. It's
just redundant data. A phandle lookup might be a bit faster with the
caching we do, but on a miss it would still walk all nodes.
The other thing with these 'extra register bits to twiddle' is that
they tend to be SoC specific and change from chip to chip, so either
way is not very portable. The real question to ask is should there be
a standard interface used or created.
quoted
quoted
+ if (cfg_node) {
+ pcie->cfg = syscon_node_to_regmap(cfg_node);
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible() (imx6,
kirin, v3-semi).
There's no phandle to use in this case. As above, I'm trying to break
people of this habit.
Thanks! I was mistaken in lots of ways here. I first assumed
"mediatek,generic-pciecfg" was local to the pcie node, but that's not
true. Then I thought there might be an ownership issue because the
regmap is not local to the device, and several drivers look up and use
the same regmap. But it looks like regmap provides some internal
locking which mitigates most or all of that concern.
Just to check -- you prefer syscon_regmap_lookup_by_compatible() over
syscon_regmap_lookup_by_phandle()?
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
@@ -682,6 +686,10 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)val|=PCIE_CSR_LTSSM_EN(port->slot)|PCIE_CSR_ASPM_L1_EN(port->slot);writel(val,pcie->base+PCIE_SYS_CFG_V2);+}elseif(pcie->cfg){+val=PCIE_CSR_LTSSM_EN(port->slot)|+PCIE_CSR_ASPM_L1_EN(port->slot);+regmap_update_bits(pcie->cfg,PCIE_SYS_CFG_V2,val,val);}/* Assert all reset signals */
@@ -985,6 +993,7 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)structdevice*dev=pcie->dev;structplatform_device*pdev=to_platform_device(dev);structresource*regs;+structdevice_node*cfg_node;interr;/* get shared registers, which are optional */
@@ -995,6 +1004,14 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)returnPTR_ERR(pcie->base);}+cfg_node=of_find_compatible_node(NULL,NULL,+"mediatek,generic-pciecfg");
This looks wrong to me. IIUC, since we start at NULL, this searches
the entire device tree for any node with
compatible = "mediatek,generic-pciecfg"
but we should only care about the specific device/node this driver
claimed.
Should this be part of the match data, i.e., struct mtk_pcie_soc?
What would you put in match data exactly?
The other way to do this is to have a DT property with the phandle
which people like to do (have everything in the node 'for their
driver'). If there's only 1 possible node (which is almost always the
case), then there is little benefit to having another property. It's
just redundant data. A phandle lookup might be a bit faster with the
caching we do, but on a miss it would still walk all nodes.
The other thing with these 'extra register bits to twiddle' is that
they tend to be SoC specific and change from chip to chip, so either
way is not very portable. The real question to ask is should there be
a standard interface used or created.
quoted
quoted
+ if (cfg_node) {
+ pcie->cfg = syscon_node_to_regmap(cfg_node);
Other drivers in drivers/pci/controller/ use
syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
layerscape, artpec6) or syscon_regmap_lookup_by_compatible() (imx6,
kirin, v3-semi).
There's no phandle to use in this case. As above, I'm trying to break
people of this habit.
Thanks! I was mistaken in lots of ways here. I first assumed
"mediatek,generic-pciecfg" was local to the pcie node, but that's not
true. Then I thought there might be an ownership issue because the
regmap is not local to the device, and several drivers look up and use
the same regmap. But it looks like regmap provides some internal
locking which mitigates most or all of that concern.
Just to check -- you prefer syscon_regmap_lookup_by_compatible() over
syscon_regmap_lookup_by_phandle()?
Yes, unless there is either more than 1 instance of the syscon or some
additional data specific to the user (e.g. a register offset).
Rob
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
To match the new dts binding. Remove "subsys",unused
interrupt and slot node.Add "interrupt-names",
"linux,pci-domain" and pciecfg node.
Signed-off-by: Chuanjia Liu <redacted>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
From: Lorenzo Pieralisi <hidden> Date: 2021-08-26 12:53:31
On Mon, 23 Aug 2021 11:27:54 +0800, Chuanjia Liu wrote:
There are two independent PCIe controllers in MT2712 and MT7622 platform.
Each of them should contain an independent MSI domain.
In old dts architecture, MSI domain will be inherited from the root
bridge, and all of the devices will share the same MSI domain.Hence that,
the PCIe devices will not work properly if the irq number which required
is more than 32.
[...]