Hi Arnd

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de]
Sent: 25 November 2016 12:04
To: Gabriele Paoloni
Cc: linux-arm-kernel@lists.infradead.org; mark.rutland@arm.com;
catalin.marinas@arm.com; linux-pci@vger.kernel.org;
liviu.dudau@arm.com; Linuxarm; lorenzo.pieralisi@arm.com; xuwei (O);
Jason Gunthorpe; T homas Petazzoni; linux-serial@vger.kernel.org;
benh@kernel.crashing.org; devicetree@vger.kernel.org; minyard@acm.org;
will.deacon@arm.com; John Garry; olof@lixom.net; robh+dt@kernel.org;
bhelgaas@go og le.com; kantyzc@163.com; zhichang.yuan 02@gmail.com;
linux-kernel@vger.kernel.org; Yuanzhichang; zourongrong@gmail.com
Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
Hip06

On Friday, November 25, 2016 8:46:11 AM CET Gabriele Paoloni wrote:

quoted

quoted

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

On Wednesday, November 23, 2016 6:07:11 PM CET Arnd Bergmann wrote:

quoted

On Wednesday, November 23, 2016 3:22:33 PM CET Gabriele Paoloni

wrote:

quoted

quoted

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

quoted

On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni

 /*

@@ -549,9 +548,14 @@ static int of_translate_one(struct device_node

*parent, struct of_bus *bus,
  * that translation is impossible (that is we are not dealing with

a

quoted

quoted

value
  * that can be mapped to a cpu physical address). This is not

really

quoted

quoted

specified
  * that way, but this is traditionally the way IBM at least do

things

quoted

quoted

+ *
+ * Whenever the translation fails, the *host pointer will be set

to

quoted

quoted

the
+ * device that lacks a tranlation, and the return code is relative

to

quoted

quoted

+ * that node.

This seems to be wrong to me. We are abusing of the error conditions.
So effectively if there is a buggy DT for an IO resource we end up
assuming that we are using a special IO device with unmapped

addresses.

quoted

The patch at the bottom apply on top of this one and I think is a

more

quoted

reasonable approach

It was meant as a logical extension to the existing interface,
translating the address as far as we can, and reporting back
how far we got.

Maybe we can return 'of_root' by instead of NULL to signify
that we have converted all the way to the root of the DT?
That would make it more consistent, but slightly more complicated
for the common case.

Mmm not sure really...the point is that now the **host parameter is
used both as a flag and also in of_translate_ioport...the problem
that I see is where we set the "host" parameter...I have a proposal
below...

[...]

pci_bus_alloc_resource(struct

quoted

quoted

pci_bus *bus,
 			void *alignf_data);

[Many lines of reply trimmed here, please make sure you don't quote too
much context when you reply, it's really annoying to read through
it otherwise]

Sorry! I'll take care of this in the future...

quoted

 /*
+ * of_isa_indirect_io - get the IO address from some isa reg

property value.

quoted

+ *	For some isa/lpc devices, no ranges property in ancestor node.
+ *	The device addresses are described directly in their regs

property.

quoted

+ *	This fixup function will be called to get the IO address of

isa/lpc

quoted

+ *	devices when the normal of_translation failed.
+ *
+ * @parent:	points to the parent dts node;
+ * @bus:		points to the of_bus which can be used to parse

address;

quoted

+ * @addr:	the address from reg property;
+ * @na:		the address cell counter of @addr;
+ * @presult:	store the address paresed from @addr;
+ *
+ * return 1 when successfully get the I/O address;
+ * 0 will return for some failures.
+ */
+static int of_get_isa_indirect_io(struct device_node *parent,
+				struct of_bus *bus, __be32 *addr,
+				int na, u64 *presult)
+{
+	unsigned int flags;
+	unsigned int rlen;
+
+	/* whether support indirectIO */
+	if (!indirect_io_enabled())
+		return 0;
+
+	if (!of_bus_isa_match(parent))
+		return 0;
+
+	flags = bus->get_flags(addr);
+	if (!(flags & IORESOURCE_IO))
+		return 0;
+
+	/* there is ranges property, apply the normal translation

directly. */

quoted

+	if (of_get_property(parent, "ranges", &rlen))
+		return 0;
+
+	*presult = of_read_number(addr + 1, na - 1);
+	/* this fixup is only valid for specific I/O range. */
+	return addr_is_indirect_io(*presult);
+}

Right, this would work. The reason I didn't go down this route is
that I wanted to keep it generic enough to allow doing the same
for PCI host bridges with a nonlinear mapping of the I/O space.

There isn't really anything special about ISA here, other than the
fact that the one driver that needs it happens to be for ISA rather
than PCI.

Yes I see your point please consider the patch at the bottom...

quoted

