These series of patches change the allocation of MSI address on MediaTek
platform, which uses dmam_alloc_coherent() to allocate the MSI address.
Changes in v2:
1. Add a separate patch for pcie-mediatek-gen3.c to prevent break the
probe flow when MSI init fails.
2. Change the location of allocate the MSI address to prevent break the
probe flow.
Jianjun Wang (3):
PCI: mediatek: Allocate MSI address with dmam_alloc_coherent()
PCI: mediatek-gen3: Do not break probe flow when MSI init fails
PCI: mediatek-gen3: Allocate MSI address with dmam_alloc_coherent()
drivers/pci/controller/pcie-mediatek-gen3.c | 126 +++++++++++---------
drivers/pci/controller/pcie-mediatek.c | 24 ++--
2 files changed, 83 insertions(+), 67 deletions(-)
--
2.18.0
Use dmam_alloc_coherent() to allocate the MSI address, instead of using
static physical address.
Signed-off-by: Jianjun Wang <[email protected]>
---
drivers/pci/controller/pcie-mediatek-gen3.c | 72 ++++++++++++---------
1 file changed, 41 insertions(+), 31 deletions(-)
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index c6a6876d233a..7cfd7ef9ad95 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -120,7 +120,6 @@ struct mtk_msi_set {
* struct mtk_gen3_pcie - PCIe port information
* @dev: pointer to PCIe device
* @base: IO mapped register base
- * @reg_base: physical register base
* @mac_reset: MAC reset control
* @phy_reset: PHY reset control
* @phy: PHY controller block
@@ -139,7 +138,6 @@ struct mtk_msi_set {
struct mtk_gen3_pcie {
struct device *dev;
void __iomem *base;
- phys_addr_t reg_base;
struct reset_control *mac_reset;
struct reset_control *phy_reset;
struct phy *phy;
@@ -309,24 +307,8 @@ static int mtk_pcie_set_trans_table(struct mtk_gen3_pcie *pcie,
static void mtk_pcie_enable_msi(struct mtk_gen3_pcie *pcie)
{
- int i;
u32 val;
- for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
- struct mtk_msi_set *msi_set = &pcie->msi_sets[i];
-
- msi_set->base = pcie->base + PCIE_MSI_SET_BASE_REG +
- i * PCIE_MSI_SET_OFFSET;
- msi_set->msg_addr = pcie->reg_base + PCIE_MSI_SET_BASE_REG +
- i * PCIE_MSI_SET_OFFSET;
-
- /* Configure the MSI capture address */
- writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
- writel_relaxed(upper_32_bits(msi_set->msg_addr),
- pcie->base + PCIE_MSI_SET_ADDR_HI_BASE +
- i * PCIE_MSI_SET_ADDR_HI_OFFSET);
- }
-
val = readl_relaxed(pcie->base + PCIE_MSI_SET_ENABLE_REG);
val |= PCIE_MSI_SET_ENABLE;
writel_relaxed(val, pcie->base + PCIE_MSI_SET_ENABLE_REG);
@@ -653,6 +635,29 @@ static int mtk_pcie_init_msi(struct mtk_gen3_pcie *pcie)
{
struct device *dev = pcie->dev;
struct device_node *node = dev->of_node;
+ struct mtk_msi_set *msi_set;
+ void *msg_vaddr[PCIE_MSI_SET_NUM];
+ int i, j, ret = -ENODEV;
+
+ for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
+ msi_set = &pcie->msi_sets[i];
+
+ msi_set->base = pcie->base + PCIE_MSI_SET_BASE_REG +
+ i * PCIE_MSI_SET_OFFSET;
+
+ msg_vaddr[i] = dmam_alloc_coherent(dev, sizeof(dma_addr_t),
+ &msi_set->msg_addr, GFP_KERNEL);
+ if (!msg_vaddr[i]) {
+ dev_err(dev, "failed to alloc and map MSI address for set %d\n", i);
+ ret = -ENOMEM;
+ goto err_alloc_addr;
+ }
+
+ /* Configure the MSI capture address */
+ writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
+ writel_relaxed(upper_32_bits(msi_set->msg_addr), pcie->base +
+ PCIE_MSI_SET_ADDR_HI_BASE + i * PCIE_MSI_SET_ADDR_HI_OFFSET);
+ }
mutex_init(&pcie->lock);
@@ -660,18 +665,24 @@ static int mtk_pcie_init_msi(struct mtk_gen3_pcie *pcie)
&mtk_msi_bottom_domain_ops, pcie);
if (!pcie->msi_bottom_domain) {
dev_err(dev, "failed to create MSI bottom domain\n");
- return -ENODEV;
+ goto err_alloc_addr;
}
pcie->msi_domain = pci_msi_create_irq_domain(dev->fwnode, &mtk_msi_domain_info,
pcie->msi_bottom_domain);
- if (!pcie->msi_domain) {
- dev_err(dev, "failed to create MSI domain\n");
- irq_domain_remove(pcie->msi_bottom_domain);
- return -ENODEV;
+ if (pcie->msi_domain)
+ return 0;
+
+ dev_err(dev, "failed to create MSI domain\n");
+ irq_domain_remove(pcie->msi_bottom_domain);
+
+err_alloc_addr:
+ for (j = 0; j < i; j++) {
+ msi_set = &pcie->msi_sets[j];
+ dmam_free_coherent(dev, sizeof(dma_addr_t), msg_vaddr[j], msi_set->msg_addr);
}
- return 0;
+ return ret;
}
static int mtk_pcie_init_intx(struct mtk_gen3_pcie *pcie)
@@ -789,20 +800,14 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
{
struct device *dev = pcie->dev;
struct platform_device *pdev = to_platform_device(dev);
- struct resource *regs;
int ret;
- regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
- if (!regs)
- return -EINVAL;
- pcie->base = devm_ioremap_resource(dev, regs);
+ pcie->base = devm_platform_ioremap_resource_byname(pdev, "pcie-mac");
if (IS_ERR(pcie->base)) {
dev_err(dev, "failed to map register base\n");
return PTR_ERR(pcie->base);
}
- pcie->reg_base = regs->start;
-
pcie->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");
if (IS_ERR(pcie->phy_reset)) {
ret = PTR_ERR(pcie->phy_reset);
@@ -1013,6 +1018,11 @@ static void mtk_pcie_irq_restore(struct mtk_gen3_pcie *pcie)
for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
struct mtk_msi_set *msi_set = &pcie->msi_sets[i];
+ /* Configure the MSI capture address */
+ writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
+ writel_relaxed(upper_32_bits(msi_set->msg_addr), pcie->base +
+ PCIE_MSI_SET_ADDR_HI_BASE + i * PCIE_MSI_SET_ADDR_HI_OFFSET);
+
writel_relaxed(msi_set->saved_irq_state,
msi_set->base + PCIE_MSI_SET_ENABLE_OFFSET);
}
--
2.18.0
Use dmam_alloc_coherent() to allocate the MSI address, instead of using
virt_to_phys().
Signed-off-by: Jianjun Wang <[email protected]>
---
drivers/pci/controller/pcie-mediatek.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 66a8f73296fc..2fb9e44369f8 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -178,6 +178,7 @@ struct mtk_pcie_soc {
* @phy: pointer to PHY control block
* @slot: port slot
* @irq: GIC irq
+ * @msg_addr: MSI message address
* @irq_domain: legacy INTx IRQ domain
* @inner_domain: inner IRQ domain
* @msi_domain: MSI IRQ domain
@@ -198,6 +199,7 @@ struct mtk_pcie_port {
struct phy *phy;
u32 slot;
int irq;
+ dma_addr_t msg_addr;
struct irq_domain *irq_domain;
struct irq_domain *inner_domain;
struct irq_domain *msi_domain;
@@ -394,12 +396,10 @@ static struct pci_ops mtk_pcie_ops_v2 = {
static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
{
struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
- phys_addr_t addr;
/* MT2712/MT7622 only support 32-bit MSI addresses */
- addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
msg->address_hi = 0;
- msg->address_lo = lower_32_bits(addr);
+ msg->address_lo = lower_32_bits(port->msg_addr);
msg->data = data->hwirq;
@@ -494,6 +494,14 @@ static struct msi_domain_info mtk_msi_domain_info = {
static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
{
struct fwnode_handle *fwnode = of_node_to_fwnode(port->pcie->dev->of_node);
+ void *msi_vaddr;
+
+ msi_vaddr = dmam_alloc_coherent(port->pcie->dev, sizeof(dma_addr_t), &port->msg_addr,
+ GFP_KERNEL);
+ if (!msi_vaddr) {
+ dev_err(port->pcie->dev, "failed to alloc and map MSI address\n");
+ return -ENOMEM;
+ }
mutex_init(&port->lock);
@@ -501,6 +509,7 @@ static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
&msi_domain_ops, port);
if (!port->inner_domain) {
dev_err(port->pcie->dev, "failed to create IRQ domain\n");
+ dmam_free_coherent(port->pcie->dev, sizeof(dma_addr_t), msi_vaddr, port->msg_addr);
return -ENOMEM;
}
@@ -508,6 +517,7 @@ static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
port->inner_domain);
if (!port->msi_domain) {
dev_err(port->pcie->dev, "failed to create MSI domain\n");
+ dmam_free_coherent(port->pcie->dev, sizeof(dma_addr_t), msi_vaddr, port->msg_addr);
irq_domain_remove(port->inner_domain);
return -ENOMEM;
}
@@ -518,10 +528,8 @@ static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
{
u32 val;
- phys_addr_t msg_addr;
- msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
- val = lower_32_bits(msg_addr);
+ val = lower_32_bits(port->msg_addr);
writel(val, port->base + PCIE_IMSI_ADDR);
val = readl(port->base + PCIE_INT_MASK);
@@ -588,7 +596,7 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
if (IS_ENABLED(CONFIG_PCI_MSI)) {
ret = mtk_pcie_allocate_msi_domains(port);
if (ret)
- return ret;
+ dev_warn(dev, "no MSI supported, only INTx available\n");
}
return 0;
@@ -732,7 +740,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
val &= ~INTX_MASK;
writel(val, port->base + PCIE_INT_MASK);
- if (IS_ENABLED(CONFIG_PCI_MSI))
+ if (IS_ENABLED(CONFIG_PCI_MSI) && port->msi_domain)
mtk_pcie_enable_msi(port);
/* Set AHB to PCIe translation windows */
--
2.18.0
Since INTx can still work, the driver probe flow should not be broken by
MSI initialization failures. Additionally, moving the MSI initialization
code into a single function enhances readability.
Fixes: 1bdafba538be ("PCI: mediatek-gen3: Add MSI support")
Signed-off-by: Jianjun Wang <[email protected]>
---
drivers/pci/controller/pcie-mediatek-gen3.c | 68 ++++++++++-----------
1 file changed, 33 insertions(+), 35 deletions(-)
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index c1ae3d19ec9a..c6a6876d233a 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -649,59 +649,51 @@ static const struct irq_domain_ops intx_domain_ops = {
.map = mtk_pcie_intx_map,
};
-static int mtk_pcie_init_irq_domains(struct mtk_gen3_pcie *pcie)
+static int mtk_pcie_init_msi(struct mtk_gen3_pcie *pcie)
{
struct device *dev = pcie->dev;
- struct device_node *intc_node, *node = dev->of_node;
- int ret;
-
- raw_spin_lock_init(&pcie->irq_lock);
-
- /* Setup INTx */
- intc_node = of_get_child_by_name(node, "interrupt-controller");
- if (!intc_node) {
- dev_err(dev, "missing interrupt-controller node\n");
- return -ENODEV;
- }
-
- pcie->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
- &intx_domain_ops, pcie);
- if (!pcie->intx_domain) {
- dev_err(dev, "failed to create INTx IRQ domain\n");
- ret = -ENODEV;
- goto out_put_node;
- }
+ struct device_node *node = dev->of_node;
- /* Setup MSI */
mutex_init(&pcie->lock);
pcie->msi_bottom_domain = irq_domain_add_linear(node, PCIE_MSI_IRQS_NUM,
&mtk_msi_bottom_domain_ops, pcie);
if (!pcie->msi_bottom_domain) {
dev_err(dev, "failed to create MSI bottom domain\n");
- ret = -ENODEV;
- goto err_msi_bottom_domain;
+ return -ENODEV;
}
- pcie->msi_domain = pci_msi_create_irq_domain(dev->fwnode,
- &mtk_msi_domain_info,
+ pcie->msi_domain = pci_msi_create_irq_domain(dev->fwnode, &mtk_msi_domain_info,
pcie->msi_bottom_domain);
if (!pcie->msi_domain) {
dev_err(dev, "failed to create MSI domain\n");
- ret = -ENODEV;
- goto err_msi_domain;
+ irq_domain_remove(pcie->msi_bottom_domain);
+ return -ENODEV;
}
- of_node_put(intc_node);
return 0;
+}
-err_msi_domain:
- irq_domain_remove(pcie->msi_bottom_domain);
-err_msi_bottom_domain:
- irq_domain_remove(pcie->intx_domain);
-out_put_node:
+static int mtk_pcie_init_intx(struct mtk_gen3_pcie *pcie)
+{
+ struct device *dev = pcie->dev;
+ struct device_node *intc_node, *node = dev->of_node;
+
+ intc_node = of_get_child_by_name(node, "interrupt-controller");
+ if (!intc_node) {
+ dev_err(dev, "missing interrupt-controller node\n");
+ return -ENODEV;
+ }
+
+ pcie->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
+ &intx_domain_ops, pcie);
of_node_put(intc_node);
- return ret;
+ if (!pcie->intx_domain) {
+ dev_err(dev, "failed to create INTx IRQ domain\n");
+ return -ENODEV;
+ }
+
+ return 0;
}
static void mtk_pcie_irq_teardown(struct mtk_gen3_pcie *pcie)
@@ -774,10 +766,16 @@ static int mtk_pcie_setup_irq(struct mtk_gen3_pcie *pcie)
struct platform_device *pdev = to_platform_device(dev);
int err;
- err = mtk_pcie_init_irq_domains(pcie);
+ raw_spin_lock_init(&pcie->irq_lock);
+
+ err = mtk_pcie_init_intx(pcie);
if (err)
return err;
+ err = mtk_pcie_init_msi(pcie);
+ if (err)
+ dev_warn(dev, "no MSI supported, only INTx available\n");
+
pcie->irq = platform_get_irq(pdev, 0);
if (pcie->irq < 0)
return pcie->irq;
--
2.18.0
On Mon, Dec 11, 2023 at 04:52:54PM +0800, Jianjun Wang wrote:
> Use dmam_alloc_coherent() to allocate the MSI address, instead of using
> virt_to_phys().
>
What is the reason for this change? So now PCIE_MSI_VECTOR becomes unused?
- Mani
> Signed-off-by: Jianjun Wang <[email protected]>
> ---
> drivers/pci/controller/pcie-mediatek.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 66a8f73296fc..2fb9e44369f8 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -178,6 +178,7 @@ struct mtk_pcie_soc {
> * @phy: pointer to PHY control block
> * @slot: port slot
> * @irq: GIC irq
> + * @msg_addr: MSI message address
> * @irq_domain: legacy INTx IRQ domain
> * @inner_domain: inner IRQ domain
> * @msi_domain: MSI IRQ domain
> @@ -198,6 +199,7 @@ struct mtk_pcie_port {
> struct phy *phy;
> u32 slot;
> int irq;
> + dma_addr_t msg_addr;
> struct irq_domain *irq_domain;
> struct irq_domain *inner_domain;
> struct irq_domain *msi_domain;
> @@ -394,12 +396,10 @@ static struct pci_ops mtk_pcie_ops_v2 = {
> static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> {
> struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);
> - phys_addr_t addr;
>
> /* MT2712/MT7622 only support 32-bit MSI addresses */
> - addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> msg->address_hi = 0;
> - msg->address_lo = lower_32_bits(addr);
> + msg->address_lo = lower_32_bits(port->msg_addr);
>
> msg->data = data->hwirq;
>
> @@ -494,6 +494,14 @@ static struct msi_domain_info mtk_msi_domain_info = {
> static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
> {
> struct fwnode_handle *fwnode = of_node_to_fwnode(port->pcie->dev->of_node);
> + void *msi_vaddr;
> +
> + msi_vaddr = dmam_alloc_coherent(port->pcie->dev, sizeof(dma_addr_t), &port->msg_addr,
> + GFP_KERNEL);
> + if (!msi_vaddr) {
> + dev_err(port->pcie->dev, "failed to alloc and map MSI address\n");
> + return -ENOMEM;
> + }
>
> mutex_init(&port->lock);
>
> @@ -501,6 +509,7 @@ static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
> &msi_domain_ops, port);
> if (!port->inner_domain) {
> dev_err(port->pcie->dev, "failed to create IRQ domain\n");
> + dmam_free_coherent(port->pcie->dev, sizeof(dma_addr_t), msi_vaddr, port->msg_addr);
> return -ENOMEM;
> }
>
> @@ -508,6 +517,7 @@ static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
> port->inner_domain);
> if (!port->msi_domain) {
> dev_err(port->pcie->dev, "failed to create MSI domain\n");
> + dmam_free_coherent(port->pcie->dev, sizeof(dma_addr_t), msi_vaddr, port->msg_addr);
> irq_domain_remove(port->inner_domain);
> return -ENOMEM;
> }
> @@ -518,10 +528,8 @@ static int mtk_pcie_allocate_msi_domains(struct mtk_pcie_port *port)
> static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> {
> u32 val;
> - phys_addr_t msg_addr;
>
> - msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> - val = lower_32_bits(msg_addr);
> + val = lower_32_bits(port->msg_addr);
> writel(val, port->base + PCIE_IMSI_ADDR);
>
> val = readl(port->base + PCIE_INT_MASK);
> @@ -588,7 +596,7 @@ static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> ret = mtk_pcie_allocate_msi_domains(port);
> if (ret)
> - return ret;
> + dev_warn(dev, "no MSI supported, only INTx available\n");
> }
>
> return 0;
> @@ -732,7 +740,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> val &= ~INTX_MASK;
> writel(val, port->base + PCIE_INT_MASK);
>
> - if (IS_ENABLED(CONFIG_PCI_MSI))
> + if (IS_ENABLED(CONFIG_PCI_MSI) && port->msi_domain)
> mtk_pcie_enable_msi(port);
>
> /* Set AHB to PCIe translation windows */
> --
> 2.18.0
>
>
--
மணிவண்ணன் சதாசிவம்
On Sat, 08 Jun 2024 10:01:52 +0100,
Manivannan Sadhasivam <[email protected]> wrote:
>
> On Mon, Dec 11, 2023 at 04:52:54PM +0800, Jianjun Wang wrote:
> > Use dmam_alloc_coherent() to allocate the MSI address, instead of using
> > virt_to_phys().
> >
>
> What is the reason for this change? So now PCIE_MSI_VECTOR becomes unused?
More importantly, this is yet another example of the DW reference
driver nonsense, where memory is allocated for *MSI*, while the whole
point of MSIs is that it is a write that doesn't target memory, making
any form of RAM allocation absolutely pointless.
This silly approach has been cargo-culted for years, and while I
caught a few in my time, you can't beat copy-paste.
IMO, this patch is only making things worse instead of fixing things.
M.
--
Without deviation from the norm, progress is not possible.
On Sun, Jun 09, 2024 at 01:32:38PM +0100, Marc Zyngier wrote:
> On Sat, 08 Jun 2024 10:01:52 +0100,
> Manivannan Sadhasivam <[email protected]> wrote:
> >
> > On Mon, Dec 11, 2023 at 04:52:54PM +0800, Jianjun Wang wrote:
> > > Use dmam_alloc_coherent() to allocate the MSI address, instead of using
> > > virt_to_phys().
> >
> > What is the reason for this change? So now PCIE_MSI_VECTOR becomes unused?
>
> More importantly, this is yet another example of the DW reference
> driver nonsense, where memory is allocated for *MSI*, while the whole
> point of MSIs is that it is a write that doesn't target memory, making
> any form of RAM allocation absolutely pointless.
>
> This silly approach has been cargo-culted for years, and while I
> caught a few in my time, you can't beat copy-paste.
>
> IMO, this patch is only making things worse instead of fixing things.
Probably partly my fault. I think there are two pieces here:
1) allocating the MSI address
2) computing the PCI bus address
I don't know how to do 1), but I do encourage people not to use
virt_to_phys() for 2), since (in general) CPU physical addresses are
not the same as PCI bus addresses.