* [U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree
@ 2016-05-16 17:44 Paul Burton
2016-05-16 17:44 ` [U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses Paul Burton
` (4 more replies)
0 siblings, 5 replies; 23+ messages in thread
From: Paul Burton @ 2016-05-16 17:44 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses
2016-05-16 17:44 [U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree Paul Burton
@ 2016-05-16 17:44 ` Paul Burton
2016-05-17 12:11 ` Simon Glass
2016-05-16 17:44 ` [U-Boot] [PATCH v2 2/5] fdt: Document the rest of struct of_bus Paul Burton
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Paul Burton @ 2016-05-16 17:44 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 2/5] fdt: Document the rest of struct of_bus
2016-05-16 17:44 [U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree Paul Burton
2016-05-16 17:44 ` [U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses Paul Burton
@ 2016-05-16 17:44 ` Paul Burton
2016-05-17 12:11 ` Simon Glass
2016-05-16 17:44 ` [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports Paul Burton
` (2 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Paul Burton @ 2016-05-16 17:44 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-16 17:44 [U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree Paul Burton
2016-05-16 17:44 ` [U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses Paul Burton
2016-05-16 17:44 ` [U-Boot] [PATCH v2 2/5] fdt: Document the rest of struct of_bus Paul Burton
@ 2016-05-16 17:44 ` Paul Burton
2016-05-16 18:58 ` Daniel Schwierzeck
2016-05-17 12:11 ` Simon Glass
2016-05-16 17:44 ` [U-Boot] [PATCH v2 4/5] malta: Tidy up UART address selection Paul Burton
2016-05-16 17:44 ` [U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART Paul Burton
4 siblings, 2 replies; 23+ messages in thread
From: Paul Burton @ 2016-05-16 17:44 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 4/5] malta: Tidy up UART address selection
2016-05-16 17:44 [U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree Paul Burton
` (2 preceding siblings ...)
2016-05-16 17:44 ` [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports Paul Burton
@ 2016-05-16 17:44 ` Paul Burton
2016-05-16 18:57 ` Daniel Schwierzeck
2016-05-16 17:44 ` [U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART Paul Burton
4 siblings, 1 reply; 23+ messages in thread
From: Paul Burton @ 2016-05-16 17:44 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART
2016-05-16 17:44 [U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree Paul Burton
` (3 preceding siblings ...)
2016-05-16 17:44 ` [U-Boot] [PATCH v2 4/5] malta: Tidy up UART address selection Paul Burton
@ 2016-05-16 17:44 ` Paul Burton
2016-05-16 18:56 ` Daniel Schwierzeck
4 siblings, 1 reply; 23+ messages in thread
From: Paul Burton @ 2016-05-16 17:44 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART
2016-05-16 17:44 ` [U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART Paul Burton
@ 2016-05-16 18:56 ` Daniel Schwierzeck
2016-05-17 6:40 ` Paul Burton
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Schwierzeck @ 2016-05-16 18:56 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 4/5] malta: Tidy up UART address selection
2016-05-16 17:44 ` [U-Boot] [PATCH v2 4/5] malta: Tidy up UART address selection Paul Burton
@ 2016-05-16 18:57 ` Daniel Schwierzeck
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Schwierzeck @ 2016-05-16 18:57 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-16 17:44 ` [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports Paul Burton
@ 2016-05-16 18:58 ` Daniel Schwierzeck
2016-05-17 12:11 ` Simon Glass
1 sibling, 0 replies; 23+ messages in thread
From: Daniel Schwierzeck @ 2016-05-16 18:58 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART
2016-05-16 18:56 ` Daniel Schwierzeck
@ 2016-05-17 6:40 ` Paul Burton
2016-05-17 10:57 ` Daniel Schwierzeck
0 siblings, 1 reply; 23+ messages in thread
From: Paul Burton @ 2016-05-17 6:40 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART
2016-05-17 6:40 ` Paul Burton
@ 2016-05-17 10:57 ` Daniel Schwierzeck
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Schwierzeck @ 2016-05-17 10:57 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses
2016-05-16 17:44 ` [U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses Paul Burton
@ 2016-05-17 12:11 ` Simon Glass
0 siblings, 0 replies; 23+ messages in thread
From: Simon Glass @ 2016-05-17 12:11 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 2/5] fdt: Document the rest of struct of_bus
2016-05-16 17:44 ` [U-Boot] [PATCH v2 2/5] fdt: Document the rest of struct of_bus Paul Burton
@ 2016-05-17 12:11 ` Simon Glass
0 siblings, 0 replies; 23+ messages in thread
From: Simon Glass @ 2016-05-17 12:11 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-16 17:44 ` [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports Paul Burton
2016-05-16 18:58 ` Daniel Schwierzeck
@ 2016-05-17 12:11 ` Simon Glass
2016-05-17 12:48 ` Paul Burton
1 sibling, 1 reply; 23+ messages in thread
From: Simon Glass @ 2016-05-17 12:11 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-17 12:11 ` Simon Glass
@ 2016-05-17 12:48 ` Paul Burton
2016-05-17 15:51 ` Daniel Schwierzeck
0 siblings, 1 reply; 23+ messages in thread
From: Paul Burton @ 2016-05-17 12:48 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-17 12:48 ` Paul Burton
@ 2016-05-17 15:51 ` Daniel Schwierzeck
2016-05-17 15:54 ` Simon Glass
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Schwierzeck @ 2016-05-17 15:51 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-17 15:51 ` Daniel Schwierzeck
@ 2016-05-17 15:54 ` Simon Glass
2016-05-17 15:58 ` Paul Burton
0 siblings, 1 reply; 23+ messages in thread
From: Simon Glass @ 2016-05-17 15:54 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-17 15:54 ` Simon Glass
@ 2016-05-17 15:58 ` Paul Burton
2016-05-17 16:00 ` Simon Glass
0 siblings, 1 reply; 23+ messages in thread
From: Paul Burton @ 2016-05-17 15:58 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-17 15:58 ` Paul Burton
@ 2016-05-17 16:00 ` Simon Glass
2016-05-18 10:04 ` Daniel Schwierzeck
0 siblings, 1 reply; 23+ messages in thread
From: Simon Glass @ 2016-05-17 16:00 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-17 16:00 ` Simon Glass
@ 2016-05-18 10:04 ` Daniel Schwierzeck
2016-05-18 14:52 ` Simon Glass
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Schwierzeck @ 2016-05-18 10:04 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-18 10:04 ` Daniel Schwierzeck
@ 2016-05-18 14:52 ` Simon Glass
2016-05-21 16:50 ` Daniel Schwierzeck
0 siblings, 1 reply; 23+ messages in thread
From: Simon Glass @ 2016-05-18 14:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-18 14:52 ` Simon Glass
@ 2016-05-21 16:50 ` Daniel Schwierzeck
2016-05-24 15:34 ` Paul Burton
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Schwierzeck @ 2016-05-21 16:50 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-21 16:50 ` Daniel Schwierzeck
@ 2016-05-24 15:34 ` Paul Burton
0 siblings, 0 replies; 23+ messages in thread
From: Paul Burton @ 2016-05-24 15:34 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-05-24 15:34 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-16 17:44 [U-Boot] [PATCH v2 0/5] Malta UART using device model & device tree Paul Burton
2016-05-16 17:44 ` [U-Boot] [PATCH v2 1/5] fdt: Support for ISA busses Paul Burton
2016-05-17 12:11 ` Simon Glass
2016-05-16 17:44 ` [U-Boot] [PATCH v2 2/5] fdt: Document the rest of struct of_bus Paul Burton
2016-05-17 12:11 ` Simon Glass
2016-05-16 17:44 ` [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports Paul Burton
2016-05-16 18:58 ` Daniel Schwierzeck
2016-05-17 12:11 ` Simon Glass
2016-05-17 12:48 ` Paul Burton
2016-05-17 15:51 ` Daniel Schwierzeck
2016-05-17 15:54 ` Simon Glass
2016-05-17 15:58 ` Paul Burton
2016-05-17 16:00 ` Simon Glass
2016-05-18 10:04 ` Daniel Schwierzeck
2016-05-18 14:52 ` Simon Glass
2016-05-21 16:50 ` Daniel Schwierzeck
2016-05-24 15:34 ` Paul Burton
2016-05-16 17:44 ` [U-Boot] [PATCH v2 4/5] malta: Tidy up UART address selection Paul Burton
2016-05-16 18:57 ` Daniel Schwierzeck
2016-05-16 17:44 ` [U-Boot] [PATCH v2 5/5] malta: Use device model & tree for UART Paul Burton
2016-05-16 18:56 ` Daniel Schwierzeck
2016-05-17 6:40 ` Paul Burton
2016-05-17 10:57 ` Daniel Schwierzeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.