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

Commit Message

Sergio Paracuellos Sept. 25, 2021, 8:32 p.m. UTC

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 <[e-mail]>
Signed-off-by: Sergio Paracuellos <[e-mail]>
---
 arch/mips/include/asm/pci.h |  2 ++
 arch/mips/pci/pci-generic.c | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

Comments

Thomas Bogendoerfer Oct. 3, 2021, 4:08 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Sergio Paracuellos <[e-mail]>
> ---
>  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 <[e-mail]>

Xi Ruoyao Dec. 16, 2021, 11:44 a.m. UTC | #2

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 <[e-mail]>
> Signed-off-by: Sergio Paracuellos <[e-mail]>

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;
> +}

Tiezhu Yang Dec. 16, 2021, 12:46 p.m. UTC | #3

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 <[e-mail]>
>> Signed-off-by: Sergio Paracuellos <[e-mail]>
>
> 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

Arnd Bergmann Dec. 16, 2021, 12:55 p.m. UTC | #4

On Thu, Dec 16, 2021 at 12:44 PM Xi Ruoyao <[e-mail]> 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 <[e-mail]>
> > Signed-off-by: Sergio Paracuellos <[e-mail]>
>
> 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

Jiaxun Yang Dec. 16, 2021, 1:07 p.m. UTC | #5

在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 <[e-mail]>
>> Signed-off-by: Sergio Paracuellos <[e-mail]>
>
> 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 <[e-mail]>
> School of Aerospace Science and Technology, Xidian University

Arnd Bergmann Dec. 16, 2021, 1:50 p.m. UTC | #6

On Thu, Dec 16, 2021 at 2:07 PM Jiaxun Yang <[e-mail]> 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

Jiaxun Yang Dec. 16, 2021, 2:14 p.m. UTC | #7

在 2021/12/16 13:50, Arnd Bergmann 写道:
> On Thu, Dec 16, 2021 at 2:07 PM Jiaxun Yang <[e-mail]> 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

Arnd Bergmann Dec. 16, 2021, 2:18 p.m. UTC | #8

On Thu, Dec 16, 2021 at 3:14 PM Jiaxun Yang <[e-mail]> wrote:
> 在 2021/12/16 13:50, Arnd Bergmann 写道:
> > On Thu, Dec 16, 2021 at 2:07 PM Jiaxun Yang <[e-mail]> 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

Jiaxun Yang Dec. 16, 2021, 2:27 p.m. UTC | #9

在 2021/12/16 14:18, Arnd Bergmann 写道:
> On Thu, Dec 16, 2021 at 3:14 PM Jiaxun Yang <[e-mail]> wrote:
>> 在 2021/12/16 13:50, Arnd Bergmann 写道:
>>> On Thu, Dec 16, 2021 at 2:07 PM Jiaxun Yang <[e-mail]> 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

Arnd Bergmann Dec. 16, 2021, 2:32 p.m. UTC | #10

On Thu, Dec 16, 2021 at 3:27 PM Jiaxun Yang <[e-mail]> 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

Jiaxun Yang Dec. 16, 2021, 2:37 p.m. UTC | #11

在 2021/12/16 14:32, Arnd Bergmann 写道:
> On Thu, Dec 16, 2021 at 3:27 PM Jiaxun Yang <[e-mail]> 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

Patch

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;
+}