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

[PATCH v3 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].

Patches related with reverts are from this merge cycle so they are only
added to the staging git tree. So to have all stuff together I'd like to
get everybody Ack's to get all of this series through staging tree if
possible :).

Thanks in advance for your time.

Changes in v3:
 - Collect Arnd's Acked-by for the patches. 
 - Be sure IO resource start address is zero and WARN_ONCE if it is not
   on MIPS pci_remap_iospace() patch. Also make use of 'resource_size'
   instead of do the logic explicitly again.

Changes in v2:
 - re-do commit messages for PCI patch as Bjorn pointed out in previous series.
 - Add Bjorn's Acked-by for PCI subsystem patch.
 - Re-do commit message of MIPS 'pci_remap_iospace()' patch to align with changes
   in the PCI patch (s/architecture dependent/architecture-specific/)
 - Add Fixes-by tag for MIPS set PCI_IOBASE patch.

[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 pci_remap_iospace()
  MIPS: implement architecture-specific '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                | 14 ++++++++++++++
 drivers/pci/pci.c                          |  2 ++
 drivers/staging/mt7621-pci/pci-mt7621.c    |  4 +---
 5 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v3 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 v3 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.

Acked-by: Arnd Bergmann <arnd@arndb.de>
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

[PATCH v3 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'.

Fixes: 222b27713d7f ("MIPS: ralink: Define PCI_IOBASE")
Acked-by: Arnd Bergmann <arnd@arndb.de>
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

[PATCH v3 4/6] PCI: Allow architecture-specific 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 treatment. Hence, put core API
function inside preprocesor conditional code for 'pci_remap_iospace'
definition.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
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

[PATCH v3 5/6] MIPS: implement architecture-specific '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()'.

Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/include/asm/pci.h |  2 ++
 arch/mips/pci/pci-generic.c | 14 ++++++++++++++
 2 files changed, 16 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..18eb8a453a86 100644
--- a/arch/mips/pci/pci-generic.c
+++ b/arch/mips/pci/pci-generic.c
@@ -46,3 +46,17 @@ 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)
+{
+	unsigned long vaddr;
+
+	if (res->start != 0) {
+		WARN_ONCE(1, "resource start address is not zero\n");
+		return -ENODEV;
+	}
+
+	vaddr = (unsigned long)ioremap(phys_addr, resource_size(res));
+	set_io_port_base(vaddr);
+	return 0;
+}
-- 
2.25.1

[PATCH v3 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.

Acked-by: Arnd Bergmann <arnd@arndb.de>
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 v3 0/6] MIPS: ralink: fix PCI IO resources

[Arnd Bergmann]

On Sat, Sep 25, 2021 at 10:33 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Patches related with reverts are from this merge cycle so they are only
> added to the staging git tree. So to have all stuff together I'd like to
> get everybody Ack's to get all of this series through staging tree if
> possible :).
>
> Thanks in advance for your time.

Looks all good to me now, just one general remark: Try to give a little more
time between respinning the entire series, otherwise you get the opposite
effect and reviewers start ignoring your emails after getting annoyed at the
number of emails. Once you are reasonably sure that no more comments are
coming in, or you have made substantial changes, it's time to resend the
series.

        Arnd

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

[Sergio Paracuellos]

Hi Arnd,

On Mon, Sep 27, 2021 at 9:51 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sat, Sep 25, 2021 at 10:33 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Patches related with reverts are from this merge cycle so they are only
> > added to the staging git tree. So to have all stuff together I'd like to
> > get everybody Ack's to get all of this series through staging tree if
> > possible :).
> >
> > Thanks in advance for your time.
>
> Looks all good to me now, just one general remark: Try to give a little more
> time between respinning the entire series, otherwise you get the opposite
> effect and reviewers start ignoring your emails after getting annoyed at the
> number of emails. Once you are reasonably sure that no more comments are
> coming in, or you have made substantial changes, it's time to resend the
> series.

Thanks for reviewing this and also for the advice. Will take it into
account from now on.

>
>         Arnd

Best regards,
    Sergio Paracuellos

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

[Thomas Bogendoerfer]

On Sat, Sep 25, 2021 at 10:32:19PM +0200, Sergio Paracuellos wrote:
> 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

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

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

