Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1483219 > unrolled thread

[PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Zhichang Yuan <yuanzhichang@hisilicon.com> - 2016-09-14 14:00 +0200
    Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-14 14:40 +0200
      Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <yuanzhichang@hisilicon.com> - 2016-09-14 17:00 +0200
        Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-14 23:40 +0200
          RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-15 10:10 +0200
            Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-15 10:30 +0200
              RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-15 14:10 +0200
                Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-15 14:30 +0200
                  RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-15 16:30 +0200
                  Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 zhichang <zhichang.yuan02@gmail.com> - 2016-09-21 12:10 +0200
                    RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-21 18:30 +0200
                      Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-21 22:20 +0200
                        RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-22 14:00 +0200
                          Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-22 14:20 +0200
                            RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-22 16:50 +0200
                              Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-22 17:10 +0200
                                RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-22 17:30 +0200
                                  Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <zhichang.yuan02@gmail.com> - 2016-09-22 17:50 +0200
                            Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 "zhichang.yuan" <zhichang.yuan02@gmail.com> - 2016-09-22 18:30 +0200
                              Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-23 12:00 +0200
                                RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-23 12:30 +0200
                                  Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-23 15:50 +0200
                                    RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-23 17:10 +0200
                                      Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-23 18:00 +0200
                                        Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 zhichang <zhichang.yuan02@gmail.com> - 2016-09-24 10:20 +0200
                                          Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Arnd Bergmann <arnd@arndb.de> - 2016-09-24 23:10 +0200
                                    RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-09-26 15:30 +0200
                                Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 zhichang <zhichang.yuan02@gmail.com> - 2016-09-24 10:10 +0200
          Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Jon Masters <jcm@jonmasters.org> - 2016-10-03 00:50 +0200
            Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 John Garry <john.garry@huawei.com> - 2016-10-04 14:10 +0200
              Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-10-06 02:30 +0200
                Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 John Garry <john.garry@huawei.com> - 2016-10-06 15:40 +0200
    Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 kbuild test robot <lkp@intel.com> - 2016-09-14 16:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1489925

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 September 2016 10:52
> To: zhichang.yuan
> Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > For this patch sketch, I have a question.
> > Do we call pci_address_to_pio in arch_of_address_to_pio to get the
> > corresponding logical IO port
> > for LPC??
> 
> 
> No, of course not, that would be silly:
> 
> The argument to pci_address_to_pio() is a phys_addr_t, and we we don't
> have one because there is no address associated with your PIO, that
> is the entire point of your driver!
> 
> Also, we already know the mapping because this is what the inb/outb
> workaround is looking at, so there is absolutely no reason to call it
> either.

Ok assume that we do not call pci_address_to_pio() for the ISA bus...
The LPC driver will register its phys address range in io_range_list,
then the IPMI driver probe will retrieve its physical address calling
of_address_to_resource and will use the indirect io to access this
address.

From the perspective of the indirect IO function the input parameter
is an unsigned long addr that (now) can be either:
1) an IO token coming from a legacy pci device
2) a phys address that lives on the LPC bus

These are conceptually two separate address spaces (and actually they 
both start from 0).

If the input parameter can live on different address spaces that are
overlapped, even if I save the used LPC range in arm64_extio_ops->start/end
there is no way for the indirect IO to tell if the input parameter is
an I/O token or a phys address that belongs to LPC...  

Am I missing something?

Thanks

Gab

