[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