diff mbox

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

Message ID EE11001F9E5DDD47B7634E2F8A612F2E1F939E0A@lhreml507-mbx (mailing list archive)
State New, archived
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...

Comments

Arnd Bergmann Nov. 25, 2016, 12:03 p.m. UTC | #1
On Friday, November 25, 2016 8:46:11 AM CET Gabriele Paoloni wrote:
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > 
> > 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
> >  /*
> > @@ -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

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.

> >   */
> >  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);"..?
> 

Yes, Zhichang also pointed out the same thing, this is not
right yet. My thought was that we need to check the #address-cells
and #size-cells of the parent node and return if they are not set,
but the bus should really have those.

What we need to do instead is check the "ranges" of the parent
and fail if there is no translation. Simply setting the host
here however won't work either because that leads to returning
OF_BAD_ADDR.


> >  		/* 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;
...
> > +
> >  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);

[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]

>  /*
> + * 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);
> +}

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.

> +/*
>   * 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;
> +		}
>  

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.

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