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

Message ID EE11001F9E5DDD47B7634E2F8A612F2E1F881744@lhreml507-mbx
State New
Headers show

Commit Message

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

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

> From: Arnd Bergmann [mailto:arnd@arndb.de]

> Sent: 21 September 2016 21:18

> To: Gabriele Paoloni

> Cc: zhichang; linux-arm-kernel@lists.infradead.org;

> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;

> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;

> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;

> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;

> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;

> kantyzc@163.com

> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on

> Hip06

> 

> On Wednesday, September 21, 2016 4:20:55 PM CEST Gabriele Paoloni

> wrote:

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

> > > From: zhichang [mailto:zhichang.yuan02@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.


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

Comments

Gabriele Paoloni Sept. 22, 2016, 2:47 p.m. | #1
Hi Arnd

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

> From: Arnd Bergmann [mailto:arnd@arndb.de]

> Sent: 22 September 2016 13:15

> To: Gabriele Paoloni

> Cc: zhichang; linux-arm-kernel@lists.infradead.org;

> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;

> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;

> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;

> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;

> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;

> kantyzc@163.com

> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on

> Hip06

> 

> On 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, &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;

> >         }

> 

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

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gabriele Paoloni Sept. 22, 2016, 3:20 p.m. | #2
> -----Original Message-----

> From: Arnd Bergmann [mailto:arnd@arndb.de]

> Sent: 22 September 2016 15:59

> To: Gabriele Paoloni

> Cc: zhichang; linux-arm-kernel@lists.infradead.org;

> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org;

> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry;

> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang;

> Linuxarm; xuwei (O); linux-serial@vger.kernel.org;

> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com;

> kantyzc@163.com

> Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on

> Hip06

> 

> On 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, &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;

> > > >         }

> > >

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

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

--- 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;
        }