RE: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

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

Hi Liviu

Thanks for reviewing

> -----Original Message-----
> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> Sent: 09 November 2016 11:40
> To: Yuanzhichang
> Cc: catalin.marinas@arm.com; will.deacon@arm.com; robh+dt@kernel.org;
> bhelgaas@google.com; mark.rutland@arm.com; olof@lixom.net;
> arnd@arndb.de; linux-arm-kernel@lists.infradead.org;
> lorenzo.pieralisi@arm.com; linux-kernel@vger.kernel.org; Linuxarm;
> devicetree@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> serial@vger.kernel.org; minyard@acm.org; benh@kernel.crashing.org;
> zourongrong@gmail.com; John Garry; Gabriele Paoloni;
> zhichang.yuan02@gmail.com; kantyzc@163.com; xuwei (O)
> Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> special ISA
> 
> On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> > This patch solves two issues:
> > 1) parse and get the right I/O range from DTS node whose parent does
> not
> > define the corresponding ranges property;
> >
> > There are some special ISA/LPC devices that work on a specific I/O
> range where
> > it is not correct to specify a ranges property in DTS parent node as
> cpu
> > addresses translated from DTS node are only for memory space on some
> > architectures, such as Arm64. Without the parent 'ranges' property,
> current
> > of_translate_address() return an error.
> > Here we add a fixup function, of_get_isa_indirect_io(). During the OF
> address
> > translation, this fixup will be called to check the 'reg' address to
> be
> > translating is for those sepcial ISA/LPC devices and get the I/O
> range
> > directly from the 'reg' property.
> >
> > 2) eliminate the I/O range conflict risk with PCI/PCIE leagecy I/O
> device;
> >
> > The current __of_address_to_resource() always translates the I/O
> range to PIO.
> > But this processing is not suitable for our ISA/LPC devices whose I/O
> range is
> > not cpu address(Arnd had stressed this in his comments on V2,V3
> patch-set).
> > Here, we bypass the mapping between cpu address and PIO for the
> special
> > ISA/LPC devices. But to drive these ISA/LPC devices, a I/O port
> address below
> > PCIBIOS_MIN_IO is needed by in*/out*(). Which means there is conflict
> risk
> > between I/O range of [0, PCIBIOS_MIN_IO) and PCI/PCIE legacy I/O
> range of [0,
> > IO_SPACE_LIMIT).
> > To avoid the I/O conflict, this patch reserve the I/O range below
> > PCIBIOS_MIN_IO.
> >
> > Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> > ---
> >  .../arm/hisilicon/hisilicon-low-pin-count.txt      | 31 ++++++++++++
> >  arch/arm64/include/asm/io.h                        |  6 +++
> >  arch/arm64/kernel/extio.c                          | 25 ++++++++++
> >  drivers/of/address.c                               | 56
> +++++++++++++++++++++-
> >  drivers/pci/pci.c                                  |  6 +--
> >  include/linux/of_address.h                         | 17 +++++++
> >  include/linux/pci.h                                |  8 ++++
> >  7 files changed, 145 insertions(+), 4 deletions(-)
> >  create mode 100644
> Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-
> count.txt
> >
> > diff --git
> a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-
> count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-
> low-pin-count.txt
> > new file mode 100644
> > index 0000000..13c8ddd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-
> pin-count.txt
> > @@ -0,0 +1,31 @@
> > +Hisilicon Hip06 low-pin-count device
> > +  Usually LPC controller is part of PCI host bridge, so the legacy
> ISA ports
> > +  locate on LPC bus can be accessed direclty. But some SoCs have
> independent
> > +  LPC controller, and access the legacy ports by triggering LPC I/O
> cycles.
> > +  Hisilicon Hip06 implements this LPC device.
> > +
> > +Required properties:
> > +- compatible: should be "hisilicon,low-pin-count"
> > +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> > +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> > +- reg: base memory range where the register set of this device is
> mapped.
> > +
> > +Note:
> > +  The node name before '@' must be "isa" to represent the binding
> stick to the
> > +  ISA/EISA binding specification.
> > +
> > +Example:
> > +
> > +isa@a01b0000 {
> > +	compatible = "hisilicom,low-pin-count";
> > +	#address-cells = <2>;
> > +	#size-cells = <1>;
> > +	reg = <0x0 0xa01b0000 0x0 0x1000>;
> > +
> > +	ipmi0: bt@e4 {
> > +		compatible = "ipmi-bt";
> > +		device_type = "ipmi";
> > +		reg = <0x01 0xe4 0x04>;
> > +		status = "disabled";
> > +	};
> > +};
> 
> 
> This documentation file needs to be part of the next patch. It has
> nothing to do with
> what you are trying to fix here.

Yes you're right...we'll move it to next one