[Thomas Bogendoerfer]

On Sat, Sep 25, 2021 at 10:32:21PM +0200, Sergio Paracuellos 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'.
> 
> Fixes: 222b27713d7f ("MIPS: ralink: Define PCI_IOBASE")
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 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>

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Thomas Bogendoerfer]

On Sat, Sep 25, 2021 at 10:32:23PM +0200, Sergio Paracuellos 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()'.
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  arch/mips/include/asm/pci.h |  2 ++
>  arch/mips/pci/pci-generic.c | 14 ++++++++++++++
>  2 files changed, 16 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..18eb8a453a86 100644
> --- a/arch/mips/pci/pci-generic.c
> +++ b/arch/mips/pci/pci-generic.c
> @@ -46,3 +46,17 @@ 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)
> +{
> +	unsigned long vaddr;
> +
> +	if (res->start != 0) {
> +		WARN_ONCE(1, "resource start address is not zero\n");
> +		return -ENODEV;
> +	}
> +
> +	vaddr = (unsigned long)ioremap(phys_addr, resource_size(res));
> +	set_io_port_base(vaddr);
> +	return 0;
> +}

Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

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

[Sergio Paracuellos]

Hi Greg,

On Sat, Sep 25, 2021 at 10:32 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> 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].
>
> Patches related with reverts are from this merge cycle so they are only
> added to the staging git tree. So to have all stuff together I'd like to
> get everybody Ack's to get all of this series through staging tree if
> possible :).
>
> Thanks in advance for your time.
>
> Changes in v3:
>  - Collect Arnd's Acked-by for the patches.
>  - Be sure IO resource start address is zero and WARN_ONCE if it is not
>    on MIPS pci_remap_iospace() patch. Also make use of 'resource_size'
>    instead of do the logic explicitly again.

I think nothing is missing to get this added through the staging tree.

Thanks in advance for your time.

Best regards,
   Sergio Paracuellos

>
> Changes in v2:
>  - re-do commit messages for PCI patch as Bjorn pointed out in previous series.
>  - Add Bjorn's Acked-by for PCI subsystem patch.
>  - Re-do commit message of MIPS 'pci_remap_iospace()' patch to align with changes
>    in the PCI patch (s/architecture dependent/architecture-specific/)
>  - Add Fixes-by tag for MIPS set PCI_IOBASE patch.
>
> [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 pci_remap_iospace()
>   MIPS: implement architecture-specific '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                | 14 ++++++++++++++
>  drivers/pci/pci.c                          |  2 ++
>  drivers/staging/mt7621-pci/pci-mt7621.c    |  4 +---
>  5 files changed, 22 insertions(+), 4 deletions(-)
>
> --
> 2.25.1
>

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

[Greg KH]

On Sun, Oct 03, 2021 at 06:21:21PM +0200, Sergio Paracuellos wrote:
> Hi Greg,
> 
> On Sat, Sep 25, 2021 at 10:32 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > 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].
> >
> > Patches related with reverts are from this merge cycle so they are only
> > added to the staging git tree. So to have all stuff together I'd like to
> > get everybody Ack's to get all of this series through staging tree if
> > possible :).
> >
> > Thanks in advance for your time.
> >
> > Changes in v3:
> >  - Collect Arnd's Acked-by for the patches.
> >  - Be sure IO resource start address is zero and WARN_ONCE if it is not
> >    on MIPS pci_remap_iospace() patch. Also make use of 'resource_size'
> >    instead of do the logic explicitly again.
> 
> I think nothing is missing to get this added through the staging tree.

Great, thanks for sticking with this, will go queue it up now.

greg k-h

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

[Sergio Paracuellos]

On Tue, Oct 5, 2021 at 12:36 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Oct 03, 2021 at 06:21:21PM +0200, Sergio Paracuellos wrote:
> > Hi Greg,
> >
> > On Sat, Sep 25, 2021 at 10:32 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > >
> > > 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].
> > >
> > > Patches related with reverts are from this merge cycle so they are only
> > > added to the staging git tree. So to have all stuff together I'd like to
> > > get everybody Ack's to get all of this series through staging tree if
> > > possible :).
> > >
> > > Thanks in advance for your time.
> > >
> > > Changes in v3:
> > >  - Collect Arnd's Acked-by for the patches.
> > >  - Be sure IO resource start address is zero and WARN_ONCE if it is not
> > >    on MIPS pci_remap_iospace() patch. Also make use of 'resource_size'
> > >    instead of do the logic explicitly again.
> >
> > I think nothing is missing to get this added through the staging tree.
>
> Great, thanks for sticking with this, will go queue it up now.

