Hi Zhichang > -----Original Message----- > From: zhichang [mailto:zhichang.yuan02@xxxxxxxxx] > Sent: 21 September 2016 11:09 > To: Arnd Bergmann; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Cc: Gabriele Paoloni; devicetree@xxxxxxxxxxxxxxx; > lorenzo.pieralisi@xxxxxxx; minyard@xxxxxxx; linux-pci@xxxxxxxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; John Garry; will.deacon@xxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; Yuanzhichang; Linuxarm; xuwei (O); linux- > serial@xxxxxxxxxxxxxxx; benh@xxxxxxxxxxxxxxxxxxx; > zourongrong@xxxxxxxxx; liviu.dudau@xxxxxxx; kantyzc@xxxxxxx > 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 > > ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
