Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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 .
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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 .
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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,
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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,
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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. .
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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. .
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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.
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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.
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 23 September 2016 14:43 > To: Gabriele Paoloni > Cc: zhichang.yuan; linux-arm-ker...@lists.infradead.org; > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com; > kant...@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:a...@arndb.de] > > > Sent: 23 September 2016 10:52 > > > To: zhichang.yuan > > > Cc: Gabriele Paoloni; linux-arm-ker...@lists.infradead.org; > > > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; > miny...@acm.org; > > > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > > > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > > > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > > > b...@kernel.crashing.org; zourongr...@gmail.com; > liviu.du...@arm.com; > > > kant...@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 <> 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: <>...in which function? Thanks Gab > > Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 23 September 2016 14:43 > To: Gabriele Paoloni > Cc: zhichang.yuan; linux-arm-ker...@lists.infradead.org; > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com; > kant...@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:a...@arndb.de] > > > Sent: 23 September 2016 10:52 > > > To: zhichang.yuan > > > Cc: Gabriele Paoloni; linux-arm-ker...@lists.infradead.org; > > > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; > miny...@acm.org; > > > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > > > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > > > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > > > b...@kernel.crashing.org; zourongr...@gmail.com; > liviu.du...@arm.com; > > > kant...@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 <> 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: <>...in which function? Thanks Gab > > Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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, ,
> 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
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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, ,
> 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
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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, ,
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
>
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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, ,
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
>
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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(_range_lock); @@ -3270,7 +3270,7 @@ phys_addr_t
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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(_range_lock); @@ -3270,7 +3270,7 @@ phys_addr_t
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: 23 September 2016 14:43
> To: Gabriele Paoloni
> Cc: zhichang.yuan; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org;
> linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
> will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
> b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com;
> kant...@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:a...@arndb.de]
> > > Sent: 23 September 2016 10:52
> > > To: zhichang.yuan
> > > Cc: Gabriele Paoloni; linux-arm-ker...@lists.infradead.org;
> > > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com;
> miny...@acm.org;
> > > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
> > > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> > > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
> > > b...@kernel.crashing.org; zourongr...@gmail.com;
> liviu.du...@arm.com;
> > > kant...@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
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: 23 September 2016 14:43
> To: Gabriele Paoloni
> Cc: zhichang.yuan; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org;
> linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
> will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
> b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com;
> kant...@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:a...@arndb.de]
> > > Sent: 23 September 2016 10:52
> > > To: zhichang.yuan
> > > Cc: Gabriele Paoloni; linux-arm-ker...@lists.infradead.org;
> > > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com;
> miny...@acm.org;
> > > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
> > > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> > > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
> > > b...@kernel.crashing.org; zourongr...@gmail.com;
> liviu.du...@arm.com;
> > > kant...@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
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:a...@arndb.de] > > Sent: 23 September 2016 10:52 > > To: zhichang.yuan > > Cc: Gabriele Paoloni; linux-arm-ker...@lists.infradead.org; > > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > > b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com; > > kant...@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
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:a...@arndb.de] > > Sent: 23 September 2016 10:52 > > To: zhichang.yuan > > Cc: Gabriele Paoloni; linux-arm-ker...@lists.infradead.org; > > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > > b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com; > > kant...@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
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 23 September 2016 10:52 > To: zhichang.yuan > Cc: Gabriele Paoloni; linux-arm-ker...@lists.infradead.org; > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com; > kant...@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 -0x1) 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
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 23 September 2016 10:52 > To: zhichang.yuan > Cc: Gabriele Paoloni; linux-arm-ker...@lists.infradead.org; > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com; > kant...@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 -0x1) 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
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. > 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 -0x1) 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
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. > 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 -0x1) 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
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On 09/22/2016 08:14 PM, Arnd Bergmann wrote:
On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote:
I think extending of_empty_ranges_quirk() may be a reasonable
solution.
What do you think Arnd?
I don't really like that idea, that quirk is meant to work around
broken DTs, but we can just make the DT valid and implement the
code properly.
Ok I understand your point where it is not right to use of_empty_ranges_quirk()
As a quirk is used to work around broken HW or broken FW (as in this case)
rather than to fix code
What about the following? I think adding the check you suggested next to
of_empty_ranges_quirk() is adding the case we need in the right point (thus
avoiding any duplication)
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
return NULL;
}
+static inline int of_isa_indirect_io(struct device_node *np)
+{
+ /*
+* check if the current node is an isa bus and if indirectio operation
+* are registered
+*/
+ return (of_bus_isa_match(np) && arm64_extio_ops);
+}
+
static int of_empty_ranges_quirk(struct device_node *np)
{
if (IS_ENABLED(CONFIG_PPC)) {
@@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent,
struct of_bus *bus,
* This code is only enabled on powerpc. --gcl
*/
ranges = of_get_property(parent, rprop, );
- if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
+ if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
!of_isa_indirect_io(parent)) {
pr_debug("OF: no ranges; cannot translate\n");
return 1;
}
I don't see what effect that would have. What do you want to
achieve with this?
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..a18d96843fae 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node
*dev,
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return -EINVAL;
taddr = of_translate_address(dev, addrp);
- if (taddr == OF_BAD_ADDR)
- return -EINVAL;
memset(r, 0, sizeof(struct resource));
+
if (flags & IORESOURCE_IO) {
unsigned long port;
- port = pci_address_to_pio(taddr);
+
+ if (taddr == OF_BAD_ADDR)
+ port = arch_of_address_to_pio(dev, addrp)
+ else
+ port = pci_address_to_pio(taddr);
+
if (port == (unsigned long)-1)
return -EINVAL;
r->start = port;
r->end = port + size - 1;
} else {
+ if (taddr == OF_BAD_ADDR)
+ return -EINVAL;
+
r->start = taddr;
r->end = taddr + size - 1;
}
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??
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.
cheers,
Zhichang
Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On 09/22/2016 08:14 PM, Arnd Bergmann wrote:
On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote:
I think extending of_empty_ranges_quirk() may be a reasonable
solution.
What do you think Arnd?
I don't really like that idea, that quirk is meant to work around
broken DTs, but we can just make the DT valid and implement the
code properly.
Ok I understand your point where it is not right to use of_empty_ranges_quirk()
As a quirk is used to work around broken HW or broken FW (as in this case)
rather than to fix code
What about the following? I think adding the check you suggested next to
of_empty_ranges_quirk() is adding the case we need in the right point (thus
avoiding any duplication)
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np)
return NULL;
}
+static inline int of_isa_indirect_io(struct device_node *np)
+{
+ /*
+* check if the current node is an isa bus and if indirectio operation
+* are registered
+*/
+ return (of_bus_isa_match(np) && arm64_extio_ops);
+}
+
static int of_empty_ranges_quirk(struct device_node *np)
{
if (IS_ENABLED(CONFIG_PPC)) {
@@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent,
struct of_bus *bus,
* This code is only enabled on powerpc. --gcl
*/
ranges = of_get_property(parent, rprop, );
- if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
+ if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
!of_isa_indirect_io(parent)) {
pr_debug("OF: no ranges; cannot translate\n");
return 1;
}
I don't see what effect that would have. What do you want to
achieve with this?
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..a18d96843fae 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node
*dev,
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return -EINVAL;
taddr = of_translate_address(dev, addrp);
- if (taddr == OF_BAD_ADDR)
- return -EINVAL;
memset(r, 0, sizeof(struct resource));
+
if (flags & IORESOURCE_IO) {
unsigned long port;
- port = pci_address_to_pio(taddr);
+
+ if (taddr == OF_BAD_ADDR)
+ port = arch_of_address_to_pio(dev, addrp)
+ else
+ port = pci_address_to_pio(taddr);
+
if (port == (unsigned long)-1)
return -EINVAL;
r->start = port;
r->end = port + size - 1;
} else {
+ if (taddr == OF_BAD_ADDR)
+ return -EINVAL;
+
r->start = taddr;
r->end = taddr + size - 1;
}
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??
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.
cheers,
Zhichang
Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On 09/22/2016 11:20 PM, Gabriele Paoloni wrote:
-Original Message-
From: Arnd Bergmann [mailto:a...@arndb.de]
Sent: 22 September 2016 15:59
To: Gabriele Paoloni
Cc: zhichang; linux-arm-ker...@lists.infradead.org;
devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org;
linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com;
kant...@163.com
Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
Hip06
On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
static int of_empty_ranges_quirk(struct device_node *np)
{
if (IS_ENABLED(CONFIG_PPC)) {
@@ -503,7 +512,7 @@ static int of_translate_one(struct
device_node
*parent, struct of_bus *bus,
* This code is only enabled on powerpc. --gcl
*/
ranges = of_get_property(parent, rprop, );
- if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
+ if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
!of_isa_indirect_io(parent)) {
pr_debug("OF: no ranges; cannot translate\n");
return 1;
}
I don't see what effect that would have. What do you want to
achieve with this?
If I read the code correctly adding the function above would end
up in a 1:1 mapping:
http://lxr.free-electrons.com/source/drivers/of/address.c#L513
so taddr will be assigned with the cpu address space specified
in the children nodes of LPC and we are not using a quirk function
(we are just checking that we have the indirect io assigned and
that we are on a ISA bus). Now probably there is a nit in my
code sketch where of_isa_indirect_io should be probably an
architecture
specific function...
But the point is that it would then return an incorrect address,
which in the worst case could be the same as another I/O space
if that happens to be at CPU address zero.
If we do not touch __of_address_to_resource after taddr is returned
by of_translate_address we will check for (flags & IORESOURCE_IO),
then we call pci_address_to_pio to retrieve the unique token (remember
that LPC driver will register the LPC io range to pci io_range_list).
I do not think that we can have any conflict with any other I/O space
as pci_register_io_range will guarantee that the LPC range does not
overlap with any other I/O range...
If we don't bypass the calling of pci_address_to_pio after
of_translate_address,
there should no conflict between LPC logical IO range and other logical
IO ranges
of other devices.
I guess Arnd want to skip all the translation for our LPC IO address.
But if we do it
like that, it seems we can't avoid the possible conflict with the
logical IO ranges of
PCI host bridges without any changes on the pci_register_io_range and
pci_address_to_pio.
Because two completely separate I/O spaces are created without
synchronization.
Best,
Zhichang
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
I don't think this is very right as you may fail for different
reasons other than a missing range property, e.g:
http://lxr.free-electrons.com/source/drivers/of/address.c#L575
And even if the only failure case was a missing range if in the
future __of_translate_address had to be reworked we would again
make a wrong assumption...you get my point?
The newly introduced function would clearly have to make
some sanity checks. The idea is that treat the case of
not being able to translate a bus specific I/O address
into a CPU address literally and fall back to another method
of translating that address.
This matches my mental model of how we find the resource:
- start with the bus address
- try to translate that into a CPU address
- if we arrive at a CPU physical address for IORESOURCE_MEM, use that
- if we arrive at a CPU physical address for IORESOURCE_IO, translate
that into a Linux IORESOURCE_IO token
- if there is no valid CPU physical address, try to translate
the address into an IORESOURCE_IO using the ISA accessor
- if that fails too, give up.
If you try to fake a CPU physical address inbetween, it just
gets more confusing.
Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On 09/22/2016 11:20 PM, Gabriele Paoloni wrote:
-Original Message-
From: Arnd Bergmann [mailto:a...@arndb.de]
Sent: 22 September 2016 15:59
To: Gabriele Paoloni
Cc: zhichang; linux-arm-ker...@lists.infradead.org;
devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org;
linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com;
kant...@163.com
Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
Hip06
On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
static int of_empty_ranges_quirk(struct device_node *np)
{
if (IS_ENABLED(CONFIG_PPC)) {
@@ -503,7 +512,7 @@ static int of_translate_one(struct
device_node
*parent, struct of_bus *bus,
* This code is only enabled on powerpc. --gcl
*/
ranges = of_get_property(parent, rprop, );
- if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
+ if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
!of_isa_indirect_io(parent)) {
pr_debug("OF: no ranges; cannot translate\n");
return 1;
}
I don't see what effect that would have. What do you want to
achieve with this?
If I read the code correctly adding the function above would end
up in a 1:1 mapping:
http://lxr.free-electrons.com/source/drivers/of/address.c#L513
so taddr will be assigned with the cpu address space specified
in the children nodes of LPC and we are not using a quirk function
(we are just checking that we have the indirect io assigned and
that we are on a ISA bus). Now probably there is a nit in my
code sketch where of_isa_indirect_io should be probably an
architecture
specific function...
But the point is that it would then return an incorrect address,
which in the worst case could be the same as another I/O space
if that happens to be at CPU address zero.
If we do not touch __of_address_to_resource after taddr is returned
by of_translate_address we will check for (flags & IORESOURCE_IO),
then we call pci_address_to_pio to retrieve the unique token (remember
that LPC driver will register the LPC io range to pci io_range_list).
I do not think that we can have any conflict with any other I/O space
as pci_register_io_range will guarantee that the LPC range does not
overlap with any other I/O range...
If we don't bypass the calling of pci_address_to_pio after
of_translate_address,
there should no conflict between LPC logical IO range and other logical
IO ranges
of other devices.
I guess Arnd want to skip all the translation for our LPC IO address.
But if we do it
like that, it seems we can't avoid the possible conflict with the
logical IO ranges of
PCI host bridges without any changes on the pci_register_io_range and
pci_address_to_pio.
Because two completely separate I/O spaces are created without
synchronization.
Best,
Zhichang
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
I don't think this is very right as you may fail for different
reasons other than a missing range property, e.g:
http://lxr.free-electrons.com/source/drivers/of/address.c#L575
And even if the only failure case was a missing range if in the
future __of_translate_address had to be reworked we would again
make a wrong assumption...you get my point?
The newly introduced function would clearly have to make
some sanity checks. The idea is that treat the case of
not being able to translate a bus specific I/O address
into a CPU address literally and fall back to another method
of translating that address.
This matches my mental model of how we find the resource:
- start with the bus address
- try to translate that into a CPU address
- if we arrive at a CPU physical address for IORESOURCE_MEM, use that
- if we arrive at a CPU physical address for IORESOURCE_IO, translate
that into a Linux IORESOURCE_IO token
- if there is no valid CPU physical address, try to translate
the address into an IORESOURCE_IO using the ISA accessor
- if that fails too, give up.
If you try to fake a CPU physical address inbetween, it just
gets more confusing.
Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: 22 September 2016 15:59
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org;
> linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
> will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
> b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com;
> kant...@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
>
> On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
> > > > static int of_empty_ranges_quirk(struct device_node *np)
> > > > {
> > > > if (IS_ENABLED(CONFIG_PPC)) {
> > > > @@ -503,7 +512,7 @@ static int of_translate_one(struct
> device_node
> > > *parent, struct of_bus *bus,
> > > > * This code is only enabled on powerpc. --gcl
> > > > */
> > > > ranges = of_get_property(parent, rprop, );
> > > > - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > > > + if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> > > !of_isa_indirect_io(parent)) {
> > > > pr_debug("OF: no ranges; cannot translate\n");
> > > > return 1;
> > > > }
> > >
> > > I don't see what effect that would have. What do you want to
> > > achieve with this?
> >
> > If I read the code correctly adding the function above would end
> > up in a 1:1 mapping:
> > http://lxr.free-electrons.com/source/drivers/of/address.c#L513
> >
> > so taddr will be assigned with the cpu address space specified
> > in the children nodes of LPC and we are not using a quirk function
> > (we are just checking that we have the indirect io assigned and
> > that we are on a ISA bus). Now probably there is a nit in my
> > code sketch where of_isa_indirect_io should be probably an
> architecture
> > specific function...
>
> But the point is that it would then return an incorrect address,
> which in the worst case could be the same as another I/O space
> if that happens to be at CPU address zero.
If we do not touch __of_address_to_resource after taddr is returned
by of_translate_address we will check for (flags & IORESOURCE_IO),
then we call pci_address_to_pio to retrieve the unique token (remember
that LPC driver will register the LPC io range to pci io_range_list).
I do not think that we can have any conflict with any other I/O space
as pci_register_io_range will guarantee that the LPC range does not
overlap with any other I/O range...
>
> > > I think all we need from this function is to return '1' if
> > > we hit an ISA I/O window, and that should happen for the two
> > > interesting cases, either no 'ranges' at all, or no translation
> > > for the range in question, so that __of_translate_address can
> > > return OF_BAD_ADDR, and we can enter the special case
> > > handling in the caller, that handles it like
> > >
> >
> > I don't think this is very right as you may fail for different
> > reasons other than a missing range property, e.g:
> > http://lxr.free-electrons.com/source/drivers/of/address.c#L575
> >
> > And even if the only failure case was a missing range if in the
> > future __of_translate_address had to be reworked we would again
> > make a wrong assumption...you get my point?
>
> The newly introduced function would clearly have to make
> some sanity checks. The idea is that treat the case of
> not being able to translate a bus specific I/O address
> into a CPU address literally and fall back to another method
> of translating that address.
>
> This matches my mental model of how we find the resource:
>
> - start with the bus address
> - try to translate that into a CPU address
> - if we arrive at a CPU physical address for IORESOURCE_MEM, use that
> - if we arrive at a CPU physical address for IORESOURCE_IO, translate
> that into a Linux IORESOURCE_IO token
> - if there is no valid CPU physical address, try to translate
> the address into an IORESOURCE_IO using the ISA accessor
> - if that fails too, give up.
>
> If you try to fake a CPU physical address inbetween, it just
> gets more confusing.
>
> Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: 22 September 2016 15:59
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org;
> linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
> will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
> b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com;
> kant...@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
>
> On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
> > > > static int of_empty_ranges_quirk(struct device_node *np)
> > > > {
> > > > if (IS_ENABLED(CONFIG_PPC)) {
> > > > @@ -503,7 +512,7 @@ static int of_translate_one(struct
> device_node
> > > *parent, struct of_bus *bus,
> > > > * This code is only enabled on powerpc. --gcl
> > > > */
> > > > ranges = of_get_property(parent, rprop, );
> > > > - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > > > + if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> > > !of_isa_indirect_io(parent)) {
> > > > pr_debug("OF: no ranges; cannot translate\n");
> > > > return 1;
> > > > }
> > >
> > > I don't see what effect that would have. What do you want to
> > > achieve with this?
> >
> > If I read the code correctly adding the function above would end
> > up in a 1:1 mapping:
> > http://lxr.free-electrons.com/source/drivers/of/address.c#L513
> >
> > so taddr will be assigned with the cpu address space specified
> > in the children nodes of LPC and we are not using a quirk function
> > (we are just checking that we have the indirect io assigned and
> > that we are on a ISA bus). Now probably there is a nit in my
> > code sketch where of_isa_indirect_io should be probably an
> architecture
> > specific function...
>
> But the point is that it would then return an incorrect address,
> which in the worst case could be the same as another I/O space
> if that happens to be at CPU address zero.
If we do not touch __of_address_to_resource after taddr is returned
by of_translate_address we will check for (flags & IORESOURCE_IO),
then we call pci_address_to_pio to retrieve the unique token (remember
that LPC driver will register the LPC io range to pci io_range_list).
I do not think that we can have any conflict with any other I/O space
as pci_register_io_range will guarantee that the LPC range does not
overlap with any other I/O range...
>
> > > I think all we need from this function is to return '1' if
> > > we hit an ISA I/O window, and that should happen for the two
> > > interesting cases, either no 'ranges' at all, or no translation
> > > for the range in question, so that __of_translate_address can
> > > return OF_BAD_ADDR, and we can enter the special case
> > > handling in the caller, that handles it like
> > >
> >
> > I don't think this is very right as you may fail for different
> > reasons other than a missing range property, e.g:
> > http://lxr.free-electrons.com/source/drivers/of/address.c#L575
> >
> > And even if the only failure case was a missing range if in the
> > future __of_translate_address had to be reworked we would again
> > make a wrong assumption...you get my point?
>
> The newly introduced function would clearly have to make
> some sanity checks. The idea is that treat the case of
> not being able to translate a bus specific I/O address
> into a CPU address literally and fall back to another method
> of translating that address.
>
> This matches my mental model of how we find the resource:
>
> - start with the bus address
> - try to translate that into a CPU address
> - if we arrive at a CPU physical address for IORESOURCE_MEM, use that
> - if we arrive at a CPU physical address for IORESOURCE_IO, translate
> that into a Linux IORESOURCE_IO token
> - if there is no valid CPU physical address, try to translate
> the address into an IORESOURCE_IO using the ISA accessor
> - if that fails too, give up.
>
> If you try to fake a CPU physical address inbetween, it just
> gets more confusing.
>
> Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
> > > static int of_empty_ranges_quirk(struct device_node *np)
> > > {
> > > if (IS_ENABLED(CONFIG_PPC)) {
> > > @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node
> > *parent, struct of_bus *bus,
> > > * This code is only enabled on powerpc. --gcl
> > > */
> > > ranges = of_get_property(parent, rprop, );
> > > - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > > + if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> > !of_isa_indirect_io(parent)) {
> > > pr_debug("OF: no ranges; cannot translate\n");
> > > return 1;
> > > }
> >
> > I don't see what effect that would have. What do you want to
> > achieve with this?
>
> If I read the code correctly adding the function above would end
> up in a 1:1 mapping:
> http://lxr.free-electrons.com/source/drivers/of/address.c#L513
>
> so taddr will be assigned with the cpu address space specified
> in the children nodes of LPC and we are not using a quirk function
> (we are just checking that we have the indirect io assigned and
> that we are on a ISA bus). Now probably there is a nit in my
> code sketch where of_isa_indirect_io should be probably an architecture
> specific function...
But the point is that it would then return an incorrect address,
which in the worst case could be the same as another I/O space
if that happens to be at CPU address zero.
> > I think all we need from this function is to return '1' if
> > we hit an ISA I/O window, and that should happen for the two
> > interesting cases, either no 'ranges' at all, or no translation
> > for the range in question, so that __of_translate_address can
> > return OF_BAD_ADDR, and we can enter the special case
> > handling in the caller, that handles it like
> >
>
> I don't think this is very right as you may fail for different
> reasons other than a missing range property, e.g:
> http://lxr.free-electrons.com/source/drivers/of/address.c#L575
>
> And even if the only failure case was a missing range if in the
> future __of_translate_address had to be reworked we would again
> make a wrong assumption...you get my point?
The newly introduced function would clearly have to make
some sanity checks. The idea is that treat the case of
not being able to translate a bus specific I/O address
into a CPU address literally and fall back to another method
of translating that address.
This matches my mental model of how we find the resource:
- start with the bus address
- try to translate that into a CPU address
- if we arrive at a CPU physical address for IORESOURCE_MEM, use that
- if we arrive at a CPU physical address for IORESOURCE_IO, translate
that into a Linux IORESOURCE_IO token
- if there is no valid CPU physical address, try to translate
the address into an IORESOURCE_IO using the ISA accessor
- if that fails too, give up.
If you try to fake a CPU physical address inbetween, it just
gets more confusing.
Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
> > > static int of_empty_ranges_quirk(struct device_node *np)
> > > {
> > > if (IS_ENABLED(CONFIG_PPC)) {
> > > @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node
> > *parent, struct of_bus *bus,
> > > * This code is only enabled on powerpc. --gcl
> > > */
> > > ranges = of_get_property(parent, rprop, );
> > > - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > > + if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> > !of_isa_indirect_io(parent)) {
> > > pr_debug("OF: no ranges; cannot translate\n");
> > > return 1;
> > > }
> >
> > I don't see what effect that would have. What do you want to
> > achieve with this?
>
> If I read the code correctly adding the function above would end
> up in a 1:1 mapping:
> http://lxr.free-electrons.com/source/drivers/of/address.c#L513
>
> so taddr will be assigned with the cpu address space specified
> in the children nodes of LPC and we are not using a quirk function
> (we are just checking that we have the indirect io assigned and
> that we are on a ISA bus). Now probably there is a nit in my
> code sketch where of_isa_indirect_io should be probably an architecture
> specific function...
But the point is that it would then return an incorrect address,
which in the worst case could be the same as another I/O space
if that happens to be at CPU address zero.
> > I think all we need from this function is to return '1' if
> > we hit an ISA I/O window, and that should happen for the two
> > interesting cases, either no 'ranges' at all, or no translation
> > for the range in question, so that __of_translate_address can
> > return OF_BAD_ADDR, and we can enter the special case
> > handling in the caller, that handles it like
> >
>
> I don't think this is very right as you may fail for different
> reasons other than a missing range property, e.g:
> http://lxr.free-electrons.com/source/drivers/of/address.c#L575
>
> And even if the only failure case was a missing range if in the
> future __of_translate_address had to be reworked we would again
> make a wrong assumption...you get my point?
The newly introduced function would clearly have to make
some sanity checks. The idea is that treat the case of
not being able to translate a bus specific I/O address
into a CPU address literally and fall back to another method
of translating that address.
This matches my mental model of how we find the resource:
- start with the bus address
- try to translate that into a CPU address
- if we arrive at a CPU physical address for IORESOURCE_MEM, use that
- if we arrive at a CPU physical address for IORESOURCE_IO, translate
that into a Linux IORESOURCE_IO token
- if there is no valid CPU physical address, try to translate
the address into an IORESOURCE_IO using the ISA accessor
- if that fails too, give up.
If you try to fake a CPU physical address inbetween, it just
gets more confusing.
Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: 22 September 2016 13:15
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org;
> linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
> will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
> b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com;
> kant...@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
>
> On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni
> wrote:
> > > > I think extending of_empty_ranges_quirk() may be a reasonable
> > > solution.
> > > > What do you think Arnd?
> > >
> > > I don't really like that idea, that quirk is meant to work around
> > > broken DTs, but we can just make the DT valid and implement the
> > > code properly.
> >
> > Ok I understand your point where it is not right to use
> of_empty_ranges_quirk()
> > As a quirk is used to work around broken HW or broken FW (as in this
> case)
> > rather than to fix code
> >
> > What about the following? I think adding the check you suggested next
> to
> > of_empty_ranges_quirk() is adding the case we need in the right point
> (thus
> > avoiding any duplication)
> >
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct
> device_node *np)
> > return NULL;
> > }
> >
> > +static inline int of_isa_indirect_io(struct device_node *np)
> > +{
> > + /*
> > +* check if the current node is an isa bus and if indirectio
> operation
> > +* are registered
> > +*/
> > + return (of_bus_isa_match(np) && arm64_extio_ops);
> > +}
> > +
> > static int of_empty_ranges_quirk(struct device_node *np)
> > {
> > if (IS_ENABLED(CONFIG_PPC)) {
> > @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
> > * This code is only enabled on powerpc. --gcl
> > */
> > ranges = of_get_property(parent, rprop, );
> > - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > + if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> !of_isa_indirect_io(parent)) {
> > pr_debug("OF: no ranges; cannot translate\n");
> > return 1;
> > }
>
> I don't see what effect that would have. What do you want to
> achieve with this?
If I read the code correctly adding the function above would end
up in a 1:1 mapping:
http://lxr.free-electrons.com/source/drivers/of/address.c#L513
so taddr will be assigned with the cpu address space specified
in the children nodes of LPC and we are not using a quirk function
(we are just checking that we have the indirect io assigned and
that we are on a ISA bus). Now probably there is a nit in my
code sketch where of_isa_indirect_io should be probably an architecture
specific function...
>
> I think all we need from this function is to return '1' if
> we hit an ISA I/O window, and that should happen for the two
> interesting cases, either no 'ranges' at all, or no translation
> for the range in question, so that __of_translate_address can
> return OF_BAD_ADDR, and we can enter the special case
> handling in the caller, that handles it like
>
I don't think this is very right as you may fail for different
reasons other than a missing range property, e.g:
http://lxr.free-electrons.com/source/drivers/of/address.c#L575
And even if the only failure case was a missing range if in the
future __of_translate_address had to be reworked we would again
make a wrong assumption...you get my point?
Thanks
Gab
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903fe9d2..a18d96843fae 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -685,17 +685,24 @@ static int __of_address_to_resource(struct
> device_node *dev,
> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> return -EINVAL;
> taddr = of_translate_address(dev, addrp);
> - if (taddr == OF_BAD_ADDR)
> - return -EINVAL;
> memset(r, 0, sizeof(struct resource));
> +
> if (flags & IORESOURCE_IO) {
> unsigned long port;
> - port = pci_address_to_pio(taddr);
> +
> + if (taddr == OF_BAD_A
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: 22 September 2016 13:15
> To: Gabriele Paoloni
> Cc: zhichang; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org;
> linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry;
> will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;
> Linuxarm; xuwei (O); linux-ser...@vger.kernel.org;
> b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com;
> kant...@163.com
> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
> Hip06
>
> On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni
> wrote:
> > > > I think extending of_empty_ranges_quirk() may be a reasonable
> > > solution.
> > > > What do you think Arnd?
> > >
> > > I don't really like that idea, that quirk is meant to work around
> > > broken DTs, but we can just make the DT valid and implement the
> > > code properly.
> >
> > Ok I understand your point where it is not right to use
> of_empty_ranges_quirk()
> > As a quirk is used to work around broken HW or broken FW (as in this
> case)
> > rather than to fix code
> >
> > What about the following? I think adding the check you suggested next
> to
> > of_empty_ranges_quirk() is adding the case we need in the right point
> (thus
> > avoiding any duplication)
> >
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct
> device_node *np)
> > return NULL;
> > }
> >
> > +static inline int of_isa_indirect_io(struct device_node *np)
> > +{
> > + /*
> > +* check if the current node is an isa bus and if indirectio
> operation
> > +* are registered
> > +*/
> > + return (of_bus_isa_match(np) && arm64_extio_ops);
> > +}
> > +
> > static int of_empty_ranges_quirk(struct device_node *np)
> > {
> > if (IS_ENABLED(CONFIG_PPC)) {
> > @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
> > * This code is only enabled on powerpc. --gcl
> > */
> > ranges = of_get_property(parent, rprop, );
> > - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> > + if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> !of_isa_indirect_io(parent)) {
> > pr_debug("OF: no ranges; cannot translate\n");
> > return 1;
> > }
>
> I don't see what effect that would have. What do you want to
> achieve with this?
If I read the code correctly adding the function above would end
up in a 1:1 mapping:
http://lxr.free-electrons.com/source/drivers/of/address.c#L513
so taddr will be assigned with the cpu address space specified
in the children nodes of LPC and we are not using a quirk function
(we are just checking that we have the indirect io assigned and
that we are on a ISA bus). Now probably there is a nit in my
code sketch where of_isa_indirect_io should be probably an architecture
specific function...
>
> I think all we need from this function is to return '1' if
> we hit an ISA I/O window, and that should happen for the two
> interesting cases, either no 'ranges' at all, or no translation
> for the range in question, so that __of_translate_address can
> return OF_BAD_ADDR, and we can enter the special case
> handling in the caller, that handles it like
>
I don't think this is very right as you may fail for different
reasons other than a missing range property, e.g:
http://lxr.free-electrons.com/source/drivers/of/address.c#L575
And even if the only failure case was a missing range if in the
future __of_translate_address had to be reworked we would again
make a wrong assumption...you get my point?
Thanks
Gab
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903fe9d2..a18d96843fae 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -685,17 +685,24 @@ static int __of_address_to_resource(struct
> device_node *dev,
> if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> return -EINVAL;
> taddr = of_translate_address(dev, addrp);
> - if (taddr == OF_BAD_ADDR)
> - return -EINVAL;
> memset(r, 0, sizeof(struct resource));
> +
> if (flags & IORESOURCE_IO) {
> unsigned long port;
> - port = pci_address_to_pio(taddr);
> +
> + if (taddr == OF_BAD_A
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote:
> > > I think extending of_empty_ranges_quirk() may be a reasonable
> > solution.
> > > What do you think Arnd?
> >
> > I don't really like that idea, that quirk is meant to work around
> > broken DTs, but we can just make the DT valid and implement the
> > code properly.
>
> Ok I understand your point where it is not right to use
> of_empty_ranges_quirk()
> As a quirk is used to work around broken HW or broken FW (as in this case)
> rather than to fix code
>
> What about the following? I think adding the check you suggested next to
> of_empty_ranges_quirk() is adding the case we need in the right point (thus
> avoiding any duplication)
>
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node
> *np)
> return NULL;
> }
>
> +static inline int of_isa_indirect_io(struct device_node *np)
> +{
> + /*
> +* check if the current node is an isa bus and if indirectio operation
> +* are registered
> +*/
> + return (of_bus_isa_match(np) && arm64_extio_ops);
> +}
> +
> static int of_empty_ranges_quirk(struct device_node *np)
> {
> if (IS_ENABLED(CONFIG_PPC)) {
> @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent,
> struct of_bus *bus,
> * This code is only enabled on powerpc. --gcl
> */
> ranges = of_get_property(parent, rprop, );
> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> + if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> !of_isa_indirect_io(parent)) {
> pr_debug("OF: no ranges; cannot translate\n");
> return 1;
> }
I don't see what effect that would have. What do you want to
achieve with this?
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..a18d96843fae 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node
*dev,
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return -EINVAL;
taddr = of_translate_address(dev, addrp);
- if (taddr == OF_BAD_ADDR)
- return -EINVAL;
memset(r, 0, sizeof(struct resource));
+
if (flags & IORESOURCE_IO) {
unsigned long port;
- port = pci_address_to_pio(taddr);
+
+ if (taddr == OF_BAD_ADDR)
+ port = arch_of_address_to_pio(dev, addrp)
+ else
+ port = pci_address_to_pio(taddr);
+
if (port == (unsigned long)-1)
return -EINVAL;
r->start = port;
r->end = port + size - 1;
} else {
+ if (taddr == OF_BAD_ADDR)
+ return -EINVAL;
+
r->start = taddr;
r->end = taddr + size - 1;
}
Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote:
> > > I think extending of_empty_ranges_quirk() may be a reasonable
> > solution.
> > > What do you think Arnd?
> >
> > I don't really like that idea, that quirk is meant to work around
> > broken DTs, but we can just make the DT valid and implement the
> > code properly.
>
> Ok I understand your point where it is not right to use
> of_empty_ranges_quirk()
> As a quirk is used to work around broken HW or broken FW (as in this case)
> rather than to fix code
>
> What about the following? I think adding the check you suggested next to
> of_empty_ranges_quirk() is adding the case we need in the right point (thus
> avoiding any duplication)
>
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node
> *np)
> return NULL;
> }
>
> +static inline int of_isa_indirect_io(struct device_node *np)
> +{
> + /*
> +* check if the current node is an isa bus and if indirectio operation
> +* are registered
> +*/
> + return (of_bus_isa_match(np) && arm64_extio_ops);
> +}
> +
> static int of_empty_ranges_quirk(struct device_node *np)
> {
> if (IS_ENABLED(CONFIG_PPC)) {
> @@ -503,7 +512,7 @@ static int of_translate_one(struct device_node *parent,
> struct of_bus *bus,
> * This code is only enabled on powerpc. --gcl
> */
> ranges = of_get_property(parent, rprop, );
> - if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> + if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
> !of_isa_indirect_io(parent)) {
> pr_debug("OF: no ranges; cannot translate\n");
> return 1;
> }
I don't see what effect that would have. What do you want to
achieve with this?
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..a18d96843fae 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node
*dev,
if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return -EINVAL;
taddr = of_translate_address(dev, addrp);
- if (taddr == OF_BAD_ADDR)
- return -EINVAL;
memset(r, 0, sizeof(struct resource));
+
if (flags & IORESOURCE_IO) {
unsigned long port;
- port = pci_address_to_pio(taddr);
+
+ if (taddr == OF_BAD_ADDR)
+ port = arch_of_address_to_pio(dev, addrp)
+ else
+ port = pci_address_to_pio(taddr);
+
if (port == (unsigned long)-1)
return -EINVAL;
r->start = port;
r->end = port + size - 1;
} else {
+ if (taddr == OF_BAD_ADDR)
+ return -EINVAL;
+
r->start = taddr;
r->end = taddr + size - 1;
}
Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 21 September 2016 21:18 > To: Gabriele Paoloni > Cc: zhichang; linux-arm-ker...@lists.infradead.org; > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com; > kant...@163.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni > wrote: > > > -Original Message- > > > From: zhichang [mailto:zhichang.yua...@gmail.com] > > > On 2016年09月15日 20:24, Arnd Bergmann wrote: > > > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni > > > wrote: > > > >>> -Original Message- > > > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele > Paoloni > > > wrote: > > > >> I think that maybe having the 1:1 range mapping doesn't > > > >> reflect well the reality but it is the less painful > > > >> solution... > > > >> > > > >> What's your view? > > > > > > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > > > > and that should be enough to translate the I/O port number. > > > > > > > > The only part we need to change here is to not go through > > > > the crazy conversion all the way from PCI I/O space to a > > > > physical address and back to a (logical) port number > > > > that we do today with of_translate_address/pci_address_to_pio. > > > > > > > Sorry for the late response! Several days' leave > > > Do you want to bypass of_translate_address and pci_address_to_pio > for > > > the registered specific PIO? > > > I think the bypass for of_translate_address is ok, but worry some > new > > > issues will emerge without the > > > conversion between physical address and logical/linux port number. > > The same function that handles the non-translated region would > do that conversion. > > > > When PCI host bridge which support IO operations is configured and > > > enabled, the pci_address_to_pio will > > > populate the logical IO range from ZERO for the first host bridge. > Our > > > LPC will also use part of the IO range > > > started from ZERO. It will make in/out enter the wrong branch > possibly. > > > > > > In V2, the 0 - 0x1000 logical IO range is reserved for LPC use > only. > > > But it seems not so good. In this way, > > > PCI has no chance to use low 4K IO range(logical). > > > > > > So, in V3, applying the conversion from physical/cpu address to > > > logical/linux IO port for any IO ranges, > > > including the LPC, but recorded the logical IO range for LPC. When > > > calling in/out with a logical port address, > > > we can check this port fall into LPC logical IO range and get back > the > > > real IO. > > Right, and the same translation can be used in > __of_address_to_resource() > going the opposite way. > > > > Do you have further comments about this?? > > > > I think there are two separate issues to be discussed: > > > > The first issue is about having of_translate_address failing due to > > "range" missing. About this Arnd suggested that it is not appropriate > > to have a range describing a bridge 1:1 mapping and this was > discussed > > before in this thread. Arnd had a suggestion about this (see below) > > however (looking twice at the code) it seems to me that such solution > > would lead to quite some duplication from __of_translate_address() > > in order to retrieve the actual addr from dt... > > I don't think we need to duplicate much, we can probably safely > assume that there are no nontrivial ranges in devices below the LPC > node, so we just walk up the bus to see if the node is a child > (or possibly grandchild etc) of the LPC bus, and treat any IO port > number under there as a physical port number, which has a known > offset from the Linux I/O port number. > > > I think extending of_empty_ranges_quirk() may be a reasonable > solution. > > What do you think Arnd? > > I don't really like that idea, that quirk is meant to work around > broken DTs, but we can just make the DT valid and impl
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 21 September 2016 21:18 > To: Gabriele Paoloni > Cc: zhichang; linux-arm-ker...@lists.infradead.org; > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > linux-...@vger.kernel.org; gre...@linuxfoundation.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-ser...@vger.kernel.org; > b...@kernel.crashing.org; zourongr...@gmail.com; liviu.du...@arm.com; > kant...@163.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni > wrote: > > > -Original Message- > > > From: zhichang [mailto:zhichang.yua...@gmail.com] > > > On 2016年09月15日 20:24, Arnd Bergmann wrote: > > > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni > > > wrote: > > > >>> -Original Message- > > > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele > Paoloni > > > wrote: > > > >> I think that maybe having the 1:1 range mapping doesn't > > > >> reflect well the reality but it is the less painful > > > >> solution... > > > >> > > > >> What's your view? > > > > > > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > > > > and that should be enough to translate the I/O port number. > > > > > > > > The only part we need to change here is to not go through > > > > the crazy conversion all the way from PCI I/O space to a > > > > physical address and back to a (logical) port number > > > > that we do today with of_translate_address/pci_address_to_pio. > > > > > > > Sorry for the late response! Several days' leave > > > Do you want to bypass of_translate_address and pci_address_to_pio > for > > > the registered specific PIO? > > > I think the bypass for of_translate_address is ok, but worry some > new > > > issues will emerge without the > > > conversion between physical address and logical/linux port number. > > The same function that handles the non-translated region would > do that conversion. > > > > When PCI host bridge which support IO operations is configured and > > > enabled, the pci_address_to_pio will > > > populate the logical IO range from ZERO for the first host bridge. > Our > > > LPC will also use part of the IO range > > > started from ZERO. It will make in/out enter the wrong branch > possibly. > > > > > > In V2, the 0 - 0x1000 logical IO range is reserved for LPC use > only. > > > But it seems not so good. In this way, > > > PCI has no chance to use low 4K IO range(logical). > > > > > > So, in V3, applying the conversion from physical/cpu address to > > > logical/linux IO port for any IO ranges, > > > including the LPC, but recorded the logical IO range for LPC. When > > > calling in/out with a logical port address, > > > we can check this port fall into LPC logical IO range and get back > the > > > real IO. > > Right, and the same translation can be used in > __of_address_to_resource() > going the opposite way. > > > > Do you have further comments about this?? > > > > I think there are two separate issues to be discussed: > > > > The first issue is about having of_translate_address failing due to > > "range" missing. About this Arnd suggested that it is not appropriate > > to have a range describing a bridge 1:1 mapping and this was > discussed > > before in this thread. Arnd had a suggestion about this (see below) > > however (looking twice at the code) it seems to me that such solution > > would lead to quite some duplication from __of_translate_address() > > in order to retrieve the actual addr from dt... > > I don't think we need to duplicate much, we can probably safely > assume that there are no nontrivial ranges in devices below the LPC > node, so we just walk up the bus to see if the node is a child > (or possibly grandchild etc) of the LPC bus, and treat any IO port > number under there as a physical port number, which has a known > offset from the Linux I/O port number. > > > I think extending of_empty_ranges_quirk() may be a reasonable > solution. > > What do you think Arnd? > > I don't really like that idea, that quirk is meant to work around > broken DTs, but we can just make the DT valid and impl
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni wrote: > > -Original Message- > > From: zhichang [mailto:zhichang.yua...@gmail.com] > > On 2016年09月15日 20:24, Arnd Bergmann wrote: > > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni > > wrote: > > >>> -Original Message- > > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni > > wrote: > > >> I think that maybe having the 1:1 range mapping doesn't > > >> reflect well the reality but it is the less painful > > >> solution... > > >> > > >> What's your view? > > > > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > > > and that should be enough to translate the I/O port number. > > > > > > The only part we need to change here is to not go through > > > the crazy conversion all the way from PCI I/O space to a > > > physical address and back to a (logical) port number > > > that we do today with of_translate_address/pci_address_to_pio. > > > > > Sorry for the late response! Several days' leave > > Do you want to bypass of_translate_address and pci_address_to_pio for > > the registered specific PIO? > > I think the bypass for of_translate_address is ok, but worry some new > > issues will emerge without the > > conversion between physical address and logical/linux port number. The same function that handles the non-translated region would do that conversion. > > When PCI host bridge which support IO operations is configured and > > enabled, the pci_address_to_pio will > > populate the logical IO range from ZERO for the first host bridge. Our > > LPC will also use part of the IO range > > started from ZERO. It will make in/out enter the wrong branch possibly. > > > > In V2, the 0 - 0x1000 logical IO range is reserved for LPC use only. > > But it seems not so good. In this way, > > PCI has no chance to use low 4K IO range(logical). > > > > So, in V3, applying the conversion from physical/cpu address to > > logical/linux IO port for any IO ranges, > > including the LPC, but recorded the logical IO range for LPC. When > > calling in/out with a logical port address, > > we can check this port fall into LPC logical IO range and get back the > > real IO. Right, and the same translation can be used in __of_address_to_resource() going the opposite way. > > Do you have further comments about this?? > > I think there are two separate issues to be discussed: > > The first issue is about having of_translate_address failing due to > "range" missing. About this Arnd suggested that it is not appropriate > to have a range describing a bridge 1:1 mapping and this was discussed > before in this thread. Arnd had a suggestion about this (see below) > however (looking twice at the code) it seems to me that such solution > would lead to quite some duplication from __of_translate_address() > in order to retrieve the actual addr from dt... I don't think we need to duplicate much, we can probably safely assume that there are no nontrivial ranges in devices below the LPC node, so we just walk up the bus to see if the node is a child (or possibly grandchild etc) of the LPC bus, and treat any IO port number under there as a physical port number, which has a known offset from the Linux I/O port number. > I think extending of_empty_ranges_quirk() may be a reasonable solution. > What do you think Arnd? I don't really like that idea, that quirk is meant to work around broken DTs, but we can just make the DT valid and implement the code properly. > The second issue is a conflict between cpu addresses used by the LPC > controller and i/o tokens from pci endpoints. > > About this what if we modify armn64_extio_ops to have a list of ranges > rather than only one range (now we have just start/end); then in the > LPC driver we can scan the LPC child devices and > 1) populate such list of ranges > 2) call pci_register_io_range for such ranges Scanning the child devices sounds really wrong, please register just one range that covers the bus to keep the workaround as simple as possible. > Then when calling __of_address_to_resource we retrieve I/O tokens > for the devices on top of the LPC driver and in the I/O accessors > we call pci_pio_to_address to figure out the cpu address and compare > it to the list of ranges in armn64_extio_ops. > > What about this? That seems really complex for something that can be quite simple. The only thing we need to worry about is that the io_range_list contains an entry for the LPC bus so we don't conflict with the PCI buses. Arnd Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni wrote: > > -Original Message- > > From: zhichang [mailto:zhichang.yua...@gmail.com] > > On 2016年09月15日 20:24, Arnd Bergmann wrote: > > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni > > wrote: > > >>> -Original Message- > > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni > > wrote: > > >> I think that maybe having the 1:1 range mapping doesn't > > >> reflect well the reality but it is the less painful > > >> solution... > > >> > > >> What's your view? > > > > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > > > and that should be enough to translate the I/O port number. > > > > > > The only part we need to change here is to not go through > > > the crazy conversion all the way from PCI I/O space to a > > > physical address and back to a (logical) port number > > > that we do today with of_translate_address/pci_address_to_pio. > > > > > Sorry for the late response! Several days' leave > > Do you want to bypass of_translate_address and pci_address_to_pio for > > the registered specific PIO? > > I think the bypass for of_translate_address is ok, but worry some new > > issues will emerge without the > > conversion between physical address and logical/linux port number. The same function that handles the non-translated region would do that conversion. > > When PCI host bridge which support IO operations is configured and > > enabled, the pci_address_to_pio will > > populate the logical IO range from ZERO for the first host bridge. Our > > LPC will also use part of the IO range > > started from ZERO. It will make in/out enter the wrong branch possibly. > > > > In V2, the 0 - 0x1000 logical IO range is reserved for LPC use only. > > But it seems not so good. In this way, > > PCI has no chance to use low 4K IO range(logical). > > > > So, in V3, applying the conversion from physical/cpu address to > > logical/linux IO port for any IO ranges, > > including the LPC, but recorded the logical IO range for LPC. When > > calling in/out with a logical port address, > > we can check this port fall into LPC logical IO range and get back the > > real IO. Right, and the same translation can be used in __of_address_to_resource() going the opposite way. > > Do you have further comments about this?? > > I think there are two separate issues to be discussed: > > The first issue is about having of_translate_address failing due to > "range" missing. About this Arnd suggested that it is not appropriate > to have a range describing a bridge 1:1 mapping and this was discussed > before in this thread. Arnd had a suggestion about this (see below) > however (looking twice at the code) it seems to me that such solution > would lead to quite some duplication from __of_translate_address() > in order to retrieve the actual addr from dt... I don't think we need to duplicate much, we can probably safely assume that there are no nontrivial ranges in devices below the LPC node, so we just walk up the bus to see if the node is a child (or possibly grandchild etc) of the LPC bus, and treat any IO port number under there as a physical port number, which has a known offset from the Linux I/O port number. > I think extending of_empty_ranges_quirk() may be a reasonable solution. > What do you think Arnd? I don't really like that idea, that quirk is meant to work around broken DTs, but we can just make the DT valid and implement the code properly. > The second issue is a conflict between cpu addresses used by the LPC > controller and i/o tokens from pci endpoints. > > About this what if we modify armn64_extio_ops to have a list of ranges > rather than only one range (now we have just start/end); then in the > LPC driver we can scan the LPC child devices and > 1) populate such list of ranges > 2) call pci_register_io_range for such ranges Scanning the child devices sounds really wrong, please register just one range that covers the bus to keep the workaround as simple as possible. > Then when calling __of_address_to_resource we retrieve I/O tokens > for the devices on top of the LPC driver and in the I/O accessors > we call pci_pio_to_address to figure out the cpu address and compare > it to the list of ranges in armn64_extio_ops. > > What about this? That seems really complex for something that can be quite simple. The only thing we need to worry about is that the io_range_list contains an entry for the LPC bus so we don't conflict with the PCI buses. Arnd Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Zhichang > -Original Message- > From: zhichang [mailto:zhichang.yua...@gmail.com] > Sent: 21 September 2016 11:09 > To: Arnd Bergmann; linux-arm-ker...@lists.infradead.org > Cc: Gabriele Paoloni; devicet...@vger.kernel.org; > lorenzo.pieral...@arm.com; miny...@acm.org; linux-...@vger.kernel.org; > gre...@linuxfoundation.org; John Garry; will.dea...@arm.com; linux- > ker...@vger.kernel.org; Yuanzhichang; Linuxarm; xuwei (O); linux- > ser...@vger.kernel.org; b...@kernel.crashing.org; > zourongr...@gmail.com; liviu.du...@arm.com; kant...@163.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > Hi, Arnd, > > > > On 2016年09月15日 20:24, Arnd Bergmann wrote: > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni > wrote: > >>> -Original Message- > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni > wrote: > >>>> > >>>> From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > >>>> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > >>>> > >>>> I quote: > >>>> "There shall be an entry in the "ranges" property for each > >>>> of the Memory and/or I/O spaces if that address space is > >>>> mapped through the bridge." > >>>> > >>>> It seems to me that it is ok to have 1:1 address mapping and that > >>>> therefore of_translate_address() should fail if "ranges" is not > >>>> present. > >>> > >>> The key here is the definition of "mapped through the bridge". > >>> I can only understand this as "directly mapped", i.e. an I/O > >>> port of the child bus corresponds directly to a memory address > >>> on the parent bus, but this is not the case here. > >>> > >>> The problem with adding the mapping here is that it looks > >>> like it should be valid to create a page table entry for > >>> the address returned from the translation and access it through > >>> a pointer dereference, but that is clearly not possible. > >> > >> I understand that somehow we are abusing of the ranges property > >> here however the point is that with the current implementation > ranges > >> is needed because otherwise the ipmi driver probe will fail here: > >> > >> of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource > >> -> of_translate_address -> __of_translate_address > >> > >> Now we had a bit of discussion internally and to avoid > >> having ranges we came up with two possible solutions: > >> > >> 1) Using bit 3 of phys.hi cell in 2.2.1 of > >> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > >> This would mean reworking of_bus_isa_get_flags in > >> http://lxr.free-electrons.com/source/drivers/of/address.c#L398 > >> and setting a new flag to be checked in __of_address_to_resource > >> > >> 2) Adding a property in the bindings of each device that is > >> a child of our LPC bus and modify __of_address_to_resource > >> to check if the property is in the DT and eventually bypass > >> of_translate_address > >> > >> However in both 1) and 2) there are some issues: > >> in 1) we are not complying with the isa binding doc (we use > >> a bit that should be zero); in 2) we need to modify the > >> bindings documentation of the devices that are connected > >> to our LPC controller (therefore modifying other devices > >> bindings to fit our special case). > >> > >> I think that maybe having the 1:1 range mapping doesn't > >> reflect well the reality but it is the less painful > >> solution... > >> > >> What's your view? > > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > > and that should be enough to translate the I/O port number. > > > > The only part we need to change here is to not go through > > the crazy conversion all the way from PCI I/O space to a > > physical address and back to a (logical) port number > > that we do today with of_translate_address/pci_address_to_pio. > > > Sorry for the late response! Several days' leave > Do you want to bypass of_translate_address and pci_address_to_pio for > the registered specific PIO? > I think the bypass for of_translate_address is ok, but worry some new > issues will emerge without the > conversion between physical address and logical/l
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Zhichang > -Original Message- > From: zhichang [mailto:zhichang.yua...@gmail.com] > Sent: 21 September 2016 11:09 > To: Arnd Bergmann; linux-arm-ker...@lists.infradead.org > Cc: Gabriele Paoloni; devicet...@vger.kernel.org; > lorenzo.pieral...@arm.com; miny...@acm.org; linux-...@vger.kernel.org; > gre...@linuxfoundation.org; John Garry; will.dea...@arm.com; linux- > ker...@vger.kernel.org; Yuanzhichang; Linuxarm; xuwei (O); linux- > ser...@vger.kernel.org; b...@kernel.crashing.org; > zourongr...@gmail.com; liviu.du...@arm.com; kant...@163.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > Hi, Arnd, > > > > On 2016年09月15日 20:24, Arnd Bergmann wrote: > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni > wrote: > >>> -Original Message- > >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni > wrote: > >>>> > >>>> From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > >>>> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > >>>> > >>>> I quote: > >>>> "There shall be an entry in the "ranges" property for each > >>>> of the Memory and/or I/O spaces if that address space is > >>>> mapped through the bridge." > >>>> > >>>> It seems to me that it is ok to have 1:1 address mapping and that > >>>> therefore of_translate_address() should fail if "ranges" is not > >>>> present. > >>> > >>> The key here is the definition of "mapped through the bridge". > >>> I can only understand this as "directly mapped", i.e. an I/O > >>> port of the child bus corresponds directly to a memory address > >>> on the parent bus, but this is not the case here. > >>> > >>> The problem with adding the mapping here is that it looks > >>> like it should be valid to create a page table entry for > >>> the address returned from the translation and access it through > >>> a pointer dereference, but that is clearly not possible. > >> > >> I understand that somehow we are abusing of the ranges property > >> here however the point is that with the current implementation > ranges > >> is needed because otherwise the ipmi driver probe will fail here: > >> > >> of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource > >> -> of_translate_address -> __of_translate_address > >> > >> Now we had a bit of discussion internally and to avoid > >> having ranges we came up with two possible solutions: > >> > >> 1) Using bit 3 of phys.hi cell in 2.2.1 of > >> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > >> This would mean reworking of_bus_isa_get_flags in > >> http://lxr.free-electrons.com/source/drivers/of/address.c#L398 > >> and setting a new flag to be checked in __of_address_to_resource > >> > >> 2) Adding a property in the bindings of each device that is > >> a child of our LPC bus and modify __of_address_to_resource > >> to check if the property is in the DT and eventually bypass > >> of_translate_address > >> > >> However in both 1) and 2) there are some issues: > >> in 1) we are not complying with the isa binding doc (we use > >> a bit that should be zero); in 2) we need to modify the > >> bindings documentation of the devices that are connected > >> to our LPC controller (therefore modifying other devices > >> bindings to fit our special case). > >> > >> I think that maybe having the 1:1 range mapping doesn't > >> reflect well the reality but it is the less painful > >> solution... > >> > >> What's your view? > > > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > > and that should be enough to translate the I/O port number. > > > > The only part we need to change here is to not go through > > the crazy conversion all the way from PCI I/O space to a > > physical address and back to a (logical) port number > > that we do today with of_translate_address/pci_address_to_pio. > > > Sorry for the late response! Several days' leave > Do you want to bypass of_translate_address and pci_address_to_pio for > the registered specific PIO? > I think the bypass for of_translate_address is ok, but worry some new > issues will emerge without the > conversion between physical address and logical/l
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi, Arnd, On 2016年09月15日 20:24, Arnd Bergmann wrote: > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni wrote: >>> -Original Message- >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote: From <<3.1.1. Open Firmware Properties for Bus Nodes>> in http://www.firmware.org/1275/bindings/isa/isa0_4d.ps I quote: "There shall be an entry in the "ranges" property for each of the Memory and/or I/O spaces if that address space is mapped through the bridge." It seems to me that it is ok to have 1:1 address mapping and that therefore of_translate_address() should fail if "ranges" is not present. >>> >>> The key here is the definition of "mapped through the bridge". >>> I can only understand this as "directly mapped", i.e. an I/O >>> port of the child bus corresponds directly to a memory address >>> on the parent bus, but this is not the case here. >>> >>> The problem with adding the mapping here is that it looks >>> like it should be valid to create a page table entry for >>> the address returned from the translation and access it through >>> a pointer dereference, but that is clearly not possible. >> >> I understand that somehow we are abusing of the ranges property >> here however the point is that with the current implementation ranges >> is needed because otherwise the ipmi driver probe will fail here: >> >> of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource >> -> of_translate_address -> __of_translate_address >> >> Now we had a bit of discussion internally and to avoid >> having ranges we came up with two possible solutions: >> >> 1) Using bit 3 of phys.hi cell in 2.2.1 of >> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps >> This would mean reworking of_bus_isa_get_flags in >> http://lxr.free-electrons.com/source/drivers/of/address.c#L398 >> and setting a new flag to be checked in __of_address_to_resource >> >> 2) Adding a property in the bindings of each device that is >> a child of our LPC bus and modify __of_address_to_resource >> to check if the property is in the DT and eventually bypass >> of_translate_address >> >> However in both 1) and 2) there are some issues: >> in 1) we are not complying with the isa binding doc (we use >> a bit that should be zero); in 2) we need to modify the >> bindings documentation of the devices that are connected >> to our LPC controller (therefore modifying other devices >> bindings to fit our special case). >> >> I think that maybe having the 1:1 range mapping doesn't >> reflect well the reality but it is the less painful >> solution... >> >> What's your view? > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > and that should be enough to translate the I/O port number. > > The only part we need to change here is to not go through > the crazy conversion all the way from PCI I/O space to a > physical address and back to a (logical) port number > that we do today with of_translate_address/pci_address_to_pio. > Sorry for the late response! Several days' leave Do you want to bypass of_translate_address and pci_address_to_pio for the registered specific PIO? I think the bypass for of_translate_address is ok, but worry some new issues will emerge without the conversion between physical address and logical/linux port number. When PCI host bridge which support IO operations is configured and enabled, the pci_address_to_pio will populate the logical IO range from ZERO for the first host bridge. Our LPC will also use part of the IO range started from ZERO. It will make in/out enter the wrong branch possibly. In V2, the 0 - 0x1000 logical IO range is reserved for LPC use only. But it seems not so good. In this way, PCI has no chance to use low 4K IO range(logical). So, in V3, applying the conversion from physical/cpu address to logical/linux IO port for any IO ranges, including the LPC, but recorded the logical IO range for LPC. When calling in/out with a logical port address, we can check this port fall into LPC logical IO range and get back the real IO. Do you have further comments about this?? > I can think of a several of ways to fix __of_address_to_resource > to just do the right thing according to the ISA binding to > make the normal drivers work. > > The easiest solution is probably to hook into the > "taddr == OF_BAD_ADDR" case in __of_address_to_resource > and add a lookup for ISA buses there, and instead check > if some special I/O port operations were registered > for the port number, using an architecture specific > function that arm64 implements. Other architectures > like x86 that don't have a direct mapping between I/O > ports and MMIO addresses would implement that same > function differently. What about add the specific quirk for Hip06 LPC in of_empty_ranges_quirk()?? you know, there are several cases in which of_translate_address return OF_BAD_ADDR. And if we only check the special port range,
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi, Arnd, On 2016年09月15日 20:24, Arnd Bergmann wrote: > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni wrote: >>> -Original Message- >>> On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote: From <<3.1.1. Open Firmware Properties for Bus Nodes>> in http://www.firmware.org/1275/bindings/isa/isa0_4d.ps I quote: "There shall be an entry in the "ranges" property for each of the Memory and/or I/O spaces if that address space is mapped through the bridge." It seems to me that it is ok to have 1:1 address mapping and that therefore of_translate_address() should fail if "ranges" is not present. >>> >>> The key here is the definition of "mapped through the bridge". >>> I can only understand this as "directly mapped", i.e. an I/O >>> port of the child bus corresponds directly to a memory address >>> on the parent bus, but this is not the case here. >>> >>> The problem with adding the mapping here is that it looks >>> like it should be valid to create a page table entry for >>> the address returned from the translation and access it through >>> a pointer dereference, but that is clearly not possible. >> >> I understand that somehow we are abusing of the ranges property >> here however the point is that with the current implementation ranges >> is needed because otherwise the ipmi driver probe will fail here: >> >> of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource >> -> of_translate_address -> __of_translate_address >> >> Now we had a bit of discussion internally and to avoid >> having ranges we came up with two possible solutions: >> >> 1) Using bit 3 of phys.hi cell in 2.2.1 of >> http://www.firmware.org/1275/bindings/isa/isa0_4d.ps >> This would mean reworking of_bus_isa_get_flags in >> http://lxr.free-electrons.com/source/drivers/of/address.c#L398 >> and setting a new flag to be checked in __of_address_to_resource >> >> 2) Adding a property in the bindings of each device that is >> a child of our LPC bus and modify __of_address_to_resource >> to check if the property is in the DT and eventually bypass >> of_translate_address >> >> However in both 1) and 2) there are some issues: >> in 1) we are not complying with the isa binding doc (we use >> a bit that should be zero); in 2) we need to modify the >> bindings documentation of the devices that are connected >> to our LPC controller (therefore modifying other devices >> bindings to fit our special case). >> >> I think that maybe having the 1:1 range mapping doesn't >> reflect well the reality but it is the less painful >> solution... >> >> What's your view? > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > and that should be enough to translate the I/O port number. > > The only part we need to change here is to not go through > the crazy conversion all the way from PCI I/O space to a > physical address and back to a (logical) port number > that we do today with of_translate_address/pci_address_to_pio. > Sorry for the late response! Several days' leave Do you want to bypass of_translate_address and pci_address_to_pio for the registered specific PIO? I think the bypass for of_translate_address is ok, but worry some new issues will emerge without the conversion between physical address and logical/linux port number. When PCI host bridge which support IO operations is configured and enabled, the pci_address_to_pio will populate the logical IO range from ZERO for the first host bridge. Our LPC will also use part of the IO range started from ZERO. It will make in/out enter the wrong branch possibly. In V2, the 0 - 0x1000 logical IO range is reserved for LPC use only. But it seems not so good. In this way, PCI has no chance to use low 4K IO range(logical). So, in V3, applying the conversion from physical/cpu address to logical/linux IO port for any IO ranges, including the LPC, but recorded the logical IO range for LPC. When calling in/out with a logical port address, we can check this port fall into LPC logical IO range and get back the real IO. Do you have further comments about this?? > I can think of a several of ways to fix __of_address_to_resource > to just do the right thing according to the ISA binding to > make the normal drivers work. > > The easiest solution is probably to hook into the > "taddr == OF_BAD_ADDR" case in __of_address_to_resource > and add a lookup for ISA buses there, and instead check > if some special I/O port operations were registered > for the port number, using an architecture specific > function that arm64 implements. Other architectures > like x86 that don't have a direct mapping between I/O > ports and MMIO addresses would implement that same > function differently. What about add the specific quirk for Hip06 LPC in of_empty_ranges_quirk()?? you know, there are several cases in which of_translate_address return OF_BAD_ADDR. And if we only check the special port range,
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
> -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 15 September 2016 13:25 > To: linux-arm-ker...@lists.infradead.org > Cc: Gabriele Paoloni; devicet...@vger.kernel.org; > lorenzo.pieral...@arm.com; miny...@acm.org; linux-...@vger.kernel.org; > gre...@linuxfoundation.org; John Garry; will.dea...@arm.com; linux- > ker...@vger.kernel.org; Yuanzhichang; Linuxarm; xuwei (O); linux- > ser...@vger.kernel.org; b...@kernel.crashing.org; > zourongr...@gmail.com; liviu.du...@arm.com; kant...@163.com; > zhichang.yua...@gmail.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni > wrote: > > > -Original Message- > > > On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni > wrote: > > > > > > > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > > > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > > > > > > I quote: > > > > "There shall be an entry in the "ranges" property for each > > > > of the Memory and/or I/O spaces if that address space is > > > > mapped through the bridge." > > > > > > > > It seems to me that it is ok to have 1:1 address mapping and that > > > > therefore of_translate_address() should fail if "ranges" is not > > > > present. > > > > > > The key here is the definition of "mapped through the bridge". > > > I can only understand this as "directly mapped", i.e. an I/O > > > port of the child bus corresponds directly to a memory address > > > on the parent bus, but this is not the case here. > > > > > > The problem with adding the mapping here is that it looks > > > like it should be valid to create a page table entry for > > > the address returned from the translation and access it through > > > a pointer dereference, but that is clearly not possible. > > > > I understand that somehow we are abusing of the ranges property > > here however the point is that with the current implementation ranges > > is needed because otherwise the ipmi driver probe will fail here: > > > > of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource > > -> of_translate_address -> __of_translate_address > > > > Now we had a bit of discussion internally and to avoid > > having ranges we came up with two possible solutions: > > > > 1) Using bit 3 of phys.hi cell in 2.2.1 of > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > This would mean reworking of_bus_isa_get_flags in > > http://lxr.free-electrons.com/source/drivers/of/address.c#L398 > > and setting a new flag to be checked in __of_address_to_resource > > > > 2) Adding a property in the bindings of each device that is > > a child of our LPC bus and modify __of_address_to_resource > > to check if the property is in the DT and eventually bypass > > of_translate_address > > > > However in both 1) and 2) there are some issues: > > in 1) we are not complying with the isa binding doc (we use > > a bit that should be zero); in 2) we need to modify the > > bindings documentation of the devices that are connected > > to our LPC controller (therefore modifying other devices > > bindings to fit our special case). > > > > I think that maybe having the 1:1 range mapping doesn't > > reflect well the reality but it is the less painful > > solution... > > > > What's your view? > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > and that should be enough to translate the I/O port number. > > The only part we need to change here is to not go through > the crazy conversion all the way from PCI I/O space to a > physical address and back to a (logical) port number > that we do today with of_translate_address/pci_address_to_pio. > > I can think of a several of ways to fix __of_address_to_resource > to just do the right thing according to the ISA binding to > make the normal drivers work. > > The easiest solution is probably to hook into the > "taddr == OF_BAD_ADDR" case in __of_address_to_resource > and add a lookup for ISA buses there, and instead check > if some special I/O port operations were registered > for the port number, using an architecture specific > function that arm64 implements. Other architectures > like x86 that don't have a direct mapping between I/O > ports and MMIO addresses would implement that same > function differently. So with respect to this patchset once we enter the "taddr == OF_BAD_ADDR" case you would add an arch specific function that checks if the resource is an I/O resource, if the parent node is an ISA bus (calling of_bus_isa_match) and if arm64_extio_ops is non NULL. I think it can work for us and it doesn't affect current devices. I will talk to Zhichang about this for the next patchset version. Many Thanks for your ideas Gab > > Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
> -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 15 September 2016 13:25 > To: linux-arm-ker...@lists.infradead.org > Cc: Gabriele Paoloni; devicet...@vger.kernel.org; > lorenzo.pieral...@arm.com; miny...@acm.org; linux-...@vger.kernel.org; > gre...@linuxfoundation.org; John Garry; will.dea...@arm.com; linux- > ker...@vger.kernel.org; Yuanzhichang; Linuxarm; xuwei (O); linux- > ser...@vger.kernel.org; b...@kernel.crashing.org; > zourongr...@gmail.com; liviu.du...@arm.com; kant...@163.com; > zhichang.yua...@gmail.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni > wrote: > > > -Original Message- > > > On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni > wrote: > > > > > > > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > > > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > > > > > > I quote: > > > > "There shall be an entry in the "ranges" property for each > > > > of the Memory and/or I/O spaces if that address space is > > > > mapped through the bridge." > > > > > > > > It seems to me that it is ok to have 1:1 address mapping and that > > > > therefore of_translate_address() should fail if "ranges" is not > > > > present. > > > > > > The key here is the definition of "mapped through the bridge". > > > I can only understand this as "directly mapped", i.e. an I/O > > > port of the child bus corresponds directly to a memory address > > > on the parent bus, but this is not the case here. > > > > > > The problem with adding the mapping here is that it looks > > > like it should be valid to create a page table entry for > > > the address returned from the translation and access it through > > > a pointer dereference, but that is clearly not possible. > > > > I understand that somehow we are abusing of the ranges property > > here however the point is that with the current implementation ranges > > is needed because otherwise the ipmi driver probe will fail here: > > > > of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource > > -> of_translate_address -> __of_translate_address > > > > Now we had a bit of discussion internally and to avoid > > having ranges we came up with two possible solutions: > > > > 1) Using bit 3 of phys.hi cell in 2.2.1 of > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > This would mean reworking of_bus_isa_get_flags in > > http://lxr.free-electrons.com/source/drivers/of/address.c#L398 > > and setting a new flag to be checked in __of_address_to_resource > > > > 2) Adding a property in the bindings of each device that is > > a child of our LPC bus and modify __of_address_to_resource > > to check if the property is in the DT and eventually bypass > > of_translate_address > > > > However in both 1) and 2) there are some issues: > > in 1) we are not complying with the isa binding doc (we use > > a bit that should be zero); in 2) we need to modify the > > bindings documentation of the devices that are connected > > to our LPC controller (therefore modifying other devices > > bindings to fit our special case). > > > > I think that maybe having the 1:1 range mapping doesn't > > reflect well the reality but it is the less painful > > solution... > > > > What's your view? > > We can check the 'i' bit for I/O space in of_bus_isa_get_flags, > and that should be enough to translate the I/O port number. > > The only part we need to change here is to not go through > the crazy conversion all the way from PCI I/O space to a > physical address and back to a (logical) port number > that we do today with of_translate_address/pci_address_to_pio. > > I can think of a several of ways to fix __of_address_to_resource > to just do the right thing according to the ISA binding to > make the normal drivers work. > > The easiest solution is probably to hook into the > "taddr == OF_BAD_ADDR" case in __of_address_to_resource > and add a lookup for ISA buses there, and instead check > if some special I/O port operations were registered > for the port number, using an architecture specific > function that arm64 implements. Other architectures > like x86 that don't have a direct mapping between I/O > ports and MMIO addresses would implement that same > function differently. So with respect to this patchset once we enter the "taddr == OF_BAD_ADDR" case you would add an arch specific function that checks if the resource is an I/O resource, if the parent node is an ISA bus (calling of_bus_isa_match) and if arm64_extio_ops is non NULL. I think it can work for us and it doesn't affect current devices. I will talk to Zhichang about this for the next patchset version. Many Thanks for your ideas Gab > > Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni wrote: > > -Original Message- > > On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote: > > > > > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > > > > I quote: > > > "There shall be an entry in the "ranges" property for each > > > of the Memory and/or I/O spaces if that address space is > > > mapped through the bridge." > > > > > > It seems to me that it is ok to have 1:1 address mapping and that > > > therefore of_translate_address() should fail if "ranges" is not > > > present. > > > > The key here is the definition of "mapped through the bridge". > > I can only understand this as "directly mapped", i.e. an I/O > > port of the child bus corresponds directly to a memory address > > on the parent bus, but this is not the case here. > > > > The problem with adding the mapping here is that it looks > > like it should be valid to create a page table entry for > > the address returned from the translation and access it through > > a pointer dereference, but that is clearly not possible. > > I understand that somehow we are abusing of the ranges property > here however the point is that with the current implementation ranges > is needed because otherwise the ipmi driver probe will fail here: > > of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource > -> of_translate_address -> __of_translate_address > > Now we had a bit of discussion internally and to avoid > having ranges we came up with two possible solutions: > > 1) Using bit 3 of phys.hi cell in 2.2.1 of > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > This would mean reworking of_bus_isa_get_flags in > http://lxr.free-electrons.com/source/drivers/of/address.c#L398 > and setting a new flag to be checked in __of_address_to_resource > > 2) Adding a property in the bindings of each device that is > a child of our LPC bus and modify __of_address_to_resource > to check if the property is in the DT and eventually bypass > of_translate_address > > However in both 1) and 2) there are some issues: > in 1) we are not complying with the isa binding doc (we use > a bit that should be zero); in 2) we need to modify the > bindings documentation of the devices that are connected > to our LPC controller (therefore modifying other devices > bindings to fit our special case). > > I think that maybe having the 1:1 range mapping doesn't > reflect well the reality but it is the less painful > solution... > > What's your view? We can check the 'i' bit for I/O space in of_bus_isa_get_flags, and that should be enough to translate the I/O port number. The only part we need to change here is to not go through the crazy conversion all the way from PCI I/O space to a physical address and back to a (logical) port number that we do today with of_translate_address/pci_address_to_pio. I can think of a several of ways to fix __of_address_to_resource to just do the right thing according to the ISA binding to make the normal drivers work. The easiest solution is probably to hook into the "taddr == OF_BAD_ADDR" case in __of_address_to_resource and add a lookup for ISA buses there, and instead check if some special I/O port operations were registered for the port number, using an architecture specific function that arm64 implements. Other architectures like x86 that don't have a direct mapping between I/O ports and MMIO addresses would implement that same function differently. Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Thursday, September 15, 2016 12:05:51 PM CEST Gabriele Paoloni wrote: > > -Original Message- > > On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote: > > > > > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > > > > I quote: > > > "There shall be an entry in the "ranges" property for each > > > of the Memory and/or I/O spaces if that address space is > > > mapped through the bridge." > > > > > > It seems to me that it is ok to have 1:1 address mapping and that > > > therefore of_translate_address() should fail if "ranges" is not > > > present. > > > > The key here is the definition of "mapped through the bridge". > > I can only understand this as "directly mapped", i.e. an I/O > > port of the child bus corresponds directly to a memory address > > on the parent bus, but this is not the case here. > > > > The problem with adding the mapping here is that it looks > > like it should be valid to create a page table entry for > > the address returned from the translation and access it through > > a pointer dereference, but that is clearly not possible. > > I understand that somehow we are abusing of the ranges property > here however the point is that with the current implementation ranges > is needed because otherwise the ipmi driver probe will fail here: > > of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource > -> of_translate_address -> __of_translate_address > > Now we had a bit of discussion internally and to avoid > having ranges we came up with two possible solutions: > > 1) Using bit 3 of phys.hi cell in 2.2.1 of > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > This would mean reworking of_bus_isa_get_flags in > http://lxr.free-electrons.com/source/drivers/of/address.c#L398 > and setting a new flag to be checked in __of_address_to_resource > > 2) Adding a property in the bindings of each device that is > a child of our LPC bus and modify __of_address_to_resource > to check if the property is in the DT and eventually bypass > of_translate_address > > However in both 1) and 2) there are some issues: > in 1) we are not complying with the isa binding doc (we use > a bit that should be zero); in 2) we need to modify the > bindings documentation of the devices that are connected > to our LPC controller (therefore modifying other devices > bindings to fit our special case). > > I think that maybe having the 1:1 range mapping doesn't > reflect well the reality but it is the less painful > solution... > > What's your view? We can check the 'i' bit for I/O space in of_bus_isa_get_flags, and that should be enough to translate the I/O port number. The only part we need to change here is to not go through the crazy conversion all the way from PCI I/O space to a physical address and back to a (logical) port number that we do today with of_translate_address/pci_address_to_pio. I can think of a several of ways to fix __of_address_to_resource to just do the right thing according to the ISA binding to make the normal drivers work. The easiest solution is probably to hook into the "taddr == OF_BAD_ADDR" case in __of_address_to_resource and add a lookup for ISA buses there, and instead check if some special I/O port operations were registered for the port number, using an architecture specific function that arm64 implements. Other architectures like x86 that don't have a direct mapping between I/O ports and MMIO addresses would implement that same function differently. Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 15 September 2016 09:22 > To: Gabriele Paoloni > Cc: linux-arm-ker...@lists.infradead.org; Yuanzhichang; > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > gre...@linuxfoundation.org; b...@kernel.crashing.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; xuwei (O); Linuxarm; > linux-ser...@vger.kernel.org; linux-...@vger.kernel.org; > zourongr...@gmail.com; liviu.du...@arm.com; kant...@163.com; > zhichang.yua...@gmail.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote: > > > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > > I quote: > > "There shall be an entry in the "ranges" property for each > > of the Memory and/or I/O spaces if that address space is > > mapped through the bridge." > > > > It seems to me that it is ok to have 1:1 address mapping and that > > therefore of_translate_address() should fail if "ranges" is not > > present. > > The key here is the definition of "mapped through the bridge". > I can only understand this as "directly mapped", i.e. an I/O > port of the child bus corresponds directly to a memory address > on the parent bus, but this is not the case here. > > The problem with adding the mapping here is that it looks > like it should be valid to create a page table entry for > the address returned from the translation and access it through > a pointer dereference, but that is clearly not possible. I understand that somehow we are abusing of the ranges property here however the point is that with the current implementation ranges is needed because otherwise the ipmi driver probe will fail here: of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource -> of_translate_address -> __of_translate_address Now we had a bit of discussion internally and to avoid having ranges we came up with two possible solutions: 1) Using bit 3 of phys.hi cell in 2.2.1 of http://www.firmware.org/1275/bindings/isa/isa0_4d.ps This would mean reworking of_bus_isa_get_flags in http://lxr.free-electrons.com/source/drivers/of/address.c#L398 and setting a new flag to be checked in __of_address_to_resource 2) Adding a property in the bindings of each device that is a child of our LPC bus and modify __of_address_to_resource to check if the property is in the DT and eventually bypass of_translate_address However in both 1) and 2) there are some issues: in 1) we are not complying with the isa binding doc (we use a bit that should be zero); in 2) we need to modify the bindings documentation of the devices that are connected to our LPC controller (therefore modifying other devices bindings to fit our special case). I think that maybe having the 1:1 range mapping doesn't reflect well the reality but it is the less painful solution... What's your view? Many Thanks Gab > > > This is also explained quite well in > > http://lxr.free-electrons.com/source/drivers/of/address.c#L490 > > > > what do you think? > > This is a separate issue, and only relevant for Apple Macintosh > machines as well as the PA-Semi sdc. > > Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 15 September 2016 09:22 > To: Gabriele Paoloni > Cc: linux-arm-ker...@lists.infradead.org; Yuanzhichang; > devicet...@vger.kernel.org; lorenzo.pieral...@arm.com; miny...@acm.org; > gre...@linuxfoundation.org; b...@kernel.crashing.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; xuwei (O); Linuxarm; > linux-ser...@vger.kernel.org; linux-...@vger.kernel.org; > zourongr...@gmail.com; liviu.du...@arm.com; kant...@163.com; > zhichang.yua...@gmail.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote: > > > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > > I quote: > > "There shall be an entry in the "ranges" property for each > > of the Memory and/or I/O spaces if that address space is > > mapped through the bridge." > > > > It seems to me that it is ok to have 1:1 address mapping and that > > therefore of_translate_address() should fail if "ranges" is not > > present. > > The key here is the definition of "mapped through the bridge". > I can only understand this as "directly mapped", i.e. an I/O > port of the child bus corresponds directly to a memory address > on the parent bus, but this is not the case here. > > The problem with adding the mapping here is that it looks > like it should be valid to create a page table entry for > the address returned from the translation and access it through > a pointer dereference, but that is clearly not possible. I understand that somehow we are abusing of the ranges property here however the point is that with the current implementation ranges is needed because otherwise the ipmi driver probe will fail here: of_ipmi_probe -> of_address_to_resource -> __of_address_to_resource -> of_translate_address -> __of_translate_address Now we had a bit of discussion internally and to avoid having ranges we came up with two possible solutions: 1) Using bit 3 of phys.hi cell in 2.2.1 of http://www.firmware.org/1275/bindings/isa/isa0_4d.ps This would mean reworking of_bus_isa_get_flags in http://lxr.free-electrons.com/source/drivers/of/address.c#L398 and setting a new flag to be checked in __of_address_to_resource 2) Adding a property in the bindings of each device that is a child of our LPC bus and modify __of_address_to_resource to check if the property is in the DT and eventually bypass of_translate_address However in both 1) and 2) there are some issues: in 1) we are not complying with the isa binding doc (we use a bit that should be zero); in 2) we need to modify the bindings documentation of the devices that are connected to our LPC controller (therefore modifying other devices bindings to fit our special case). I think that maybe having the 1:1 range mapping doesn't reflect well the reality but it is the less painful solution... What's your view? Many Thanks Gab > > > This is also explained quite well in > > http://lxr.free-electrons.com/source/drivers/of/address.c#L490 > > > > what do you think? > > This is a separate issue, and only relevant for Apple Macintosh > machines as well as the PA-Semi sdc. > > Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote: > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > I quote: > "There shall be an entry in the "ranges" property for each > of the Memory and/or I/O spaces if that address space is > mapped through the bridge." > > It seems to me that it is ok to have 1:1 address mapping and that > therefore of_translate_address() should fail if "ranges" is not > present. The key here is the definition of "mapped through the bridge". I can only understand this as "directly mapped", i.e. an I/O port of the child bus corresponds directly to a memory address on the parent bus, but this is not the case here. The problem with adding the mapping here is that it looks like it should be valid to create a page table entry for the address returned from the translation and access it through a pointer dereference, but that is clearly not possible. > This is also explained quite well in > http://lxr.free-electrons.com/source/drivers/of/address.c#L490 > > what do you think? This is a separate issue, and only relevant for Apple Macintosh machines as well as the PA-Semi sdc. Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Thursday, September 15, 2016 8:02:27 AM CEST Gabriele Paoloni wrote: > > From <<3.1.1. Open Firmware Properties for Bus Nodes>> in > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > I quote: > "There shall be an entry in the "ranges" property for each > of the Memory and/or I/O spaces if that address space is > mapped through the bridge." > > It seems to me that it is ok to have 1:1 address mapping and that > therefore of_translate_address() should fail if "ranges" is not > present. The key here is the definition of "mapped through the bridge". I can only understand this as "directly mapped", i.e. an I/O port of the child bus corresponds directly to a memory address on the parent bus, but this is not the case here. The problem with adding the mapping here is that it looks like it should be valid to create a page table entry for the address returned from the translation and access it through a pointer dereference, but that is clearly not possible. > This is also explained quite well in > http://lxr.free-electrons.com/source/drivers/of/address.c#L490 > > what do you think? This is a separate issue, and only relevant for Apple Macintosh machines as well as the PA-Semi sdc. Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 14 September 2016 22:32 > To: linux-arm-ker...@lists.infradead.org > Cc: Yuanzhichang; devicet...@vger.kernel.org; > lorenzo.pieral...@arm.com; Gabriele Paoloni; miny...@acm.org; > gre...@linuxfoundation.org; b...@kernel.crashing.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; xuwei (O); Linuxarm; > linux-ser...@vger.kernel.org; linux-...@vger.kernel.org; > zourongr...@gmail.com; liviu.du...@arm.com; kant...@163.com; > zhichang.yua...@gmail.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote: > > > > On 2016/9/14 20:33, Arnd Bergmann wrote: > > > On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan > wrote: > > > > > >> +Required properties: > > >> +- compatible: should be "hisilicon,low-pin-count" > > >> +- #address-cells: must be 2 which stick to the ISA/EISA binding > doc. > > >> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc. > > >> +- reg: base address and length of the register set for the > device. > > >> +- ranges: define a 1:1 mapping between the I/O space of the child > device and > > >> + the parent. > > > > > > Do we still need the "ranges" here? The property in your example > seems > > > wrong. > > > > I think "ranges" is needed. > > without this, of_translate_address --> __of_translate_address --> > of_translate_one will fail when translating the child's IO resource. > > > > > > > >> +ranges = <0x01 0xe4 0x0 0xe4 0x1000>; > > > > > > You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4? > > The hip06 LPC is defined as isa type. > > So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4 > of child will be 1:1 mapped as 0xe4. > > It means no translation. > > No, "no translation" would be leaving out the ranges, we should > fix the code so it handles this case according to the specification > of the ISA DT binding, rather than adding an incorrect ranges > property to make it work with the incorrect Linux implementation. > > of_translate_address() should fail here, and whichever code calls > it should try something else, possibly something we have to > implement that can return the correct IORESOURCE_* type. >From <<3.1.1. Open Firmware Properties for Bus Nodes>> in http://www.firmware.org/1275/bindings/isa/isa0_4d.ps I quote: "There shall be an entry in the "ranges" property for each of the Memory and/or I/O spaces if that address space is mapped through the bridge." It seems to me that it is ok to have 1:1 address mapping and that therefore of_translate_address() should fail if "ranges" is not present. This is also explained quite well in http://lxr.free-electrons.com/source/drivers/of/address.c#L490 what do you think? Thanks Gab > > > > > > > I don't get this part. The bus driver should not care what its > > > children are, just register and PIO ranges that the bus can handle > > > in theory, i.e. from 0x000 to 0xfff. > > > > Just as we discussed in V2, the legacy PIO range is specific to some > > device, such as for ipmi bt, 0xe4 - 0xe7 will be populated. > > I don't want to occupy a larger PIO range in which only small part > PIOs > > are used by our LPC. At this moment, two PIO ranges are using > > through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff. > > > > If we configure 0-0x1000 for the LPC to cover those two ranges, most > > PIO are wasted and other PIO device on other buses lose the chance to > > use the PIO below 0x1000. > > Otherwise, PIO conflict will happen. So, My idea is only occupied > > the PIO ranges which are really needed for the children. > > The only thing it can realistically conflict with would be another > LPC bus behind on a PCI host bridge. On ARM64, all regular PCI > devices that have IORESOURCE_IO ports are intentionally moved > to (bus) port numbers above 0x1000. > > > 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. > > > Arnd
RE: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
Hi Arnd > -Original Message- > From: Arnd Bergmann [mailto:a...@arndb.de] > Sent: 14 September 2016 22:32 > To: linux-arm-ker...@lists.infradead.org > Cc: Yuanzhichang; devicet...@vger.kernel.org; > lorenzo.pieral...@arm.com; Gabriele Paoloni; miny...@acm.org; > gre...@linuxfoundation.org; b...@kernel.crashing.org; John Garry; > will.dea...@arm.com; linux-kernel@vger.kernel.org; xuwei (O); Linuxarm; > linux-ser...@vger.kernel.org; linux-...@vger.kernel.org; > zourongr...@gmail.com; liviu.du...@arm.com; kant...@163.com; > zhichang.yua...@gmail.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote: > > > > On 2016/9/14 20:33, Arnd Bergmann wrote: > > > On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan > wrote: > > > > > >> +Required properties: > > >> +- compatible: should be "hisilicon,low-pin-count" > > >> +- #address-cells: must be 2 which stick to the ISA/EISA binding > doc. > > >> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc. > > >> +- reg: base address and length of the register set for the > device. > > >> +- ranges: define a 1:1 mapping between the I/O space of the child > device and > > >> + the parent. > > > > > > Do we still need the "ranges" here? The property in your example > seems > > > wrong. > > > > I think "ranges" is needed. > > without this, of_translate_address --> __of_translate_address --> > of_translate_one will fail when translating the child's IO resource. > > > > > > > >> +ranges = <0x01 0xe4 0x0 0xe4 0x1000>; > > > > > > You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4? > > The hip06 LPC is defined as isa type. > > So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4 > of child will be 1:1 mapped as 0xe4. > > It means no translation. > > No, "no translation" would be leaving out the ranges, we should > fix the code so it handles this case according to the specification > of the ISA DT binding, rather than adding an incorrect ranges > property to make it work with the incorrect Linux implementation. > > of_translate_address() should fail here, and whichever code calls > it should try something else, possibly something we have to > implement that can return the correct IORESOURCE_* type. >From <<3.1.1. Open Firmware Properties for Bus Nodes>> in http://www.firmware.org/1275/bindings/isa/isa0_4d.ps I quote: "There shall be an entry in the "ranges" property for each of the Memory and/or I/O spaces if that address space is mapped through the bridge." It seems to me that it is ok to have 1:1 address mapping and that therefore of_translate_address() should fail if "ranges" is not present. This is also explained quite well in http://lxr.free-electrons.com/source/drivers/of/address.c#L490 what do you think? Thanks Gab > > > > > > > I don't get this part. The bus driver should not care what its > > > children are, just register and PIO ranges that the bus can handle > > > in theory, i.e. from 0x000 to 0xfff. > > > > Just as we discussed in V2, the legacy PIO range is specific to some > > device, such as for ipmi bt, 0xe4 - 0xe7 will be populated. > > I don't want to occupy a larger PIO range in which only small part > PIOs > > are used by our LPC. At this moment, two PIO ranges are using > > through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff. > > > > If we configure 0-0x1000 for the LPC to cover those two ranges, most > > PIO are wasted and other PIO device on other buses lose the chance to > > use the PIO below 0x1000. > > Otherwise, PIO conflict will happen. So, My idea is only occupied > > the PIO ranges which are really needed for the children. > > The only thing it can realistically conflict with would be another > LPC bus behind on a PCI host bridge. On ARM64, all regular PCI > devices that have IORESOURCE_IO ports are intentionally moved > to (bus) port numbers above 0x1000. > > > 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. > > > Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote: > > On 2016/9/14 20:33, Arnd Bergmann wrote: > > On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote: > > > >> +Required properties: > >> +- compatible: should be "hisilicon,low-pin-count" > >> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc. > >> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc. > >> +- reg: base address and length of the register set for the device. > >> +- ranges: define a 1:1 mapping between the I/O space of the child device > >> and > >> +the parent. > > > > Do we still need the "ranges" here? The property in your example seems > > wrong. > > I think "ranges" is needed. > without this, of_translate_address --> __of_translate_address --> > of_translate_one will fail when translating the child's IO resource. > > > > >> + ranges = <0x01 0xe4 0x0 0xe4 0x1000>; > > > > You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4? > The hip06 LPC is defined as isa type. > So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4 of > child will be 1:1 mapped as 0xe4. > It means no translation. No, "no translation" would be leaving out the ranges, we should fix the code so it handles this case according to the specification of the ISA DT binding, rather than adding an incorrect ranges property to make it work with the incorrect Linux implementation. of_translate_address() should fail here, and whichever code calls it should try something else, possibly something we have to implement that can return the correct IORESOURCE_* type. > > > > I don't get this part. The bus driver should not care what its > > children are, just register and PIO ranges that the bus can handle > > in theory, i.e. from 0x000 to 0xfff. > > Just as we discussed in V2, the legacy PIO range is specific to some > device, such as for ipmi bt, 0xe4 - 0xe7 will be populated. > I don't want to occupy a larger PIO range in which only small part PIOs > are used by our LPC. At this moment, two PIO ranges are using > through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff. > > If we configure 0-0x1000 for the LPC to cover those two ranges, most > PIO are wasted and other PIO device on other buses lose the chance to > use the PIO below 0x1000. > Otherwise, PIO conflict will happen. So, My idea is only occupied > the PIO ranges which are really needed for the children. The only thing it can realistically conflict with would be another LPC bus behind on a PCI host bridge. On ARM64, all regular PCI devices that have IORESOURCE_IO ports are intentionally moved to (bus) port numbers above 0x1000. > 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. Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Wednesday, September 14, 2016 10:50:44 PM CEST zhichang.yuan wrote: > > On 2016/9/14 20:33, Arnd Bergmann wrote: > > On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote: > > > >> +Required properties: > >> +- compatible: should be "hisilicon,low-pin-count" > >> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc. > >> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc. > >> +- reg: base address and length of the register set for the device. > >> +- ranges: define a 1:1 mapping between the I/O space of the child device > >> and > >> +the parent. > > > > Do we still need the "ranges" here? The property in your example seems > > wrong. > > I think "ranges" is needed. > without this, of_translate_address --> __of_translate_address --> > of_translate_one will fail when translating the child's IO resource. > > > > >> + ranges = <0x01 0xe4 0x0 0xe4 0x1000>; > > > > You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4? > The hip06 LPC is defined as isa type. > So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4 of > child will be 1:1 mapped as 0xe4. > It means no translation. No, "no translation" would be leaving out the ranges, we should fix the code so it handles this case according to the specification of the ISA DT binding, rather than adding an incorrect ranges property to make it work with the incorrect Linux implementation. of_translate_address() should fail here, and whichever code calls it should try something else, possibly something we have to implement that can return the correct IORESOURCE_* type. > > > > I don't get this part. The bus driver should not care what its > > children are, just register and PIO ranges that the bus can handle > > in theory, i.e. from 0x000 to 0xfff. > > Just as we discussed in V2, the legacy PIO range is specific to some > device, such as for ipmi bt, 0xe4 - 0xe7 will be populated. > I don't want to occupy a larger PIO range in which only small part PIOs > are used by our LPC. At this moment, two PIO ranges are using > through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff. > > If we configure 0-0x1000 for the LPC to cover those two ranges, most > PIO are wasted and other PIO device on other buses lose the chance to > use the PIO below 0x1000. > Otherwise, PIO conflict will happen. So, My idea is only occupied > the PIO ranges which are really needed for the children. The only thing it can realistically conflict with would be another LPC bus behind on a PCI host bridge. On ARM64, all regular PCI devices that have IORESOURCE_IO ports are intentionally moved to (bus) port numbers above 0x1000. > 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. Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On 2016/9/14 20:33, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote:
>
>> +Required properties:
>> +- compatible: should be "hisilicon,low-pin-count"
>> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
>> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
>> +- reg: base address and length of the register set for the device.
>> +- ranges: define a 1:1 mapping between the I/O space of the child device and
>> + the parent.
>
> Do we still need the "ranges" here? The property in your example seems
> wrong.
I think "ranges" is needed.
without this, of_translate_address --> __of_translate_address -->
of_translate_one will fail when translating the child's IO resource.
>
>> +ranges = <0x01 0xe4 0x0 0xe4 0x1000>;
>
> You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4?
The hip06 LPC is defined as isa type.
So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4 of child
will be 1:1 mapped as 0xe4.
It means no translation.
>
>> +/**
>> + * hisilpc_children_map_sysio - setup the mapping between system Io and
>> + * physical IO
>> + *
>> + * @child: the device whose IO is handling
>> + * @data: some device specific data. For ACPI device, should be NULL.
>> + *
>> + * Returns >=0 means the mapping is successfully created;
>> + * others mean some failures.
>> + */
>> +static int hisilpc_children_map_sysio(struct device * child, void * data)
>> +{
>> +struct resource *iores;
>> +unsigned long cpuio;
>> +struct extio_ops *opsnode;
>> +int ret;
>> +struct hisilpc_dev *lpcdev;
>> +
>> +if (!child || !child->parent)
>> +return -EINVAL;
>> +
>> +iores = platform_get_resource_byname(to_platform_device(child),
>> +IORESOURCE_IO, "dev_io");
>> +if (!iores)
>> +return -ENODEV;
>> +
>> +/*
>> + * can not use devm_kzalloc to allocate slab for child before its driver
>> + * start probing. Here allocate the slab with the name of parent.
>> + */
>> +opsnode = devm_kzalloc(child->parent, sizeof(*opsnode), GFP_KERNEL);
>> +if (!opsnode)
>> +return -ENOMEM;
>> +
>> +cpuio = data ? *((unsigned long *)data) : 0;
>> +
>> +opsnode->start = iores->start;
>> +opsnode->end = iores->end;
>> +opsnode->ptoffset = cpuio ? (cpuio - iores->start) : 0;
>> +
>> +dev_info(child, "map sys port[%lx - %lx] offset=0x%lx",
>> +(unsigned long)iores->start,
>> +(unsigned long)iores->end,
>> +opsnode->ptoffset);
>> +
>> +opsnode->pfin = hisilpc_comm_inb;
>> +opsnode->pfout = hisilpc_comm_outb;
>> +
>> +lpcdev = platform_get_drvdata(to_platform_device(child->parent));
>> +opsnode->devpara = lpcdev;
>> +
>> +/* only apply indirect-IO to ipmi child device */
>
> I don't get this part. The bus driver should not care what its
> children are, just register and PIO ranges that the bus can handle
> in theory, i.e. from 0x000 to 0xfff.
Just as we discussed in V2, the legacy PIO range is specific to some device,
such as for ipmi bt, 0xe4 - 0xe7 will be populated.
I don't want to occupy a larger PIO range in which only small part PIOs are
used by our LPC. At this moment, two PIO ranges are using
through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff.
If we configure 0-0x1000 for the LPC to cover those two ranges, most PIO are
wasted and other PIO device on other buses lose the chance to use the PIO below
0x1000.
Otherwise, PIO conflict will happen. So, My idea is only occupied the PIO
ranges which are really needed for the children.
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.
If we think it is nearly no chance that LPC PIO range conflict with other
buses, we can allocate 0xe4 - 0x2ff to LPC and store it to arm64_extio_ops. In
this case,
the special processing for ipmi device is not needed.
Best,
Zhichang
>
> Arnd
>
>
> .
>
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On 2016/9/14 20:33, Arnd Bergmann wrote:
> On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote:
>
>> +Required properties:
>> +- compatible: should be "hisilicon,low-pin-count"
>> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
>> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
>> +- reg: base address and length of the register set for the device.
>> +- ranges: define a 1:1 mapping between the I/O space of the child device and
>> + the parent.
>
> Do we still need the "ranges" here? The property in your example seems
> wrong.
I think "ranges" is needed.
without this, of_translate_address --> __of_translate_address -->
of_translate_one will fail when translating the child's IO resource.
>
>> +ranges = <0x01 0xe4 0x0 0xe4 0x1000>;
>
> You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4?
The hip06 LPC is defined as isa type.
So, 0x01 0xe4 is the local IO address of 0xe4. With this ranges, 0xe4 of child
will be 1:1 mapped as 0xe4.
It means no translation.
>
>> +/**
>> + * hisilpc_children_map_sysio - setup the mapping between system Io and
>> + * physical IO
>> + *
>> + * @child: the device whose IO is handling
>> + * @data: some device specific data. For ACPI device, should be NULL.
>> + *
>> + * Returns >=0 means the mapping is successfully created;
>> + * others mean some failures.
>> + */
>> +static int hisilpc_children_map_sysio(struct device * child, void * data)
>> +{
>> +struct resource *iores;
>> +unsigned long cpuio;
>> +struct extio_ops *opsnode;
>> +int ret;
>> +struct hisilpc_dev *lpcdev;
>> +
>> +if (!child || !child->parent)
>> +return -EINVAL;
>> +
>> +iores = platform_get_resource_byname(to_platform_device(child),
>> +IORESOURCE_IO, "dev_io");
>> +if (!iores)
>> +return -ENODEV;
>> +
>> +/*
>> + * can not use devm_kzalloc to allocate slab for child before its driver
>> + * start probing. Here allocate the slab with the name of parent.
>> + */
>> +opsnode = devm_kzalloc(child->parent, sizeof(*opsnode), GFP_KERNEL);
>> +if (!opsnode)
>> +return -ENOMEM;
>> +
>> +cpuio = data ? *((unsigned long *)data) : 0;
>> +
>> +opsnode->start = iores->start;
>> +opsnode->end = iores->end;
>> +opsnode->ptoffset = cpuio ? (cpuio - iores->start) : 0;
>> +
>> +dev_info(child, "map sys port[%lx - %lx] offset=0x%lx",
>> +(unsigned long)iores->start,
>> +(unsigned long)iores->end,
>> +opsnode->ptoffset);
>> +
>> +opsnode->pfin = hisilpc_comm_inb;
>> +opsnode->pfout = hisilpc_comm_outb;
>> +
>> +lpcdev = platform_get_drvdata(to_platform_device(child->parent));
>> +opsnode->devpara = lpcdev;
>> +
>> +/* only apply indirect-IO to ipmi child device */
>
> I don't get this part. The bus driver should not care what its
> children are, just register and PIO ranges that the bus can handle
> in theory, i.e. from 0x000 to 0xfff.
Just as we discussed in V2, the legacy PIO range is specific to some device,
such as for ipmi bt, 0xe4 - 0xe7 will be populated.
I don't want to occupy a larger PIO range in which only small part PIOs are
used by our LPC. At this moment, two PIO ranges are using
through the device property configuration, 0xe4-0xe7, 0x2f8-0x2ff.
If we configure 0-0x1000 for the LPC to cover those two ranges, most PIO are
wasted and other PIO device on other buses lose the chance to use the PIO below
0x1000.
Otherwise, PIO conflict will happen. So, My idea is only occupied the PIO
ranges which are really needed for the children.
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.
If we think it is nearly no chance that LPC PIO range conflict with other
buses, we can allocate 0xe4 - 0x2ff to LPC and store it to arm64_extio_ops. In
this case,
the special processing for ipmi device is not needed.
Best,
Zhichang
>
> Arnd
>
>
> .
>
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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= (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 infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
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= (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 infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote:
> +Required properties:
> +- compatible: should be "hisilicon,low-pin-count"
> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> +- reg: base address and length of the register set for the device.
> +- ranges: define a 1:1 mapping between the I/O space of the child device and
> + the parent.
Do we still need the "ranges" here? The property in your example seems
wrong.
> + ranges = <0x01 0xe4 0x0 0xe4 0x1000>;
You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4?
> +/**
> + * hisilpc_children_map_sysio - setup the mapping between system Io and
> + * physical IO
> + *
> + * @child: the device whose IO is handling
> + * @data: some device specific data. For ACPI device, should be NULL.
> + *
> + * Returns >=0 means the mapping is successfully created;
> + * others mean some failures.
> + */
> +static int hisilpc_children_map_sysio(struct device * child, void * data)
> +{
> + struct resource *iores;
> + unsigned long cpuio;
> + struct extio_ops *opsnode;
> + int ret;
> + struct hisilpc_dev *lpcdev;
> +
> + if (!child || !child->parent)
> + return -EINVAL;
> +
> + iores = platform_get_resource_byname(to_platform_device(child),
> + IORESOURCE_IO, "dev_io");
> + if (!iores)
> + return -ENODEV;
> +
> + /*
> + * can not use devm_kzalloc to allocate slab for child before its driver
> + * start probing. Here allocate the slab with the name of parent.
> + */
> + opsnode = devm_kzalloc(child->parent, sizeof(*opsnode), GFP_KERNEL);
> + if (!opsnode)
> + return -ENOMEM;
> +
> + cpuio = data ? *((unsigned long *)data) : 0;
> +
> + opsnode->start = iores->start;
> + opsnode->end = iores->end;
> + opsnode->ptoffset = cpuio ? (cpuio - iores->start) : 0;
> +
> + dev_info(child, "map sys port[%lx - %lx] offset=0x%lx",
> + (unsigned long)iores->start,
> + (unsigned long)iores->end,
> + opsnode->ptoffset);
> +
> + opsnode->pfin = hisilpc_comm_inb;
> + opsnode->pfout = hisilpc_comm_outb;
> +
> + lpcdev = platform_get_drvdata(to_platform_device(child->parent));
> + opsnode->devpara = lpcdev;
> +
> + /* only apply indirect-IO to ipmi child device */
I don't get this part. The bus driver should not care what its
children are, just register and PIO ranges that the bus can handle
in theory, i.e. from 0x000 to 0xfff.
Arnd
Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
On Wednesday, September 14, 2016 8:15:52 PM CEST Zhichang Yuan wrote:
> +Required properties:
> +- compatible: should be "hisilicon,low-pin-count"
> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> +- reg: base address and length of the register set for the device.
> +- ranges: define a 1:1 mapping between the I/O space of the child device and
> + the parent.
Do we still need the "ranges" here? The property in your example seems
wrong.
> + ranges = <0x01 0xe4 0x0 0xe4 0x1000>;
You translate I/O port 0x00e4 through 0x10e4 to CPU address 0x0e4?
> +/**
> + * hisilpc_children_map_sysio - setup the mapping between system Io and
> + * physical IO
> + *
> + * @child: the device whose IO is handling
> + * @data: some device specific data. For ACPI device, should be NULL.
> + *
> + * Returns >=0 means the mapping is successfully created;
> + * others mean some failures.
> + */
> +static int hisilpc_children_map_sysio(struct device * child, void * data)
> +{
> + struct resource *iores;
> + unsigned long cpuio;
> + struct extio_ops *opsnode;
> + int ret;
> + struct hisilpc_dev *lpcdev;
> +
> + if (!child || !child->parent)
> + return -EINVAL;
> +
> + iores = platform_get_resource_byname(to_platform_device(child),
> + IORESOURCE_IO, "dev_io");
> + if (!iores)
> + return -ENODEV;
> +
> + /*
> + * can not use devm_kzalloc to allocate slab for child before its driver
> + * start probing. Here allocate the slab with the name of parent.
> + */
> + opsnode = devm_kzalloc(child->parent, sizeof(*opsnode), GFP_KERNEL);
> + if (!opsnode)
> + return -ENOMEM;
> +
> + cpuio = data ? *((unsigned long *)data) : 0;
> +
> + opsnode->start = iores->start;
> + opsnode->end = iores->end;
> + opsnode->ptoffset = cpuio ? (cpuio - iores->start) : 0;
> +
> + dev_info(child, "map sys port[%lx - %lx] offset=0x%lx",
> + (unsigned long)iores->start,
> + (unsigned long)iores->end,
> + opsnode->ptoffset);
> +
> + opsnode->pfin = hisilpc_comm_inb;
> + opsnode->pfout = hisilpc_comm_outb;
> +
> + lpcdev = platform_get_drvdata(to_platform_device(child->parent));
> + opsnode->devpara = lpcdev;
> +
> + /* only apply indirect-IO to ipmi child device */
I don't get this part. The bus driver should not care what its
children are, just register and PIO ranges that the bus can handle
in theory, i.e. from 0x000 to 0xfff.
Arnd
