* [U-Boot] [PATCH 1/9] ioport.h: Remove struct resource & co
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-01-29 14:06 ` Marek Vasut
2016-01-29 13:54 ` [U-Boot] [PATCH 2/9] fdt: Support for ISA busses Paul Burton
` (9 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 UTC (permalink / raw)
To: u-boot
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 <paul.burton@imgtec.com>
---
drivers/usb/dwc3/core.h | 1 -
include/linux/ioport.h | 104 ------------------------------------------------
2 files changed, 105 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 72d2fcd..a0fbc8c 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -716,7 +716,6 @@ struct dwc3 {
struct device *dev;
struct platform_device *xhci;
- struct resource xhci_resources[DWC3_XHCI_RESOURCES_NUM];
struct dwc3_event_buffer **ev_buffs;
struct dwc3_ep *eps[DWC3_ENDPOINTS_NUM];
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 7129504..74f562d 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -8,27 +8,6 @@
#ifndef _LINUX_IOPORT_H
#define _LINUX_IOPORT_H
-#ifndef __ASSEMBLY__
-#include <linux/compiler.h>
-#include <linux/types.h>
-/*
- * Resources are tree-like, allowing
- * nesting etc..
- */
-struct resource {
- resource_size_t start;
- resource_size_t end;
- const char *name;
- unsigned long flags;
- struct resource *parent, *sibling, *child;
-};
-
-struct resource_list {
- struct resource_list *next;
- struct resource *res;
- struct pci_dev *dev;
-};
-
/*
* IO resources have these defined flags.
*/
@@ -106,87 +85,4 @@ struct resource_list {
/* PCI control bits. Shares IORESOURCE_BITS with above PCI ROM. */
#define IORESOURCE_PCI_FIXED (1<<4) /* Do not move resource */
-/* PC/ISA/whatever - the normal PC address spaces: IO and memory */
-extern struct resource ioport_resource;
-extern struct resource iomem_resource;
-
-extern int request_resource(struct resource *root, struct resource *new);
-extern int release_resource(struct resource *new);
-extern void reserve_region_with_split(struct resource *root,
- resource_size_t start, resource_size_t end,
- const char *name);
-extern int insert_resource(struct resource *parent, struct resource *new);
-extern void insert_resource_expand_to_fit(struct resource *root, struct resource *new);
-extern int allocate_resource(struct resource *root, struct resource *new,
- resource_size_t size, resource_size_t min,
- resource_size_t max, resource_size_t align,
- void (*alignf)(void *, struct resource *,
- resource_size_t, resource_size_t),
- void *alignf_data);
-int adjust_resource(struct resource *res, resource_size_t start,
- resource_size_t size);
-resource_size_t resource_alignment(struct resource *res);
-static inline resource_size_t resource_size(const struct resource *res)
-{
- return res->end - res->start + 1;
-}
-static inline unsigned long resource_type(const struct resource *res)
-{
- return res->flags & IORESOURCE_TYPE_BITS;
-}
-
-/* Convenience shorthand with allocation */
-#define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0)
-#define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
-#define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
-#define request_mem_region_exclusive(start,n,name) \
- __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE)
-#define rename_region(region, newname) do { (region)->name = (newname); } while (0)
-
-extern struct resource * __request_region(struct resource *,
- resource_size_t start,
- resource_size_t n,
- const char *name, int flags);
-
-/* Compatibility cruft */
-#define release_region(start,n) __release_region(&ioport_resource, (start), (n))
-#define check_mem_region(start,n) __check_region(&iomem_resource, (start), (n))
-#define release_mem_region(start,n) __release_region(&iomem_resource, (start), (n))
-
-extern int __check_region(struct resource *, resource_size_t, resource_size_t);
-extern void __release_region(struct resource *, resource_size_t,
- resource_size_t);
-
-static inline int __deprecated check_region(resource_size_t s,
- resource_size_t n)
-{
- return __check_region(&ioport_resource, s, n);
-}
-
-/* Wrappers for managed devices */
-struct device;
-#define devm_request_region(dev,start,n,name) \
- __devm_request_region(dev, &ioport_resource, (start), (n), (name))
-#define devm_request_mem_region(dev,start,n,name) \
- __devm_request_region(dev, &iomem_resource, (start), (n), (name))
-
-extern struct resource * __devm_request_region(struct device *dev,
- struct resource *parent, resource_size_t start,
- resource_size_t n, const char *name);
-
-#define devm_release_region(dev, start, n) \
- __devm_release_region(dev, &ioport_resource, (start), (n))
-#define devm_release_mem_region(dev, start, n) \
- __devm_release_region(dev, &iomem_resource, (start), (n))
-
-extern void __devm_release_region(struct device *dev, struct resource *parent,
- resource_size_t start, resource_size_t n);
-extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
-extern int iomem_is_exclusive(u64 addr);
-
-extern int
-walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
- void *arg, int (*func)(unsigned long, unsigned long, void *));
-
-#endif /* __ASSEMBLY__ */
#endif /* _LINUX_IOPORT_H */
--
2.7.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 1/9] ioport.h: Remove struct resource & co
2016-01-29 13:54 ` [U-Boot] [PATCH 1/9] ioport.h: Remove struct resource & co Paul Burton
@ 2016-01-29 14:06 ` Marek Vasut
2016-01-29 15:58 ` Paul Burton
0 siblings, 1 reply; 26+ messages in thread
From: Marek Vasut @ 2016-01-29 14:06 UTC (permalink / raw)
To: u-boot
On Friday, January 29, 2016 at 02:54:47 PM, Paul Burton wrote:
> 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 <paul.burton@imgtec.com>
> ---
>
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [U-Boot] [PATCH 1/9] ioport.h: Remove struct resource & co
2016-01-29 14:06 ` Marek Vasut
@ 2016-01-29 15:58 ` Paul Burton
2016-01-29 20:50 ` Marek Vasut
0 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 15:58 UTC (permalink / raw)
To: u-boot
On Fri, Jan 29, 2016 at 03:06:33PM +0100, Marek Vasut wrote:
> On Friday, January 29, 2016 at 02:54:47 PM, Paul Burton wrote:
> > 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 <paul.burton@imgtec.com>
> > ---
> >
> > 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
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.
Thanks,
Paul
^ permalink raw reply [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 1/9] ioport.h: Remove struct resource & co
2016-01-29 15:58 ` Paul Burton
@ 2016-01-29 20:50 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2016-01-29 20:50 UTC (permalink / raw)
To: u-boot
On Friday, January 29, 2016 at 04:58:40 PM, Paul Burton wrote:
> On Fri, Jan 29, 2016 at 03:06:33PM +0100, Marek Vasut wrote:
> > On Friday, January 29, 2016 at 02:54:47 PM, Paul Burton wrote:
> > > 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 <paul.burton@imgtec.com>
> > > ---
> > >
> > > 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
>
> Hi Marek,
Hi,
> 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.
But the downside is, if we start collecting u-boot specific patches on
top of the driver, syncing with Linux will become painful.
I have to admit I am not decided which way to take with this driver. One
option might be to add simple #ifdef __linux__ around the code you want
to disable, this would be least intrusive way to keep the code around and
make it kinda easy to sync with Linux.
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 26+ messages in thread
* [U-Boot] [PATCH 2/9] fdt: Support for ISA busses
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
2016-01-29 13:54 ` [U-Boot] [PATCH 1/9] ioport.h: Remove struct resource & co Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-01-29 14:56 ` Simon Glass
2016-01-29 13:54 ` [U-Boot] [PATCH 3/9] fdt: Support providing IORESOURCE_* flags from translation Paul Burton
` (8 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 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>
---
common/fdt_support.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
drivers/core/Kconfig | 4 +++
2 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 75d0858..0aba77d 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -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);
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;
+
+ 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();
+ 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__,
diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index c5c9d2a..339f52f 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -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().
+config OF_ISA_BUS
+ bool "Support the ISA bus in fdt_translate_address"
+ depends on OF_TRANSLATE
+
endmenu
--
2.7.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 2/9] fdt: Support for ISA busses
2016-01-29 13:54 ` [U-Boot] [PATCH 2/9] fdt: Support for ISA busses Paul Burton
@ 2016-01-29 14:56 ` Simon Glass
2016-01-29 16:04 ` Paul Burton
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2016-01-29 14:56 UTC (permalink / raw)
To: u-boot
Hi Paul,
On 29 January 2016 at 06:54, 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>
> ---
>
> common/fdt_support.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> drivers/core/Kconfig | 4 +++
> 2 files changed, 85 insertions(+), 2 deletions(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 75d0858..0aba77d 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -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?
> 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;
> +
> + 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?
> + 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__,
> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> index c5c9d2a..339f52f 100644
> --- a/drivers/core/Kconfig
> +++ b/drivers/core/Kconfig
> @@ -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().
>
> +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.
> +
> endmenu
> --
> 2.7.0
>
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 2/9] fdt: Support for ISA busses
2016-01-29 14:56 ` Simon Glass
@ 2016-01-29 16:04 ` Paul Burton
2016-01-29 18:23 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 16:04 UTC (permalink / raw)
To: u-boot
On Fri, Jan 29, 2016 at 07:56:11AM -0700, Simon Glass wrote:
> Hi Paul,
>
> On 29 January 2016 at 06:54, 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>
> > ---
> >
> > common/fdt_support.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > drivers/core/Kconfig | 4 +++
> > 2 files changed, 85 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/fdt_support.c b/common/fdt_support.c
> > index 75d0858..0aba77d 100644
> > --- a/common/fdt_support.c
> > +++ b/common/fdt_support.c
> > @@ -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).
> > 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;
> > +
> > + 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?
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.
> > + 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__,
> > diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> > index c5c9d2a..339f52f 100644
> > --- a/drivers/core/Kconfig
> > +++ b/drivers/core/Kconfig
> > @@ -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().
> >
> > +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
> > +
> > endmenu
> > --
> > 2.7.0
> >
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 2/9] fdt: Support for ISA busses
2016-01-29 16:04 ` Paul Burton
@ 2016-01-29 18:23 ` Simon Glass
0 siblings, 0 replies; 26+ messages in thread
From: Simon Glass @ 2016-01-29 18:23 UTC (permalink / raw)
To: u-boot
Hi Paul,
On 29 January 2016 at 09:04, Paul Burton <paul.burton@imgtec.com> wrote:
> On Fri, Jan 29, 2016 at 07:56:11AM -0700, Simon Glass wrote:
>> Hi Paul,
>>
>> On 29 January 2016 at 06:54, 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>
>> > ---
>> >
>> > common/fdt_support.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>> > drivers/core/Kconfig | 4 +++
>> > 2 files changed, 85 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/common/fdt_support.c b/common/fdt_support.c
>> > index 75d0858..0aba77d 100644
>> > --- a/common/fdt_support.c
>> > +++ b/common/fdt_support.c
>> > @@ -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).
Indeed. Every patch should improve the code, not just add a feature :-)
>
>> > 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;
>> > +
>> > + if (ARRAY_SIZE(of_busses) == 1)
>> > + return &of_busses[0];
return of_busses
>> > +
>> > + for (bus = &of_busses[0]; bus; bus++) {
for (bus = of_busses; ...
>> > + if (!bus->match || bus->match(blob, parentoffset))
>> > + return bus;
>> > + }
>> > +
>> > + BUG();
>>
>> 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.
Got it. Please add a comment. Also BUG() adds a lot of code including
printf(). Can you use assert() instead?
>
>> > + 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__,
>> > diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
>> > index c5c9d2a..339f52f 100644
>> > --- a/drivers/core/Kconfig
>> > +++ b/drivers/core/Kconfig
>> > @@ -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().
>> >
>> > +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
>
>> > +
>> > endmenu
>> > --
>> > 2.7.0
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* [U-Boot] [PATCH 3/9] fdt: Support providing IORESOURCE_* flags from translation
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
2016-01-29 13:54 ` [U-Boot] [PATCH 1/9] ioport.h: Remove struct resource & co Paul Burton
2016-01-29 13:54 ` [U-Boot] [PATCH 2/9] fdt: Support for ISA busses Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-01-29 14:56 ` Simon Glass
2016-01-29 13:54 ` [U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT Paul Burton
` (7 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 UTC (permalink / raw)
To: u-boot
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 <paul.burton@imgtec.com>
---
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(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 0aba77d..d0c9d56 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -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>
@@ -965,6 +966,7 @@ struct of_bus {
u64 (*map)(fdt32_t *addr, const fdt32_t *range,
int na, int ns, int pna);
int (*translate)(fdt32_t *addr, u64 offset, int na);
+ unsigned int (*get_flags)(const fdt32_t *addr);
};
/* 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);
}
/**
diff --git a/drivers/core/device.c b/drivers/core/device.c
index f5def35..0492dd7 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -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);
diff --git a/include/dm/device.h b/include/dm/device.h
index 1cf8150..b5dd62c 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -454,6 +454,16 @@ int device_find_next_child(struct udevice **devp);
fdt_addr_t dev_get_addr(struct udevice *dev);
/**
+ * dev_get_addr_flags() - Get the reg property of a device
+ *
+ * @dev: Pointer to a device
+ * @flags: Pointer to flags, if non-NULL will contain IORESOURCE_*
+ *
+ * @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
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 296add0..b716f3e 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -175,6 +175,8 @@ int fdt_fixup_nor_flash_size(void *blob);
void fdt_fixup_mtdparts(void *fdt, void *node_info, int node_info_size);
void fdt_del_node_and_alias(void *blob, const char *alias);
u64 fdt_translate_address(void *blob, int node_offset, const __be32 *in_addr);
+u64 fdt_translate_address_flags(void *blob, int node_offset,
+ const fdt32_t *in_addr, unsigned int *flags);
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 3/9] fdt: Support providing IORESOURCE_* flags from translation
2016-01-29 13:54 ` [U-Boot] [PATCH 3/9] fdt: Support providing IORESOURCE_* flags from translation Paul Burton
@ 2016-01-29 14:56 ` Simon Glass
0 siblings, 0 replies; 26+ messages in thread
From: Simon Glass @ 2016-01-29 14:56 UTC (permalink / raw)
To: u-boot
Hi Paul,
On 29 January 2016 at 06:54, Paul Burton <paul.burton@imgtec.com> wrote:
> 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 <paul.burton@imgtec.com>
> ---
>
> 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(-)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 0aba77d..d0c9d56 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -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>
> @@ -965,6 +966,7 @@ struct of_bus {
> u64 (*map)(fdt32_t *addr, const fdt32_t *range,
> int na, int ns, int pna);
> int (*translate)(fdt32_t *addr, u64 offset, int na);
> + unsigned int (*get_flags)(const fdt32_t *addr);
Please add a function comment here.
> };
>
> /* 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);
> }
>
> /**
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index f5def35..0492dd7 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -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);
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 1cf8150..b5dd62c 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -454,6 +454,16 @@ int device_find_next_child(struct udevice **devp);
> fdt_addr_t dev_get_addr(struct udevice *dev);
>
> /**
> + * dev_get_addr_flags() - Get the reg property of a device
> + *
> + * @dev: Pointer to a device
> + * @flags: Pointer to flags, if non-NULL will contain IORESOURCE_*
Meaning what? Can you expand on this a bit in the comment? Where do
the flags come from?
> + *
> + * @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
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 296add0..b716f3e 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -175,6 +175,8 @@ int fdt_fixup_nor_flash_size(void *blob);
> void fdt_fixup_mtdparts(void *fdt, void *node_info, int node_info_size);
> void fdt_del_node_and_alias(void *blob, const char *alias);
> u64 fdt_translate_address(void *blob, int node_offset, const __be32 *in_addr);
> +u64 fdt_translate_address_flags(void *blob, int node_offset,
> + const fdt32_t *in_addr, unsigned int *flags);
Please add a full function comment here. Bonus points if you do the
one immediately above also.
> 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
>
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* [U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
` (2 preceding siblings ...)
2016-01-29 13:54 ` [U-Boot] [PATCH 3/9] fdt: Support providing IORESOURCE_* flags from translation Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-01-29 14:56 ` Simon Glass
2016-01-29 13:54 ` [U-Boot] [PATCH 5/9] MIPS: Remove SLOW_DOWN_IO Paul Burton
` (6 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 UTC (permalink / raw)
To: u-boot
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 <paul.burton@imgtec.com>
---
drivers/serial/ns16550.c | 46 ++++++++++++++++++++++++++++++++++++++--------
include/ns16550.h | 31 ++++++++++++++++++-------------
2 files changed, 56 insertions(+), 21 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 93dad33..b1d5319 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -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);
+}
+
/* 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); \
+} 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);
diff --git a/include/ns16550.h b/include/ns16550.h
index 4e62067..097f64b 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -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;
+#ifdef CONFIG_DM_SERIAL
+ int (*in)(NS16550_t port, int offset);
+ void (*out)(NS16550_t port, int offset, int value);
+#endif
+};
+
/*
* These are the definitions for the FIFO Control Register
*/
--
2.7.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT
2016-01-29 13:54 ` [U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT Paul Burton
@ 2016-01-29 14:56 ` Simon Glass
2016-01-29 16:09 ` Paul Burton
0 siblings, 1 reply; 26+ messages in thread
From: Simon Glass @ 2016-01-29 14:56 UTC (permalink / raw)
To: u-boot
Hi Paul,
On 29 January 2016 at 06:54, Paul Burton <paul.burton@imgtec.com> wrote:
> 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 <paul.burton@imgtec.com>
> ---
>
> drivers/serial/ns16550.c | 46 ++++++++++++++++++++++++++++++++++++++--------
> include/ns16550.h | 31 ++++++++++++++++++-------------
> 2 files changed, 56 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 93dad33..b1d5319 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -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);
> +}
> +
> /* 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.
> +} 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);
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 4e62067..097f64b 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -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;
> +#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.
> +
> /*
> * These are the definitions for the FIFO Control Register
> */
> --
> 2.7.0
>
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT
2016-01-29 14:56 ` Simon Glass
@ 2016-01-29 16:09 ` Paul Burton
2016-01-29 18:23 ` Simon Glass
0 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 16:09 UTC (permalink / raw)
To: u-boot
On Fri, Jan 29, 2016 at 07:56:19AM -0700, Simon Glass wrote:
> Hi Paul,
>
> On 29 January 2016 at 06:54, Paul Burton <paul.burton@imgtec.com> wrote:
> > 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 <paul.burton@imgtec.com>
> > ---
> >
> > drivers/serial/ns16550.c | 46 ++++++++++++++++++++++++++++++++++++++--------
> > include/ns16550.h | 31 ++++++++++++++++++-------------
> > 2 files changed, 56 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> > index 93dad33..b1d5319 100644
> > --- a/drivers/serial/ns16550.c
> > +++ b/drivers/serial/ns16550.c
> > @@ -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);
> > +}
> > +
> > /* 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.
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.
> > +} 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);
> > diff --git a/include/ns16550.h b/include/ns16550.h
> > index 4e62067..097f64b 100644
> > --- a/include/ns16550.h
> > +++ b/include/ns16550.h
> > @@ -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;
> > +#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
> > +
> > /*
> > * These are the definitions for the FIFO Control Register
> > */
> > --
> > 2.7.0
> >
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT
2016-01-29 16:09 ` Paul Burton
@ 2016-01-29 18:23 ` Simon Glass
0 siblings, 0 replies; 26+ messages in thread
From: Simon Glass @ 2016-01-29 18:23 UTC (permalink / raw)
To: u-boot
Hi Paul,
On 29 January 2016 at 09:09, Paul Burton <paul.burton@imgtec.com> wrote:
> On Fri, Jan 29, 2016 at 07:56:19AM -0700, Simon Glass wrote:
>> Hi Paul,
>>
>> On 29 January 2016 at 06:54, Paul Burton <paul.burton@imgtec.com> wrote:
>> > 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 <paul.burton@imgtec.com>
>> > ---
>> >
>> > drivers/serial/ns16550.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>> > include/ns16550.h | 31 ++++++++++++++++++-------------
>> > 2 files changed, 56 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> > index 93dad33..b1d5319 100644
>> > --- a/drivers/serial/ns16550.c
>> > +++ b/drivers/serial/ns16550.c
>> > @@ -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);
>> > +}
>> > +
>> > /* 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.
>
> 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.
I think it's actually worse. We are currently able to use the debug
UART without needing the platform data at all. So even my comment is
not correct - the port type should in fact be a parameter to
serial_out().
>
>> > +} 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);
>> > diff --git a/include/ns16550.h b/include/ns16550.h
>> > index 4e62067..097f64b 100644
>> > --- a/include/ns16550.h
>> > +++ b/include/ns16550.h
>> > @@ -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;
>> > +#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.
OK. If you want to move it, please do so in a separate patch.
BTW IMO this is one of the worst drivers in U-Boot. I'm looking
forward to when it improves - and I think adding an IO/memory
parameter will point it in the right direction.
Perhaps we should have a flags word which tells serial_out() whether
to use IO/memory, 8-bit or 32-bit, endian, etc? (Not asking for you to
do this, just asking your opinion).
>
> Thanks,
> Paul
>
>> > +
>> > /*
>> > * These are the definitions for the FIFO Control Register
>> > */
>> > --
>> > 2.7.0
>> >
>>
>> Regards,
>> Simon
Regards,
Simon
^ permalink raw reply [flat|nested] 26+ messages in thread
* [U-Boot] [PATCH 5/9] MIPS: Remove SLOW_DOWN_IO
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
` (3 preceding siblings ...)
2016-01-29 13:54 ` [U-Boot] [PATCH 4/9] ns16550: Support I/O accessors selected by DT Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-02-01 21:27 ` Daniel Schwierzeck
2016-01-29 13:54 ` [U-Boot] [PATCH 6/9] MIPS: Support dynamic I/O port base address Paul Burton
` (5 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 UTC (permalink / raw)
To: u-boot
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 <paul.burton@imgtec.com>
---
arch/mips/include/asm/io.h | 40 +++-------------------------------------
1 file changed, 3 insertions(+), 37 deletions(-)
diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
index f71e342..4f9ec19 100644
--- a/arch/mips/include/asm/io.h
+++ b/arch/mips/include/asm/io.h
@@ -26,11 +26,6 @@
#include <spaces.h>
/*
- * Slowdown I/O port space accesses for antique hardware.
- */
-#undef CONF_SLOWDOWN_IO
-
-/*
* Raw operations are never swapped in software. OTOH values that raw
* operations are working on may or may not have been swapped by the bus
* hardware. An example use would be for flash memory that's used for
@@ -72,33 +67,6 @@ static inline void set_io_port_base(unsigned long base)
}
/*
- * Thanks to James van Artsdalen for a better timing-fix than
- * the two short jumps: using outb's to a nonexistent port seems
- * to guarantee better timings even on fast machines.
- *
- * On the other hand, I'd like to be sure of a non-existent port:
- * I feel a bit unsafe about using 0x80 (should be safe, though)
- *
- * Linus
- *
- */
-
-#define __SLOW_DOWN_IO \
- __asm__ __volatile__( \
- "sb\t$0,0x80(%0)" \
- : : "r" (mips_io_port_base));
-
-#ifdef CONF_SLOWDOWN_IO
-#ifdef REALLY_SLOW_IO
-#define SLOW_DOWN_IO { __SLOW_DOWN_IO; __SLOW_DOWN_IO; __SLOW_DOWN_IO; __SLOW_DOWN_IO; }
-#else
-#define SLOW_DOWN_IO __SLOW_DOWN_IO
-#endif
-#else
-#define SLOW_DOWN_IO
-#endif
-
-/*
* virt_to_phys - map virtual addresses to physical
* @address: address to remap
*
@@ -316,7 +284,7 @@ static inline type pfx##read##bwlq(const volatile void __iomem *mem) \
return pfx##ioswab##bwlq(__mem, __val); \
}
-#define __BUILD_IOPORT_SINGLE(pfx, bwlq, type, p, slow) \
+#define __BUILD_IOPORT_SINGLE(pfx, bwlq, type, p) \
\
static inline void pfx##out##bwlq##p(type val, unsigned long port) \
{ \
@@ -333,7 +301,6 @@ static inline void pfx##out##bwlq##p(type val, unsigned long port) \
BUILD_BUG_ON(sizeof(type) > sizeof(unsigned long)); \
\
*__addr = __val; \
- slow; \
} \
\
static inline type pfx##in##bwlq##p(unsigned long port) \
@@ -346,7 +313,6 @@ static inline type pfx##in##bwlq##p(unsigned long port) \
BUILD_BUG_ON(sizeof(type) > sizeof(unsigned long)); \
\
__val = *__addr; \
- slow; \
\
return pfx##ioswab##bwlq(__addr, __val); \
}
@@ -367,8 +333,8 @@ BUILDIO_MEM(l, u32)
BUILDIO_MEM(q, u64)
#define __BUILD_IOPORT_PFX(bus, bwlq, type) \
- __BUILD_IOPORT_SINGLE(bus, bwlq, type, ,) \
- __BUILD_IOPORT_SINGLE(bus, bwlq, type, _p, SLOW_DOWN_IO)
+ __BUILD_IOPORT_SINGLE(bus, bwlq, type, ) \
+ __BUILD_IOPORT_SINGLE(bus, bwlq, type, _p)
#define BUILDIO_IOPORT(bwlq, type) \
__BUILD_IOPORT_PFX(, bwlq, type) \
--
2.7.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 5/9] MIPS: Remove SLOW_DOWN_IO
2016-01-29 13:54 ` [U-Boot] [PATCH 5/9] MIPS: Remove SLOW_DOWN_IO Paul Burton
@ 2016-02-01 21:27 ` Daniel Schwierzeck
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Schwierzeck @ 2016-02-01 21:27 UTC (permalink / raw)
To: u-boot
2016-01-29 14:54 GMT+01:00 Paul Burton <paul.burton@imgtec.com>:
> 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 <paul.burton@imgtec.com>
> ---
>
> arch/mips/include/asm/io.h | 40 +++-------------------------------------
> 1 file changed, 3 insertions(+), 37 deletions(-)
>
applied to u-boot-mips, thanks
--
- Daniel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [U-Boot] [PATCH 6/9] MIPS: Support dynamic I/O port base address
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
` (4 preceding siblings ...)
2016-01-29 13:54 ` [U-Boot] [PATCH 5/9] MIPS: Remove SLOW_DOWN_IO Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-02-01 21:27 ` Daniel Schwierzeck
2016-01-29 13:54 ` [U-Boot] [PATCH 7/9] malta: Set I/O port base early Paul Burton
` (4 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 UTC (permalink / raw)
To: u-boot
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 <paul.burton@imgtec.com>
---
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
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 1b39c4c..585887c 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -23,6 +23,7 @@ config TARGET_QEMU_MIPS
config TARGET_MALTA
bool "Support malta"
+ select DYNAMIC_IO_PORT_BASE
select SUPPORTS_BIG_ENDIAN
select SUPPORTS_LITTLE_ENDIAN
select SUPPORTS_CPU_MIPS32_R1
@@ -217,6 +218,9 @@ config MIPS_L1_CACHE_SHIFT
default "4" if MIPS_L1_CACHE_SHIFT_4
default "5"
+config DYNAMIC_IO_PORT_BASE
+ bool
+
endif
endmenu
diff --git a/arch/mips/include/asm/global_data.h b/arch/mips/include/asm/global_data.h
index 2d9a0c9..a1ca257 100644
--- a/arch/mips/include/asm/global_data.h
+++ b/arch/mips/include/asm/global_data.h
@@ -12,6 +12,9 @@
/* Architecture-specific global data */
struct arch_global_data {
+#ifdef CONFIG_DYNAMIC_IO_PORT_BASE
+ unsigned long io_port_base;
+#endif
#ifdef CONFIG_JZSOC
/* There are other clocks in the jz4740 */
unsigned long per_clk; /* Peripheral bus clock */
diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
index 4f9ec19..723a60a 100644
--- a/arch/mips/include/asm/io.h
+++ b/arch/mips/include/asm/io.h
@@ -41,31 +41,37 @@
#define IO_SPACE_LIMIT 0xffff
-/*
- * On MIPS I/O ports are memory mapped, so we access them using normal
- * load/store instructions. mips_io_port_base is the virtual address to
- * which all ports are being mapped. For sake of efficiency some code
- * assumes that this is an address that can be loaded with a single lui
- * instruction, so the lower 16 bits must be zero. Should be true on
- * on any sane architecture; generic code does not use this assumption.
- */
-extern const unsigned long mips_io_port_base;
+#ifdef CONFIG_DYNAMIC_IO_PORT_BASE
+
+static inline ulong mips_io_port_base(void)
+{
+ DECLARE_GLOBAL_DATA_PTR;
+
+ return gd->arch.io_port_base;
+}
-/*
- * Gcc will generate code to load the value of mips_io_port_base after each
- * function call which may be fairly wasteful in some cases. So we don't
- * play quite by the book. We tell gcc mips_io_port_base is a long variable
- * which solves the code generation issue. Now we need to violate the
- * aliasing rules a little to make initialization possible and finally we
- * will need the barrier() to fight side effects of the aliasing chat.
- * This trickery will eventually collapse under gcc's optimizer. Oh well.
- */
static inline void set_io_port_base(unsigned long base)
{
- * (unsigned long *) &mips_io_port_base = base;
+ DECLARE_GLOBAL_DATA_PTR;
+
+ gd->arch.io_port_base = base;
barrier();
}
+#else /* !CONFIG_DYNAMIC_IO_PORT_BASE */
+
+static inline ulong mips_io_port_base(void)
+{
+ return 0;
+}
+
+static inline void set_io_port_base(unsigned long base)
+{
+ BUG_ON(base);
+}
+
+#endif /* !CONFIG_DYNAMIC_IO_PORT_BASE */
+
/*
* virt_to_phys - map virtual addresses to physical
* @address: address to remap
@@ -293,7 +299,7 @@ static inline void pfx##out##bwlq##p(type val, unsigned long port) \
\
war_octeon_io_reorder_wmb(); \
\
- __addr = (void *)__swizzle_addr_##bwlq(mips_io_port_base + port); \
+ __addr = (void *)__swizzle_addr_##bwlq(mips_io_port_base() + port); \
\
__val = pfx##ioswab##bwlq(__addr, val); \
\
@@ -308,7 +314,7 @@ static inline type pfx##in##bwlq##p(unsigned long port) \
volatile type *__addr; \
type __val; \
\
- __addr = (void *)__swizzle_addr_##bwlq(mips_io_port_base + port); \
+ __addr = (void *)__swizzle_addr_##bwlq(mips_io_port_base() + port); \
\
BUILD_BUG_ON(sizeof(type) > sizeof(unsigned long)); \
\
diff --git a/arch/mips/lib/Makefile b/arch/mips/lib/Makefile
index ac536da..b7ce5df 100644
--- a/arch/mips/lib/Makefile
+++ b/arch/mips/lib/Makefile
@@ -7,7 +7,6 @@
obj-y += cache.o
obj-y += cache_init.o
-obj-y += io.o
obj-$(CONFIG_CMD_BOOTM) += bootm.o
diff --git a/arch/mips/lib/io.c b/arch/mips/lib/io.c
deleted file mode 100644
index b2d4a09..0000000
--- a/arch/mips/lib/io.c
+++ /dev/null
@@ -1,12 +0,0 @@
-/*
- * (C) Copyright 2003
- * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
- *
- * SPDX-License-Identifier: GPL-2.0+
- */
-
-/*
- * mips_io_port_base is the begin of the address space to which x86 style
- * I/O ports are mapped.
- */
-const unsigned long mips_io_port_base = -1;
--
2.7.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 6/9] MIPS: Support dynamic I/O port base address
2016-01-29 13:54 ` [U-Boot] [PATCH 6/9] MIPS: Support dynamic I/O port base address Paul Burton
@ 2016-02-01 21:27 ` Daniel Schwierzeck
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Schwierzeck @ 2016-02-01 21:27 UTC (permalink / raw)
To: u-boot
2016-01-29 14:54 GMT+01:00 Paul Burton <paul.burton@imgtec.com>:
> 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 <paul.burton@imgtec.com>
> ---
>
> 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
>
applied to u-boot-mips, thanks
--
- Daniel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [U-Boot] [PATCH 7/9] malta: Set I/O port base early
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
` (5 preceding siblings ...)
2016-01-29 13:54 ` [U-Boot] [PATCH 6/9] MIPS: Support dynamic I/O port base address Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-02-01 21:24 ` Daniel Schwierzeck
2016-01-29 13:54 ` [U-Boot] [PATCH 8/9] malta: Use I/O accessors for SuperI/O controller Paul Burton
` (3 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 UTC (permalink / raw)
To: u-boot
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 <paul.burton@imgtec.com>
---
board/imgtec/malta/malta.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/board/imgtec/malta/malta.c b/board/imgtec/malta/malta.c
index cae4a21..6f4aebc 100644
--- a/board/imgtec/malta/malta.c
+++ b/board/imgtec/malta/malta.c
@@ -146,6 +146,8 @@ int board_early_init_f(void)
return -1;
}
+ set_io_port_base((ulong)io_base);
+
/* setup FDC37M817 super I/O controller */
malta_superio_init(io_base);
@@ -179,8 +181,6 @@ void pci_init_board(void)
switch (malta_sys_con()) {
case SYSCON_GT64120:
- set_io_port_base(CKSEG1ADDR(MALTA_GT_PCIIO_BASE));
-
gt64120_pci_init((void *)CKSEG1ADDR(MALTA_GT_BASE),
0x00000000, 0x00000000, CONFIG_SYS_MEM_SIZE,
0x10000000, 0x10000000, 128 * 1024 * 1024,
@@ -189,8 +189,6 @@ void pci_init_board(void)
default:
case SYSCON_MSC01:
- set_io_port_base(CKSEG1ADDR(MALTA_MSC01_PCIIO_BASE));
-
msc01_pci_init((void *)CKSEG1ADDR(MALTA_MSC01_PCI_BASE),
0x00000000, 0x00000000, CONFIG_SYS_MEM_SIZE,
MALTA_MSC01_PCIMEM_MAP,
--
2.7.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 8/9] malta: Use I/O accessors for SuperI/O controller
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
` (6 preceding siblings ...)
2016-01-29 13:54 ` [U-Boot] [PATCH 7/9] malta: Set I/O port base early Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-02-01 21:26 ` Daniel Schwierzeck
2016-01-29 13:54 ` [U-Boot] [PATCH 9/9] malta: Use device model & tree for UART Paul Burton
` (2 subsequent siblings)
10 siblings, 1 reply; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 UTC (permalink / raw)
To: u-boot
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 <paul.burton@imgtec.com>
---
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(-)
diff --git a/board/imgtec/malta/malta.c b/board/imgtec/malta/malta.c
index 6f4aebc..e31331a 100644
--- a/board/imgtec/malta/malta.c
+++ b/board/imgtec/malta/malta.c
@@ -130,26 +130,26 @@ void _machine_restart(void)
int board_early_init_f(void)
{
- void *io_base;
+ ulong io_base;
/* choose correct PCI I/O base */
switch (malta_sys_con()) {
case SYSCON_GT64120:
- io_base = (void *)CKSEG1ADDR(MALTA_GT_PCIIO_BASE);
+ io_base = CKSEG1ADDR(MALTA_GT_PCIIO_BASE);
break;
case SYSCON_MSC01:
- io_base = (void *)CKSEG1ADDR(MALTA_MSC01_PCIIO_BASE);
+ io_base = CKSEG1ADDR(MALTA_MSC01_PCIIO_BASE);
break;
default:
return -1;
}
- set_io_port_base((ulong)io_base);
+ set_io_port_base(io_base);
/* setup FDC37M817 super I/O controller */
- malta_superio_init(io_base);
+ malta_superio_init();
return 0;
}
diff --git a/board/imgtec/malta/superio.c b/board/imgtec/malta/superio.c
index eaa14df..7865ae2 100644
--- a/board/imgtec/malta/superio.c
+++ b/board/imgtec/malta/superio.c
@@ -45,19 +45,19 @@ static struct {
{ SIOCONF_ACTIVATE, 0x01 },
};
-void malta_superio_init(void *io_base)
+void malta_superio_init(void)
{
unsigned i;
/* enter config state */
- writeb(SIOCONF_ENTER_SETUP, io_base + SIO_CONF_PORT);
+ outb(SIOCONF_ENTER_SETUP, SIO_CONF_PORT);
/* configure peripherals */
for (i = 0; i < ARRAY_SIZE(sio_config); i++) {
- writeb(sio_config[i].key, io_base + SIO_CONF_PORT);
- writeb(sio_config[i].data, io_base + SIO_DATA_PORT);
+ outb(sio_config[i].key, SIO_CONF_PORT);
+ outb(sio_config[i].data, SIO_DATA_PORT);
}
/* exit config state */
- writeb(SIOCONF_EXIT_SETUP, io_base + SIO_CONF_PORT);
+ outb(SIOCONF_EXIT_SETUP, SIO_CONF_PORT);
}
diff --git a/board/imgtec/malta/superio.h b/board/imgtec/malta/superio.h
index 1450da5..271c462 100644
--- a/board/imgtec/malta/superio.h
+++ b/board/imgtec/malta/superio.h
@@ -10,6 +10,6 @@
#ifndef __BOARD_MALTA_SUPERIO_H__
#define __BOARD_MALTA_SUPERIO_H__
-extern void malta_superio_init(void *io_base);
+void malta_superio_init(void);
#endif /* __BOARD_MALTA_SUPERIO_H__ */
--
2.7.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 8/9] malta: Use I/O accessors for SuperI/O controller
2016-01-29 13:54 ` [U-Boot] [PATCH 8/9] malta: Use I/O accessors for SuperI/O controller Paul Burton
@ 2016-02-01 21:26 ` Daniel Schwierzeck
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Schwierzeck @ 2016-02-01 21:26 UTC (permalink / raw)
To: u-boot
2016-01-29 14:54 GMT+01:00 Paul Burton <paul.burton@imgtec.com>:
> 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 <paul.burton@imgtec.com>
> ---
>
> 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(-)
>
applied to u-boot-mips, thanks
--
- Daniel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [U-Boot] [PATCH 9/9] malta: Use device model & tree for UART
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
` (7 preceding siblings ...)
2016-01-29 13:54 ` [U-Boot] [PATCH 8/9] malta: Use I/O accessors for SuperI/O controller Paul Burton
@ 2016-01-29 13:54 ` Paul Burton
2016-01-29 14:05 ` [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Marek Vasut
2016-01-29 14:50 ` Daniel Schwierzeck
10 siblings, 0 replies; 26+ messages in thread
From: Paul Burton @ 2016-01-29 13:54 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.
The device tree includes the other 2 UARTs found on a Malta system too,
in order that they can be used easily by tweaking the chosen node.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---
arch/mips/Kconfig | 5 +++++
arch/mips/dts/Makefile | 2 +-
arch/mips/dts/mti,malta.dts | 50 +++++++++++++++++++++++++++++++++++++++++++++
board/imgtec/malta/malta.c | 13 ------------
configs/malta_defconfig | 1 +
configs/maltael_defconfig | 1 +
include/configs/malta.h | 8 +-------
7 files changed, 59 insertions(+), 21 deletions(-)
create mode 100644 arch/mips/dts/mti,malta.dts
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 585887c..aaaf3c0 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 47b6eb5..24b5e5a 100644
--- a/arch/mips/dts/Makefile
+++ b/arch/mips/dts/Makefile
@@ -2,7 +2,7 @@
# SPDX-License-Identifier: GPL-2.0+
#
-dtb-y +=
+dtb-$(CONFIG_TARGET_MALTA) += mti,malta.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..30a8b50
--- /dev/null
+++ b/arch/mips/dts/mti,malta.dts
@@ -0,0 +1,50 @@
+/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>;
+
+ uart1: serial at 2f8 {
+ compatible = "ns16550a";
+
+ reg = <1 0x2f8 0x40>;
+ reg-shift = <0>;
+
+ clock-frequency = <1843200>;
+ };
+
+ uart0: serial at 3f8 {
+ compatible = "ns16550a";
+
+ reg = <1 0x3f8 0x40>;
+ reg-shift = <0>;
+
+ clock-frequency = <1843200>;
+
+ u-boot,dm-pre-reloc;
+ };
+ };
+
+ uart2: serial at 1f000900 {
+ compatible = "ns16550a";
+
+ reg = <0x1f000900 0x40>;
+ reg-shift = <3>;
+
+ clock-frequency = <3686400>;
+ };
+};
diff --git a/board/imgtec/malta/malta.c b/board/imgtec/malta/malta.c
index e31331a..dc6a510 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/configs/malta_defconfig b/configs/malta_defconfig
index 2ebd58b..16839e1 100644
--- a/configs/malta_defconfig
+++ b/configs/malta_defconfig
@@ -7,3 +7,4 @@ CONFIG_TARGET_MALTA=y
# CONFIG_CMD_NFS is not set
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 d24d217..f42af8e 100644
--- a/configs/maltael_defconfig
+++ b/configs/maltael_defconfig
@@ -8,3 +8,4 @@ CONFIG_SYS_LITTLE_ENDIAN=y
# CONFIG_CMD_NFS is not set
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 aecc8ce..5311d24 100644
--- a/include/configs/malta.h
+++ b/include/configs/malta.h
@@ -13,6 +13,7 @@
#define CONFIG_MALTA
#define CONFIG_BOARD_EARLY_INIT_F
#define CONFIG_DISPLAY_BOARDINFO
+#define CONFIG_OF_LIBFDT
#define CONFIG_MEMSIZE_IN_BYTES
@@ -77,13 +78,6 @@
*/
#define CONFIG_BAUDRATE 115200
-#define CONFIG_SYS_NS16550_SERIAL
-#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_CONS_INDEX 1
-
/*
* Flash configuration
*/
--
2.7.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 0/9] Malta UART using device model & device tree
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
` (8 preceding siblings ...)
2016-01-29 13:54 ` [U-Boot] [PATCH 9/9] malta: Use device model & tree for UART Paul Burton
@ 2016-01-29 14:05 ` Marek Vasut
2016-01-29 14:50 ` Daniel Schwierzeck
10 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2016-01-29 14:05 UTC (permalink / raw)
To: u-boot
On Friday, January 29, 2016 at 02:54:46 PM, Paul Burton wrote:
> 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
Oh wow, so much activity on the mips front suddenly :)
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 26+ messages in thread* [U-Boot] [PATCH 0/9] Malta UART using device model & device tree
2016-01-29 13:54 [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Paul Burton
` (9 preceding siblings ...)
2016-01-29 14:05 ` [U-Boot] [PATCH 0/9] Malta UART using device model & device tree Marek Vasut
@ 2016-01-29 14:50 ` Daniel Schwierzeck
10 siblings, 0 replies; 26+ messages in thread
From: Daniel Schwierzeck @ 2016-01-29 14:50 UTC (permalink / raw)
To: u-boot
Hi Paul,
Am 29.01.2016 um 14:54 schrieb Paul Burton:
> 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.
it's great to see this, thanks.
>
> 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
for the moment I'll grap those four patches. The other ones need
feedback from others.
> malta: Use device model & tree for UART
>
--
- Daniel
^ permalink raw reply [flat|nested] 26+ messages in thread