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

Commit Message

Gabriele Paoloni Sept. 22, 2016, 11:55 a.m. UTC

Hi Arnd

> -----Original Message-----

> From: Arnd Bergmann [mailto:[e-mail]]

> Sent: 21 September 2016 21:18

> To: Gabriele Paoloni

> Cc: zhichang; [e-mail];

> [e-mail]; [e-mail]; [e-mail];

> [e-mail]; [e-mail]; John Garry;

> [e-mail]; [e-mail]; Yuanzhichang;

> Linuxarm; xuwei (O); [e-mail];

> [e-mail]; [e-mail]; [e-mail];

> [e-mail]

> 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:[e-mail]]

> > > 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.


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)
 


> 

> > 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.


Thanks

I discussed with Zhichang and we agreed to use only one LPC range
to be registered with pci_register_io_range.

We'll rework the accessors to check if the retrieved I/O tokens
belong to LPC or PCI IO range...

Cheers

Gab


> 

> 	Arnd

> 

> 	Arnd

Patch

--- 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, &rlen);
-       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;
        }