[PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[Jianjun Wang]

Remove the usage of virt_to_phys, as it will cause sparse warnings when
building on some platforms.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 3bcfc4e58ba2..dc1e5fd6c7aa 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;
+	phys_addr_t msg_addr;
 	struct irq_domain *irq_domain;
 	struct irq_domain *inner_domain;
 	struct irq_domain *msi_domain;
@@ -393,12 +395,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;
 
@@ -510,10 +510,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);
@@ -913,6 +911,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
 	struct mtk_pcie_port *port;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
+	struct resource *regs;
 	char name[10];
 	int err;
 
@@ -921,12 +920,18 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
 		return -ENOMEM;
 
 	snprintf(name, sizeof(name), "port%d", slot);
-	port->base = devm_platform_ioremap_resource_byname(pdev, name);
+	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
+	if (!regs)
+		return -EINVAL;
+
+	port->base = devm_ioremap_resource(dev, regs);
 	if (IS_ERR(port->base)) {
 		dev_err(dev, "failed to map port%d base\n", slot);
 		return PTR_ERR(port->base);
 	}
 
+	port->msg_addr = regs->start + PCIE_MSI_VECTOR;
+
 	snprintf(name, sizeof(name), "sys_ck%d", slot);
 	port->sys_ck = devm_clk_get(dev, name);
 	if (IS_ERR(port->sys_ck)) {
-- 
2.46.0

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[Manivannan Sadhasivam]

On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> Remove the usage of virt_to_phys, as it will cause sparse warnings when
> building on some platforms.
> 

Strange. What are those warnings and platforms?

- Mani

> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 3bcfc4e58ba2..dc1e5fd6c7aa 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;
> +	phys_addr_t msg_addr;
>  	struct irq_domain *irq_domain;
>  	struct irq_domain *inner_domain;
>  	struct irq_domain *msi_domain;
> @@ -393,12 +395,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;
>  
> @@ -510,10 +510,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);
> @@ -913,6 +911,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
>  	struct mtk_pcie_port *port;
>  	struct device *dev = pcie->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *regs;
>  	char name[10];
>  	int err;
>  
> @@ -921,12 +920,18 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
>  		return -ENOMEM;
>  
>  	snprintf(name, sizeof(name), "port%d", slot);
> -	port->base = devm_platform_ioremap_resource_byname(pdev, name);
> +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +	if (!regs)
> +		return -EINVAL;
> +
> +	port->base = devm_ioremap_resource(dev, regs);
>  	if (IS_ERR(port->base)) {
>  		dev_err(dev, "failed to map port%d base\n", slot);
>  		return PTR_ERR(port->base);
>  	}
>  
> +	port->msg_addr = regs->start + PCIE_MSI_VECTOR;
> +
>  	snprintf(name, sizeof(name), "sys_ck%d", slot);
>  	port->sys_ck = devm_clk_get(dev, name);
>  	if (IS_ERR(port->sys_ck)) {
> -- 
> 2.46.0
> 

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

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[Jianjun Wang (王建军)]

On Wed, 2025-01-15 at 23:01 +0530, Manivannan Sadhasivam wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> > Remove the usage of virt_to_phys, as it will cause sparse warnings
> > when
> > building on some platforms.
> > 
> 
> Strange. What are those warnings and platforms?

There are some warning messages when building tests with different
configs on different platforms (e.g., allmodconfig.arm,
allmodconfig.i386, allmodconfig.mips, etc.):

pcie-mediatek.c:399:40: sparse: warning: incorrect type in argument 1
(different address spaces)
pcie-mediatek.c:399:40: sparse:    expected void const volatile *x
pcie-mediatek.c:399:40: sparse:    got void [noderef] __iomem *
pcie-mediatek.c:515:44: sparse: warning: incorrect type in argument 1
(different address spaces)
pcie-mediatek.c:515:44: sparse:    expected void const volatile *x
pcie-mediatek.c:515:44: sparse:    got void [noderef] __iomem *

Thanks.

> 
> - Mani
> 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c
> > b/drivers/pci/controller/pcie-mediatek.c
> > index 3bcfc4e58ba2..dc1e5fd6c7aa 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;
> > +     phys_addr_t msg_addr;
> >       struct irq_domain *irq_domain;
> >       struct irq_domain *inner_domain;
> >       struct irq_domain *msi_domain;
> > @@ -393,12 +395,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;
> > 
> > @@ -510,10 +510,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);
> > @@ -913,6 +911,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie
> > *pcie,
> >       struct mtk_pcie_port *port;
> >       struct device *dev = pcie->dev;
> >       struct platform_device *pdev = to_platform_device(dev);
> > +     struct resource *regs;
> >       char name[10];
> >       int err;
> > 
> > @@ -921,12 +920,18 @@ static int mtk_pcie_parse_port(struct
> > mtk_pcie *pcie,
> >               return -ENOMEM;
> > 
> >       snprintf(name, sizeof(name), "port%d", slot);
> > -     port->base = devm_platform_ioremap_resource_byname(pdev,
> > name);
> > +     regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > name);
> > +     if (!regs)
> > +             return -EINVAL;
> > +
> > +     port->base = devm_ioremap_resource(dev, regs);
> >       if (IS_ERR(port->base)) {
> >               dev_err(dev, "failed to map port%d base\n", slot);
> >               return PTR_ERR(port->base);
> >       }
> > 
> > +     port->msg_addr = regs->start + PCIE_MSI_VECTOR;
> > +
> >       snprintf(name, sizeof(name), "sys_ck%d", slot);
> >       port->sys_ck = devm_clk_get(dev, name);
> >       if (IS_ERR(port->sys_ck)) {
> > --
> > 2.46.0
> > 
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[Bjorn Helgaas]

On Thu, Jan 23, 2025 at 08:11:19AM +0000, Jianjun Wang (王建军) wrote:
> On Wed, 2025-01-15 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> > > Remove the usage of virt_to_phys, as it will cause sparse warnings
> > > when
> > > building on some platforms.
> > 
> > Strange. What are those warnings and platforms?
> 
> There are some warning messages when building tests with different
> configs on different platforms (e.g., allmodconfig.arm,
> allmodconfig.i386, allmodconfig.mips, etc.):
> 
> pcie-mediatek.c:399:40: sparse: warning: incorrect type in argument 1
> (different address spaces)
> pcie-mediatek.c:399:40: sparse:    expected void const volatile *x
> pcie-mediatek.c:399:40: sparse:    got void [noderef] __iomem *
> pcie-mediatek.c:515:44: sparse: warning: incorrect type in argument 1
> (different address spaces)
> pcie-mediatek.c:515:44: sparse:    expected void const volatile *x
> pcie-mediatek.c:515:44: sparse:    got void [noderef] __iomem *
>
> > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > > ---
> > >  drivers/pci/controller/pcie-mediatek.c | 19 ++++++++++++-------
> > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-mediatek.c
> > > b/drivers/pci/controller/pcie-mediatek.c
> > > index 3bcfc4e58ba2..dc1e5fd6c7aa 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;
> > > +     phys_addr_t msg_addr;
> > >       struct irq_domain *irq_domain;
> > >       struct irq_domain *inner_domain;
> > >       struct irq_domain *msi_domain;
> > > @@ -393,12 +395,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);