> 
> > If we don't, it seems the LPC specific IO address will conflict with
> PCI
> > host bridges' logical IO.
> >
> > Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is
> > normal for ISA similar
> > devices), after arch_of_address_to_pio(), the r->start will be set as
> > 0x100, r->end will be set as
> > 0x3FF.  And if there is one PCI host bridge who request a IO window
> size
> > over 0x400 at the same
> > time, the  corresponding r->start and r->end will be set as 0x0,
> 0x3FF
> > after of_address_to_resource
> > for this host bridge.  Then the IO conflict happens.
> 
> You would still need to reserve some space in the io_range_list
> to avoid possible conflicts, which is a bit ugly with the current
> definition of pci_register_io_range, but I'm sure can be done.
> 
> One way I can think of would be to change pci_register_io_range()
> to just return the logical port number directly (it already
> knows it!), and pass an invalid physical address (e.g.
> #define ISA_WORKAROUND_IO_PORT_WINDOW -0x10000) into it for
> invalid translations.
> 
> Another alternative that just occurred to me would be to move
> the pci_address_to_pio() call from __of_address_to_resource()
> into of_bus_pci_translate() and then do the special handling
> for the ISA/LPC bus in of_bus_isa_translate().
> 
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1490122

On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote:
> Hi Arnd
> 
> > -----Original Message-----
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > Sent: 23 September 2016 10:52
> > To: zhichang.yuan
> > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org;
> > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> > Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> > benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> > kantyzc@163.com
> > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> > Hip06
> > 
> > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > > For this patch sketch, I have a question.
> > > Do we call pci_address_to_pio in arch_of_address_to_pio to get the
> > > corresponding logical IO port
> > > for LPC??
> > 
> > 
> > No, of course not, that would be silly:
> > 
> > The argument to pci_address_to_pio() is a phys_addr_t, and we we don't
> > have one because there is no address associated with your PIO, that
> > is the entire point of your driver!
> > 
> > Also, we already know the mapping because this is what the inb/outb
> > workaround is looking at, so there is absolutely no reason to call it
> > either.
> 
> Ok assume that we do not call pci_address_to_pio() for the ISA bus...
> The LPC driver will register its phys address range in io_range_list,
> then the IPMI driver probe will retrieve its physical address calling
> of_address_to_resource and will use the indirect io to access this
> address.
> 
> From the perspective of the indirect IO function the input parameter
> is an unsigned long addr that (now) can be either:
> 1) an IO token coming from a legacy pci device
> 2) a phys address that lives on the LPC bus
> 
> These are conceptually two separate address spaces (and actually they 
> both start from 0).

Why? Any IORESOURCE_IO address always refers to the logical I/O port
range in Linux, not the physical address that is used on a bus.

> If the input parameter can live on different address spaces that are
> overlapped, even if I save the used LPC range in arm64_extio_ops->start/end
> there is no way for the indirect IO to tell if the input parameter is
> an I/O token or a phys address that belongs to LPC...  

The start address is the offset: if you get an address between 'start'
and 'end', you subtract the 'start' from it, and use that to call
the registered driver function. That works because we can safely
assume that the bus address range that the LPC driver registers starts
zero.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1490216

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 September 2016 14:43
> To: Gabriele Paoloni
> Cc: zhichang.yuan; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote:
> > Hi Arnd
> >
> > > -----Original Message-----
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > Sent: 23 September 2016 10:52
> > > To: zhichang.yuan
> > > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org;
> > > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com;
> minyard@acm.org;
> > > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> > > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> > > Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> > > benh@kernel.crashing.org; zourongrong@gmail.com;
> liviu.dudau@arm.com;
> > > kantyzc@163.com
> > > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> > > Hip06
> > >
> > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > > > For this patch sketch, I have a question.
> > > > Do we call pci_address_to_pio in arch_of_address_to_pio to get
> the
> > > > corresponding logical IO port
> > > > for LPC??
> > >
> > >
> > > No, of course not, that would be silly:
> > >
> > > The argument to pci_address_to_pio() is a phys_addr_t, and we we
> don't
> > > have one because there is no address associated with your PIO, that
> > > is the entire point of your driver!
> > >
> > > Also, we already know the mapping because this is what the inb/outb
> > > workaround is looking at, so there is absolutely no reason to call
> it
> > > either.
> >
> > Ok assume that we do not call pci_address_to_pio() for the ISA bus...
> > The LPC driver will register its phys address range in io_range_list,
> > then the IPMI driver probe will retrieve its physical address calling
> > of_address_to_resource and will use the indirect io to access this
> > address.
> >
> > From the perspective of the indirect IO function the input parameter
> > is an unsigned long addr that (now) can be either:
> > 1) an IO token coming from a legacy pci device
> > 2) a phys address that lives on the LPC bus
> >
> > These are conceptually two separate address spaces (and actually they
> > both start from 0).
> 
> Why? Any IORESOURCE_IO address always refers to the logical I/O port
> range in Linux, not the physical address that is used on a bus.

