[PATCH 0/6] MIPS: ralink: fix PCI IO resources

[PATCH 0/6] MIPS: ralink: fix PCI IO resources

[Sergio Paracuellos]

MIPs ralink need a special tratement regarding the way it handles PCI IO
resources. On MIPS I/O ports are memory mapped, so we access them using normal
load/store instructions. MIPS 'plat_mem_setup()' function does a call to
'set_io_port_base(KSEG1)'. There, variable 'mips_io_port_base'
is set then using this address which is a virtual address to which all
ports are being mapped. Ralink I/O space has a mapping of bus address
equal to the window into the mmio space, with an offset of IO start range
cpu address. This means that to have this working we need:
- linux port numbers in the range 0-0xffff.
- pci port numbers in the range 0-0xffff.
- io_offset being zero.

These means at the end to have bus address 0 mapped to IO range cpu address.
We need a way of properly set 'mips_io_port_base' with a virtually mapped
value of the IO cpu address.

This series do the following approach:
1) Revert two bad commit from a previous attempt of make this work [0].
2) Set PCI_IOBASE to mips 'mips_io_port_base'.
3) Allow architecture dependent 'pci_remap_iospace'.
4) Implement 'pci_remap_iospace' for MIPS.
5) Be sure IOBASE address for IO window is set with correct value.

More context about this series appoach in this mail thread [1].

Thanks in advance for your time.

[0]: https://www.spinics.net/lists/kernel/msg4051474.html
[1]: https://lkml.org/lkml/2021/9/22/6

Sergio Paracuellos (6):
  Revert "MIPS: ralink: don't define PC_IOBASE but increase
    IO_SPACE_LIMIT"
  Revert "staging: mt7621-pci: set end limit for 'ioport_resource'"
  MIPS: ralink: set PCI_IOBASE to 'mips_io_port_base'
  PCI: allow architecture specific implementation of pci_remap_iospace()
  MIPS: implement architecture dependent 'pci_remap_iospace()'
  staging: mt7621-pci: properly adjust base address for the IO window

 arch/mips/include/asm/mach-ralink/spaces.h | 4 +++-
 arch/mips/include/asm/pci.h                | 2 ++
 arch/mips/pci/pci-generic.c                | 9 +++++++++
 drivers/pci/pci.c                          | 2 ++
 drivers/staging/mt7621-pci/pci-mt7621.c    | 4 +---
 5 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/6] Revert "MIPS: ralink: don't define PC_IOBASE but increase IO_SPACE_LIMIT"

[Sergio Paracuellos]

This reverts commit 159697474db41732ef3b6c2e8d9395f09d1f659e.

There is no real need to increase IO_SPACE_LIMIT if PCI_IOBASE
is properly set to 'mips_io_port_base'. Hence revert this commit
first before doing anything else.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/include/asm/mach-ralink/spaces.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
index 31a3525213cf..87d085c9ad61 100644
--- a/arch/mips/include/asm/mach-ralink/spaces.h
+++ b/arch/mips/include/asm/mach-ralink/spaces.h
@@ -2,7 +2,9 @@
 #ifndef __ASM_MACH_RALINK_SPACES_H_
 #define __ASM_MACH_RALINK_SPACES_H_
 
-#define IO_SPACE_LIMIT	0x1fffffff
+#define PCI_IOBASE	_AC(0xa0000000, UL)
+#define PCI_IOSIZE	SZ_16M
+#define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
 
 #include <asm/mach-generic/spaces.h>
 #endif
-- 
2.25.1

[PATCH 2/6] Revert "staging: mt7621-pci: set end limit for 'ioport_resource'"

[Sergio Paracuellos]

This reverts commit 50fb34eca2944fd67493717c9fbda125336f1655.

Since IO_SPACE_LIMIT is not really being changed there is no
real need to adjust the ioport_resource end limit.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 86d9c3d122e2..6acfc94a16e7 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -526,8 +526,6 @@ static int mt7621_pci_probe(struct platform_device *pdev)
 	if (!dev->of_node)
 		return -ENODEV;
 
-	ioport_resource.end = IO_SPACE_LIMIT;
-
 	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
 	if (!bridge)
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH 2/6] Revert "staging: mt7621-pci: set end limit for 'ioport_resource'"

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> This reverts commit 50fb34eca2944fd67493717c9fbda125336f1655.
>
> Since IO_SPACE_LIMIT is not really being changed there is no
> real need to adjust the ioport_resource end limit.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 2/6] Revert "staging: mt7621-pci: set end limit for 'ioport_resource'"

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> This reverts commit 50fb34eca2944fd67493717c9fbda125336f1655.
>
> Since IO_SPACE_LIMIT is not really being changed there is no
> real need to adjust the ioport_resource end limit.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 3/6] MIPS: ralink: set PCI_IOBASE to 'mips_io_port_base'