Thanks for working on this!  We should definitely try to get rid of
virt_to_phys().

> > >       msg->address_hi = 0;
> > > -     msg->address_lo = lower_32_bits(addr);
> > > +     msg->address_lo = lower_32_bits(port->msg_addr);
> > > 
> > >       msg->data = data->hwirq;
> > > 
> > > @@ -510,10 +510,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);
> > > @@ -913,6 +911,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie
> > > *pcie,
> > >       struct mtk_pcie_port *port;
> > >       struct device *dev = pcie->dev;
> > >       struct platform_device *pdev = to_platform_device(dev);
> > > +     struct resource *regs;
> > >       char name[10];
> > >       int err;
> > > 
> > > @@ -921,12 +920,18 @@ static int mtk_pcie_parse_port(struct
> > > mtk_pcie *pcie,
> > >               return -ENOMEM;
> > > 
> > >       snprintf(name, sizeof(name), "port%d", slot);
> > > -     port->base = devm_platform_ioremap_resource_byname(pdev,
> > > name);
> > > +     regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > name);
> > > +     if (!regs)
> > > +             return -EINVAL;
> > > +
> > > +     port->base = devm_ioremap_resource(dev, regs);
> > >       if (IS_ERR(port->base)) {
> > >               dev_err(dev, "failed to map port%d base\n", slot);
> > >               return PTR_ERR(port->base);
> > >       }
> > > 
> > > +     port->msg_addr = regs->start + PCIE_MSI_VECTOR;

