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

[Gabriele Paoloni]

Hi Zhichang

> -----Original Message-----
> From: zhichang [mailto:zhichang.yuan02 at gmail.com]
> Sent: 21 September 2016 11:09
> To: Arnd Bergmann; linux-arm-kernel at lists.infradead.org
> Cc: Gabriele Paoloni; devicetree at vger.kernel.org;
> lorenzo.pieralisi at arm.com; minyard at acm.org; linux-pci at vger.kernel.org;
> gregkh at linuxfoundation.org; John Garry; will.deacon at arm.com; linux-
> kernel at vger.kernel.org; Yuanzhichang; Linuxarm; xuwei (O); linux-
> serial at vger.kernel.org; benh at kernel.crashing.org;
> zourongrong at gmail.com; liviu.dudau at arm.com; kantyzc at 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/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 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 think extending of_empty_ranges_quirk() may be a reasonable solution.
What do you think Arnd?
  
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

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?

Thanks

Gab

> 
> 
> > 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, it seems a bit risky. If
> some device want to use this port range
> when no hip06 LPC is configured, the checking does not work. I think we
> should also check the relevant device.
> 
> 
> Best,
> Zhichang
> 
> 
> >
> > 	Arnd
> >