diff mbox

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

Message ID EE11001F9E5DDD47B7634E2F8A612F2E1F939E0A@lhreml507-mbx
State New
Headers show

Commit Message

Gabriele Paoloni Nov. 25, 2016, 8:46 a.m. UTC
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...

-- 
2.7.4
diff mbox

Patch

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