I think this still assumes that a CPU physical address (regs->start)
is the same as the PCI bus address used for MSI, so this doesn't seem
like the right solution to me.

Apparently they happen to be the same on this platform because (I
assume) MSIs actually do work, but it's not a good pattern for drivers
to copy.  I think what we really need is a dma_addr_t, and I think
there are one or two PCI controller drivers that do that.

> > >       snprintf(name, sizeof(name), "sys_ck%d", slot);
> > >       port->sys_ck = devm_clk_get(dev, name);
> > >       if (IS_ERR(port->sys_ck)) {
> > > --
> > > 2.46.0
> > > 
> > 
> > --
> > மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[manivannan.sadhasivam]

On Mon, Jan 27, 2025 at 06:41:50PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 23, 2025 at 08:11:19AM +0000, Jianjun Wang (王建军) wrote:
> > On Wed, 2025-01-15 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> > > > Remove the usage of virt_to_phys, as it will cause sparse warnings
> > > > when
> > > > building on some platforms.
> > > 
> > > Strange. What are those warnings and platforms?
> > 
> > There are some warning messages when building tests with different
> > configs on different platforms (e.g., allmodconfig.arm,
> > allmodconfig.i386, allmodconfig.mips, etc.):
> > 
> > pcie-mediatek.c:399:40: sparse: warning: incorrect type in argument 1
> > (different address spaces)
> > pcie-mediatek.c:399:40: sparse:    expected void const volatile *x
> > pcie-mediatek.c:399:40: sparse:    got void [noderef] __iomem *
> > pcie-mediatek.c:515:44: sparse: warning: incorrect type in argument 1
> > (different address spaces)
> > pcie-mediatek.c:515:44: sparse:    expected void const volatile *x
> > pcie-mediatek.c:515:44: sparse:    got void [noderef] __iomem *
> >
> > > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > > > ---
> > > >  drivers/pci/controller/pcie-mediatek.c | 19 ++++++++++++-------
> > > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pcie-mediatek.c
> > > > b/drivers/pci/controller/pcie-mediatek.c
> > > > index 3bcfc4e58ba2..dc1e5fd6c7aa 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;
> > > > +     phys_addr_t msg_addr;
> > > >       struct irq_domain *irq_domain;
> > > >       struct irq_domain *inner_domain;
> > > >       struct irq_domain *msi_domain;
> > > > @@ -393,12 +395,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);
> 
> Thanks for working on this!  We should definitely try to get rid of
> virt_to_phys().
> 
> > > >       msg->address_hi = 0;
> > > > -     msg->address_lo = lower_32_bits(addr);
> > > > +     msg->address_lo = lower_32_bits(port->msg_addr);
> > > > 
> > > >       msg->data = data->hwirq;
> > > > 
> > > > @@ -510,10 +510,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);
> > > > @@ -913,6 +911,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie
> > > > *pcie,
> > > >       struct mtk_pcie_port *port;
> > > >       struct device *dev = pcie->dev;
> > > >       struct platform_device *pdev = to_platform_device(dev);
> > > > +     struct resource *regs;
> > > >       char name[10];
> > > >       int err;
> > > > 
> > > > @@ -921,12 +920,18 @@ static int mtk_pcie_parse_port(struct
> > > > mtk_pcie *pcie,
> > > >               return -ENOMEM;
> > > > 
> > > >       snprintf(name, sizeof(name), "port%d", slot);
> > > > -     port->base = devm_platform_ioremap_resource_byname(pdev,
> > > > name);
> > > > +     regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > name);
> > > > +     if (!regs)
> > > > +             return -EINVAL;
> > > > +
> > > > +     port->base = devm_ioremap_resource(dev, regs);
> > > >       if (IS_ERR(port->base)) {
> > > >               dev_err(dev, "failed to map port%d base\n", slot);
> > > >               return PTR_ERR(port->base);
> > > >       }
> > > > 
> > > > +     port->msg_addr = regs->start + PCIE_MSI_VECTOR;
> 
> I think this still assumes that a CPU physical address (regs->start)
> is the same as the PCI bus address used for MSI, so this doesn't seem
> like the right solution to me.
> 
> Apparently they happen to be the same on this platform because (I
> assume) MSIs actually do work, but it's not a good pattern for drivers
> to copy.  I think what we really need is a dma_addr_t, and I think
> there are one or two PCI controller drivers that do that.
> 

