[U-Boot] [PATCH 2/9] fdt: Support for ISA busses
Simon Glass
sjg at chromium.org
Fri Jan 29 19:23:24 CET 2016
Hi Paul,
On 29 January 2016 at 09:04, Paul Burton <paul.burton at 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 at 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 at 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
More information about the U-Boot
mailing list