Thanks!

Best regards,
    Sergio Paracuellos

>
> greg k-h

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Xi Ruoyao]

On Sat, 2021-09-25 at 22:32 +0200, Sergio Paracuellos 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()'.
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Hi,

the change is causing a WARNING on loongson64g-4core-ls7a:

[    0.105781] loongson-pci 1a000000.pci:       IO 0x0018020000..0x001803ffff ->
 0x0000020000
[    0.105792] loongson-pci 1a000000.pci:      MEM 0x0040000000..0x007fffffff ->
 0x0040000000
[    0.105801] ------------[ cut here ]------------
[    0.105804] WARNING: CPU: 0 PID: 1 at arch/mips/pci/pci-generic.c:55 pci_remap_iospace+0x80/0x88
[    0.105815] resource start address is not zero

I'm not sure how to fix this one.

> ---
>  arch/mips/include/asm/pci.h |  2 ++
>  arch/mips/pci/pci-generic.c | 14 ++++++++++++++
>  2 files changed, 16 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..18eb8a453a86 100644
> --- a/arch/mips/pci/pci-generic.c
> +++ b/arch/mips/pci/pci-generic.c
> @@ -46,3 +46,17 @@ 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)
> +{
> +       unsigned long vaddr;
> +
> +       if (res->start != 0) {
> +               WARN_ONCE(1, "resource start address is not zero\n");
> +               return -ENODEV;
> +       }
> +
> +       vaddr = (unsigned long)ioremap(phys_addr, resource_size(res));
> +       set_io_port_base(vaddr);
> +       return 0;
> +}

-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Tiezhu Yang]

On 12/16/2021 07:44 PM, Xi Ruoyao wrote:
> On Sat, 2021-09-25 at 22:32 +0200, Sergio Paracuellos 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()'.
>>
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> Hi,
>
> the change is causing a WARNING on loongson64g-4core-ls7a:
>
> [    0.105781] loongson-pci 1a000000.pci:       IO 0x0018020000..0x001803ffff ->
>  0x0000020000
> [    0.105792] loongson-pci 1a000000.pci:      MEM 0x0040000000..0x007fffffff ->
>  0x0040000000
> [    0.105801] ------------[ cut here ]------------
> [    0.105804] WARNING: CPU: 0 PID: 1 at arch/mips/pci/pci-generic.c:55 pci_remap_iospace+0x80/0x88
> [    0.105815] resource start address is not zero
>
> I'm not sure how to fix this one.
>

MIPS: Only define pci_remap_iospace() for Ralink

https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/commit/?h=mips-fixes&id=09d97da660ff77df20984496aa0abcd6b88819f2

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Arnd Bergmann]

On Thu, Dec 16, 2021 at 12:44 PM Xi Ruoyao <xry111@mengyan1223.wang> wrote:
>
> On Sat, 2021-09-25 at 22:32 +0200, Sergio Paracuellos 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()'.
> >
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> Hi,
>
> the change is causing a WARNING on loongson64g-4core-ls7a:
>
> [    0.105781] loongson-pci 1a000000.pci:       IO 0x0018020000..0x001803ffff ->
>  0x0000020000
> [    0.105792] loongson-pci 1a000000.pci:      MEM 0x0040000000..0x007fffffff ->
>  0x0040000000
> [    0.105801] ------------[ cut here ]------------
> [    0.105804] WARNING: CPU: 0 PID: 1 at arch/mips/pci/pci-generic.c:55 pci_remap_iospace+0x80/0x88
> [    0.105815] resource start address is not zero
>
> I'm not sure how to fix this one.

It looks like this machine has two I/O spaces, one for ISA at 0x18000000/0x00000
and one for PCI at 0x18020000/0x20000, but the implementation assumes there
is only one. If you want to use pci_remap_iospace() on this platform,
it needs to
be extended to allow more than one such space.

       Arnd

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Jiaxun Yang]

