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

From: zhichang.yuan
Date: Thu Sep 22 2016 - 12:27:35 EST

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