[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

Arnd Bergmann Sept. 22, 2016, 12:14 p.m. | #1
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?

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




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

Gabriele Paoloni Sept. 22, 2016, 2:47 p.m. | #2
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
Arnd Bergmann Sept. 22, 2016, 2:59 p.m. | #3
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.

> > 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
Gabriele Paoloni Sept. 22, 2016, 3:20 p.m. | #4
> -----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
zhichang.yuan Sept. 22, 2016, 3:46 p.m. | #5
On 09/22/2016 11:20 PM, Gabriele Paoloni wrote:
>

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

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


--
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
zhichang.yuan Sept. 22, 2016, 4:27 p.m. | #6
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, &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?

>

> 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


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