[Sergio Paracuellos]

By default MIPS architecture use function 'set_io_port_base()' to set the
virtual address of the first IO port. This function at the end sets variable
'mips_io_port_base' with the desired address. To align things and allow
to change first IO port location address for PCI, set PCI_IOBASE definition
as 'mips_io_port_base'. Also, we only need a size of 64 KB.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/include/asm/mach-ralink/spaces.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
index 87d085c9ad61..05d14c21c417 100644
--- a/arch/mips/include/asm/mach-ralink/spaces.h
+++ b/arch/mips/include/asm/mach-ralink/spaces.h
@@ -2,8 +2,8 @@
 #ifndef __ASM_MACH_RALINK_SPACES_H_
 #define __ASM_MACH_RALINK_SPACES_H_
 
-#define PCI_IOBASE	_AC(0xa0000000, UL)
-#define PCI_IOSIZE	SZ_16M
+#define PCI_IOBASE	mips_io_port_base
+#define PCI_IOSIZE	SZ_64K
 #define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
 
 #include <asm/mach-generic/spaces.h>
-- 
2.25.1

Re: [PATCH 3/6] MIPS: ralink: set PCI_IOBASE to 'mips_io_port_base'

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> By default MIPS architecture use function 'set_io_port_base()' to set the
> virtual address of the first IO port. This function at the end sets variable
> 'mips_io_port_base' with the desired address. To align things and allow
> to change first IO port location address for PCI, set PCI_IOBASE definition
> as 'mips_io_port_base'. Also, we only need a size of 64 KB.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  arch/mips/include/asm/mach-ralink/spaces.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> index 87d085c9ad61..05d14c21c417 100644
> --- a/arch/mips/include/asm/mach-ralink/spaces.h
> +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> @@ -2,8 +2,8 @@
>  #ifndef __ASM_MACH_RALINK_SPACES_H_
>  #define __ASM_MACH_RALINK_SPACES_H_
>
> -#define PCI_IOBASE     _AC(0xa0000000, UL)
> -#define PCI_IOSIZE     SZ_16M
> +#define PCI_IOBASE     mips_io_port_base
> +#define PCI_IOSIZE     SZ_64K
>  #define IO_SPACE_LIMIT (PCI_IOSIZE - 1)

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 3/6] MIPS: ralink: set PCI_IOBASE to 'mips_io_port_base'

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> By default MIPS architecture use function 'set_io_port_base()' to set the
> virtual address of the first IO port. This function at the end sets variable
> 'mips_io_port_base' with the desired address. To align things and allow
> to change first IO port location address for PCI, set PCI_IOBASE definition
> as 'mips_io_port_base'. Also, we only need a size of 64 KB.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  arch/mips/include/asm/mach-ralink/spaces.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> index 87d085c9ad61..05d14c21c417 100644
> --- a/arch/mips/include/asm/mach-ralink/spaces.h
> +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> @@ -2,8 +2,8 @@
>  #ifndef __ASM_MACH_RALINK_SPACES_H_
>  #define __ASM_MACH_RALINK_SPACES_H_
>
> -#define PCI_IOBASE     _AC(0xa0000000, UL)
> -#define PCI_IOSIZE     SZ_16M
> +#define PCI_IOBASE     mips_io_port_base
> +#define PCI_IOSIZE     SZ_64K
>  #define IO_SPACE_LIMIT (PCI_IOSIZE - 1)

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 4/6] PCI: allow architecture specific implementation of pci_remap_iospace()

[Sergio Paracuellos]

pci_remap_iospace() was originally meant as an architecture specific helper,
but it moved into generic code after all architectures had the same requirements.
MIPS has different requirements so it should not be shared. The way for doing
this will be using a macro 'pci_remap_iospace' defined for those architectures
that need a special treatement. Hence, put core api function inside preprocesor
conditional code for 'pci_remap_iospace' definition.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/pci/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce2ab62b64cf..0ec57bb01a88 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4123,6 +4123,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
  * architectures that have memory mapped IO functions defined (and the
  * PCI_IOBASE value defined) should call this function.
  */
