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

[zhichang.yuan]

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