+/*
  * Translate an address from the device-tree into a CPU physical

address,

quoted

  * this walks up the tree and applies the various bus mappings on

the

quoted

  * way.

@@ -600,14 +643,23 @@ static u64 __of_translate_address(struct

device_node *dev,

quoted

 			result = of_read_number(addr, na);
 			break;
 		}
+		/*
+		 * For indirectIO device which has no ranges property, get
+		 * the address from reg directly.
+		 */
+		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
+			pr_debug("isa indirectIO matched(%s)..addr =

0x%llx\n",

quoted

+				of_node_full_name(dev), result);
+			*host = of_node_get(parent);
+			break;
+		}

If we do the special case for ISA as you suggest above, I would still
want
to keep it in of_translate_ioport(), I think that's a useful change by
itself in my patch.

This comes without saying...the patch I proposed here already applied on
top of your changes and, in fact, you can see that I set 
"*host = of_node_get(parent);" above in my patch to be used by 
of_translate_ioport()
 

	Arnde

Below I have reworked my patch (that still applies on top of your one) to
make it not ISA specific.
Notice that of_translate_ioport() still stands and that in addr_is_indirect_io()
we make sure the bus address to be in the bus address range that the special
host operates on...

---
 drivers/of/address.c       | 56 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/of_address.h | 17 ++++++++++++++
 2 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 5decaba..2b34931 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c

@@ -540,6 +540,48 @@ static u64 of_translate_one(struct device_node *parent, struct of_bus *bus,
 }
 
 /*
+ * of_get_indirect_io - get the IO address from some reg property value.
+ *	For some special host devices, we have no ranges property node and
+ *	we directly use the bus addresses of the children regs property.
+ *	This fixup function will be called to get the IO address of isa/lpc
+ *	devices when the normal of_translation failed.
+ *
+ * @parent:	points to the parent dts node;
+ * @bus:		points to the of_bus which can be used to parse address;
+ * @addr:	the address from reg property;
+ * @na:		the address cell counter of @addr;
+ * @presult:	store the address parsed from @addr;
+ *
+ * return 1 when successfully get the I/O address;
+ * 0 will return for some failures.
+ */
+static int of_get_indirect_io(struct device_node *parent,
+				struct of_bus *bus, __be32 *addr,
+				int na, u64 *presult)
+{
+	unsigned int flags;
+	unsigned int rlen;
+
+	/* whether support indirectIO */
+	if (!indirect_io_enabled())
+		return 0;
+
+	flags = bus->get_flags(addr);
+	if (!(flags & IORESOURCE_IO))
+		return 0;
+
+	/* there is ranges property, apply the normal translation directly. */
+	if (of_get_property(parent, "ranges", &rlen))
+		return 0;
+
+	*presult = of_read_number(addr + 1, na - 1);
+
+	/* check if the bus address falls into the range of
+	 * the special host device*/
+	return addr_is_indirect_io(*presult);
+}
+
+/*
  * Translate an address from the device-tree into a CPU physical address,
  * this walks up the tree and applies the various bus mappings on the
  * way.

@@ -601,13 +643,23 @@ static u64 __of_translate_address(struct device_node *dev,
 			break;
 		}
 
+		/*
+		 * For indirectIO device which has no ranges property, get
+		 * the address from reg directly.
+		 */
+		if (of_get_indirect_io(dev, bus, addr, na, &result)) {
+			pr_debug("isa indirectIO matched(%s)..addr = 0x%llx\n",
+				of_node_full_name(dev), result);
+			*host = of_node_get(parent);
+			break;
+		}
+
 		/* Get new parent bus and counts */
 		pbus = of_match_bus(parent);
 		pbus->count_cells(dev, &pna, &pns);
 		if (!OF_CHECK_COUNTS(pna, pns)) {
-			pr_debug("Bad cell count for %s\n",
+			pr_err("Bad cell count for %s\n",
 				 of_node_full_name(dev));
-			*host = of_node_get(parent);
 			break;
 		}
 

diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 3786473..14848d8 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h

@@ -24,6 +24,23 @@ struct of_pci_range {
 #define for_each_of_pci_range(parser, range) \
 	for (; of_pci_range_parser_one(parser, range);)
 
+#ifndef indirect_io_enabled
+#define indirect_io_enabled indirect_io_enabled
+static inline bool indirect_io_enabled(void)
+{
+	return false;
+}
+#endif
+
+#ifndef addr_is_indirect_io
+#define addr_is_indirect_io addr_is_indirect_io
+static inline int addr_is_indirect_io(u64 taddr)
+{
+	return 0;
+}
+#endif
+
+
 /* Translate a DMA address from device space to CPU space */
 extern u64 of_translate_dma_address(struct device_node *dev,
 				    const __be32 *in_addr);

-- 
2.7.4