在2021年12月16日十二月 上午11:44，Xi Ruoyao写道：
> On Sat, 2021-09-25 at 22:32 +0200, Sergio Paracuellos 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()'.
>> 
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> Hi,
>
> the change is causing a WARNING on loongson64g-4core-ls7a:
>
> [    0.105781] loongson-pci 1a000000.pci:       IO 
> 0x0018020000..0x001803ffff ->
>  0x0000020000
> [    0.105792] loongson-pci 1a000000.pci:      MEM 
> 0x0040000000..0x007fffffff ->
>  0x0040000000
> [    0.105801] ------------[ cut here ]------------
> [    0.105804] WARNING: CPU: 0 PID: 1 at arch/mips/pci/pci-generic.c:55 
> pci_remap_iospace+0x80/0x88
> [    0.105815] resource start address is not zero
>
> I'm not sure how to fix this one.
>
>> ---
>>  arch/mips/include/asm/pci.h |  2 ++
>>  arch/mips/pci/pci-generic.c | 14 ++++++++++++++
>>  2 files changed, 16 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..18eb8a453a86 100644
>> --- a/arch/mips/pci/pci-generic.c
>> +++ b/arch/mips/pci/pci-generic.c
>> @@ -46,3 +46,17 @@ 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)
>> +{
>> +       unsigned long vaddr;
>> +
>> +       if (res->start != 0) {
>> +               WARN_ONCE(1, "resource start address is not zero\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       vaddr = (unsigned long)ioremap(phys_addr, resource_size(res));
>> +       set_io_port_base(vaddr);
>> +       return 0;
>> +}

Hi all,

Another way could be keeping a linked list about PIO->PHYS mapping instead of using the single io_port_base variable.

Thanks.

>
> -- 
> Xi Ruoyao <xry111@mengyan1223.wang>
> School of Aerospace Science and Technology, Xidian University

-- 
- Jiaxun

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Arnd Bergmann]

On Thu, Dec 16, 2021 at 2:07 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 在2021年12月16日十二月 上午11:44，Xi Ruoyao写道：

> Another way could be keeping a linked list about PIO->PHYS mapping instead of using the single io_port_base variable.

I think that would add a lot of complexity that isn't needed here. Not
sure if all MIPS CPUs
can do it, but the approach used on Arm is what fits in best with the
PCI drivers, these
reserve a virtual address range for the ports, and ioremap the
physical addresses into
the PIO range according to the mapping.

For the loongson case specifically, that's not even needed though, as
the two buses
have physically contiguous I/O port ranges, the code just needs to
detect this special
case.

        Arnd

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Jiaxun Yang]

在 2021/12/16 13:50, Arnd Bergmann 写道:
> On Thu, Dec 16, 2021 at 2:07 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>> 在2021年12月16日十二月 上午11:44，Xi Ruoyao写道：
>> Another way could be keeping a linked list about PIO->PHYS mapping instead of using the single io_port_base variable.
> I think that would add a lot of complexity that isn't needed here. Not
> sure if all MIPS CPUs
> can do it, but the approach used on Arm is what fits in best with the
> PCI drivers, these
> reserve a virtual address range for the ports, and ioremap the
> physical addresses into
> the PIO range according to the mapping.

Yes, the Arm way was my previous approach when introducing PCI IO map
for Loongson.

It got refactored by this patch as TLB entries are expensive on MIPS,
also the size of IO range doesn't always fits a page.

>
> For the loongson case specifically, that's not even needed though, as
> the two buses
> have physically contiguous I/O port ranges, the code just needs to
> detect this special
> case.

We have MIPS Boston board (from imgtec) which has discontinuous IO
range.

Thanks.

>
>          Arnd
- Jiaxun

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Arnd Bergmann]