If I read the code correctly when you get an I/O token you just add it
to PCI_IOBASE.
This is enough since pci_remap_iospace set the virtual address to 
PCI_IOBASE + the I/O token offset; so we can read/write to
vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly
to the respective PCI cpu address (that is set in the I/O range property
of the host controller)

In the patchset accessors LPC operates directly on the cpu addresses
and the input parameter of the accessors can be either an IO token or
a cpu address

+static inline void outb(u8 value, unsigned long addr)
+{
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
+			addr <= arm64_extio_ops->end)

Here below we operate on cpu address

+		extio_outb(value, addr);
+	else
+#endif

In the case below we have an I/O token added to PCI_IOBASE
to calculate the virtual address 

+		writeb(value, PCI_IOBASE + addr);
+}

My point is that if do not call pci_address_to_pio() in 
__of_address_to_resource for the ISA LPC exception then the accessors
are called either by passing an IO token or a cpu address...and from
the accessors perspective we do not know...

Thanks
Gab 

> 
> > If the input parameter can live on different address spaces that are
> > overlapped, even if I save the used LPC range in arm64_extio_ops-
> >start/end
> > there is no way for the indirect IO to tell if the input parameter is
> > an I/O token or a phys address that belongs to LPC...
> 
> The start address is the offset: if you get an address between 'start'
> and 'end', you subtract the 'start' from it, and use that to call
> the registered driver function. That works because we can safely
> assume that the bus address range that the LPC driver registers starts
> zero.
> 
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1490291

On Friday, September 23, 2016 2:59:55 PM CEST Gabriele Paoloni wrote:
> 
> > > From the perspective of the indirect IO function the input parameter
> > > is an unsigned long addr that (now) can be either:
> > > 1) an IO token coming from a legacy pci device
> > > 2) a phys address that lives on the LPC bus
> > >
> > > These are conceptually two separate address spaces (and actually they
> > > both start from 0).
> > 
> > Why? Any IORESOURCE_IO address always refers to the logical I/O port
> > range in Linux, not the physical address that is used on a bus.
> 
> If I read the code correctly when you get an I/O token you just add it
> to PCI_IOBASE.
> This is enough since pci_remap_iospace set the virtual address to 
> PCI_IOBASE + the I/O token offset; so we can read/write to
> vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly
> to the respective PCI cpu address (that is set in the I/O range property
> of the host controller)
> 
> In the patchset accessors LPC operates directly on the cpu addresses
> and the input parameter of the accessors can be either an IO token or
> a cpu address
> 
> +static inline void outb(u8 value, unsigned long addr)
> +{
> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> +       if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
> +                       addr <= arm64_extio_ops->end)
> 
> Here below we operate on cpu address
> 
> +               extio_outb(value, addr);
> +       else
> +#endif

I missed this bug earlier, this obviously needs to be

		arm64_extio_ops->outb(value, addr - arm64_extio_ops->start);

or possibly

		arm64_extio_ops->outb(arm64_extio_ops, value, addr);

as the outb function won't know what the offset is, but
that needed to be fixed regardless.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1490586

Hi, Arnd,