> 
> 
> > diff --git a/arch/arm64/include/asm/io.h
> b/arch/arm64/include/asm/io.h
> > index 136735d..c26b7cc 100644
> > --- a/arch/arm64/include/asm/io.h
> > +++ b/arch/arm64/include/asm/io.h
> > @@ -175,6 +175,12 @@ static inline u64 __raw_readq(const volatile
> void __iomem *addr)
> >  #define outsl outsl
> >
> >  DECLARE_EXTIO(l, u32)
> > +
> > +#define indirect_io_enabled indirect_io_enabled
> > +extern bool indirect_io_enabled(void);
> > +
> > +#define addr_is_indirect_io addr_is_indirect_io
> > +extern int addr_is_indirect_io(u64 taddr);
> >  #endif
> >
> >
> > diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> > index 647b3fa..3d45fa8 100644
> > --- a/arch/arm64/kernel/extio.c
> > +++ b/arch/arm64/kernel/extio.c
> > @@ -19,6 +19,31 @@
> >
> >  struct extio_ops *arm64_extio_ops;
> >
> > +/**
> > + * indirect_io_enabled - check whether indirectIO is enabled.
> > + *	arm64_extio_ops will be set only when indirectIO mechanism had
> been
> > + *	initialized.
> > + *
> > + * Returns true when indirectIO is enabled.
> > + */
> > +bool indirect_io_enabled(void)
> > +{
> > +	return arm64_extio_ops ? true : false;
> > +}
> > +
> > +/**
> > + * addr_is_indirect_io - check whether the input taddr is for
> indirectIO.
> > + * @taddr: the io address to be checked.
> > + *
> > + * Returns 1 when taddr is in the range; otherwise return 0.
> > + */
> > +int addr_is_indirect_io(u64 taddr)
> > +{
> > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
> taddr)
> 
> start >= taddr ?

Nope... if  (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
then taddr is outside the range [start; end] and will return 0; otherwise
it will return 1...

> 
> > +		return 0;
> > +
> > +	return 1;
> > +}
> >
> >  BUILD_EXTIO(b, u8)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 02b2903..cc2a05d 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> device_node *np)
> >  	return false;
> >  }
> >
> > +
> > +/*
> > + * 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.
> 
> Bah, you are returning a signed int, why 0 for failure? Return a
> negative value with
> error codes. Otherwise change the return value into a bool.

Yes we'll move to bool

> 
> > + */
> > +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. */
> 
> s/there is ranges/if we have a 'ranges'/

Thanks for spotting this

> 
> > +	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);
> > +}
> > +
> >  static int 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)
> > @@ -595,6 +639,15 @@ 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);
> > +			break;
> > +		}
> >
> >  		/* Get new parent bus and counts */
> >  		pbus = of_match_bus(parent);
> > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> device_node *dev,
> >  	if (taddr == OF_BAD_ADDR)
> >  		return -EINVAL;
> >  	memset(r, 0, sizeof(struct resource));
> > -	if (flags & IORESOURCE_IO) {
> > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> >  		unsigned long port;
> > +
> >  		port = pci_address_to_pio(taddr);
> >  		if (port == (unsigned long)-1)
> >  			return -EINVAL;
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index ba34907..1a08511 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
> addr, resource_size_t size)
> >
> >  #ifdef PCI_IOBASE
> >  	struct io_range *range;
> > -	resource_size_t allocated_size = 0;
> > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> >
> >  	/* check if the range hasn't been previously recorded */
> >  	spin_lock(&io_range_lock);
> > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
> pio)
> >
> >  #ifdef PCI_IOBASE
> >  	struct io_range *range;
> > -	resource_size_t allocated_size = 0;
> > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> 
> Have you checked that pci_pio_to_address still returns valid values
> after this? I know that
> you are trying to take into account PCIBIOS_MIN_IO limit when
> allocating reserving the IO ranges,
> but the values added in the io_range_list are still starting from zero,
> no from PCIBIOS_MIN_IO,

I think you're wrong here as in pci_address_to_pio we have:
+	resource_size_t offset = PCIBIOS_MIN_IO;

This should be enough to guarantee that the PIOs start at
PCIBIOS_MIN_IO...right?


> so the calculation of the address in this function could return
> negative values casted to pci_addr_t.
> 
> Maybe you want to adjust the range->start value in
> pci_register_io_range() as well to have it
> offset by PCIBIOS_MIN_IO as well.
> 
> Best regards,
> Liviu
> 
> >
> >  	if (pio > IO_SPACE_LIMIT)
> >  		return address;
> > @@ -3335,7 +3335,7 @@ unsigned long __weak
> pci_address_to_pio(phys_addr_t address)
> >  {
> >  #ifdef PCI_IOBASE
> >  	struct io_range *res;
> > -	resource_size_t offset = 0;
> > +	resource_size_t offset = PCIBIOS_MIN_IO;
> >  	unsigned long addr = -1;
> >
> >  	spin_lock(&io_range_lock);
> > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > index 3786473..deec469 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);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 0e49f70..7f6bbb6 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> pci_bus *bus)
> >  /* provide the legacy pci_dma_* API */
> >  #include <linux/pci-dma-compat.h>
> >
> > +/*
> > + * define this macro here to refrain from compilation error for some
> > + * platforms. Please keep this macro at the end of this header file.
> > + */
> > +#ifndef PCIBIOS_MIN_IO
> > +#define PCIBIOS_MIN_IO		0
> > +#endif
> > +
> >  #endif /* LINUX_PCI_H */
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