I don't see why we would need 'dma_addr_t' here. The MSI address is a static
physical address on this platform and that is not a DMA address. Other drivers
can *only* copy this pattern if they also have the physical address allocated
for MSI.

- Mani

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

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[manivannan.sadhasivam]

On Thu, Jan 23, 2025 at 08:11:19AM +0000, Jianjun Wang (王建军) wrote:
> On Wed, 2025-01-15 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> > > Remove the usage of virt_to_phys, as it will cause sparse warnings
> > > when
> > > building on some platforms.
> > > 
> > 
> > Strange. What are those warnings and platforms?
> 
> There are some warning messages when building tests with different
> configs on different platforms (e.g., allmodconfig.arm,
> allmodconfig.i386, allmodconfig.mips, etc.):
> 
> pcie-mediatek.c:399:40: sparse: warning: incorrect type in argument 1
> (different address spaces)
> pcie-mediatek.c:399:40: sparse:    expected void const volatile *x
> pcie-mediatek.c:399:40: sparse:    got void [noderef] __iomem *
> pcie-mediatek.c:515:44: sparse: warning: incorrect type in argument 1
> (different address spaces)
> pcie-mediatek.c:515:44: sparse:    expected void const volatile *x
> pcie-mediatek.c:515:44: sparse:    got void [noderef] __iomem *
> 

This need to be added to the patch description.

- Mani

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

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[Bjorn Helgaas]

On Sat, Feb 01, 2025 at 09:54:16PM +0530, manivannan.sadhasivam@linaro.org wrote:
> On Mon, Jan 27, 2025 at 06:41:50PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 23, 2025 at 08:11:19AM +0000, Jianjun Wang (王建军) wrote:
> > > On Wed, 2025-01-15 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > > > On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> > > > > Remove the usage of virt_to_phys, as it will cause sparse warnings
> > > > > when
> > > > > building on some platforms.

> > > > >       snprintf(name, sizeof(name), "port%d", slot);
> > > > > -     port->base = devm_platform_ioremap_resource_byname(pdev,
> > > > > name);
> > > > > +     regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > name);
> > > > > +     if (!regs)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     port->base = devm_ioremap_resource(dev, regs);
> > > > >       if (IS_ERR(port->base)) {
> > > > >               dev_err(dev, "failed to map port%d base\n", slot);
> > > > >               return PTR_ERR(port->base);
> > > > >       }
> > > > > 
> > > > > +     port->msg_addr = regs->start + PCIE_MSI_VECTOR;
> > 
> > I think this still assumes that a CPU physical address
> > (regs->start) is the same as the PCI bus address used for MSI, so
> > this doesn't seem like the right solution to me.
> > 
> > Apparently they happen to be the same on this platform because (I
> > assume) MSIs actually do work, but it's not a good pattern for
> > drivers to copy.  I think what we really need is a dma_addr_t, and
> > I think there are one or two PCI controller drivers that do that.
> 
> I don't see why we would need 'dma_addr_t' here. The MSI address is
> a static physical address on this platform and that is not a DMA
> address. Other drivers can *only* copy this pattern if they also
> have the physical address allocated for MSI.

Isn't an MSI on PCI just a DMA write to a certain address?  My
assumption is that if you put an analyzer on that link, an MSI from a
device would be structurally indistinguishable from a DMA write from
the device.  The MSI uses a different address, but doesn't it use the
same size and kind of address, at least from the PCIe protocol
perspective?

Bjorn

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[manivannan.sadhasivam]

