Malta UART using device model & device tree

We only use struct resource in a single place (drivers/usb/dwc3/core.h) for a field (xhci_resources) which is never used. Only ARM currently defines resource_size_t which means linux/ioport.h only compiles there. In preparation for making use of the IORESOURCE_ flags, remove struct resource & the various declarations of functions which we don't implement.

Signed-off-by Paul Burton <[email protected]>

---

 drivers/usb/dwc3/core.h |   1 -
 include/linux/ioport.h  | 104

------------------------------------------------ 2 files changed, 105 deletions(-)

Quoted 7 lines · Paul Burton

We only use struct resource in a single place (drivers/usb/dwc3/core.h) for a field (xhci_resources) which is never used. Only ARM currently defines resource_size_t which means linux/ioport.h only compiles there. In preparation for making use of the IORESOURCE_ flags, remove struct resource & the various declarations of functions which we don't implement.

Quoted 7 lines

Signed-off-by Paul Burton <[email protected]>

---

 drivers/usb/dwc3/core.h |   1 -
 include/linux/ioport.h  | 104

------------------------------------------------ 2 files changed, 105 deletions(-)

I believe the driver is imported from Linux kernel, so it'd be much better to sync the driver with mainline Linux instead of starting to diverge.

Best regards,
Marek Vasut

Quoted 17 lines · Marek Vasut

Quoted 15 lines · Paul Burton

We only use struct resource in a single place (drivers/usb/dwc3/core.h) for a field (xhci_resources) which is never used. Only ARM currently defines resource_size_t which means linux/ioport.h only compiles there. In preparation for making use of the IORESOURCE_ flags, remove struct resource & the various declarations of functions which we don't implement.

Signed-off-by Paul Burton <[email protected]>

---

 drivers/usb/dwc3/core.h |   1 -
 include/linux/ioport.h  | 104

------------------------------------------------ 2 files changed, 105 deletions(-)

Quoted 6 lines

I believe the driver is imported from Linux kernel, so it'd be much better to sync the driver with mainline Linux instead of starting to diverge.

Best regards,
Marek Vasut

Hi Marek,

The problem is that the driver can't use struct resource because U-Boot has none of the infrastructure around it. The driver model doesn't use struct resource, there's basically nothing in U-Boot to fill out the struct. So unless that changes this dwc3 driver will always have to handle resources differently to on Linux.

I therefore don't see any good reason to keep around an unused struct which will only currently compile for one architecture, for a driver which can't use it in U-Boot anyway.

The alternative to this patch would be to define resource_size_t for other architectures, but then we're just left with dead code.

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

---

 common/fdt_support.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/core/Kconfig |  4 +++
 2 files changed, 85 insertions(+), 2 deletions(-)

common/fdt_support.c +0 -0

index 75d0858..0aba77d 100644

common/fdt_support.c +65 -0

        void            (*count_cells)(void *blob, int parentoffset,
                                int *addrc, int *sizec);
        u64             (*map)(fdt32_t *addr, const fdt32_t *range,

@@ -1013,8 +1014,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",

@@ -1025,6 +1088,22 @@ static struct of_bus of_busses[] = {
        },
 };
 
+static struct of_bus *of_match_bus(void *blob, int parentoffset)
+{
+       struct of_bus *bus;

common/fdt_support.c +5 -2

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

@@ -1104,7 +1183,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);