On 2016年09月23日 23:55, Arnd Bergmann wrote:
> On Friday, September 23, 2016 2:59:55 PM CEST Gabriele Paoloni wrote:
>>
>>>> From the perspective of the indirect IO function the input parameter
>>>> is an unsigned long addr that (now) can be either:
>>>> 1) an IO token coming from a legacy pci device
>>>> 2) a phys address that lives on the LPC bus
>>>>
>>>> These are conceptually two separate address spaces (and actually they
>>>> both start from 0).
>>>
>>> Why? Any IORESOURCE_IO address always refers to the logical I/O port
>>> range in Linux, not the physical address that is used on a bus.
>>
>> If I read the code correctly when you get an I/O token you just add it
>> to PCI_IOBASE.
>> This is enough since pci_remap_iospace set the virtual address to 
>> PCI_IOBASE + the I/O token offset; so we can read/write to
>> vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly
>> to the respective PCI cpu address (that is set in the I/O range property
>> of the host controller)
>>
>> In the patchset accessors LPC operates directly on the cpu addresses
>> and the input parameter of the accessors can be either an IO token or
>> a cpu address
>>
>> +static inline void outb(u8 value, unsigned long addr)
>> +{
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +       if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
>> +                       addr <= arm64_extio_ops->end)
>>
>> Here below we operate on cpu address
>>
>> +               extio_outb(value, addr);
>> +       else
>> +#endif
> 
> I missed this bug earlier, this obviously needs to be
> 
> 		arm64_extio_ops->outb(value, addr - arm64_extio_ops->start);
> 
> or possibly
> 
> 		arm64_extio_ops->outb(arm64_extio_ops, value, addr);
> 
> as the outb function won't know what the offset is, but
> that needed to be fixed regardless.

In V3, the outb is :

void outb(u8 value, unsigned long addr)
{
	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
			arm64_extio_ops->end < addr)
		writeb(value, PCI_IOBASE + addr);
	else
		if (arm64_extio_ops->pfout)
			arm64_extio_ops->pfout(arm64_extio_ops->devpara,
				addr + arm64_extio_ops->ptoffset, &value,
				sizeof(u8), 1);
}

here, arm64_extio_ops->ptoffset is the offset between the real legacy IO address
and the logical IO address, similar to the offset of primary address and
secondary address in PCI bridge.

But in V3, LPC driver call pci_address_to_pio to request the logical IO as PCI
host bridge during its probing.


cheers,
Zhichang



> 
> 	Arnd
> 

[toc] | [prev] | [next] | [standalone]

#1490753

On Saturday, September 24, 2016 4:14:15 PM CEST zhichang wrote:
> 
> In V3, the outb is :
> 
> void outb(u8 value, unsigned long addr)
> {
>         if (!arm64_extio_ops || arm64_extio_ops->start > addr ||
>                         arm64_extio_ops->end < addr)
>                 writeb(value, PCI_IOBASE + addr);
>         else
>                 if (arm64_extio_ops->pfout)
>                         arm64_extio_ops->pfout(arm64_extio_ops->devpara,
>                                 addr + arm64_extio_ops->ptoffset, &value,
>                                 sizeof(u8), 1);
> }
> 
> here, arm64_extio_ops->ptoffset is the offset between the real legacy IO address
> and the logical IO address, similar to the offset of primary address and
> secondary address in PCI bridge.

Ok, though we can probably simplify this by making the assumption that
'ptoffset' is the negative of 'start', as the bus we register should
always start at port zero.

> But in V3, LPC driver call pci_address_to_pio to request the logical IO as PCI
> host bridge during its probing.