On Sat, Feb 01, 2025 at 11:07:40AM -0600, Bjorn Helgaas wrote:
> On Sat, Feb 01, 2025 at 09:54:16PM +0530, manivannan.sadhasivam@linaro.org wrote:
> > On Mon, Jan 27, 2025 at 06:41:50PM -0600, Bjorn Helgaas wrote:
> > > On Thu, Jan 23, 2025 at 08:11:19AM +0000, Jianjun Wang (王建军) wrote:
> > > > On Wed, 2025-01-15 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > > > > On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> > > > > > Remove the usage of virt_to_phys, as it will cause sparse warnings
> > > > > > when
> > > > > > building on some platforms.
> 
> > > > > >       snprintf(name, sizeof(name), "port%d", slot);
> > > > > > -     port->base = devm_platform_ioremap_resource_byname(pdev,
> > > > > > name);
> > > > > > +     regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > > name);
> > > > > > +     if (!regs)
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     port->base = devm_ioremap_resource(dev, regs);
> > > > > >       if (IS_ERR(port->base)) {
> > > > > >               dev_err(dev, "failed to map port%d base\n", slot);
> > > > > >               return PTR_ERR(port->base);
> > > > > >       }
> > > > > > 
> > > > > > +     port->msg_addr = regs->start + PCIE_MSI_VECTOR;
> > > 
> > > I think this still assumes that a CPU physical address
> > > (regs->start) is the same as the PCI bus address used for MSI, so
> > > this doesn't seem like the right solution to me.
> > > 
> > > Apparently they happen to be the same on this platform because (I
> > > assume) MSIs actually do work, but it's not a good pattern for
> > > drivers to copy.  I think what we really need is a dma_addr_t, and
> > > I think there are one or two PCI controller drivers that do that.
> > 
> > I don't see why we would need 'dma_addr_t' here. The MSI address is
> > a static physical address on this platform and that is not a DMA
> > address. Other drivers can *only* copy this pattern if they also
> > have the physical address allocated for MSI.
> 
> Isn't an MSI on PCI just a DMA write to a certain address?

That's from the endpoint prespective when it triggers MSI.

> My
> assumption is that if you put an analyzer on that link, an MSI from a
> device would be structurally indistinguishable from a DMA write from
> the device.  The MSI uses a different address, but doesn't it use the
> same size and kind of address, at least from the PCIe protocol
> perspective?
> 

Yeah, but in this case the address allocated to MSI belongs to a hardcoded
region in the host memory (not allocated by the DMA APIs which will have the
region attributed as DMA capable). So it doesn't belong to the DMA domain, and
we cannot use 'dma_addr_t'.

- Mani

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

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[Bjorn Helgaas]

On Mon, Feb 03, 2025 at 11:20:49PM +0530, manivannan.sadhasivam@linaro.org wrote:
> On Sat, Feb 01, 2025 at 11:07:40AM -0600, Bjorn Helgaas wrote:
> > On Sat, Feb 01, 2025 at 09:54:16PM +0530, manivannan.sadhasivam@linaro.org wrote:
> > > On Mon, Jan 27, 2025 at 06:41:50PM -0600, Bjorn Helgaas wrote:
> > > > On Thu, Jan 23, 2025 at 08:11:19AM +0000, Jianjun Wang (王建军) wrote:
> > > > > On Wed, 2025-01-15 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > > > > > On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> > > > > > > Remove the usage of virt_to_phys, as it will cause sparse warnings
> > > > > > > when
> > > > > > > building on some platforms.
> > 
> > > > > > >       snprintf(name, sizeof(name), "port%d", slot);
> > > > > > > -     port->base = devm_platform_ioremap_resource_byname(pdev,
> > > > > > > name);
> > > > > > > +     regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > > > name);
> > > > > > > +     if (!regs)
> > > > > > > +             return -EINVAL;
> > > > > > > +
> > > > > > > +     port->base = devm_ioremap_resource(dev, regs);
> > > > > > >       if (IS_ERR(port->base)) {
> > > > > > >               dev_err(dev, "failed to map port%d base\n", slot);
> > > > > > >               return PTR_ERR(port->base);
> > > > > > >       }
> > > > > > > 
> > > > > > > +     port->msg_addr = regs->start + PCIE_MSI_VECTOR;
> > > > 
> > > > I think this still assumes that a CPU physical address
> > > > (regs->start) is the same as the PCI bus address used for MSI, so
> > > > this doesn't seem like the right solution to me.