+#ifndef pci_remap_iospace
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
 {
 #if defined(PCI_IOBASE) && defined(CONFIG_MMU)
@@ -4146,6 +4147,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
 #endif
 }
 EXPORT_SYMBOL(pci_remap_iospace);
+#endif
 
 /**
  * pci_unmap_iospace - Unmap the memory mapped I/O space
-- 
2.25.1

Re: [PATCH 4/6] PCI: allow architecture specific implementation of pci_remap_iospace()

[Bjorn Helgaas]

s/PCI: allow architecture specific implementation of/
  PCI: Allow architecture-specific pci_remap_iospace()/

(in subject)

On Fri, Sep 24, 2021 at 11:11:37PM +0200, Sergio Paracuellos wrote:
> pci_remap_iospace() was originally meant as an architecture specific helper,
> but it moved into generic code after all architectures had the same requirements.
> MIPS has different requirements so it should not be shared. The way for doing
> this will be using a macro 'pci_remap_iospace' defined for those architectures
> that need a special treatement. Hence, put core api function inside preprocesor
> conditional code for 'pci_remap_iospace' definition.

Rewrap above to fit in 75 columns so "git log" output doesn't wrap.
Add blank line between paragraphs.

s/treatement/treatment/
s/api/API/

> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

With above fixed,

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ce2ab62b64cf..0ec57bb01a88 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4123,6 +4123,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>   * architectures that have memory mapped IO functions defined (and the
>   * PCI_IOBASE value defined) should call this function.
>   */
> +#ifndef pci_remap_iospace
>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  {
>  #if defined(PCI_IOBASE) && defined(CONFIG_MMU)
> @@ -4146,6 +4147,7 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  #endif
>  }
>  EXPORT_SYMBOL(pci_remap_iospace);
> +#endif
>  
>  /**
>   * pci_unmap_iospace - Unmap the memory mapped I/O space
> -- 
> 2.25.1
> 

Re: [PATCH 4/6] PCI: allow architecture specific implementation of pci_remap_iospace()

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> pci_remap_iospace() was originally meant as an architecture specific helper,
> but it moved into generic code after all architectures had the same requirements.
> MIPS has different requirements so it should not be shared. The way for doing
> this will be using a macro 'pci_remap_iospace' defined for those architectures
> that need a special treatement. Hence, put core api function inside preprocesor
> conditional code for 'pci_remap_iospace' definition.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 4/6] PCI: allow architecture specific implementation of pci_remap_iospace()

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> pci_remap_iospace() was originally meant as an architecture specific helper,
> but it moved into generic code after all architectures had the same requirements.
> MIPS has different requirements so it should not be shared. The way for doing
> this will be using a macro 'pci_remap_iospace' defined for those architectures
> that need a special treatement. Hence, put core api function inside preprocesor
> conditional code for 'pci_remap_iospace' definition.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Sergio Paracuellos]

To make PCI IO work we need to properly virtually map IO cpu physical address
and set this virtual address as the address of the first PCI IO port which
is set using function 'set_io_port_base()'.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/include/asm/pci.h | 2 ++
 arch/mips/pci/pci-generic.c | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/mips/include/asm/pci.h b/arch/mips/include/asm/pci.h
index 9ffc8192adae..35270984a5f0 100644
--- a/arch/mips/include/asm/pci.h
+++ b/arch/mips/include/asm/pci.h
@@ -20,6 +20,8 @@
 #include <linux/list.h>
 #include <linux/of.h>
 
+#define pci_remap_iospace pci_remap_iospace
+
 #ifdef CONFIG_PCI_DRIVERS_LEGACY
 
 /*
diff --git a/arch/mips/pci/pci-generic.c b/arch/mips/pci/pci-generic.c
index 95b00017886c..877ec9d6a614 100644
--- a/arch/mips/pci/pci-generic.c
+++ b/arch/mips/pci/pci-generic.c
@@ -46,3 +46,12 @@ void pcibios_fixup_bus(struct pci_bus *bus)
 {
 	pci_read_bridge_bases(bus);
 }
+
+int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
+{
+	size_t size = (res->end - res->start) + 1;
+	unsigned long vaddr = (unsigned long)ioremap(phys_addr, size);
+
+	set_io_port_base(vaddr);
+	return 0;
+}
-- 
2.25.1

Re: [PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> To make PCI IO work we need to properly virtually map IO cpu physical address
> and set this virtual address as the address of the first PCI IO port which
> is set using function 'set_io_port_base()'.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

> +int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> +{
> +       size_t size = (res->end - res->start) + 1;
> +       unsigned long vaddr = (unsigned long)ioremap(phys_addr, size);
> +
> +       set_io_port_base(vaddr);
> +       return 0;
> +}

It might be good to check that res->start is zero here, otherwise
the io_port_base would be off. That could happen if you ever have more
than one bridge.

        Arnd

Re: [PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> To make PCI IO work we need to properly virtually map IO cpu physical address
> and set this virtual address as the address of the first PCI IO port which
> is set using function 'set_io_port_base()'.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

> +int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> +{
> +       size_t size = (res->end - res->start) + 1;
> +       unsigned long vaddr = (unsigned long)ioremap(phys_addr, size);
> +
> +       set_io_port_base(vaddr);
> +       return 0;
> +}

It might be good to check that res->start is zero here, otherwise
the io_port_base would be off. That could happen if you ever have more
than one bridge.

        Arnd

Re: [PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Sergio Paracuellos]

Hi Arnd,

On Sat, Sep 25, 2021 at 7:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > To make PCI IO work we need to properly virtually map IO cpu physical address
> > and set this virtual address as the address of the first PCI IO port which
> > is set using function 'set_io_port_base()'.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks!

>
> > +int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > +{
> > +       size_t size = (res->end - res->start) + 1;
> > +       unsigned long vaddr = (unsigned long)ioremap(phys_addr, size);
> > +
> > +       set_io_port_base(vaddr);
> > +       return 0;
> > +}
>
> It might be good to check that res->start is zero here, otherwise
> the io_port_base would be off. That could happen if you ever have more
> than one bridge.

Do you mean something like the following?

int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
{
    unsigned long vaddr;
    size_t size;

    if (res->start != 0) {
         // Should I WARN_ONCE or just show an error/warning message??
         WARN_ONCE(1, "resource start must be zero\n");
         return -ENODEV;
   }

     size = (res->end - res->start) + 1;
     vaddr = (unsigned long)ioremap(phys_addr, size);
     return 0;
}

Thanks,
    Sergio Paracuellos
>
>         Arnd

Re: [PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Sergio Paracuellos]

Hi Arnd,

On Sat, Sep 25, 2021 at 7:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > To make PCI IO work we need to properly virtually map IO cpu physical address
> > and set this virtual address as the address of the first PCI IO port which
> > is set using function 'set_io_port_base()'.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks!

>
> > +int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > +{
> > +       size_t size = (res->end - res->start) + 1;
> > +       unsigned long vaddr = (unsigned long)ioremap(phys_addr, size);
> > +
> > +       set_io_port_base(vaddr);
> > +       return 0;
> > +}
>
> It might be good to check that res->start is zero here, otherwise
> the io_port_base would be off. That could happen if you ever have more
> than one bridge.

Do you mean something like the following?

int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
{
    unsigned long vaddr;
    size_t size;

    if (res->start != 0) {
         // Should I WARN_ONCE or just show an error/warning message??
         WARN_ONCE(1, "resource start must be zero\n");
         return -ENODEV;
   }

     size = (res->end - res->start) + 1;
     vaddr = (unsigned long)ioremap(phys_addr, size);
     return 0;
}

Thanks,
    Sergio Paracuellos
>
>         Arnd

Re: [PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Arnd Bergmann]

On Sat, Sep 25, 2021 at 8:10 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> > It might be good to check that res->start is zero here, otherwise
> > the io_port_base would be off. That could happen if you ever have more
> > than one bridge.
>
> Do you mean something like the following?

Yes, exactly.

> int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> {
>     unsigned long vaddr;
>     size_t size;
>
>     if (res->start != 0) {
>          // Should I WARN_ONCE or just show an error/warning message??
>          WARN_ONCE(1, "resource start must be zero\n");
>          return -ENODEV;
>    }

I don't care if it's WARN(), WARN_ONCE() or pr_warn(). If we ever see the
message, the system is not working and the person who caused the problem
will figure it out.

        Arnd

Re: [PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Arnd Bergmann]

On Sat, Sep 25, 2021 at 8:10 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> > It might be good to check that res->start is zero here, otherwise
> > the io_port_base would be off. That could happen if you ever have more
> > than one bridge.
>
> Do you mean something like the following?

Yes, exactly.

> int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> {
>     unsigned long vaddr;
>     size_t size;
>
>     if (res->start != 0) {
>          // Should I WARN_ONCE or just show an error/warning message??
>          WARN_ONCE(1, "resource start must be zero\n");
>          return -ENODEV;
>    }

I don't care if it's WARN(), WARN_ONCE() or pr_warn(). If we ever see the
message, the system is not working and the person who caused the problem
will figure it out.

        Arnd

Re: [PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Sergio Paracuellos]

Hi Arnd,

On Sat, Sep 25, 2021 at 9:34 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, Sep 25, 2021 at 8:10 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > > It might be good to check that res->start is zero here, otherwise
> > > the io_port_base would be off. That could happen if you ever have more
> > > than one bridge.
> >
> > Do you mean something like the following?
>
> Yes, exactly.
>
> > int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > {
> >     unsigned long vaddr;
> >     size_t size;
> >
> >     if (res->start != 0) {
> >          // Should I WARN_ONCE or just show an error/warning message??
> >          WARN_ONCE(1, "resource start must be zero\n");
> >          return -ENODEV;
> >    }
>
> I don't care if it's WARN(), WARN_ONCE() or pr_warn(). If we ever see the
> message, the system is not working and the person who caused the problem
> will figure it out.

Pretty clear, thanks. I will collect you Acked-by's and make this
change and send v3.

Best regards,
    Sergio Paracuellos
>
>         Arnd

Re: [PATCH 5/6] MIPS: implement architecture dependent 'pci_remap_iospace()'

[Sergio Paracuellos]

Hi Arnd,

On Sat, Sep 25, 2021 at 9:34 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, Sep 25, 2021 at 8:10 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > > It might be good to check that res->start is zero here, otherwise
> > > the io_port_base would be off. That could happen if you ever have more
> > > than one bridge.
> >
> > Do you mean something like the following?
>
> Yes, exactly.
>
> > int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > {
> >     unsigned long vaddr;
> >     size_t size;
> >
> >     if (res->start != 0) {
> >          // Should I WARN_ONCE or just show an error/warning message??
> >          WARN_ONCE(1, "resource start must be zero\n");
> >          return -ENODEV;
> >    }
>
> I don't care if it's WARN(), WARN_ONCE() or pr_warn(). If we ever see the
> message, the system is not working and the person who caused the problem
> will figure it out.

Pretty clear, thanks. I will collect you Acked-by's and make this
change and send v3.

Best regards,
    Sergio Paracuellos
>
>         Arnd

[PATCH 6/6] staging: mt7621-pci: properly adjust base address for the IO window

[Sergio Paracuellos]

The value to adjust in the bridge register RALINK_PCI_IOBASE must take into
account the raw value from DT, not only the translated linux port number.
As long as io_offset is zero, the two are the same, but if you were to use
multiple host bridge in the system, or pick a different bus address in DT,
you can have a nonzero io_offset. At this means to take into account the
bus address which is used to calculate this offset, substracting it from
the IO resource start address.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 6acfc94a16e7..503cb1fca2e0 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -482,7 +482,7 @@ static int mt7621_pcie_enable_ports(struct pci_host_bridge *host)
 
 	/* Setup MEMWIN and IOWIN */
 	pcie_write(pcie, 0xffffffff, RALINK_PCI_MEMBASE);