On Thu, Dec 16, 2021 at 3:14 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 在 2021/12/16 13:50, Arnd Bergmann 写道:
> > On Thu, Dec 16, 2021 at 2:07 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> >> 在2021年12月16日十二月 上午11:44，Xi Ruoyao写道：
> >> Another way could be keeping a linked list about PIO->PHYS mapping instead of using the single io_port_base variable.
> > I think that would add a lot of complexity that isn't needed here. Not
> > sure if all MIPS CPUs
> > can do it, but the approach used on Arm is what fits in best with the
> > PCI drivers, these
> > reserve a virtual address range for the ports, and ioremap the
> > physical addresses into
> > the PIO range according to the mapping.
>
> Yes, the Arm way was my previous approach when introducing PCI IO map
> for Loongson.
>
> It got refactored by this patch as TLB entries are expensive on MIPS,
> also the size of IO range doesn't always fits a page.

Are PIO accesses common enough that the TLB entry makes a difference?
I would imagine that on most systems with a PCI bus, there is not even
a single device that exposes an I/O resource, and even on those devices that
do, the kernel drivers tend to pick MMIO whenever both are available.

      Arnd

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Jiaxun Yang]

在 2021/12/16 14:18, Arnd Bergmann 写道:
> On Thu, Dec 16, 2021 at 3:14 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>> 在 2021/12/16 13:50, Arnd Bergmann 写道:
>>> On Thu, Dec 16, 2021 at 2:07 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>>>> 在2021年12月16日十二月 上午11:44，Xi Ruoyao写道：
>>>> Another way could be keeping a linked list about PIO->PHYS mapping instead of using the single io_port_base variable.
>>> I think that would add a lot of complexity that isn't needed here. Not
>>> sure if all MIPS CPUs
>>> can do it, but the approach used on Arm is what fits in best with the
>>> PCI drivers, these
>>> reserve a virtual address range for the ports, and ioremap the
>>> physical addresses into
>>> the PIO range according to the mapping.
>> Yes, the Arm way was my previous approach when introducing PCI IO map
>> for Loongson.
>>
>> It got refactored by this patch as TLB entries are expensive on MIPS,
>> also the size of IO range doesn't always fits a page.
> Are PIO accesses common enough that the TLB entry makes a difference?
> I would imagine that on most systems with a PCI bus, there is not even
> a single device that exposes an I/O resource, and even on those devices that
> do, the kernel drivers tend to pick MMIO whenever both are available.

Actually that was claimed by the author of this patch :-)
I can understand the point. As he is working on a ramips system utlizes 
1004Kec,
which has only 32 TLB entries, saving a entry can give considerable 
improvement.

For Loongson as we have legacy i8042/i8259 which can only be accessed via
PIO, the access is very common.

For other systems I guess it's not that common.

Thanks.


>
>        Arnd
- Jiaxun

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Arnd Bergmann]

On Thu, Dec 16, 2021 at 3:27 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 在 2021/12/16 14:18, Arnd Bergmann 写道:
> >> It got refactored by this patch as TLB entries are expensive on MIPS,
> >> also the size of IO range doesn't always fits a page.
> > Are PIO accesses common enough that the TLB entry makes a difference?
> > I would imagine that on most systems with a PCI bus, there is not even
> > a single device that exposes an I/O resource, and even on those devices that
> > do, the kernel drivers tend to pick MMIO whenever both are available.
>
> Actually that was claimed by the author of this patch :-)
> I can understand the point. As he is working on a ramips system utlizes
> 1004Kec,
> which has only 32 TLB entries, saving a entry can give considerable
> improvement.

Ok

> For Loongson as we have legacy i8042/i8259 which can only be accessed via
> PIO, the access is very common.

Ah, right. It makes a lot of sense that anything based on ISA PC peripherals
would need it, regardless of the PCI support.

       Arnd

Re: [PATCH v3 5/6] MIPS: implement architecture-specific 'pci_remap_iospace()'

[Jiaxun Yang]

在 2021/12/16 14:32, Arnd Bergmann 写道:
> On Thu, Dec 16, 2021 at 3:27 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>> 在 2021/12/16 14:18, Arnd Bergmann 写道:
<...>
>> Ah, right. It makes a lot of sense that anything based on ISA PC peripherals
>> would need it, regardless of the PCI support.

I'll draft a RFC patch with linked list approach later on.
For now Tiezhu's 09d97da660ff ("MIPS: Only define pci_remap_iospace() 
for Ralink")
seems working.

Thanks.

>>
>>         Arnd
- Jiaxun