Right, so this still needs to be fixed.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1491293

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 September 2016 14:43
> To: Gabriele Paoloni
> Cc: zhichang.yuan; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;
> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;
> kantyzc@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote:
> > Hi Arnd
> >
> > > -----Original Message-----
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > Sent: 23 September 2016 10:52
> > > To: zhichang.yuan
> > > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org;
> > > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com;
> minyard@acm.org;
> > > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;
> > > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> > > Linuxarm; xuwei (O); linux-serial@vger.kernel.org;
> > > benh@kernel.crashing.org; zourongrong@gmail.com;
> liviu.dudau@arm.com;
> > > kantyzc@163.com
> > > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> > > Hip06
> > >
> > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
> > > > For this patch sketch, I have a question.
> > > > Do we call pci_address_to_pio in arch_of_address_to_pio to get
> the
> > > > corresponding logical IO port
> > > > for LPC??
> > >
> > >
> > > No, of course not, that would be silly:
> > >
> > > The argument to pci_address_to_pio() is a phys_addr_t, and we we
> don't
> > > have one because there is no address associated with your PIO, that
> > > is the entire point of your driver!
> > >
> > > Also, we already know the mapping because this is what the inb/outb
> > > workaround is looking at, so there is absolutely no reason to call
> it
> > > either.
> >
> > Ok assume that we do not call pci_address_to_pio() for the ISA bus...
> > The LPC driver will register its phys address range in io_range_list,
> > then the IPMI driver probe will retrieve its physical address calling
> > of_address_to_resource and will use the indirect io to access this
> > address.
> >
> > From the perspective of the indirect IO function the input parameter
> > is an unsigned long addr that (now) can be either:
> > 1) an IO token coming from a legacy pci device
> > 2) a phys address that lives on the LPC bus
> >
> > These are conceptually two separate address spaces (and actually they
> > both start from 0).
> 
> Why? Any IORESOURCE_IO address always refers to the logical I/O port
> range in Linux, not the physical address that is used on a bus.
> 
> > If the input parameter can live on different address spaces that are
> > overlapped, even if I save the used LPC range in arm64_extio_ops-
> >start/end
> > there is no way for the indirect IO to tell if the input parameter is
> > an I/O token or a phys address that belongs to LPC...
>

Assume that in the probe function the LPC drivers calls pci_register_io_range
for the LPC cpu address range (0 to PCIBIOS_MIN_I0) and does not scan
the children DT nodes.

Consider for example the ipmi driver:
When the reg property is read to retrieve the ipmi <<i/o port>> in
http://lxr.free-electrons.com/source/drivers/char/ipmi/ipmi_si_intf.c#L2622
if we do not call pci_address_to_pio in __of_address_to_resource the input
parameter of inb/outb will be the cpu address of the ipmi (not translated
to a unique token id).

So inb/outb at this stage can be called passing either a cpu address or a
token io port.

If we set arm64_extio_ops->start/end to 0 and PCIBIOS_MIN_I0 respectively
we still cannot tell inside inb/outb if the passed address is a token or
an LPC cpu address as the ipmi cpu address can overlap with another device
I/O token...

My suggestion is to call pci_address_to_pio even for devices living on
the LPC bus; then in the LPC probe we set arm64_extio_ops->start/end to
the I/O tokens that correspond to the LPC cpu address range (in the LPC probe
function we call pci_address_to_pio after we have called pci_register_io_range);
finally in inb/outb we know that we can get only an I/O token as input
parameter and we check it against arm64_extio_ops->start/end to decide
whether to call the LPC accessors or readb/writeb...

> The start address is the offset: if you get an address between 'start'
> and 'end', you subtract the 'start' from it, and use that to call
> the registered driver function. That works because we can safely
> assume that the bus address range that the LPC driver registers starts
> zero.

Sorry I cannot follow what you said here above: <<if you get an address 
between 'start' and 'end'>>...in which function?

Thanks

Gab

> 
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1490585

Hi, Arnd,


On 2016年09月23日 17:51, Arnd Bergmann wrote:
> On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote:
>> For this patch sketch, I have a question.
>> Do we call pci_address_to_pio in arch_of_address_to_pio to get the 
>> corresponding logical IO port
>> for LPC??
> 
> 
> No, of course not, that would be silly:
> 
> The argument to pci_address_to_pio() is a phys_addr_t, and we we don't
> have one because there is no address associated with your PIO, that
> is the entire point of your driver!
> 
ok. I think I know you points. The physical addresses of LPC are only the LPC
domain addresses, not the really CPU physical addresses. That is just why you
don't support the ranges property usage in patch V3. Consequently, It is not so
reasonable to call pci_address_to_pio() with LPC address because that function
is only suitable for cpu physical address.