-	pcie_write(pcie, entry->res->start, RALINK_PCI_IOBASE);
+	pcie_write(pcie, entry->res->start - entry->offset, RALINK_PCI_IOBASE);
 
 	list_for_each_entry(port, &pcie->ports, list) {
 		if (port->enabled) {
-- 
2.25.1

Re: [PATCH 6/6] staging: mt7621-pci: properly adjust base address for the IO window

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> The value to adjust in the bridge register RALINK_PCI_IOBASE must take into
> account the raw value from DT, not only the translated linux port number.
> As long as io_offset is zero, the two are the same, but if you were to use
> multiple host bridge in the system, or pick a different bus address in DT,
> you can have a nonzero io_offset. At this means to take into account the
> bus address which is used to calculate this offset, substracting it from
> the IO resource start address.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 6/6] staging: mt7621-pci: properly adjust base address for the IO window

[Arnd Bergmann]

On Fri, Sep 24, 2021 at 11:11 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> The value to adjust in the bridge register RALINK_PCI_IOBASE must take into
> account the raw value from DT, not only the translated linux port number.
> As long as io_offset is zero, the two are the same, but if you were to use
> multiple host bridge in the system, or pick a different bus address in DT,
> you can have a nonzero io_offset. At this means to take into account the
> bus address which is used to calculate this offset, substracting it from
> the IO resource start address.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>