@@ -1133,7 +1212,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)) {
                        printf("%s: Bad cell count for %s\n", __FUNCTION__,

drivers/core/Kconfig +0 -0

index c5c9d2a..339f52f 100644

drivers/core/Kconfig +1 -1

+
 endmenu
--

2.7.0

Hi Paul,

Quoted 12 lines · 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 <[email protected]>

---

 common/fdt_support.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/core/Kconfig |  4 +++
 2 files changed, 85 insertions(+), 2 deletions(-)

common/fdt_support.c static void of_dump_addr(const char *s, const fdt32_t *addr, int na) { } · line 962

Quoted 9 lines

common/fdt_support.c +1 -0

index 75d0858..0aba77d 100644
@@ -959,6 +959,7 @@ static void of_dump_addr(const char *s, const fdt32_t *addr, int na) { }
 struct of_bus {
        const char      *name;
        const char      *addresses;
+       int             (*match)(void *blob, int parentoffset);

Can you please add a comment to this method explaining what it does, args, return value?

Quoted 78 lines

common/fdt_support.c +62 -0

        void            (*count_cells)(void *blob, int parentoffset,
                                int *addrc, int *sizec);
        u64             (*map)(fdt32_t *addr, const fdt32_t *range,

@@ -1013,8 +1014,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",

@@ -1025,6 +1088,22 @@ static struct of_bus of_busses[] = {
        },
 };
 

common/fdt_support.c static struct of_bus of_busses[] = { · line 1103

Quoted 13 lines

common/fdt_support.c +13 -0

+static struct of_bus *of_match_bus(void *blob, int parentoffset)
+{
+       struct of_bus *bus;
+
+       if (ARRAY_SIZE(of_busses) == 1)
+               return &of_busses[0];
+
+       for (bus = &of_busses[0]; bus; bus++) {
+               if (!bus->match || bus->match(blob, parentoffset))
+                       return bus;
+       }
+
+       BUG();

What will this do? Can we propagate the error up instead?

Quoted 32 lines

common/fdt_support.c +5 -2

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

@@ -1104,7 +1183,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);

@@ -1133,7 +1212,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)) {
                        printf("%s: Bad cell count for %s\n", __FUNCTION__,

drivers/core/Kconfig +0 -0

index c5c9d2a..339f52f 100644
@@ -178,4 +178,8 @@ config SPL_OF_TRANSLATE
          used for the address translation. This function is faster and
          smaller in size than fdt_translate_address().
 

drivers/core/Kconfig config SPL_OF_TRANSLATE · line 183

Quoted 3 lines

drivers/core/Kconfig +3 -0

+config OF_ISA_BUS
+       bool "Support the ISA bus in fdt_translate_address"
+       depends on OF_TRANSLATE

Please add help here, perhaps with a small example.

Quoted 5 lines

drivers/core/Kconfig +1 -1

+
 endmenu
--

2.7.0

Regards,
Simon

Quoted 15 lines · Simon Glass

Hi Paul,

Quoted 12 lines · 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 <[email protected]>

---

 common/fdt_support.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/core/Kconfig |  4 +++
 2 files changed, 85 insertions(+), 2 deletions(-)

Quoted 12 lines

common/fdt_support.c static void of_dump_addr(const char *s, const fdt32_t *addr, int na) { } · line 962

Quoted 9 lines

common/fdt_support.c +1 -0

index 75d0858..0aba77d 100644
@@ -959,6 +959,7 @@ static void of_dump_addr(const char *s, const fdt32_t *addr, int na) { }
 struct of_bus {
        const char      *name;
        const char      *addresses;
+       int             (*match)(void *blob, int parentoffset);

Can you please add a comment to this method explaining what it does, args, return value?

Hi Simon,

Ok, will do (I'll try not to use "but the rest don't!" as an excuse).

Quoted 79 lines

Quoted 79 lines

common/fdt_support.c +62 -0

        void            (*count_cells)(void *blob, int parentoffset,
                                int *addrc, int *sizec);
        u64             (*map)(fdt32_t *addr, const fdt32_t *range,

@@ -1013,8 +1014,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",

@@ -1025,6 +1088,22 @@ static struct of_bus of_busses[] = {
        },
 };
 

Quoted 6 lines

common/fdt_support.c static struct of_bus of_busses[] = { · line 1096

Quoted 6 lines

common/fdt_support.c +6 -0

+static struct of_bus *of_match_bus(void *blob, int parentoffset)
+{
+       struct of_bus *bus;
+
+       if (ARRAY_SIZE(of_busses) == 1)
+               return &of_busses[0];

Quoted 2 lines

common/fdt_support.c static struct of_bus of_busses[] = { · line 1098

Quoted 2 lines

common/fdt_support.c +2 -0

+
+       for (bus = &of_busses[0]; bus; bus++) {

Quoted 6 lines

Quoted 5 lines

common/fdt_support.c +5 -0

+               if (!bus->match || bus->match(blob, parentoffset))
+                       return bus;
+       }
+
+       BUG();

Quoted 1 line

What will this do? Can we propagate the error up instead?

It would mean we'd somehow not got the default bus struct (which has a NULL bus->match) in the of_busses array. That should never happen, hence if it does something has gone horribly wrong & there's nothing sane the caller can do about it.

Quoted 33 lines

Quoted 33 lines

common/fdt_support.c +5 -2

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

@@ -1104,7 +1183,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);

@@ -1133,7 +1212,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)) {
                        printf("%s: Bad cell count for %s\n", __FUNCTION__,

drivers/core/Kconfig +0 -0

index c5c9d2a..339f52f 100644
@@ -178,4 +178,8 @@ config SPL_OF_TRANSLATE
          used for the address translation. This function is faster and
          smaller in size than fdt_translate_address().
 

Quoted 5 lines

drivers/core/Kconfig config SPL_OF_TRANSLATE · line 183

Quoted 3 lines

drivers/core/Kconfig +3 -0

+config OF_ISA_BUS
+       bool "Support the ISA bus in fdt_translate_address"
+       depends on OF_TRANSLATE

Please add help here, perhaps with a small example.

Ok.

Thanks,
Paul

Quoted 4 lines

Quoted 4 lines

drivers/core/Kconfig +1 -1

+
 endmenu
--

2.7.0

Support providing flags indicating the type of resource that a translated address corresponds to. This will allow for callers to look out for IORESOURCE_IO to use I/O accessors instead of always assuming simple memory-mapped addresses.

Signed-off-by Paul Burton <[email protected]>

---

 common/fdt_support.c  | 32 ++++++++++++++++++++++++++++++--
 drivers/core/device.c | 23 ++++++++++++++++++++---
 include/dm/device.h   | 23 +++++++++++++++++++++++
 include/fdt_support.h |  2 ++
 4 files changed, 75 insertions(+), 5 deletions(-)

common/fdt_support.c +1 -0

index 0aba77d..d0c9d56 100644
@@ -11,6 +11,7 @@
 #include <inttypes.h>
 #include <stdio_dev.h>
 #include <linux/ctype.h>
+#include <linux/ioport.h>
 #include <linux/types.h>
 #include <asm/global_data.h>
 #include <libfdt.h>

common/fdt_support.c +28 -2

 };
 
 /* Default translator (generic bus) */

@@ -1063,6 +1065,19 @@ static int of_bus_isa_translate(fdt32_t *addr, u64 offset, int na)
        return of_bus_default_translate(addr + 1, offset, na - 1);
 }
 
+static unsigned int of_bus_isa_get_flags(const fdt32_t *addr)
+{
+       unsigned int flags = 0;
+       u32 w = be32_to_cpup(addr);
+
+       if (w & 1)
+               flags |= IORESOURCE_IO;
+       else
+               flags |= IORESOURCE_MEM;
+
+       return flags;
+}
+
 #endif /* CONFIG_OF_ISA_BUS */
 
 /* Array of bus specific translators */

@@ -1076,6 +1091,7 @@ static struct of_bus of_busses[] = {
                .count_cells = of_bus_isa_count_cells,
                .map = of_bus_isa_map,
                .translate = of_bus_isa_translate,
+               .get_flags = of_bus_isa_get_flags,
        },
 #endif /* CONFIG_OF_ISA_BUS */
        /* Default */

@@ -1168,7 +1184,7 @@ static int of_translate_one(void * blob, int parent, struct of_bus *bus,
  * that way, but this is traditionally the way IBM at least do things
  */
 static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in_addr,
-                                 const char *rprop)
+                                 const char *rprop, unsigned int *flags)
 {
        int parent;
        struct of_bus *bus, *pbus;

@@ -1198,6 +1214,11 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
            bus->name, na, ns, fdt_get_name(blob, parent, NULL));
        of_dump_addr("OF: translating address:", addr, na);
 
+       if (flags && bus->get_flags)
+               *flags = bus->get_flags(in_addr);
+       else if (flags)
+               *flags = IORESOURCE_MEM;
+
        /* Translate */
        for (;;) {
                /* Switch to parent bus */

@@ -1240,9 +1261,16 @@ static u64 __of_translate_address(void *blob, int node_offset, const fdt32_t *in
        return result;
 }
 
+u64 fdt_translate_address_flags(void *blob, int node_offset,
+                               const fdt32_t *in_addr, unsigned int *flags)
+{
+       return __of_translate_address(blob, node_offset, in_addr, "ranges",
+                                     flags);
+}
+
 u64 fdt_translate_address(void *blob, int node_offset, const fdt32_t *in_addr)
 {
-       return __of_translate_address(blob, node_offset, in_addr, "ranges");
+       return fdt_translate_address_flags(blob, node_offset, in_addr, NULL);
 }
 
 /**

drivers/core/device.c +20 -3

index f5def35..0492dd7 100644
@@ -590,7 +590,8 @@ const char *dev_get_uclass_name(struct udevice *dev)
        return dev->uclass->uc_drv->name;
 }
 
-fdt_addr_t dev_get_addr_index(struct udevice *dev, int index)
+fdt_addr_t dev_get_addr_index_flags(struct udevice *dev, int index,
+                                   unsigned int *flags)
 {
 #if CONFIG_IS_ENABLED(OF_CONTROL)
        fdt_addr_t addr;

@@ -624,8 +625,8 @@ fdt_addr_t dev_get_addr_index(struct udevice *dev, int index)
                 * Use the full-fledged translate function for complex
                 * bus setups.
                 */
-               addr = fdt_translate_address((void *)gd->fdt_blob,
-                                            dev->of_offset, reg);
+               addr = fdt_translate_address_flags((void *)gd->fdt_blob,
+                                                  dev->of_offset, reg, flags);
        } else {
                /*
                 * Use the "simple" translate function for less complex

@@ -640,6 +641,9 @@ fdt_addr_t dev_get_addr_index(struct udevice *dev, int index)
                            UCLASS_SIMPLE_BUS)
                                addr = simple_bus_translate(dev->parent, addr);
                }
+
+               if (flags)
+                       *flags = 0;
        }
 
        /*

@@ -652,10 +656,23 @@ fdt_addr_t dev_get_addr_index(struct udevice *dev, int index)
 
        return addr;
 #else
+       if (flags)
+               *flags = 0;
+
        return FDT_ADDR_T_NONE;
 #endif
 }
 
+fdt_addr_t dev_get_addr_flags(struct udevice *dev, unsigned int *flags)
+{
+       return dev_get_addr_index_flags(dev, 0, flags);
+}
+
+fdt_addr_t dev_get_addr_index(struct udevice *dev, int index)
+{
+       return dev_get_addr_index_flags(dev, index, NULL);
+}
+
 fdt_addr_t dev_get_addr(struct udevice *dev)
 {
        return dev_get_addr_index(dev, 0);

include/dm/device.h +0 -0

index 1cf8150..b5dd62c 100644

include/dm/device.h +19 -0

+ *
+ * @return addr
+ */
+fdt_addr_t dev_get_addr_flags(struct udevice *dev, unsigned int *flags);
+
+/**
  * dev_get_addr_index() - Get the indexed reg property of a device
  *
  * @dev: Pointer to a device

@@ -465,6 +475,19 @@ fdt_addr_t dev_get_addr(struct udevice *dev);
 fdt_addr_t dev_get_addr_index(struct udevice *dev, int index);
 
 /**
+ * dev_get_addr_index_flags() - Get the indexed reg property of a device
+ *
+ * @dev: Pointer to a device
+ * @index: the 'reg' property can hold a list of <addr, size> pairs
+ *        and @index is used to select which one is required
+ * @flags: Pointer to flags, if non-NULL will contain IORESOURCE_*
+ *
+ * @return addr
+ */
+fdt_addr_t dev_get_addr_index_flags(struct udevice *dev, int index,
+                                   unsigned int *flags);
+
+/**
  * device_has_children() - check if a device has any children
  *
  * @dev:       Device to check

include/fdt_support.h +0 -0

index 296add0..b716f3e 100644

include/fdt_support.h +0 -1

 int fdt_node_offset_by_compat_reg(void *blob, const char *compat,
                                        phys_addr_t compat_off);
 int fdt_alloc_phandle(void *blob);
--

2.7.0

Support making use of I/O accessors for ports when a device tree specified an I/O resource. This allows for systems where we have a mix of ns16550 ports, some of which should be accessed via I/O ports & some of which should be accessed via readb/writeb.

Signed-off-by Paul Burton <[email protected]>

---

 drivers/serial/ns16550.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 include/ns16550.h        | 31 ++++++++++++++++++-------------
 2 files changed, 56 insertions(+), 21 deletions(-)

drivers/serial/ns16550.c +18 -1

index 93dad33..b1d5319 100644
@@ -11,6 +11,7 @@
 #include <ns16550.h>
 #include <serial.h>
 #include <watchdog.h>
+#include <linux/ioport.h>
 #include <linux/types.h>
 #include <asm/io.h>
 

@@ -102,7 +103,7 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
        offset *= 1 << plat->reg_shift;
        addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
        /*
-        * As far as we know it doesn't make sense to support selection of
+        * For many platforms it doesn't make sense to support selection of
         * these options at run-time, so use the existing CONFIG options.
         */
        serial_out_shift(addr, plat->reg_shift, value);

@@ -119,13 +120,33 @@ static int ns16550_readb(NS16550_t port, int offset)
        return serial_in_shift(addr, plat->reg_shift);
 }
 
+static void ns16550_outb(NS16550_t port, int offset, int value)
+{
+       struct ns16550_platdata *plat = port->plat;
+
+       offset *= 1 << plat->reg_shift;
+       outb(value, plat->base + offset);
+}
+
+static int ns16550_inb(NS16550_t port, int offset)
+{
+       struct ns16550_platdata *plat = port->plat;
+
+       offset *= 1 << plat->reg_shift;
+       return inb(plat->base + offset);
+}
+

drivers/serial/ns16550.c +17 -1

+} while (0)
+
+#define serial_in(addr) ({                                             \
+       int offset = (unsigned char *)addr - (unsigned char *)com_port; \
+       int value = com_port->plat->in(com_port, offset);               \
+       value;                                                          \
+})
 #endif
 
 static inline int calc_divisor(NS16550_t port, int clock, int baudrate)

@@ -365,9 +386,10 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 {
        struct ns16550_platdata *plat = dev->platdata;
        fdt_addr_t addr;
+       unsigned int flags;
 
        /* try Processor Local Bus device first */
-       addr = dev_get_addr(dev);
+       addr = dev_get_addr_flags(dev, &flags);
 #if defined(CONFIG_PCI) && defined(CONFIG_DM_PCI)
        if (addr == FDT_ADDR_T_NONE) {
                /* then try pci device */

@@ -400,6 +422,14 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
        if (addr == FDT_ADDR_T_NONE)
                return -EINVAL;
 
+       if (flags & IORESOURCE_IO) {
+               plat->in = ns16550_inb;
+               plat->out = ns16550_outb;
+       } else {
+               plat->in = ns16550_readb;
+               plat->out = ns16550_writeb;
+       }
+
        plat->base = addr;
        plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
                                         "reg-shift", 0);

include/ns16550.h +7 -13

index 4e62067..097f64b 100644
@@ -45,19 +45,6 @@
        unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
 #endif
 
-/**
- * struct ns16550_platdata - information about a NS16550 port
- *
- * @base:              Base register address
- * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
- * @clock:             UART base clock speed in Hz
- */
-struct ns16550_platdata {
-       unsigned long base;
-       int reg_shift;
-       int clock;
-};
-
 struct udevice;
 
 struct NS16550 {

@@ -100,6 +87,24 @@ struct NS16550 {
 
 typedef struct NS16550 *NS16550_t;
 
+/**
+ * struct ns16550_platdata - information about a NS16550 port
+ *
+ * @base:              Base register address
+ * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
+ * @clock:             UART base clock speed in Hz
+ * @is_io:             Use I/O, not memory, accessors

include/ns16550.h +1 -1

+
 /*
  * These are the definitions for the FIFO Control Register
  */
--

2.7.0

Hi Paul,

Quoted 53 lines · Paul Burton

Support making use of I/O accessors for ports when a device tree specified an I/O resource. This allows for systems where we have a mix of ns16550 ports, some of which should be accessed via I/O ports & some of which should be accessed via readb/writeb.

Signed-off-by Paul Burton <[email protected]>

---

 drivers/serial/ns16550.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 include/ns16550.h        | 31 ++++++++++++++++++-------------
 2 files changed, 56 insertions(+), 21 deletions(-)

drivers/serial/ns16550.c +18 -1

index 93dad33..b1d5319 100644
@@ -11,6 +11,7 @@
 #include <ns16550.h>
 #include <serial.h>
 #include <watchdog.h>
+#include <linux/ioport.h>
 #include <linux/types.h>
 #include <asm/io.h>
 

@@ -102,7 +103,7 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
        offset *= 1 << plat->reg_shift;
        addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
        /*
-        * As far as we know it doesn't make sense to support selection of
+        * For many platforms it doesn't make sense to support selection of
         * these options at run-time, so use the existing CONFIG options.
         */
        serial_out_shift(addr, plat->reg_shift, value);

@@ -119,13 +120,33 @@ static int ns16550_readb(NS16550_t port, int offset)
        return serial_in_shift(addr, plat->reg_shift);
 }
 
+static void ns16550_outb(NS16550_t port, int offset, int value)
+{
+       struct ns16550_platdata *plat = port->plat;
+
+       offset *= 1 << plat->reg_shift;
+       outb(value, plat->base + offset);
+}
+
+static int ns16550_inb(NS16550_t port, int offset)
+{
+       struct ns16550_platdata *plat = port->plat;
+
+       offset *= 1 << plat->reg_shift;
+       return inb(plat->base + offset);
+}
+

drivers/serial/ns16550.c static int ns16550_readb(NS16550_t port, int offset) · line 142

Quoted 10 lines

drivers/serial/ns16550.c +3 -6

 /* We can clean these up once everything is moved to driver model */
-#define serial_out(value, addr)        \
-       ns16550_writeb(com_port, \
-               (unsigned char *)addr - (unsigned char *)com_port, value)
-#define serial_in(addr) \
-       ns16550_readb(com_port, \
-               (unsigned char *)addr - (unsigned char *)com_port)
+#define serial_out(value, addr)        do {                                    \
+       int offset = (unsigned char *)addr - (unsigned char *)com_port; \
+       com_port->plat->out(com_port, offset, value);                   \

I really don't want to introduce function pointers here. This driver is a mess but my hope was that when we remove the non-driver-model code we can clean it up.

I suggest storing the access method in com_port->plat and then using if() or switch() to select the right one. We can always add #ifdefs to keep the code size down if it becomes a problem.

Quoted 77 lines

drivers/serial/ns16550.c +17 -1

+} while (0)
+
+#define serial_in(addr) ({                                             \
+       int offset = (unsigned char *)addr - (unsigned char *)com_port; \
+       int value = com_port->plat->in(com_port, offset);               \
+       value;                                                          \
+})
 #endif
 
 static inline int calc_divisor(NS16550_t port, int clock, int baudrate)

@@ -365,9 +386,10 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 {
        struct ns16550_platdata *plat = dev->platdata;
        fdt_addr_t addr;
+       unsigned int flags;
 
        /* try Processor Local Bus device first */
-       addr = dev_get_addr(dev);
+       addr = dev_get_addr_flags(dev, &flags);
 #if defined(CONFIG_PCI) && defined(CONFIG_DM_PCI)
        if (addr == FDT_ADDR_T_NONE) {
                /* then try pci device */

@@ -400,6 +422,14 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
        if (addr == FDT_ADDR_T_NONE)
                return -EINVAL;
 
+       if (flags & IORESOURCE_IO) {
+               plat->in = ns16550_inb;
+               plat->out = ns16550_outb;
+       } else {
+               plat->in = ns16550_readb;
+               plat->out = ns16550_writeb;
+       }
+
        plat->base = addr;
        plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
                                         "reg-shift", 0);

include/ns16550.h +12 -13

index 4e62067..097f64b 100644
@@ -45,19 +45,6 @@
        unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
 #endif
 
-/**
- * struct ns16550_platdata - information about a NS16550 port
- *
- * @base:              Base register address
- * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
- * @clock:             UART base clock speed in Hz
- */
-struct ns16550_platdata {
-       unsigned long base;
-       int reg_shift;
-       int clock;
-};
-
 struct udevice;
 
 struct NS16550 {

@@ -100,6 +87,24 @@ struct NS16550 {
 
 typedef struct NS16550 *NS16550_t;
 
+/**
+ * struct ns16550_platdata - information about a NS16550 port
+ *
+ * @base:              Base register address
+ * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
+ * @clock:             UART base clock speed in Hz
+ * @is_io:             Use I/O, not memory, accessors
+ */
+struct ns16550_platdata {
+       unsigned long base;
+       int reg_shift;
+       int clock;

include/ns16550.h struct NS16550 { · line 106

Quoted 5 lines

include/ns16550.h +5 -0

+#ifdef CONFIG_DM_SERIAL
+       int (*in)(NS16550_t port, int offset);
+       void (*out)(NS16550_t port, int offset, int value);
+#endif
+};

Does the above need to move? It would reduce the diff if you left it in the same place.

Quoted 7 lines

include/ns16550.h +1 -1

+
 /*
  * These are the definitions for the FIFO Control Register
  */
--

2.7.0

Regards,
Simon

Quoted 71 lines · Simon Glass

Hi Paul,

Quoted 53 lines · Paul Burton

Support making use of I/O accessors for ports when a device tree specified an I/O resource. This allows for systems where we have a mix of ns16550 ports, some of which should be accessed via I/O ports & some of which should be accessed via readb/writeb.

Signed-off-by Paul Burton <[email protected]>

---

 drivers/serial/ns16550.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 include/ns16550.h        | 31 ++++++++++++++++++-------------
 2 files changed, 56 insertions(+), 21 deletions(-)

drivers/serial/ns16550.c +18 -1

index 93dad33..b1d5319 100644
@@ -11,6 +11,7 @@
 #include <ns16550.h>
 #include <serial.h>
 #include <watchdog.h>
+#include <linux/ioport.h>
 #include <linux/types.h>
 #include <asm/io.h>
 

@@ -102,7 +103,7 @@ static void ns16550_writeb(NS16550_t port, int offset, int value)
        offset *= 1 << plat->reg_shift;
        addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset;
        /*
-        * As far as we know it doesn't make sense to support selection of
+        * For many platforms it doesn't make sense to support selection of
         * these options at run-time, so use the existing CONFIG options.
         */
        serial_out_shift(addr, plat->reg_shift, value);

@@ -119,13 +120,33 @@ static int ns16550_readb(NS16550_t port, int offset)
        return serial_in_shift(addr, plat->reg_shift);
 }
 
+static void ns16550_outb(NS16550_t port, int offset, int value)
+{
+       struct ns16550_platdata *plat = port->plat;
+
+       offset *= 1 << plat->reg_shift;
+       outb(value, plat->base + offset);
+}
+
+static int ns16550_inb(NS16550_t port, int offset)
+{
+       struct ns16550_platdata *plat = port->plat;
+
+       offset *= 1 << plat->reg_shift;
+       return inb(plat->base + offset);
+}
+

drivers/serial/ns16550.c static int ns16550_readb(NS16550_t port, int offset) · line 142

Quoted 10 lines

drivers/serial/ns16550.c +3 -6

 /* We can clean these up once everything is moved to driver model */
-#define serial_out(value, addr)        \
-       ns16550_writeb(com_port, \
-               (unsigned char *)addr - (unsigned char *)com_port, value)
-#define serial_in(addr) \
-       ns16550_readb(com_port, \
-               (unsigned char *)addr - (unsigned char *)com_port)
+#define serial_out(value, addr)        do {                                    \
+       int offset = (unsigned char *)addr - (unsigned char *)com_port; \
+       com_port->plat->out(com_port, offset, value);                   \

I really don't want to introduce function pointers here. This driver is a mess but my hope was that when we remove the non-driver-model code we can clean it up.

Quoted 3 lines

I suggest storing the access method in com_port->plat and then using if() or switch() to select the right one. We can always add #ifdefs to keep the code size down if it becomes a problem.

Hi Simon,

I originally had a field in the platform data & an if statement, but switched to this because it seemed neater & avoids checks on every read or write.

I can put it back if you insist, but whilst I agree that the driver needs some cleanup I'm not sure this would qualify.

Quoted 78 lines

Quoted 78 lines

drivers/serial/ns16550.c +17 -1

+} while (0)
+
+#define serial_in(addr) ({                                             \
+       int offset = (unsigned char *)addr - (unsigned char *)com_port; \
+       int value = com_port->plat->in(com_port, offset);               \
+       value;                                                          \
+})
 #endif
 
 static inline int calc_divisor(NS16550_t port, int clock, int baudrate)

@@ -365,9 +386,10 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 {
        struct ns16550_platdata *plat = dev->platdata;
        fdt_addr_t addr;
+       unsigned int flags;
 
        /* try Processor Local Bus device first */
-       addr = dev_get_addr(dev);
+       addr = dev_get_addr_flags(dev, &flags);
 #if defined(CONFIG_PCI) && defined(CONFIG_DM_PCI)
        if (addr == FDT_ADDR_T_NONE) {
                /* then try pci device */

@@ -400,6 +422,14 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
        if (addr == FDT_ADDR_T_NONE)
                return -EINVAL;
 
+       if (flags & IORESOURCE_IO) {
+               plat->in = ns16550_inb;
+               plat->out = ns16550_outb;
+       } else {
+               plat->in = ns16550_readb;
+               plat->out = ns16550_writeb;
+       }
+
        plat->base = addr;
        plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
                                         "reg-shift", 0);

include/ns16550.h +12 -13

index 4e62067..097f64b 100644
@@ -45,19 +45,6 @@
        unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
 #endif
 
-/**
- * struct ns16550_platdata - information about a NS16550 port
- *
- * @base:              Base register address
- * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
- * @clock:             UART base clock speed in Hz
- */
-struct ns16550_platdata {
-       unsigned long base;
-       int reg_shift;
-       int clock;
-};
-
 struct udevice;
 
 struct NS16550 {

@@ -100,6 +87,24 @@ struct NS16550 {
 
 typedef struct NS16550 *NS16550_t;
 
+/**
+ * struct ns16550_platdata - information about a NS16550 port
+ *
+ * @base:              Base register address
+ * @reg_shift:         Shift size of registers (0=byte, 1=16bit, 2=32bit...)
+ * @clock:             UART base clock speed in Hz
+ * @is_io:             Use I/O, not memory, accessors
+ */
+struct ns16550_platdata {
+       unsigned long base;
+       int reg_shift;
+       int clock;

Quoted 8 lines

include/ns16550.h struct NS16550 { · line 106

Quoted 5 lines

include/ns16550.h +5 -0

+#ifdef CONFIG_DM_SERIAL
+       int (*in)(NS16550_t port, int offset);
+       void (*out)(NS16550_t port, int offset, int value);
+#endif
+};

Does the above need to move? It would reduce the diff if you left it in the same place.

It needs the declaration of NS16550_t. A forward declaration would do I suppose, but it seemed neater to not need it.

Thanks,
Paul

Quoted 10 lines

Quoted 7 lines

include/ns16550.h +1 -1

+
 /*
  * These are the definitions for the FIFO Control Register
  */
--

2.7.0

Regards,
Simon

CONF_SLOWDOWN_IO is never set for any target, so remove the dead code in the SLOW_DOWN_IO macro. This is done in preparation for changes to mips_io_port_base which can be avoided in this path by removing it entirely.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/include/asm/io.h | 40 +++-------------------------------------
 1 file changed, 3 insertions(+), 37 deletions(-)

The existing mips_io_port_base variable isn't suitable for use early during boot since it will be stored in the .data section which may not be writable pre-relocation. Fix this by moving the I/O port base address into struct arch_global_data. In order to avoid adding this field for all targets, make this dependant upon a new Kconfig entry CONFIG_DYNAMIC_IO_PORT_BASE. Malta is the only board which sets a non-zero I/O port base, so select this option only for Malta.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/Kconfig                   |  4 ++++
 arch/mips/include/asm/global_data.h |  3 +++
 arch/mips/include/asm/io.h          | 48 +++++++++++++++++++++----------------
 arch/mips/lib/Makefile              |  1 -
 arch/mips/lib/io.c                  | 12 ----------
 5 files changed, 34 insertions(+), 34 deletions(-)
 delete mode 100644 arch/mips/lib/io.c

Set the I/O port base earlier, from board_early_init_f, in preparation for it being used by the serial driver.

Signed-off-by Paul Burton <[email protected]>

---

 board/imgtec/malta/malta.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Rather than passing the I/O port base address to the Super I/O code, switch it to using outb such that it makes use of the I/O port base address automatically.

Drop the extern keyword to satisfy checkpatch whilst here.

Signed-off-by Paul Burton <[email protected]>

---

 board/imgtec/malta/malta.c   | 10 +++++-----
 board/imgtec/malta/superio.c | 10 +++++-----
 board/imgtec/malta/superio.h |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

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

Paul Burton (9):
ioport.h: Remove struct resource & co
fdt: Support for ISA busses
fdt: Support providing IORESOURCE_* flags from translation
ns16550: Support I/O accessors selected by DT
MIPS: Remove SLOW_DOWN_IO
MIPS: Support dynamic I/O port base address
malta: Set I/O port base early
malta: Use I/O accessors for SuperI/O controller
malta: Use device model & tree for UART

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

Paul Burton (9):
ioport.h: Remove struct resource & co
fdt: Support for ISA busses
fdt: Support providing IORESOURCE_* flags from translation
ns16550: Support I/O accessors selected by DT

MIPS: Remove SLOW_DOWN_IO
MIPS: Support dynamic I/O port base address
malta: Set I/O port base early
malta: Use I/O accessors for SuperI/O controller

malta: Use device model & tree for UART