But just as you said in the next email reply to Gabriele, "Any IORESOURCE_IO
address always refers to the logical I/O port range in Linux, not the physical
address that is used on a bus.", Any devices which support IO accesses should
have their own unique logical IO range to drive the corresponding hardware. It
means that the drivers should know the mapping between physical port/memory
address and logical IO depend on the device specific I/O mode. At this moment,
only PCI host bridge setup a logical IO range allocation mechanism to manipulate
this logical IO range, and this way applies cpu physical address(memory) as the
input. Now, our LPC also need subrange from this common logical IO range, but
with legacy I/O port rather than CPU memory address. Ok, it break the
precondition of pci_register_io_range/pci_pio_to_address, we should not use them
directly for LPC although the calling of pci_pio_to_address is simple and less
change on the relevant code. We had done like that in V3...

So, the key issue is how to get a logical IO subrange which is not conflicted
with others, such as pci host bridges??

I list several ideas for discussion:

1. reserve a specific logical IO subrange for LPC

I describe this in "Note 1" below. Please check it.
This way seems simple without much changes, but it is not generic.

2. setup a separate logical IO subrange allocation mechanism specific for LPC/ISA

Just as your suggestion before, add the arch_of_address_to_pio() for the devices
which operate I/O with legacy I/O port address rather than memory address in
MMIO mode. That arch_of_address_to_pio() will return non-conflict logical IO
with PCI host bridge at last. But the logical IO range is global, those
functions for LPC/ISA specific logical IO subrange allocation must be
synchronized with pci_register_io_range/pci_pio_to_address to know what logical
ranges had been populated. It is not good for the implement dispersion on same
issue.

3. setup a new underlying method to control the logical IO range management

Based on the existing resource management, add a simplified logical IO range
management support which only request the logical IO ranges according the IO
range size ( similar to IORESOURCE_SIZEALIGN mode ), no matter what type the
physical address is. Then revise the current pci_register_io_range to adopt this
new method. Of-course, LPC/ISA request the logical IO with this new method too.

This is just a proposition. It is more workload compared with other solutions.

What do you think about these? Any more ideas?


> Also, we already know the mapping because this is what the inb/outb
> workaround is looking at, so there is absolutely no reason to call it
> either.
> 
>> If we don't, it seems the LPC specific IO address will conflict with PCI 
>> host bridges' logical IO.
>>
>> Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is 
>> normal for ISA similar
>> devices), after arch_of_address_to_pio(), the r->start will be set as 
>> 0x100, r->end will be set as
>> 0x3FF.  And if there is one PCI host bridge who request a IO window size 
>> over 0x400 at the same
>> time, the  corresponding r->start and r->end will be set as 0x0, 0x3FF 
>> after of_address_to_resource
>> for this host bridge.  Then the IO conflict happens.
> 
> You would still need to reserve some space in the io_range_list
> to avoid possible conflicts, which is a bit ugly with the current
> definition of pci_register_io_range, but I'm sure can be done.
> 

Note 1) Do you remember patch V2? There, I modified the pci.c like that to
reserve 0 - PCIBIOS_MIN_IO (it is 0x1000) :

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d51..ac2e569 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3221,7 +3221,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resourc

 #ifdef PCI_IOBASE
        struct io_range *range;
-       resource_size_t allocated_size = 0;
+       resource_size_t allocated_size = PCIBIOS_MIN_IO;

        /* check if the range hasn't been previously recorded */
        spin_lock(&io_range_lock);
@@ -3270,7 +3270,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)

 #ifdef PCI_IOBASE
        struct io_range *range;
-       resource_size_t allocated_size = 0;
+       resource_size_t allocated_size = PCIBIOS_MIN_IO;

        if (pio > IO_SPACE_LIMIT)
                return address;
