RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni <gabriele.paoloni@huawei.com>]

Hi Arnd

Many thanks for your contribution, much appreciated

I have some comments...see inline below 

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: 23 November 2016 23:23
> To: linux-arm-kernel@lists.infradead.org
> Cc: Gabriele Paoloni; 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.yuan02@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 Wednesday, November 23, 2016 6:07:11 PM CET Arnd Bergmann wrote:
> > On Wednesday, November 23, 2016 3:22:33 PM CET Gabriele Paoloni
> wrote:
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > > On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni
> wrote:
> >
> > Please don't proliferate the use of
> > pci_pio_to_address/pci_address_to_pio here, computing the physical
> > address from the logical address is trivial, you just need to
> > subtract the start of the range that you already use when matching
> > the port number range.
> >
> > The only thing we need here is to make of_address_to_resource()
> > return the correct logical port number that was registered for
> > a given host device when asked to translate an address that
> > does not have a CPU address associated with it.
> 
> Ok, I admit this was a little harder than I expected, but see below
> for a rough outline of how I think it can be done.
> 
> This makes it possible to translate bus specific I/O port numbers
> from device nodes into Linux port numbers, and gives a way to register
> them. We could take this further and completely remove
> pci_pio_to_address
> and pci_address_to_pio if we make the I/O port translation always
> go through the io_range list, looking up up the hostbridge by fwnode,
> but we don't have to do that now.
> 
> The patch is completely untested and probably buggy, it just seemed
> easier to put out a prototype than to keep going in circles with the
> discussion.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index bf601d4df8cf..6cadf0501bb0 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -730,7 +730,8 @@ static void acpi_pci_root_validate_resources(struct
> device *dev,
>  	}
>  }
> 
> -static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
> +static void acpi_pci_root_remap_iospace(struct fwnode_handle *node,
> +					struct resource_entry *entry)
>  {
>  #ifdef PCI_IOBASE
>  	struct resource *res = entry->res;
> @@ -739,11 +740,7 @@ static void acpi_pci_root_remap_iospace(struct
> resource_entry *entry)
>  	resource_size_t length = resource_size(res);
>  	unsigned long port;
> 
> -	if (pci_register_io_range(cpu_addr, length))
> -		goto err;
> -
> -	port = pci_address_to_pio(cpu_addr);
> -	if (port == (unsigned long)-1)
> +	if (pci_register_io_range(node, cpu_addr, length, &port))
>  		goto err;
> 
>  	res->start = port;
> @@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct
> acpi_pci_root_info *info)
>  	else {
>  		resource_list_for_each_entry_safe(entry, tmp, list) {
>  			if (entry->res->flags & IORESOURCE_IO)
> -				acpi_pci_root_remap_iospace(entry);
> +				acpi_pci_root_remap_iospace(&device->fwnode,
> +							    entry);
> 
>  			if (entry->res->flags & IORESOURCE_DISABLED)
>  				resource_list_destroy_entry(entry);
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index a50025a3777f..df96955a43f8 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -760,8 +760,10 @@ static int __nbd_ioctl(struct block_device *bdev,
> struct nbd_device *nbd,
>  		set_bit(NBD_RUNNING, &nbd->runtime_flags);
>  		blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd-
> >num_connections);
>  		args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
> -		if (!args)
> +		if (!args) {
> +			error = -ENOMEM;
>  			goto out_err;
> +		}
>  		nbd->task_recv = current;
>  		mutex_unlock(&nbd->config_lock);
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903fe9d2..5decaba96eed 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -2,6 +2,7 @@
>  #define pr_fmt(fmt)	"OF: " fmt
> 
>  #include <linux/device.h>
> +#include <linux/fwnode.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
>  #include <linux/module.h>
> @@ -323,14 +324,9 @@ int of_pci_range_to_resource(struct of_pci_range
> *range,
> 
>  	if (res->flags & IORESOURCE_IO) {
>  		unsigned long port;
> -		err = pci_register_io_range(range->cpu_addr, range->size);
> +		err = pci_register_io_range(&np->fwnode, range->cpu_addr,
> range->size, &port);
>  		if (err)
>  			goto invalid_range;
> -		port = pci_address_to_pio(range->cpu_addr);
> -		if (port == (unsigned long)-1) {
> -			err = -EINVAL;
> -			goto invalid_range;
> -		}
>  		res->start = port;
>  	} else {
>  		if ((sizeof(resource_size_t) < 8) &&
> @@ -479,7 +475,7 @@ static int of_empty_ranges_quirk(struct device_node
> *np)
>  	return false;
>  }
> 
> -static int of_translate_one(struct device_node *parent, struct of_bus
> *bus,
> +static u64 of_translate_one(struct device_node *parent, struct of_bus
> *bus,
>  			    struct of_bus *pbus, __be32 *addr,
>  			    int na, int ns, int pna, const char *rprop)
>  {
> @@ -507,7 +503,7 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
>  	ranges = of_get_property(parent, rprop, &rlen);
>  	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>  		pr_debug("no ranges; cannot translate\n");
> -		return 1;
> +		return OF_BAD_ADDR;
>  	}
>  	if (ranges == NULL || rlen == 0) {
>  		offset = of_read_number(addr, na);
> @@ -528,7 +524,7 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
>  	}
>  	if (offset == OF_BAD_ADDR) {
>  		pr_debug("not found !\n");
> -		return 1;
> +		return offset;
>  	}
>  	memcpy(addr, ranges + na, 4 * pna);
> 
> @@ -537,7 +533,10 @@ static int of_translate_one(struct device_node
> *parent, struct of_bus *bus,
>  	pr_debug("with offset: %llx\n", (unsigned long long)offset);
> 
>  	/* Translate it into parent bus space */
> -	return pbus->translate(addr, offset, pna);
> +	if (pbus->translate(addr, offset, pna))
> +		return OF_BAD_ADDR;
> +
> +	return offset;
>  }
> 
>  /*
> @@ -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
> value
>   * that can be mapped to a cpu physical address). This is not really
> specified
>   * that way, but this is traditionally the way IBM at least do things
> + *
> + * Whenever the translation fails, the *host pointer will be set to
> the
> + * device that lacks a tranlation, and the return code is relative to
> + * 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.

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


>   */
>  static u64 __of_translate_address(struct device_node *dev,
> -				  const __be32 *in_addr, const char *rprop)
> +				  const __be32 *in_addr, const char *rprop,
> +				  struct device_node **host)
>  {
>  	struct device_node *parent = NULL;
>  	struct of_bus *bus, *pbus;
> @@ -564,6 +568,7 @@ static u64 __of_translate_address(struct
> device_node *dev,
>  	/* Increase refcount at current level */
>  	of_node_get(dev);
> 
> +	*host = NULL;
>  	/* Get parent & match bus type */
>  	parent = of_get_parent(dev);
>  	if (parent == NULL)
> @@ -600,8 +605,9 @@ static u64 __of_translate_address(struct
> device_node *dev,
>  		pbus = of_match_bus(parent);
>  		pbus->count_cells(dev, &pna, &pns);
>  		if (!OF_CHECK_COUNTS(pna, pns)) {
> -			pr_err("Bad cell count for %s\n",
> -			       of_node_full_name(dev));
> +			pr_debug("Bad cell count for %s\n",
> +				 of_node_full_name(dev));
> +			*host = of_node_get(parent);
>  			break;
>  		}
> 
> @@ -609,7 +615,9 @@ static u64 __of_translate_address(struct
> device_node *dev,
>  		    pbus->name, pna, pns, of_node_full_name(parent));
> 
>  		/* Apply bus translation */
> -		if (of_translate_one(dev, bus, pbus, addr, na, ns, pna,
> rprop))
> +		result = of_translate_one(dev, bus, pbus, addr, na, ns,
> +					  pna, rprop);
> +		if (result == OF_BAD_ADDR)

It seems to me that here you missed "*host = of_node_get(parent);"..?

>  			break;
> 
>  		/* Complete the move up one level */
> @@ -628,13 +636,32 @@ static u64 __of_translate_address(struct
> device_node *dev,
> 
>  u64 of_translate_address(struct device_node *dev, const __be32
> *in_addr)
>  {
> -	return __of_translate_address(dev, in_addr, "ranges");
> +	struct device_node *host;
> +	u64 ret;
> +
> +	ret =  __of_translate_address(dev, in_addr, "ranges", &host);
> +	if (host) {
> +		of_node_put(host);
> +		return OF_BAD_ADDR;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(of_translate_address);
> 
>  u64 of_translate_dma_address(struct device_node *dev, const __be32
> *in_addr)
>  {
> -	return __of_translate_address(dev, in_addr, "dma-ranges");
> +	struct device_node *host;
> +	u64 ret;
> +
> +	ret = __of_translate_address(dev, in_addr, "dma-ranges", &host);
> +
> +	if (host) {
> +		of_node_put(host);
> +		return OF_BAD_ADDR;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(of_translate_dma_address);
> 
> @@ -676,29 +703,48 @@ const __be32 *of_get_address(struct device_node
> *dev, int index, u64 *size,
>  }
>  EXPORT_SYMBOL(of_get_address);
> 
> +extern unsigned long extio_translate(struct fwnode_handle *node,
> unsigned long offset);
> +
> +u64 of_translate_ioport(struct device_node *dev, const __be32
> *in_addr)
> +{
> +	u64 taddr;
> +	unsigned long port;
> +	struct device_node *host;
> +
> +	taddr = __of_translate_address(dev, in_addr, "ranges", &host);
> +	if (host) {
> +		/* host specific port access */
> +		port = extio_translate(&host->fwnode, taddr);
> +		of_node_put(host);
> +	} else {
> +		/* memory mapped I/O range */
> +		port = pci_address_to_pio(taddr);
> +		if (port == (unsigned long)-1)
> +			return OF_BAD_ADDR;
> +	}
> +
> +	return port;
> +}
> +
>  static int __of_address_to_resource(struct device_node *dev,
>  		const __be32 *addrp, u64 size, unsigned int flags,
>  		const char *name, struct resource *r)
>  {
>  	u64 taddr;
> 
> -	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> +	if (flags & IORESOURCE_MEM)
> +		taddr = of_translate_address(dev, addrp);
> +	else if (flags & IORESOURCE_IO)
> +		taddr = of_translate_ioport(dev, addrp);
> +	else
>  		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 (port == (unsigned long)-1)
> -			return -EINVAL;
> -		r->start = port;
> -		r->end = port + size - 1;
> -	} else {
> -		r->start = taddr;
> -		r->end = taddr + size - 1;
> -	}
> +
> +	r->start = taddr;
> +	r->end = taddr + size - 1;
>  	r->flags = flags;
>  	r->name = name ? name : dev->full_name;
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index eda6a7cf0e54..320ab9fbf6af 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3249,6 +3249,7 @@ EXPORT_SYMBOL(pci_request_regions_exclusive);
>  #ifdef PCI_IOBASE
>  struct io_range {
>  	struct list_head list;
> +	struct fwnode_handle *node;
>  	phys_addr_t start;
>  	resource_size_t size;
>  };
> @@ -3257,11 +3258,14 @@ static LIST_HEAD(io_range_list);
>  static DEFINE_SPINLOCK(io_range_lock);
>  #endif
> 
> +#define IO_RANGE_IOEXT (resource_size_t)(-1ull)
> +
>  /*
>   * Record the PCI IO range (expressed as CPU physical address + size).
>   * Return a negative value if an error has occured, zero otherwise
>   */
> -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t
> size)
> +int __weak pci_register_io_range(struct fwnode_handle *node,
> phys_addr_t addr,
> +				 resource_size_t size, unsigned long *port)
>  {
>  	int err = 0;
> 
> @@ -3272,7 +3276,12 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
>  	/* check if the range hasn't been previously recorded */
>  	spin_lock(&io_range_lock);
>  	list_for_each_entry(range, &io_range_list, list) {
> -		if (addr >= range->start && addr + size <= range->start +
> size) {
> +		if (node == range->node)
> +			goto end_register;
> +

It seems to me that the condition above is sufficient; i.e.
we can remove the one here below...?

> +		if (addr != IO_RANGE_IOEXT &&
> +		    addr >= range->start &&
> +		    addr + size <= range->start + size) {
>  			/* range already registered, bail out */
>  			goto end_register;
>  		}
> @@ -3298,6 +3307,7 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
>  		goto end_register;
>  	}
> 
> +	range->node = node;
>  	range->start = addr;
>  	range->size = size;
> 
> @@ -3305,11 +3315,26 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
> 
>  end_register:
>  	spin_unlock(&io_range_lock);
> +
> +	*port = allocated_size;
> +#else
> +	/*
> +	 * powerpc and microblaze have their own registration,
> +	 * just look up the value here
> +	 */
> +	*port = pci_address_to_pio(addr);
>  #endif
> 
>  	return err;
>  }
> 
> +#ifdef CONFIG_IOEXT
> +int ioext_register_io_range
> +{
> +	return pci_register_io_range(node, IO_RANGE_IOEXT, size, port);
> +}
> +#endif
> +
>  phys_addr_t pci_pio_to_address(unsigned long pio)
>  {
>  	phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6bd94a803e8f..b7a8fa3da3ca 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1192,7 +1192,8 @@ int __must_check pci_bus_alloc_resource(struct
> pci_bus *bus,
>  			void *alignf_data);
> 
> 
> -int pci_register_io_range(phys_addr_t addr, resource_size_t size);
> +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t
> addr,
> +			  resource_size_t size, unsigned long *port);
>  unsigned long pci_address_to_pio(phys_addr_t addr);
>  phys_addr_t pci_pio_to_address(unsigned long pio);
>  int pci_remap_iospace(const struct resource *res, phys_addr_t
> phys_addr);

I think the patch below is a more reasonable approach to identify
a host that does not support address translation and it should 
guarantee safety against broken DTs...

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 5decaba..9bfc526 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -540,6 +540,49 @@ static u64 of_translate_one(struct device_node *parent, struct of_bus *bus,
 }
 

Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>

 /*
+ * of_isa_indirect_io - get the IO address from some isa reg property value.
+ *	For some isa/lpc devices, no ranges property in ancestor node.
+ *	The device addresses are described directly in their 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 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. */
+	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);
+}
+
+/*
  * 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.
@@ -600,14 +643,23 @@ static u64 __of_translate_address(struct device_node *dev,
 			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",
+				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