Apart from the question of what type should be used, what do you think
about this part?  I don't think we should assume that the address on
PCI is identical to the CPU physical address.  IOMMUs and (I assume)
iATUs can make them different, can't they?  If so, this looks like an
implicit assumption that PCI bus==CPU physical, and I think we should
make that a little more explicit somehow.

> > > > Apparently they happen to be the same on this platform because (I
> > > > assume) MSIs actually do work, but it's not a good pattern for
> > > > drivers to copy.  I think what we really need is a dma_addr_t, and
> > > > I think there are one or two PCI controller drivers that do that.
> > > 
> > > I don't see why we would need 'dma_addr_t' here. The MSI address is
> > > a static physical address on this platform and that is not a DMA
> > > address. Other drivers can *only* copy this pattern if they also
> > > have the physical address allocated for MSI.
> > 
> > Isn't an MSI on PCI just a DMA write to a certain address?
> 
> That's from the endpoint prespective when it triggers MSI.
> 
> > My assumption is that if you put an analyzer on that link, an MSI
> > from a device would be structurally indistinguishable from a DMA
> > write from the device.  The MSI uses a different address, but
> > doesn't it use the same size and kind of address, at least from
> > the PCIe protocol perspective?
> 
> Yeah, but in this case the address allocated to MSI belongs to a
> hardcoded region in the host memory (not allocated by the DMA APIs
> which will have the region attributed as DMA capable). So it doesn't
> belong to the DMA domain, and we cannot use 'dma_addr_t'.

Doesn't .irq_compose_msi_msg() build the Message Address/Data pair
that is programmed into a device's MSI Capability or MSI-X Table?
The device will eventually use that to initiate a DMA write to that
address.

In that sense, I would argue that the Message Address does belong to
the DMA domain.  I don't think the size of the address (32 vs 64 bits)
is determined by the CPU physical address size (phys_addr_t); it's
determined by the size of DMA addresses.

Bjorn

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[manivannan.sadhasivam]

On Mon, Feb 03, 2025 at 12:52:46PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 03, 2025 at 11:20:49PM +0530, manivannan.sadhasivam@linaro.org wrote:
> > On Sat, Feb 01, 2025 at 11:07:40AM -0600, Bjorn Helgaas wrote:
> > > On Sat, Feb 01, 2025 at 09:54:16PM +0530, manivannan.sadhasivam@linaro.org wrote:
> > > > On Mon, Jan 27, 2025 at 06:41:50PM -0600, Bjorn Helgaas wrote:
> > > > > On Thu, Jan 23, 2025 at 08:11:19AM +0000, Jianjun Wang (王建军) wrote:
> > > > > > On Wed, 2025-01-15 at 23:01 +0530, Manivannan Sadhasivam wrote:
> > > > > > > On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> > > > > > > > Remove the usage of virt_to_phys, as it will cause sparse warnings
> > > > > > > > when
> > > > > > > > building on some platforms.
> > > 
> > > > > > > >       snprintf(name, sizeof(name), "port%d", slot);
> > > > > > > > -     port->base = devm_platform_ioremap_resource_byname(pdev,
> > > > > > > > name);
> > > > > > > > +     regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > > > > > name);
> > > > > > > > +     if (!regs)
> > > > > > > > +             return -EINVAL;
> > > > > > > > +
> > > > > > > > +     port->base = devm_ioremap_resource(dev, regs);
> > > > > > > >       if (IS_ERR(port->base)) {
> > > > > > > >               dev_err(dev, "failed to map port%d base\n", slot);
> > > > > > > >               return PTR_ERR(port->base);
> > > > > > > >       }
> > > > > > > > 
> > > > > > > > +     port->msg_addr = regs->start + PCIE_MSI_VECTOR;
> > > > > 
> > > > > I think this still assumes that a CPU physical address
> > > > > (regs->start) is the same as the PCI bus address used for MSI, so
> > > > > this doesn't seem like the right solution to me.
> 
> Apart from the question of what type should be used, what do you think
> about this part?  I don't think we should assume that the address on
> PCI is identical to the CPU physical address.  IOMMUs and (I assume)
> iATUs can make them different, can't they?

