[U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree

[U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree

[Paul Burton]

This series converts the MIPS Malta development board to make use of
device model & device tree to probe the UART. This results in a tidier
way to handle differences between Malta boards & starts Malta on the
path to device model conversion.

Paul Burton (5):
  fdt: Support for ISA busses
  fdt: Document the rest of struct of_bus
  dm: ns16550: Don't map_physmem for I/O ports
  malta: Tidy up UART address selection
  malta: Use device model & tree for UART

 arch/mips/Kconfig           |   5 ++
 arch/mips/dts/Makefile      |   3 +-
 arch/mips/dts/mti,malta.dts |  32 ++++++++++++
 board/imgtec/malta/malta.c  |  13 -----
 common/fdt_support.c        | 120 ++++++++++++++++++++++++++++++++++++++++++--
 configs/malta_defconfig     |   1 +
 configs/maltael_defconfig   |   1 +
 drivers/core/Kconfig        |  23 +++++++++
 drivers/serial/ns16550.c    |   8 +++
 include/configs/malta.h     |   8 +--
 10 files changed, 190 insertions(+), 24 deletions(-)
 create mode 100644 arch/mips/dts/mti,malta.dts

-- 
2.8.2

[U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses

[Paul Burton]

Support ISA busses in much the same way as Linux does. This allows for
ISA bus addresses to be translated, and only if CONFIG_OF_ISA_BUS is
selected in order to avoid including the code in builds which won't need
it.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---

Changes in v2:
- Document the match function pointer in struct of_bus.
- Make CONFIG_OF_ISA_BUS not user-selectable, and explain it in help text.
- assert() that of_match_bus didn't fail instead of BUG(), and comment why.
- s/&of_busses[0]/of_busses/

 common/fdt_support.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/core/Kconfig |  23 ++++++++++++
 2 files changed, 121 insertions(+), 3 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 42e5d8a..96b5d0a 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -964,10 +964,21 @@ static void of_dump_addr(const char *s, const fdt32_t *addr, int na)
 static void of_dump_addr(const char *s, const fdt32_t *addr, int na) { }
 #endif
 
-/* Callbacks for bus specific translators */
+/**
+ * struct of_bus - Callbacks for bus specific translators
+ * @match:	Return non-zero if the node whose parent is at
+ *		parentoffset in the FDT blob corresponds to a bus
+ *		of this type, otherwise return zero. If NULL a match
+ *		is assumed.
+ *
+ * Each bus type will include a struct of_bus in the of_busses array,
+ * providing implementations of some or all of the functions used to
+ * match the bus & handle address translation for its children.
+ */
 struct of_bus {
 	const char	*name;
 	const char	*addresses;
+	int		(*match)(void *blob, int parentoffset);
 	void		(*count_cells)(void *blob, int parentoffset,
 				int *addrc, int *sizec);
 	u64		(*map)(fdt32_t *addr, const fdt32_t *range,
@@ -1022,8 +1033,70 @@ static int of_bus_default_translate(fdt32_t *addr, u64 offset, int na)
 	return 0;
 }
 
+#ifdef CONFIG_OF_ISA_BUS
+
+/* ISA bus translator */
+static int of_bus_isa_match(void *blob, int parentoffset)
+{
+	const char *name;
+
+	name = fdt_get_name(blob, parentoffset, NULL);
+	if (!name)
+		return 0;
+
+	return !strcmp(name, "isa");
+}
+
+static void of_bus_isa_count_cells(void *blob, int parentoffset,
+				   int *addrc, int *sizec)
+{
+	if (addrc)
+		*addrc = 2;
+	if (sizec)
+		*sizec = 1;
+}
+
+static u64 of_bus_isa_map(fdt32_t *addr, const fdt32_t *range,
+			  int na, int ns, int pna)
+{
+	u64 cp, s, da;
+
+	/* Check address type match */
+	if ((addr[0] ^ range[0]) & cpu_to_be32(1))
+		return OF_BAD_ADDR;
+
+	cp = of_read_number(range + 1, na - 1);
+	s  = of_read_number(range + na + pna, ns);
+	da = of_read_number(addr + 1, na - 1);
+
+	debug("OF: ISA map, cp=%" PRIu64 ", s=%" PRIu64
+	      ", da=%" PRIu64 "\n", cp, s, da);
+
+	if (da < cp || da >= (cp + s))
+		return OF_BAD_ADDR;
+	return da - cp;
+}
+
+static int of_bus_isa_translate(fdt32_t *addr, u64 offset, int na)
+{
+	return of_bus_default_translate(addr + 1, offset, na - 1);
+}
+
+#endif /* CONFIG_OF_ISA_BUS */
+
 /* Array of bus specific translators */
 static struct of_bus of_busses[] = {
+#ifdef CONFIG_OF_ISA_BUS
+	/* ISA */
+	{
+		.name = "isa",
+		.addresses = "reg",
+		.match = of_bus_isa_match,
+		.count_cells = of_bus_isa_count_cells,
+		.map = of_bus_isa_map,
+		.translate = of_bus_isa_translate,
+	},
+#endif /* CONFIG_OF_ISA_BUS */
 	/* Default */
 	{
 		.name = "default",
@@ -1034,6 +1107,28 @@ static struct of_bus of_busses[] = {
 	},
 };
 
+static struct of_bus *of_match_bus(void *blob, int parentoffset)
+{
+	struct of_bus *bus;
+
+	if (ARRAY_SIZE(of_busses) == 1)
+		return of_busses;
+
+	for (bus = of_busses; bus; bus++) {
+		if (!bus->match || bus->match(blob, parentoffset))
+			return bus;
+	}
+
+	/*
+	 * We should always have matched the default bus at least, since
+	 * it has a NULL match field. If we didn't then it somehow isn't
+	 * in the of_busses array or something equally catastrophic has
+	 * gone wrong.
+	 */
+	assert(0);
+	return NULL;
+}
+
 static int of_translate_one(void * blob, int parent, struct of_bus *bus,
 			    struct of_bus *pbus, fdt32_t *addr,
 			    int na, int ns, int pna, const char *rprop)
@@ -1113,7 +1208,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
 	parent = fdt_parent_offset(blob, node_offset);
 	if (parent < 0)
 		goto bail;
-	bus = &of_busses[0];
+	bus = of_match_bus(blob, parent);
 
 	/* Cound address cells & copy address locally */
 	bus->count_cells(blob, parent, &na, &ns);
@@ -1142,7 +1237,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
 		}
 
 		/* Get new parent bus and counts */
-		pbus = &of_busses[0];
+		pbus = of_match_bus(blob, parent);
 		pbus->count_cells(blob, parent, &pna, &pns);
 		if (!OF_CHECK_COUNTS(pna, pns)) {
 			printf("%s: Bad cell count for %s\n", __FUNCTION__,
diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index c5c9d2a..8749561 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -178,4 +178,27 @@ config SPL_OF_TRANSLATE
 	  used for the address translation. This function is faster and
 	  smaller in size than fdt_translate_address().
 
+config OF_ISA_BUS
+	bool
+	depends on OF_TRANSLATE
+	help
+	  Is this option is enabled then support for the ISA bus will
+	  be included for addresses read from DT. This is something that
+	  should be known to be required or not based upon the board
+	  being targetted, and whether or not it makes use of an ISA bus.
+
+	  The bus is matched based upon its node name equalling "isa". The
+	  busses #address-cells should equal 2, with the first cell being
+	  used to hold flags & flag 0x1 indicating that the address range
+	  should be accessed using I/O port in/out accessors. The second
+	  cell holds the offset into ISA bus address space. The #size-cells
+	  property should equal 1, and of course holds the size of the
+	  address range used by a device.
+
+	  If this option is not enabled then support for the ISA bus is
+	  not included and any such busses used in DT will be treated as
+	  typical simple-bus compatible busses. This will lead to
+	  mistranslation of device addresses, so ensure that this is
+	  enabled if your board does include an ISA bus.
+
 endmenu
-- 
2.8.2

[U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses

[Simon Glass]

Hi Paul,

On 16 May 2016 at 11:44, Paul Burton <paul.burton@imgtec.com> wrote:
> Support ISA busses in much the same way as Linux does. This allows for
> ISA bus addresses to be translated, and only if CONFIG_OF_ISA_BUS is
> selected in order to avoid including the code in builds which won't need
> it.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> ---
>
> Changes in v2:
> - Document the match function pointer in struct of_bus.
> - Make CONFIG_OF_ISA_BUS not user-selectable, and explain it in help text.
> - assert() that of_match_bus didn't fail instead of BUG(), and comment why.
> - s/&of_busses[0]/of_busses/
>
>  common/fdt_support.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/core/Kconfig |  23 ++++++++++++
>  2 files changed, 121 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Tiny nit below.

>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 42e5d8a..96b5d0a 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -964,10 +964,21 @@ static void of_dump_addr(const char *s, const fdt32_t *addr, int na)
>  static void of_dump_addr(const char *s, const fdt32_t *addr, int na) { }
>  #endif
>
> -/* Callbacks for bus specific translators */
> +/**
> + * struct of_bus - Callbacks for bus specific translators
> + * @match:     Return non-zero if the node whose parent is at
> + *             parentoffset in the FDT blob corresponds to a bus
> + *             of this type, otherwise return zero. If NULL a match
> + *             is assumed.
> + *
> + * Each bus type will include a struct of_bus in the of_busses array,
> + * providing implementations of some or all of the functions used to
> + * match the bus & handle address translation for its children.
> + */
>  struct of_bus {
>         const char      *name;
>         const char      *addresses;
> +       int             (*match)(void *blob, int parentoffset);
>         void            (*count_cells)(void *blob, int parentoffset,
>                                 int *addrc, int *sizec);
>         u64             (*map)(fdt32_t *addr, const fdt32_t *range,
> @@ -1022,8 +1033,70 @@ static int of_bus_default_translate(fdt32_t *addr, u64 offset, int na)
>         return 0;
>  }
>
> +#ifdef CONFIG_OF_ISA_BUS
> +
> +/* ISA bus translator */
> +static int of_bus_isa_match(void *blob, int parentoffset)
> +{
> +       const char *name;
> +
> +       name = fdt_get_name(blob, parentoffset, NULL);
> +       if (!name)
> +               return 0;
> +
> +       return !strcmp(name, "isa");
> +}
> +
> +static void of_bus_isa_count_cells(void *blob, int parentoffset,
> +                                  int *addrc, int *sizec)
> +{
> +       if (addrc)
> +               *addrc = 2;
> +       if (sizec)
> +               *sizec = 1;
> +}
> +
> +static u64 of_bus_isa_map(fdt32_t *addr, const fdt32_t *range,
> +                         int na, int ns, int pna)
> +{
> +       u64 cp, s, da;
> +
> +       /* Check address type match */
> +       if ((addr[0] ^ range[0]) & cpu_to_be32(1))
> +               return OF_BAD_ADDR;
> +
> +       cp = of_read_number(range + 1, na - 1);
> +       s  = of_read_number(range + na + pna, ns);
> +       da = of_read_number(addr + 1, na - 1);
> +
> +       debug("OF: ISA map, cp=%" PRIu64 ", s=%" PRIu64
> +             ", da=%" PRIu64 "\n", cp, s, da);
> +
> +       if (da < cp || da >= (cp + s))
> +               return OF_BAD_ADDR;
> +       return da - cp;
> +}
> +
> +static int of_bus_isa_translate(fdt32_t *addr, u64 offset, int na)
> +{
> +       return of_bus_default_translate(addr + 1, offset, na - 1);
> +}
> +
> +#endif /* CONFIG_OF_ISA_BUS */
> +
>  /* Array of bus specific translators */
>  static struct of_bus of_busses[] = {
> +#ifdef CONFIG_OF_ISA_BUS
> +       /* ISA */
> +       {
> +               .name = "isa",
> +               .addresses = "reg",
> +               .match = of_bus_isa_match,
> +               .count_cells = of_bus_isa_count_cells,
> +               .map = of_bus_isa_map,
> +               .translate = of_bus_isa_translate,
> +       },
> +#endif /* CONFIG_OF_ISA_BUS */
>         /* Default */
>         {
>                 .name = "default",
> @@ -1034,6 +1107,28 @@ static struct of_bus of_busses[] = {
>         },
>  };
>
> +static struct of_bus *of_match_bus(void *blob, int parentoffset)
> +{
> +       struct of_bus *bus;
> +
> +       if (ARRAY_SIZE(of_busses) == 1)
> +               return of_busses;
> +
> +       for (bus = of_busses; bus; bus++) {
> +               if (!bus->match || bus->match(blob, parentoffset))
> +                       return bus;
> +       }
> +
> +       /*
> +        * We should always have matched the default bus at least, since
> +        * it has a NULL match field. If we didn't then it somehow isn't
> +        * in the of_busses array or something equally catastrophic has
> +        * gone wrong.
> +        */
> +       assert(0);
> +       return NULL;
> +}
> +
>  static int of_translate_one(void * blob, int parent, struct of_bus *bus,
>                             struct of_bus *pbus, fdt32_t *addr,
>                             int na, int ns, int pna, const char *rprop)
> @@ -1113,7 +1208,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
>         parent = fdt_parent_offset(blob, node_offset);
>         if (parent < 0)
>                 goto bail;
> -       bus = &of_busses[0];
> +       bus = of_match_bus(blob, parent);
>
>         /* Cound address cells & copy address locally */
>         bus->count_cells(blob, parent, &na, &ns);
> @@ -1142,7 +1237,7 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
>                 }
>
>                 /* Get new parent bus and counts */
> -               pbus = &of_busses[0];
> +               pbus = of_match_bus(blob, parent);
>                 pbus->count_cells(blob, parent, &pna, &pns);
>                 if (!OF_CHECK_COUNTS(pna, pns)) {
>                         printf("%s: Bad cell count for %s\n", __FUNCTION__,
> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> index c5c9d2a..8749561 100644
> --- a/drivers/core/Kconfig
> +++ b/drivers/core/Kconfig
> @@ -178,4 +178,27 @@ config SPL_OF_TRANSLATE
>           used for the address translation. This function is faster and
>           smaller in size than fdt_translate_address().
>
> +config OF_ISA_BUS
> +       bool
> +       depends on OF_TRANSLATE
> +       help
> +         Is this option is enabled then support for the ISA bus will
> +         be included for addresses read from DT. This is something that
> +         should be known to be required or not based upon the board
> +         being targetted, and whether or not it makes use of an ISA bus.
> +
> +         The bus is matched based upon its node name equalling "isa". The
> +         busses #address-cells should equal 2, with the first cell being

bus'

> +         used to hold flags & flag 0x1 indicating that the address range
> +         should be accessed using I/O port in/out accessors. The second
> +         cell holds the offset into ISA bus address space. The #size-cells
> +         property should equal 1, and of course holds the size of the
> +         address range used by a device.
> +
> +         If this option is not enabled then support for the ISA bus is
> +         not included and any such busses used in DT will be treated as
> +         typical simple-bus compatible busses. This will lead to
> +         mistranslation of device addresses, so ensure that this is
> +         enabled if your board does include an ISA bus.
> +
>  endmenu
> --
> 2.8.2
>

Regards,
Simon

[U-Boot] [PATCH v2 2/5] fdt: Document the rest of struct of_bus

[Paul Burton]

Provide some documentation for the fields of struct of_bus, for
consistency with that provided for the new match field.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---

Changes in v2:
- New patch.

 common/fdt_support.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 96b5d0a..5d8eb12 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -966,10 +966,29 @@ static void of_dump_addr(const char *s, const fdt32_t *addr, int na) { }
 
 /**
  * struct of_bus - Callbacks for bus specific translators
+ * @name:	A string used to identify this bus in debug output.
+ * @addresses:	The name of the DT property from which addresses are
+ *		to be read, typically "reg".
  * @match:	Return non-zero if the node whose parent is at
  *		parentoffset in the FDT blob corresponds to a bus
  *		of this type, otherwise return zero. If NULL a match
  *		is assumed.
+ * @count_cells:Count how many cells (be32 values) a node whose parent
+ *		is at parentoffset in the FDT blob will require to
+ *		represent its address (written to *addrc) & size
+ *		(written to *sizec).
+ * @map:	Map the address addr from the address space of this
+ *		bus to that of its parent, making use of the ranges
+ *		read from DT to an array at range. na and ns are the
+ *		number of cells (be32 values) used to hold and address
+ *		or size, respectively, for this bus. pna is the number
+ *		of cells used to hold an address for the parent bus.
+ *		Returns the address in the address space of the parent
+ *		bus.
+ * @translate:	Update the value of the address cells at addr within an
+ *		FDT by adding offset to it. na specifies the number of
+ *		cells used to hold the address being translated. Returns
+ *		zero on success, non-zero on error.
  *
  * Each bus type will include a struct of_bus in the of_busses array,
  * providing implementations of some or all of the functions used to
-- 
2.8.2

[U-Boot] [PATCH v2 2/5] fdt: Document the rest of struct of_bus

[Simon Glass]

On 16 May 2016 at 11:44, Paul Burton <paul.burton@imgtec.com> wrote:
> Provide some documentation for the fields of struct of_bus, for
> consistency with that provided for the new match field.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> ---
>
> Changes in v2:
> - New patch.
>
>  common/fdt_support.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Paul Burton]

If the UART is to be accessed using I/O port accessors (inb & outb) then
using map_physmem doesn't make sense, since it operates in a different
memory space. Remove the call to map_physmem when
CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses
to not be mangled by the incorrect mapping.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---

Changes in v2:
- New patch, part of a simplified approach tackling only a single Malta UART.

 drivers/serial/ns16550.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 28da9dd..e58e6aa 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
 	unsigned char *addr;
 
 	offset *= 1 << plat->reg_shift;
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+	addr = (unsigned char *)plat->base + offset;
+#else
 	addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
+#endif
 	/*
 	 * As far as we know it doesn't make sense to support selection of
 	 * these options at run-time, so use the existing CONFIG options.
@@ -114,7 +118,11 @@ static int ns16550_readb(NS16550_t port, int offset)
 	unsigned char *addr;
 
 	offset *= 1 << plat->reg_shift;
+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
+	addr = (unsigned char *)plat->base + offset;
+#else
 	addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
+#endif
 
 	return serial_in_shift(addr + plat->reg_offset, plat->reg_shift);
 }
-- 
2.8.2

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Daniel Schwierzeck]

Am 16.05.2016 um 19:44 schrieb Paul Burton:
> If the UART is to be accessed using I/O port accessors (inb & outb) then
> using map_physmem doesn't make sense, since it operates in a different
> memory space. Remove the call to map_physmem when
> CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses
> to not be mangled by the incorrect mapping.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>

Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
Tested-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>

> ---
> 
> Changes in v2:
> - New patch, part of a simplified approach tackling only a single Malta UART.
> 
>  drivers/serial/ns16550.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 28da9dd..e58e6aa 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
>  	unsigned char *addr;
>  
>  	offset *= 1 << plat->reg_shift;
> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +	addr = (unsigned char *)plat->base + offset;
> +#else
>  	addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> +#endif
>  	/*
>  	 * As far as we know it doesn't make sense to support selection of
>  	 * these options at run-time, so use the existing CONFIG options.
> @@ -114,7 +118,11 @@ static int ns16550_readb(NS16550_t port, int offset)
>  	unsigned char *addr;
>  
>  	offset *= 1 << plat->reg_shift;
> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +	addr = (unsigned char *)plat->base + offset;
> +#else
>  	addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> +#endif
>  
>  	return serial_in_shift(addr + plat->reg_offset, plat->reg_shift);
>  }
> 

-- 
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160516/ea38488e/attachment.sig>

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Simon Glass]

Hi Paul,

On 16 May 2016 at 11:44, Paul Burton <paul.burton@imgtec.com> wrote:
> If the UART is to be accessed using I/O port accessors (inb & outb) then
> using map_physmem doesn't make sense, since it operates in a different
> memory space. Remove the call to map_physmem when
> CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses
> to not be mangled by the incorrect mapping.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> ---
>
> Changes in v2:
> - New patch, part of a simplified approach tackling only a single Malta UART.
>
>  drivers/serial/ns16550.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 28da9dd..e58e6aa 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
>         unsigned char *addr;
>
>         offset *= 1 << plat->reg_shift;
> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +       addr = (unsigned char *)plat->base + offset;
> +#else
>         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> +#endif

Please don't add CONFIG #ifdefs in these functions. Perhaps it needs
to be another parameter? Possibly a flag. But with driver-model we
need to be able to support both options in the core code.

This driver is a real mess. I'm hoping we can simplify it once we have
everything on driver model.

>         /*
>          * As far as we know it doesn't make sense to support selection of
>          * these options at run-time, so use the existing CONFIG options.
> @@ -114,7 +118,11 @@ static int ns16550_readb(NS16550_t port, int offset)
>         unsigned char *addr;
>
>         offset *= 1 << plat->reg_shift;
> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +       addr = (unsigned char *)plat->base + offset;
> +#else
>         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> +#endif
>
>         return serial_in_shift(addr + plat->reg_offset, plat->reg_shift);
>  }
> --
> 2.8.2
>

Regards,
Simon

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Paul Burton]

On Tue, May 17, 2016 at 06:11:22AM -0600, Simon Glass wrote:
> Hi Paul,
> 
> On 16 May 2016 at 11:44, Paul Burton <paul.burton@imgtec.com> wrote:
> > If the UART is to be accessed using I/O port accessors (inb & outb) then
> > using map_physmem doesn't make sense, since it operates in a different
> > memory space. Remove the call to map_physmem when
> > CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses
> > to not be mangled by the incorrect mapping.
> >
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > ---
> >
> > Changes in v2:
> > - New patch, part of a simplified approach tackling only a single Malta UART.
> >
> >  drivers/serial/ns16550.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> > index 28da9dd..e58e6aa 100644
> > --- a/drivers/serial/ns16550.c
> > +++ b/drivers/serial/ns16550.c
> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
> >         unsigned char *addr;
> >
> >         offset *= 1 << plat->reg_shift;
> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> > +       addr = (unsigned char *)plat->base + offset;
> > +#else
> >         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> > +#endif
> 
> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs
> to be another parameter? Possibly a flag. But with driver-model we
> need to be able to support both options in the core code.

Hi Simon,

Are you sure systems rely on using I/O ports with map_physmem? The only
other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones,
in include/configs/x86-common.h, and so far as I can tell they don't use
device model which suggests this code has simply been untested before. I
don't see why you would use map_physmem on an I/O port address that is
then going to be passed to inb/outb & I think the code here is simply
wrong to do so.

> This driver is a real mess. I'm hoping we can simplify it once we have
> everything on driver model.

Agreed, that would be wonderful.

Thanks,
    Paul

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Daniel Schwierzeck]

2016-05-17 14:48 GMT+02:00 Paul Burton <paul.burton@imgtec.com>:
> On Tue, May 17, 2016 at 06:11:22AM -0600, Simon Glass wrote:
>> Hi Paul,
>>
>> On 16 May 2016 at 11:44, Paul Burton <paul.burton@imgtec.com> wrote:
>> > If the UART is to be accessed using I/O port accessors (inb & outb) then
>> > using map_physmem doesn't make sense, since it operates in a different
>> > memory space. Remove the call to map_physmem when
>> > CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses
>> > to not be mangled by the incorrect mapping.
>> >
>> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>> > ---
>> >
>> > Changes in v2:
>> > - New patch, part of a simplified approach tackling only a single Malta UART.
>> >
>> >  drivers/serial/ns16550.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> > index 28da9dd..e58e6aa 100644
>> > --- a/drivers/serial/ns16550.c
>> > +++ b/drivers/serial/ns16550.c
>> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
>> >         unsigned char *addr;
>> >
>> >         offset *= 1 << plat->reg_shift;
>> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>> > +       addr = (unsigned char *)plat->base + offset;
>> > +#else
>> >         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
>> > +#endif
>>
>> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs
>> to be another parameter? Possibly a flag. But with driver-model we
>> need to be able to support both options in the core code.
>
> Hi Simon,
>
> Are you sure systems rely on using I/O ports with map_physmem? The only
> other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones,
> in include/configs/x86-common.h, and so far as I can tell they don't use
> device model which suggests this code has simply been untested before. I
> don't see why you would use map_physmem on an I/O port address that is
> then going to be passed to inb/outb & I think the code here is simply
> wrong to do so.

the current code looks wrong. serial_in_shift() is expanded to inb()
in case of CONFIG_SYS_NS16550_PORT_MAPPED and to
in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case
a map_physmem() is required and should be done in serial_in_shift()
itself or preferrably only once in
ns16550_serial_ofdata_to_platdata().

I think the correct approach would be the following:

--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -100,7 +100,8 @@ static void ns16550_writeb(NS16550_t port, int
offset, int value)
        unsigned char *addr;

        offset *= 1 << plat->reg_shift;
-       addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
+       addr = plat->base + offset;
+
        /*
         * As far as we know it doesn't make sense to support selection of
         * these options at run-time, so use the existing CONFIG options.
@@ -114,7 +115,7 @@ static int ns16550_readb(NS16550_t port, int offset)
        unsigned char *addr;

        offset *= 1 << plat->reg_shift;
-       addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
+       addr = plat->base + offset;

        return serial_in_shift(addr + plat->reg_offset, plat->reg_shift);
 }
@@ -400,7 +401,12 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
        if (addr == FDT_ADDR_T_NONE)
                return -EINVAL;

+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
        plat->base = addr;
+#else
+       plat->base = map_physmem(addr, 0, MAP_NOCACHE);
+#endif
+
        plat->reg_offset = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
                                     "reg-offset", 0);
        plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,

-- 
- Daniel

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Simon Glass]

Hi Daniel,

On 17 May 2016 at 09:51, Daniel Schwierzeck
<daniel.schwierzeck@gmail.com> wrote:
>
> 2016-05-17 14:48 GMT+02:00 Paul Burton <paul.burton@imgtec.com>:
> > On Tue, May 17, 2016 at 06:11:22AM -0600, Simon Glass wrote:
> >> Hi Paul,
> >>
> >> On 16 May 2016 at 11:44, Paul Burton <paul.burton@imgtec.com> wrote:
> >> > If the UART is to be accessed using I/O port accessors (inb & outb) then
> >> > using map_physmem doesn't make sense, since it operates in a different
> >> > memory space. Remove the call to map_physmem when
> >> > CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses
> >> > to not be mangled by the incorrect mapping.
> >> >
> >> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> >> > ---
> >> >
> >> > Changes in v2:
> >> > - New patch, part of a simplified approach tackling only a single Malta UART.
> >> >
> >> >  drivers/serial/ns16550.c | 8 ++++++++
> >> >  1 file changed, 8 insertions(+)
> >> >
> >> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> >> > index 28da9dd..e58e6aa 100644
> >> > --- a/drivers/serial/ns16550.c
> >> > +++ b/drivers/serial/ns16550.c
> >> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
> >> >         unsigned char *addr;
> >> >
> >> >         offset *= 1 << plat->reg_shift;
> >> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> >> > +       addr = (unsigned char *)plat->base + offset;
> >> > +#else
> >> >         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> >> > +#endif
> >>
> >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs
> >> to be another parameter? Possibly a flag. But with driver-model we
> >> need to be able to support both options in the core code.
> >
> > Hi Simon,
> >
> > Are you sure systems rely on using I/O ports with map_physmem? The only
> > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones,
> > in include/configs/x86-common.h, and so far as I can tell they don't use
> > device model which suggests this code has simply been untested before. I
> > don't see why you would use map_physmem on an I/O port address that is
> > then going to be passed to inb/outb & I think the code here is simply
> > wrong to do so.
>
> the current code looks wrong. serial_in_shift() is expanded to inb()
> in case of CONFIG_SYS_NS16550_PORT_MAPPED and to
> in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case
> a map_physmem() is required and should be done in serial_in_shift()
> itself or preferrably only once in
> ns16550_serial_ofdata_to_platdata().
>
> I think the correct approach would be the following:

This is better I think. But how about adding a device tree binding to
select I/O access? In principle each device might have its own
settings.

>
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -100,7 +100,8 @@ static void ns16550_writeb(NS16550_t port, int
> offset, int value)
>         unsigned char *addr;
>
>         offset *= 1 << plat->reg_shift;
> -       addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> +       addr = plat->base + offset;
> +
>         /*
>          * As far as we know it doesn't make sense to support selection of
>          * these options at run-time, so use the existing CONFIG options.
> @@ -114,7 +115,7 @@ static int ns16550_readb(NS16550_t port, int offset)
>         unsigned char *addr;
>
>         offset *= 1 << plat->reg_shift;
> -       addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> +       addr = plat->base + offset;
>
>         return serial_in_shift(addr + plat->reg_offset, plat->reg_shift);
>  }
> @@ -400,7 +401,12 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
>         if (addr == FDT_ADDR_T_NONE)
>                 return -EINVAL;
>
> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>         plat->base = addr;
> +#else
> +       plat->base = map_physmem(addr, 0, MAP_NOCACHE);
> +#endif
> +
>         plat->reg_offset = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>                                      "reg-offset", 0);
>         plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>
> --
> - Daniel

Regards,
Simon

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Paul Burton]

On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote:
> > >> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> > >> > index 28da9dd..e58e6aa 100644
> > >> > --- a/drivers/serial/ns16550.c
> > >> > +++ b/drivers/serial/ns16550.c
> > >> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
> > >> >         unsigned char *addr;
> > >> >
> > >> >         offset *= 1 << plat->reg_shift;
> > >> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> > >> > +       addr = (unsigned char *)plat->base + offset;
> > >> > +#else
> > >> >         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
> > >> > +#endif
> > >>
> > >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs
> > >> to be another parameter? Possibly a flag. But with driver-model we
> > >> need to be able to support both options in the core code.
> > >
> > > Hi Simon,
> > >
> > > Are you sure systems rely on using I/O ports with map_physmem? The only
> > > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones,
> > > in include/configs/x86-common.h, and so far as I can tell they don't use
> > > device model which suggests this code has simply been untested before. I
> > > don't see why you would use map_physmem on an I/O port address that is
> > > then going to be passed to inb/outb & I think the code here is simply
> > > wrong to do so.
> >
> > the current code looks wrong. serial_in_shift() is expanded to inb()
> > in case of CONFIG_SYS_NS16550_PORT_MAPPED and to
> > in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case
> > a map_physmem() is required and should be done in serial_in_shift()
> > itself or preferrably only once in
> > ns16550_serial_ofdata_to_platdata().
> >
> > I think the correct approach would be the following:
> 
> This is better I think. But how about adding a device tree binding to
> select I/O access? In principle each device might have its own
> settings.

Note that's what I worked towards last time I had a crack at this, but
it just expanded into an attempt to tackle the mess that is ns16550.c &
rather lost sight of the original goal of making Malta work.

https://patchwork.ozlabs.org/patch/575643/
https://patchwork.ozlabs.org/patch/577194/

Thanks,
    Paul

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Simon Glass]

Hi Paul,

On 17 May 2016 at 09:58, Paul Burton <paul.burton@imgtec.com> wrote:
> On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote:
>> > >> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> > >> > index 28da9dd..e58e6aa 100644
>> > >> > --- a/drivers/serial/ns16550.c
>> > >> > +++ b/drivers/serial/ns16550.c
>> > >> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
>> > >> >         unsigned char *addr;
>> > >> >
>> > >> >         offset *= 1 << plat->reg_shift;
>> > >> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>> > >> > +       addr = (unsigned char *)plat->base + offset;
>> > >> > +#else
>> > >> >         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
>> > >> > +#endif
>> > >>
>> > >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs
>> > >> to be another parameter? Possibly a flag. But with driver-model we
>> > >> need to be able to support both options in the core code.
>> > >
>> > > Hi Simon,
>> > >
>> > > Are you sure systems rely on using I/O ports with map_physmem? The only
>> > > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones,
>> > > in include/configs/x86-common.h, and so far as I can tell they don't use
>> > > device model which suggests this code has simply been untested before. I
>> > > don't see why you would use map_physmem on an I/O port address that is
>> > > then going to be passed to inb/outb & I think the code here is simply
>> > > wrong to do so.
>> >
>> > the current code looks wrong. serial_in_shift() is expanded to inb()
>> > in case of CONFIG_SYS_NS16550_PORT_MAPPED and to
>> > in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case
>> > a map_physmem() is required and should be done in serial_in_shift()
>> > itself or preferrably only once in
>> > ns16550_serial_ofdata_to_platdata().
>> >
>> > I think the correct approach would be the following:
>>
>> This is better I think. But how about adding a device tree binding to
>> select I/O access? In principle each device might have its own
>> settings.
>
> Note that's what I worked towards last time I had a crack at this, but
> it just expanded into an attempt to tackle the mess that is ns16550.c &
> rather lost sight of the original goal of making Malta work.
>
> https://patchwork.ozlabs.org/patch/575643/
> https://patchwork.ozlabs.org/patch/577194/

Yes it is tricky. What do you think about the suggestions above?

Regards,
Simon

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Daniel Schwierzeck]

Am 17.05.2016 um 18:00 schrieb Simon Glass:
> Hi Paul,
> 
> On 17 May 2016 at 09:58, Paul Burton <paul.burton@imgtec.com> wrote:
>> On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote:
>>>>>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>>>>>> index 28da9dd..e58e6aa 100644
>>>>>>> --- a/drivers/serial/ns16550.c
>>>>>>> +++ b/drivers/serial/ns16550.c
>>>>>>> @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
>>>>>>>         unsigned char *addr;
>>>>>>>
>>>>>>>         offset *= 1 << plat->reg_shift;
>>>>>>> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>>>>>>> +       addr = (unsigned char *)plat->base + offset;
>>>>>>> +#else
>>>>>>>         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
>>>>>>> +#endif
>>>>>>
>>>>>> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs
>>>>>> to be another parameter? Possibly a flag. But with driver-model we
>>>>>> need to be able to support both options in the core code.
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> Are you sure systems rely on using I/O ports with map_physmem? The only
>>>>> other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones,
>>>>> in include/configs/x86-common.h, and so far as I can tell they don't use
>>>>> device model which suggests this code has simply been untested before. I
>>>>> don't see why you would use map_physmem on an I/O port address that is
>>>>> then going to be passed to inb/outb & I think the code here is simply
>>>>> wrong to do so.
>>>>
>>>> the current code looks wrong. serial_in_shift() is expanded to inb()
>>>> in case of CONFIG_SYS_NS16550_PORT_MAPPED and to
>>>> in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case
>>>> a map_physmem() is required and should be done in serial_in_shift()
>>>> itself or preferrably only once in
>>>> ns16550_serial_ofdata_to_platdata().
>>>>
>>>> I think the correct approach would be the following:
>>>
>>> This is better I think. But how about adding a device tree binding to
>>> select I/O access? In principle each device might have its own
>>> settings.
>>
>> Note that's what I worked towards last time I had a crack at this, but
>> it just expanded into an attempt to tackle the mess that is ns16550.c &
>> rather lost sight of the original goal of making Malta work.
>>
>> https://patchwork.ozlabs.org/patch/575643/
>> https://patchwork.ozlabs.org/patch/577194/
> 
> Yes it is tricky. What do you think about the suggestions above?

for now we should only fix the broken port-based I/O. Additional DT
bindings could be added later. I'd like to merge the DM support on MIPS
Malta in this merge window ;)

-- 
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160518/71425fa1/attachment.sig>

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Simon Glass]

Hi Daniel,

On 18 May 2016 at 04:04, Daniel Schwierzeck
<daniel.schwierzeck@gmail.com> wrote:
>
>
> Am 17.05.2016 um 18:00 schrieb Simon Glass:
>> Hi Paul,
>>
>> On 17 May 2016 at 09:58, Paul Burton <paul.burton@imgtec.com> wrote:
>>> On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote:
>>>>>>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>>>>>>> index 28da9dd..e58e6aa 100644
>>>>>>>> --- a/drivers/serial/ns16550.c
>>>>>>>> +++ b/drivers/serial/ns16550.c
>>>>>>>> @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
>>>>>>>>         unsigned char *addr;
>>>>>>>>
>>>>>>>>         offset *= 1 << plat->reg_shift;
>>>>>>>> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>>>>>>>> +       addr = (unsigned char *)plat->base + offset;
>>>>>>>> +#else
>>>>>>>>         addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
>>>>>>>> +#endif
>>>>>>>
>>>>>>> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs
>>>>>>> to be another parameter? Possibly a flag. But with driver-model we
>>>>>>> need to be able to support both options in the core code.
>>>>>>
>>>>>> Hi Simon,
>>>>>>
>>>>>> Are you sure systems rely on using I/O ports with map_physmem? The only
>>>>>> other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones,
>>>>>> in include/configs/x86-common.h, and so far as I can tell they don't use
>>>>>> device model which suggests this code has simply been untested before. I
>>>>>> don't see why you would use map_physmem on an I/O port address that is
>>>>>> then going to be passed to inb/outb & I think the code here is simply
>>>>>> wrong to do so.
>>>>>
>>>>> the current code looks wrong. serial_in_shift() is expanded to inb()
>>>>> in case of CONFIG_SYS_NS16550_PORT_MAPPED and to
>>>>> in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case
>>>>> a map_physmem() is required and should be done in serial_in_shift()
>>>>> itself or preferrably only once in
>>>>> ns16550_serial_ofdata_to_platdata().
>>>>>
>>>>> I think the correct approach would be the following:
>>>>
>>>> This is better I think. But how about adding a device tree binding to
>>>> select I/O access? In principle each device might have its own
>>>> settings.
>>>
>>> Note that's what I worked towards last time I had a crack at this, but
>>> it just expanded into an attempt to tackle the mess that is ns16550.c &
>>> rather lost sight of the original goal of making Malta work.
>>>
>>> https://patchwork.ozlabs.org/patch/575643/
>>> https://patchwork.ozlabs.org/patch/577194/
>>
>> Yes it is tricky. What do you think about the suggestions above?
>
> for now we should only fix the broken port-based I/O. Additional DT
> bindings could be added later. I'd like to merge the DM support on MIPS
> Malta in this merge window ;)

In that case your patch of moving it to ofdata_to_platdata() looks OK to me.

Regards,
Simon

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Daniel Schwierzeck]

Hi Paul,

Am 18.05.2016 um 16:52 schrieb Simon Glass:
>>>>>>>
>>>>>>> Are you sure systems rely on using I/O ports with map_physmem? The only
>>>>>>> other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones,
>>>>>>> in include/configs/x86-common.h, and so far as I can tell they don't use
>>>>>>> device model which suggests this code has simply been untested before. I
>>>>>>> don't see why you would use map_physmem on an I/O port address that is
>>>>>>> then going to be passed to inb/outb & I think the code here is simply
>>>>>>> wrong to do so.
>>>>>>
>>>>>> the current code looks wrong. serial_in_shift() is expanded to inb()
>>>>>> in case of CONFIG_SYS_NS16550_PORT_MAPPED and to
>>>>>> in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case
>>>>>> a map_physmem() is required and should be done in serial_in_shift()
>>>>>> itself or preferrably only once in
>>>>>> ns16550_serial_ofdata_to_platdata().
>>>>>>
>>>>>> I think the correct approach would be the following:
>>>>>
>>>>> This is better I think. But how about adding a device tree binding to
>>>>> select I/O access? In principle each device might have its own
>>>>> settings.
>>>>
>>>> Note that's what I worked towards last time I had a crack at this, but
>>>> it just expanded into an attempt to tackle the mess that is ns16550.c &
>>>> rather lost sight of the original goal of making Malta work.
>>>>
>>>> https://patchwork.ozlabs.org/patch/575643/
>>>> https://patchwork.ozlabs.org/patch/577194/
>>>
>>> Yes it is tricky. What do you think about the suggestions above?
>>
>> for now we should only fix the broken port-based I/O. Additional DT
>> bindings could be added later. I'd like to merge the DM support on MIPS
>> Malta in this merge window ;)
> 
> In that case your patch of moving it to ofdata_to_platdata() looks OK to me.

are you going to send a v4 of this patch? I can also do this if you
like. Then I could apply all other v3 patches of this series.

-- 
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160521/c85be92f/attachment.sig>

[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Paul Burton]

On Sat, May 21, 2016 at 06:50:37PM +0200, Daniel Schwierzeck wrote:
> are you going to send a v4 of this patch? I can also do this if you
> like. Then I could apply all other v3 patches of this series.

Hi Daniel,

I'm happy with either of those options. If you'd prefer me to submit a
v4 let me know, but I'd be happy with you replacing patch 3/5.

Thanks,
    Paul

[U-Boot] [PATCH v2 4/5] malta: Tidy up UART address selection

[Paul Burton]

The address of the UART differs based upon the system controller because
it's actually within the I/O port region, which is in a different
location for each system controller. Rather than handling this as 2
UARTs with the correct one selected at runtime, use I/O port accessors
for the UART such that access to it gets translated into the I/O port
region automatically.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---

Changes in v2:
- New patch, simplifying the later DT conversion.

 board/imgtec/malta/malta.c | 13 -------------
 include/configs/malta.h    |  4 ++--
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/board/imgtec/malta/malta.c b/board/imgtec/malta/malta.c
index 3a9e780..4955043 100644
--- a/board/imgtec/malta/malta.c
+++ b/board/imgtec/malta/malta.c
@@ -12,7 +12,6 @@
 #include <pci_gt64120.h>
 #include <pci_msc01.h>
 #include <rtc.h>
-#include <serial.h>
 
 #include <asm/addrspace.h>
 #include <asm/io.h>
@@ -161,18 +160,6 @@ int misc_init_r(void)
 	return 0;
 }
 
-struct serial_device *default_serial_console(void)
-{
-	switch (malta_sys_con()) {
-	case SYSCON_GT64120:
-		return &eserial1_device;
-
-	default:
-	case SYSCON_MSC01:
-		return &eserial2_device;
-	}
-}
-
 void pci_init_board(void)
 {
 	pci_dev_t bdf;
diff --git a/include/configs/malta.h b/include/configs/malta.h
index 04dca71..1c3c83c 100644
--- a/include/configs/malta.h
+++ b/include/configs/malta.h
@@ -67,10 +67,10 @@
 #define CONFIG_BAUDRATE			115200
 
 #define CONFIG_SYS_NS16550_SERIAL
+#define CONFIG_SYS_NS16550_PORT_MAPPED
 #define CONFIG_SYS_NS16550_REG_SIZE	1
 #define CONFIG_SYS_NS16550_CLK		(115200 * 16)
-#define CONFIG_SYS_NS16550_COM1		0xb80003f8
-#define CONFIG_SYS_NS16550_COM2		0xbb0003f8
+#define CONFIG_SYS_NS16550_COM1		0x3f8
 #define CONFIG_CONS_INDEX		1
 
 /*
-- 
2.8.2

[U-Boot] [PATCH v2 4/5] malta: Tidy up UART address selection

[Daniel Schwierzeck]

Am 16.05.2016 um 19:44 schrieb Paul Burton:
> The address of the UART differs based upon the system controller because
> it's actually within the I/O port region, which is in a different
> location for each system controller. Rather than handling this as 2
> UARTs with the correct one selected at runtime, use I/O port accessors
> for the UART such that access to it gets translated into the I/O port
> region automatically.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>

Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>

> ---
> 
> Changes in v2:
> - New patch, simplifying the later DT conversion.
> 
>  board/imgtec/malta/malta.c | 13 -------------
>  include/configs/malta.h    |  4 ++--
>  2 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/board/imgtec/malta/malta.c b/board/imgtec/malta/malta.c
> index 3a9e780..4955043 100644
> --- a/board/imgtec/malta/malta.c
> +++ b/board/imgtec/malta/malta.c
> @@ -12,7 +12,6 @@
>  #include <pci_gt64120.h>
>  #include <pci_msc01.h>
>  #include <rtc.h>
> -#include <serial.h>
>  
>  #include <asm/addrspace.h>
>  #include <asm/io.h>
> @@ -161,18 +160,6 @@ int misc_init_r(void)
>  	return 0;
>  }
>  
> -struct serial_device *default_serial_console(void)
> -{
> -	switch (malta_sys_con()) {
> -	case SYSCON_GT64120:
> -		return &eserial1_device;
> -
> -	default:
> -	case SYSCON_MSC01:
> -		return &eserial2_device;
> -	}
> -}
> -
>  void pci_init_board(void)
>  {
>  	pci_dev_t bdf;
> diff --git a/include/configs/malta.h b/include/configs/malta.h
> index 04dca71..1c3c83c 100644
> --- a/include/configs/malta.h
> +++ b/include/configs/malta.h
> @@ -67,10 +67,10 @@
>  #define CONFIG_BAUDRATE			115200
>  
>  #define CONFIG_SYS_NS16550_SERIAL
> +#define CONFIG_SYS_NS16550_PORT_MAPPED
>  #define CONFIG_SYS_NS16550_REG_SIZE	1
>  #define CONFIG_SYS_NS16550_CLK		(115200 * 16)
> -#define CONFIG_SYS_NS16550_COM1		0xb80003f8
> -#define CONFIG_SYS_NS16550_COM2		0xbb0003f8
> +#define CONFIG_SYS_NS16550_COM1		0x3f8
>  #define CONFIG_CONS_INDEX		1
>  
>  /*
> 

-- 
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160516/1c340218/attachment.sig>

[U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART

[Paul Burton]

Make use of device model & device tree to probe the UART driver. This is
the initial step in bringing Malta up to date with driver model, and
allows for cleaner handling of the different I/O addresses for different
system controllers by specifying the ISA bus address instead of a
translated memory address.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---

Changes in v2:
- Only handle the UART we already use, for simplicity.

 arch/mips/Kconfig           |  5 +++++
 arch/mips/dts/Makefile      |  3 ++-
 arch/mips/dts/mti,malta.dts | 32 ++++++++++++++++++++++++++++++++
 configs/malta_defconfig     |  1 +
 configs/maltael_defconfig   |  1 +
 include/configs/malta.h     |  6 ------
 6 files changed, 41 insertions(+), 7 deletions(-)
 create mode 100644 arch/mips/dts/mti,malta.dts

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index fe37d1f..e407b19 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -23,7 +23,12 @@ config TARGET_QEMU_MIPS
 
 config TARGET_MALTA
 	bool "Support malta"
+	select DM
+	select DM_SERIAL
 	select DYNAMIC_IO_PORT_BASE
+	select OF_CONTROL
+	select OF_EMBED
+	select OF_ISA_BUS
 	select SUPPORTS_BIG_ENDIAN
 	select SUPPORTS_LITTLE_ENDIAN
 	select SUPPORTS_CPU_MIPS32_R1
diff --git a/arch/mips/dts/Makefile b/arch/mips/dts/Makefile
index b513918..45d623f 100644
--- a/arch/mips/dts/Makefile
+++ b/arch/mips/dts/Makefile
@@ -2,7 +2,8 @@
 # SPDX-License-Identifier:	GPL-2.0+
 #
 
-dtb-$(CONFIG_TARGET_PIC32MZDASK) += pic32mzda_sk.dtb
+dtb-$(CONFIG_TARGET_MALTA)		+= mti,malta.dtb
+dtb-$(CONFIG_TARGET_PIC32MZDASK)	+= pic32mzda_sk.dtb
 
 targets += $(dtb-y)
 
diff --git a/arch/mips/dts/mti,malta.dts b/arch/mips/dts/mti,malta.dts
new file mode 100644
index 0000000..1dba606
--- /dev/null
+++ b/arch/mips/dts/mti,malta.dts
@@ -0,0 +1,32 @@
+/dts-v1/;
+
+/memreserve/ 0x00000000 0x00001000;	/* Exception vectors */
+/memreserve/ 0x000f0000 0x00010000;	/* PIIX4 ISA memory */
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	compatible = "mti,malta";
+
+	chosen {
+		stdout-path = &uart0;
+	};
+
+	isa {
+		compatible = "isa";
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <1 0 0 0x1000>;
+
+		uart0: serial at 3f8 {
+			compatible = "ns16550a";
+
+			reg = <1 0x3f8 0x40>;
+			reg-shift = <0>;
+
+			clock-frequency = <1843200>;
+
+			u-boot,dm-pre-reloc;
+		};
+	};
+};
diff --git a/configs/malta_defconfig b/configs/malta_defconfig
index a16f10b..0b5075f 100644
--- a/configs/malta_defconfig
+++ b/configs/malta_defconfig
@@ -11,3 +11,4 @@ CONFIG_CMD_DHCP=y
 CONFIG_CMD_PING=y
 CONFIG_SYS_NS16550=y
 CONFIG_USE_PRIVATE_LIBGCC=y
+CONFIG_DEFAULT_DEVICE_TREE="mti,malta"
diff --git a/configs/maltael_defconfig b/configs/maltael_defconfig
index 5289797..876e806 100644
--- a/configs/maltael_defconfig
+++ b/configs/maltael_defconfig
@@ -12,3 +12,4 @@ CONFIG_CMD_DHCP=y
 CONFIG_CMD_PING=y
 CONFIG_SYS_NS16550=y
 CONFIG_USE_PRIVATE_LIBGCC=y
+CONFIG_DEFAULT_DEVICE_TREE="mti,malta"
diff --git a/include/configs/malta.h b/include/configs/malta.h
index 1c3c83c..e03935b 100644
--- a/include/configs/malta.h
+++ b/include/configs/malta.h
@@ -65,13 +65,7 @@
  * Serial driver
  */
 #define CONFIG_BAUDRATE			115200
-
-#define CONFIG_SYS_NS16550_SERIAL
 #define CONFIG_SYS_NS16550_PORT_MAPPED
-#define CONFIG_SYS_NS16550_REG_SIZE	1
-#define CONFIG_SYS_NS16550_CLK		(115200 * 16)
-#define CONFIG_SYS_NS16550_COM1		0x3f8
-#define CONFIG_CONS_INDEX		1
 
 /*
  * Flash configuration
-- 
2.8.2

[U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART

[Daniel Schwierzeck]

Am 16.05.2016 um 19:44 schrieb Paul Burton:
> Make use of device model & device tree to probe the UART driver. This is
> the initial step in bringing Malta up to date with driver model, and
> allows for cleaner handling of the different I/O addresses for different
> system controllers by specifying the ISA bus address instead of a
> translated memory address.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>

Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>

nits below

> ---
> 
> Changes in v2:
> - Only handle the UART we already use, for simplicity.
> 
>  arch/mips/Kconfig           |  5 +++++
>  arch/mips/dts/Makefile      |  3 ++-
>  arch/mips/dts/mti,malta.dts | 32 ++++++++++++++++++++++++++++++++
>  configs/malta_defconfig     |  1 +
>  configs/maltael_defconfig   |  1 +
>  include/configs/malta.h     |  6 ------
>  6 files changed, 41 insertions(+), 7 deletions(-)
>  create mode 100644 arch/mips/dts/mti,malta.dts
> 
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index fe37d1f..e407b19 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -23,7 +23,12 @@ config TARGET_QEMU_MIPS
>  
>  config TARGET_MALTA
>  	bool "Support malta"
> +	select DM
> +	select DM_SERIAL

actually DM_SERIAL should be selected in the defconfig. But Malta is a single target so it's ok.

>  	select DYNAMIC_IO_PORT_BASE
> +	select OF_CONTROL
> +	select OF_EMBED

but OF_EMBED should go into the defconfig. The user should decide if the DTB should be appended or be embedded.

> +	select OF_ISA_BUS
>  	select SUPPORTS_BIG_ENDIAN
>  	select SUPPORTS_LITTLE_ENDIAN
>  	select SUPPORTS_CPU_MIPS32_R1
> diff --git a/arch/mips/dts/Makefile b/arch/mips/dts/Makefile
> index b513918..45d623f 100644
> --- a/arch/mips/dts/Makefile
> +++ b/arch/mips/dts/Makefile
> @@ -2,7 +2,8 @@
>  # SPDX-License-Identifier:	GPL-2.0+
>  #
>  
> -dtb-$(CONFIG_TARGET_PIC32MZDASK) += pic32mzda_sk.dtb
> +dtb-$(CONFIG_TARGET_MALTA)		+= mti,malta.dtb
> +dtb-$(CONFIG_TARGET_PIC32MZDASK)	+= pic32mzda_sk.dtb

this doesn't apply to u-boot-mips/next but I can fix it.

>  
>  targets += $(dtb-y)
>  
> diff --git a/arch/mips/dts/mti,malta.dts b/arch/mips/dts/mti,malta.dts
> new file mode 100644
> index 0000000..1dba606
> --- /dev/null
> +++ b/arch/mips/dts/mti,malta.dts
> @@ -0,0 +1,32 @@
> +/dts-v1/;
> +
> +/memreserve/ 0x00000000 0x00001000;	/* Exception vectors */
> +/memreserve/ 0x000f0000 0x00010000;	/* PIIX4 ISA memory */
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	compatible = "mti,malta";
> +
> +	chosen {
> +		stdout-path = &uart0;
> +	};
> +
> +	isa {
> +		compatible = "isa";
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges = <1 0 0 0x1000>;

is following dtc warning fixable?

Warning (unit_address_vs_reg): Node /isa has a reg or ranges property, but no unit name

> +
> +		uart0: serial at 3f8 {
> +			compatible = "ns16550a";
> +
> +			reg = <1 0x3f8 0x40>;
> +			reg-shift = <0>;
> +
> +			clock-frequency = <1843200>;
> +
> +			u-boot,dm-pre-reloc;
> +		};
> +	};
> +};
> diff --git a/configs/malta_defconfig b/configs/malta_defconfig
> index a16f10b..0b5075f 100644
> --- a/configs/malta_defconfig
> +++ b/configs/malta_defconfig
> @@ -11,3 +11,4 @@ CONFIG_CMD_DHCP=y
>  CONFIG_CMD_PING=y
>  CONFIG_SYS_NS16550=y
>  CONFIG_USE_PRIVATE_LIBGCC=y
> +CONFIG_DEFAULT_DEVICE_TREE="mti,malta"
> diff --git a/configs/maltael_defconfig b/configs/maltael_defconfig
> index 5289797..876e806 100644
> --- a/configs/maltael_defconfig
> +++ b/configs/maltael_defconfig
> @@ -12,3 +12,4 @@ CONFIG_CMD_DHCP=y
>  CONFIG_CMD_PING=y
>  CONFIG_SYS_NS16550=y
>  CONFIG_USE_PRIVATE_LIBGCC=y
> +CONFIG_DEFAULT_DEVICE_TREE="mti,malta"
> diff --git a/include/configs/malta.h b/include/configs/malta.h
> index 1c3c83c..e03935b 100644
> --- a/include/configs/malta.h
> +++ b/include/configs/malta.h
> @@ -65,13 +65,7 @@
>   * Serial driver
>   */
>  #define CONFIG_BAUDRATE			115200
> -
> -#define CONFIG_SYS_NS16550_SERIAL
>  #define CONFIG_SYS_NS16550_PORT_MAPPED
> -#define CONFIG_SYS_NS16550_REG_SIZE	1
> -#define CONFIG_SYS_NS16550_CLK		(115200 * 16)
> -#define CONFIG_SYS_NS16550_COM1		0x3f8
> -#define CONFIG_CONS_INDEX		1
>  
>  /*
>   * Flash configuration
> 

-- 
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160516/600b8cb0/attachment.sig>

[U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART

[Paul Burton]

On Mon, May 16, 2016 at 08:56:32PM +0200, Daniel Schwierzeck wrote:
> > diff --git a/arch/mips/dts/mti,malta.dts b/arch/mips/dts/mti,malta.dts
> > new file mode 100644
> > index 0000000..1dba606
> > --- /dev/null
> > +++ b/arch/mips/dts/mti,malta.dts
> > @@ -0,0 +1,32 @@
> > +/dts-v1/;
> > +
> > +/memreserve/ 0x00000000 0x00001000;	/* Exception vectors */
> > +/memreserve/ 0x000f0000 0x00010000;	/* PIIX4 ISA memory */
> > +
> > +/ {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +	compatible = "mti,malta";
> > +
> > +	chosen {
> > +		stdout-path = &uart0;
> > +	};
> > +
> > +	isa {
> > +		compatible = "isa";
> > +		#address-cells = <2>;
> > +		#size-cells = <1>;
> > +		ranges = <1 0 0 0x1000>;
> 
> is following dtc warning fixable?
> 
> Warning (unit_address_vs_reg): Node /isa has a reg or ranges property, but no unit name

Hi Daniel,

Thanks for reviewing. I'm not seeing this warning with DTC 1.4.1. Which
version are you using?

Thanks,
    Paul

[U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART

[Daniel Schwierzeck]

2016-05-17 8:40 GMT+02:00 Paul Burton <paul.burton@imgtec.com>:
> On Mon, May 16, 2016 at 08:56:32PM +0200, Daniel Schwierzeck wrote:
>> > diff --git a/arch/mips/dts/mti,malta.dts b/arch/mips/dts/mti,malta.dts
>> > new file mode 100644
>> > index 0000000..1dba606
>> > --- /dev/null
>> > +++ b/arch/mips/dts/mti,malta.dts
>> > @@ -0,0 +1,32 @@
>> > +/dts-v1/;
>> > +
>> > +/memreserve/ 0x00000000 0x00001000;        /* Exception vectors */
>> > +/memreserve/ 0x000f0000 0x00010000;        /* PIIX4 ISA memory */
>> > +
>> > +/ {
>> > +   #address-cells = <1>;
>> > +   #size-cells = <1>;
>> > +   compatible = "mti,malta";
>> > +
>> > +   chosen {
>> > +           stdout-path = &uart0;
>> > +   };
>> > +
>> > +   isa {
>> > +           compatible = "isa";
>> > +           #address-cells = <2>;
>> > +           #size-cells = <1>;
>> > +           ranges = <1 0 0 0x1000>;
>>
>> is following dtc warning fixable?
>>
>> Warning (unit_address_vs_reg): Node /isa has a reg or ranges property, but no unit name
>
> Hi Daniel,
>
> Thanks for reviewing. I'm not seeing this warning with DTC 1.4.1. Which
> version are you using?
>

Currently I'm using DTC mainline. There was a patch [1] by Stephen
Warren applied some time ago which causes those warnings.

[1] https://git.kernel.org/cgit/utils/dtc/dtc.git/commit/?id=c9d9121683b35281239305e15adddfff2b462cf9

-- 
- Daniel