@@ -3293,7 +3293,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t addres
 {
 #ifdef PCI_IOBASE
        struct io_range *res;
-       resource_size_t offset = 0;
+       resource_size_t offset = PCIBIOS_MIN_IO;
        unsigned long addr = -1;

        spin_lock(&io_range_lock);

Based on this, a exclusive logical IO subrange is for LPC now. Then we certainly
can add some special handling in __of_address_to_resource or
__of_translate_address --> of_translate_one to return the untranslated LPC/ISA
IO address. But to be honest, I think we don't need this special handling in
address.c anymore. We had known the LPC/ISA IO is 1:1 to logical IO, just think
any logical IO port among 0 - 0x1000 should call the LPC registered I/O hooks in
the new in/out().

Furthermore, we can make the reservation is not fixed as PCIBIOS_MIN_IO. If the
LPC/ISA probing is run before PCI host bridge probing, we can reserve a
non-fixed logcial IO subrange what LPC/ISA ask for.

This solution is based on an assumption that no any other devices have to
request the specific logical IO subrange for LPC/ISA. Probably this assumption
is ok on arm64, you known, there is no real IO space as X86. But anyway, this
reservation is not so generic, depended on some special handling.

Does this idea match your comments??


> One way I can think of would be to change pci_register_io_range()
> to just return the logical port number directly (it already
> knows it!), and pass an invalid physical address (e.g. 
> #define ISA_WORKAROUND_IO_PORT_WINDOW -0x10000) into it for
> invalid translations.
> 

I am not so clear know your idea here.  Do you want to select an unpopulated CPU
address as the parent address in range property?? or anything else???


> Another alternative that just occurred to me would be to move
> the pci_address_to_pio() call from __of_address_to_resource()
> into of_bus_pci_translate() and then do the special handling
> for the ISA/LPC bus in of_bus_isa_translate().

As for this idea, do you mean that of_translate_address will directly return the
final logical IO start address?? It seems to extend the definition of
of_translate_address.


Thanks,
Zhichang

> 
> 	Arnd
> 

[toc] | [prev] | [next] | [standalone]

#1494615

On 09/14/2016 02:32 PM, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote:

>> And there are probably multiple child devices under LPC, the global arm64_extio_ops only can cover one PIO range. It is fortunate only ipmi driver can not support I/O
>> operation registering, serial driver has serial_in/serial_out to
>> be registered. So, only the PIO range for ipmi device is stored
>> in arm64_extio_ops and the indirect-IO
>> works well for ipmi device.
> 
> You should not do that in the serial driver, please just use the
> normal 8250 driver that works fine once you handle the entire
> port range.

Just for the record, Arnd has the right idea. There is only one type of
UART permitted by SBSA (PL011). We carved out an exception for a design
that was already in flight and allowed it to be 16550. That other design
was then corrected in future generations to be PL011 as we required it
to be. Then there's the Hip06. I've given feedback elsewhere about the
need for there to be (at most) two types of UART in the wild. This "LPC"
stuff needs cleaning up (feedback given elsewhere already on that), but
we won't be adding a third serial driver into the mix in order to make
it work. There will be standard ARM servers. There will not be the
kinda-sorta-standard. Thanks.

Jon.

[toc] | [prev] | [next] | [standalone]

#1495389

On 02/10/2016 23:03, Jon Masters wrote:
> On 09/14/2016 02:32 PM, Arnd Bergmann wrote:
>> On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote:
>
>>> And there are probably multiple child devices under LPC, the global arm64_extio_ops only can cover one PIO range. It is fortunate only ipmi driver can not support I/O
>>> operation registering, serial driver has serial_in/serial_out to
>>> be registered. So, only the PIO range for ipmi device is stored
>>> in arm64_extio_ops and the indirect-IO
>>> works well for ipmi device.
>>
>> You should not do that in the serial driver, please just use the
>> normal 8250 driver that works fine once you handle the entire
>> port range.
>
> Just for the record, Arnd has the right idea. There is only one type of
> UART permitted by SBSA (PL011). We carved out an exception for a design
> that was already in flight and allowed it to be 16550. That other design
> was then corrected in future generations to be PL011 as we required it
> to be. Then there's the Hip06. I've given feedback elsewhere about the
> need for there to be (at most) two types of UART in the wild. This "LPC"
> stuff needs cleaning up (feedback given elsewhere already on that), but
> we won't be adding a third serial driver into the mix in order to make
> it work. There will be standard ARM servers. There will not be the
> kinda-sorta-standard. Thanks.
>

Right, so I think Zhichang can make the necessary generic changes to 
8250 OF driver to support IO port as well as MMIO-based.

However an LPC-based earlycon driver is still required.

A note on hip07-based D05 (for those unaware): this does not use 
LPC-based uart. It uses PL011. The hardware guys have managed some 
trickery where they loopback the serial line around the BMC/CPLD. But we 
still need it for hip06 D03 and any other boards which want to use LPC 
bus for uart.

A question on SBSA: does it propose how to provide serial via BMC for SOL?


> Jon.
>
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1496127

On Tue, 2016-10-04 at 13:02 +0100, John Garry wrote:
> Right, so I think Zhichang can make the necessary generic changes to 
> 8250 OF driver to support IO port as well as MMIO-based.
> 
> However an LPC-based earlycon driver is still required.
> 
> A note on hip07-based D05 (for those unaware): this does not use 
> LPC-based uart. It uses PL011. The hardware guys have managed some 
> trickery where they loopback the serial line around the BMC/CPLD. But we 
> still need it for hip06 D03 and any other boards which want to use LPC 
> bus for uart.
> 
> A question on SBSA: does it propose how to provide serial via BMC for SOL?

Probably another reason to keep 8250 as a legal option ... The (very
popular) Aspeed BMCs tend to do this via a 8250-looking virtual UART on
LPC.

Cheers,
Ben,

[toc] | [prev] | [next] | [standalone]

#1496654

On 06/10/2016 01:18, Benjamin Herrenschmidt wrote:
> On Tue, 2016-10-04 at 13:02 +0100, John Garry wrote:
>> Right, so I think Zhichang can make the necessary generic changes to
>> 8250 OF driver to support IO port as well as MMIO-based.
>>
>> However an LPC-based earlycon driver is still required.
>>
>> A note on hip07-based D05 (for those unaware): this does not use
>> LPC-based uart. It uses PL011. The hardware guys have managed some
>> trickery where they loopback the serial line around the BMC/CPLD. But we
>> still need it for hip06 D03 and any other boards which want to use LPC
>> bus for uart.
>>
>> A question on SBSA: does it propose how to provide serial via BMC for SOL?
>
> Probably another reason to keep 8250 as a legal option ... The (very
> popular) Aspeed BMCs tend to do this via a 8250-looking virtual UART on
> LPC.
>
> Cheers,
> Ben,

I think we're talking about the same thing for our LPC-based UART.

John

>
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1483361

[Multipart message — attachments visible in raw view] — view raw

Hi zhichang.yuan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.8-rc6 next-20160914]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Zhichang-Yuan/ARM64-LPC-legacy-ISA-I-O-support/20160914-202858
config: openrisc-or1ksim_defconfig (attached as .config)
compiler: or32-linux-gcc (GCC) 4.5.1-or32-1.0rc1
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=openrisc 

All errors (new ones prefixed by >>):

   drivers/of/address.c: In function '__of_address_to_resource':
>> drivers/of/address.c:702:5: error: implicit declaration of function 'of_bus_pci_match'

vim +/of_bus_pci_match +702 drivers/of/address.c

   696				return -EINVAL;
   697			/*
   698			 * special processing for non-pci device gurantee the linux start pio
   699			 * is not ZERO. Otherwise, some drivers' initialization will fail.
   700			 */
   701			if (!port && (!IS_ENABLED(CONFIG_OF_ADDRESS_PCI) ||
 > 702					!of_bus_pci_match(dev)))
   703				port += 1;
   704	
   705			r->start = port;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web