IOMMU/iATU only gets into play if the address gets mapped and translated. If you
look at the patch, even though it maps the physical address to 'port->base', the
MSI address 'port->msg_addr' is actually populated with 'regs->start +
PCIE_MSI_VECTOR' which corresponds to the physical address without any
translation or mapping.

>  If so, this looks like an
> implicit assumption that PCI bus==CPU physical, and I think we should
> make that a little more explicit somehow.
> 

In this specific case, MSI address == Physical address.

> > > > > Apparently they happen to be the same on this platform because (I
> > > > > assume) MSIs actually do work, but it's not a good pattern for
> > > > > drivers to copy.  I think what we really need is a dma_addr_t, and
> > > > > I think there are one or two PCI controller drivers that do that.
> > > > 
> > > > I don't see why we would need 'dma_addr_t' here. The MSI address is
> > > > a static physical address on this platform and that is not a DMA
> > > > address. Other drivers can *only* copy this pattern if they also
> > > > have the physical address allocated for MSI.
> > > 
> > > Isn't an MSI on PCI just a DMA write to a certain address?
> > 
> > That's from the endpoint prespective when it triggers MSI.
> > 
> > > My assumption is that if you put an analyzer on that link, an MSI
> > > from a device would be structurally indistinguishable from a DMA
> > > write from the device.  The MSI uses a different address, but
> > > doesn't it use the same size and kind of address, at least from
> > > the PCIe protocol perspective?
> > 
> > Yeah, but in this case the address allocated to MSI belongs to a
> > hardcoded region in the host memory (not allocated by the DMA APIs
> > which will have the region attributed as DMA capable). So it doesn't
> > belong to the DMA domain, and we cannot use 'dma_addr_t'.
> 
> Doesn't .irq_compose_msi_msg() build the Message Address/Data pair
> that is programmed into a device's MSI Capability or MSI-X Table?
> The device will eventually use that to initiate a DMA write to that
> address.
> 

The device can indeed initiate the DMA transaction on this MSI address, but that
doesn't mean that the host will also use DMA to handle it. Given that the
physical address is used directly for MSI, the host might do non-DMA access
while receiving MSIs.

To clarify, if the device initiates a MSI by sending the MWr TLP to the host,
the host RC will intercept it and will signal the host CPU of the interrupt.
In this process, there is no need for the host to use DMA. DMA would certainly
make sense if the endpoint is issuing MWr/MRd TLPs to write/read to the host
DDR.

- Mani

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

Re: [PATCH] PCI: mediatek: Remove the usage of virt_to_phys

[Manivannan Sadhasivam]

On Tue, Jan 07, 2025 at 01:20:58PM +0800, Jianjun Wang wrote:
> Remove the usage of virt_to_phys, as it will cause sparse warnings when
> building on some platforms.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>

I'm changing the status of this patch as 'Changes Requested' in patchwork. Feel
free to submit the next iteration incorporating review comments.

- Mani

> ---
>  drivers/pci/controller/pcie-mediatek.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 3bcfc4e58ba2..dc1e5fd6c7aa 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;
> +	phys_addr_t msg_addr;
>  	struct irq_domain *irq_domain;
>  	struct irq_domain *inner_domain;
>  	struct irq_domain *msi_domain;
> @@ -393,12 +395,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;
>  
> @@ -510,10 +510,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);
> @@ -913,6 +911,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
>  	struct mtk_pcie_port *port;
>  	struct device *dev = pcie->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *regs;
>  	char name[10];
>  	int err;
>  
> @@ -921,12 +920,18 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
>  		return -ENOMEM;
>  
>  	snprintf(name, sizeof(name), "port%d", slot);
> -	port->base = devm_platform_ioremap_resource_byname(pdev, name);
> +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +	if (!regs)
> +		return -EINVAL;
> +
> +	port->base = devm_ioremap_resource(dev, regs);
>  	if (IS_ERR(port->base)) {
>  		dev_err(dev, "failed to map port%d base\n", slot);
>  		return PTR_ERR(port->base);
>  	}
>  
> +	port->msg_addr = regs->start + PCIE_MSI_VECTOR;
> +
>  	snprintf(name, sizeof(name), "sys_ck%d", slot);
>  	port->sys_ck = devm_clk_get(dev, name);
>  	if (IS_ERR(port->sys_ck)) {
> -- 
> 2.46.0
> 

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