| Message ID | 1493952008-9762-1-git-send-email-kever.yang@rock-chips.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Kever, Am Freitag, 5. Mai 2017, 10:39:35 CEST schrieb Kever Yang: > ARM64 is using 64bit address which address cell is 2 instead of 1, > update to support it when of-platdata enabled. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> This helps make OF_PLATDATA work on my firefly-rk3399 so yay :-), but I don't think it's that easy to solve, see below: > --- > > Changes in v3: > - move of_plat_get_number() into lib/of_plat.c > > Changes in v2: > - rename the fdtdec_get_number() to of_plat_get_number() > > drivers/core/regmap.c | 9 +++++++++ > include/of_plat.h | 22 ++++++++++++++++++++++ > lib/Makefile | 3 +++ > lib/of_plat.c | 17 +++++++++++++++++ > 4 files changed, 51 insertions(+) > create mode 100644 include/of_plat.h > create mode 100644 lib/of_plat.c > > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c > index 3bec3df..c03279e 100644 > --- a/drivers/core/regmap.c > +++ b/drivers/core/regmap.c > @@ -12,6 +12,7 @@ > #include <malloc.h> > #include <mapmem.h> > #include <regmap.h> > +#include <of_plat.h> > > #include <asm/io.h> > > @@ -49,11 +50,19 @@ int regmap_init_mem_platdata(struct udevice *dev, u32 *reg, int count, > if (!map) > return -ENOMEM; > > +#ifdef CONFIG_PHYS_64BIT > + map->base = of_plat_get_number(reg, 2); > + for (range = map->range; count > 0; reg += 4, range++, count--) { > + range->start = of_plat_get_number(reg, 2); > + range->size = of_plat_get_number(reg + 2, 2); > + } I may just be missing something, but how can you be sure that the cell-size is always 2? For example, there were discussions about 64bit platforms not really needing to add all the 0x0 elements, when the whole io-registers are well below the 4GB mark and for example at least one sunxi also uses this, see for example: allwinner/sun50i-a64.dtsi, altera/socfpga_stratix10.dtsi, broadcom/bcm283x.dtsi and problably more using #address-cells = <1>, #size-cells = <1> for their memory mapped io. And from what I've seen dtoc simply converts the reg property and just ignores #address-cells and #size-cells (or I'm overlooking something). Possible solutions that come to mind would be make dtoc also convert #address-cells and #size-cells, making regmap and everybody check it or alternatively make dtoc convert regs to cell-size 2 in all cases when CONFIG_PHYS_64BIT is set. Heiko
Hi Heiko, Thanks for your comments. On 05/05/2017 09:10 PM, Heiko Stuebner wrote: > Hi Kever, > > Am Freitag, 5. Mai 2017, 10:39:35 CEST schrieb Kever Yang: >> ARM64 is using 64bit address which address cell is 2 instead of 1, >> update to support it when of-platdata enabled. >> >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > This helps make OF_PLATDATA work on my firefly-rk3399 so yay :-), > but I don't think it's that easy to solve, see below: > >> --- >> >> Changes in v3: >> - move of_plat_get_number() into lib/of_plat.c >> >> Changes in v2: >> - rename the fdtdec_get_number() to of_plat_get_number() >> >> drivers/core/regmap.c | 9 +++++++++ >> include/of_plat.h | 22 ++++++++++++++++++++++ >> lib/Makefile | 3 +++ >> lib/of_plat.c | 17 +++++++++++++++++ >> 4 files changed, 51 insertions(+) >> create mode 100644 include/of_plat.h >> create mode 100644 lib/of_plat.c >> >> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c >> index 3bec3df..c03279e 100644 >> --- a/drivers/core/regmap.c >> +++ b/drivers/core/regmap.c >> @@ -12,6 +12,7 @@ >> #include <malloc.h> >> #include <mapmem.h> >> #include <regmap.h> >> +#include <of_plat.h> >> >> #include <asm/io.h> >> >> @@ -49,11 +50,19 @@ int regmap_init_mem_platdata(struct udevice *dev, u32 *reg, int count, >> if (!map) >> return -ENOMEM; >> >> +#ifdef CONFIG_PHYS_64BIT >> + map->base = of_plat_get_number(reg, 2); >> + for (range = map->range; count > 0; reg += 4, range++, count--) { >> + range->start = of_plat_get_number(reg, 2); >> + range->size = of_plat_get_number(reg + 2, 2); >> + } > I may just be missing something, but how can you be sure that the cell-size > is always 2? > > For example, there were discussions about 64bit platforms not really > needing to add all the 0x0 elements, when the whole io-registers are well > below the 4GB mark and for example at least one sunxi also uses this, see > for example: > allwinner/sun50i-a64.dtsi, altera/socfpga_stratix10.dtsi, > broadcom/bcm283x.dtsi and problably more using #address-cells = <1>, > #size-cells = <1> for their memory mapped io. > > And from what I've seen dtoc simply converts the reg property and just > ignores #address-cells and #size-cells (or I'm overlooking something). > > Possible solutions that come to mind would be make dtoc also convert > #address-cells and #size-cells, making regmap and everybody check it > or alternatively make dtoc convert regs to cell-size 2 in all cases when > CONFIG_PHYS_64BIT is set. @Simon, could you take a look about this issue, I really not good at dtoc and libfdt, I think maybe we can re-use fdtdec_get_number() if dtoc do not do the fdt32_to_cpu() int dtoc? Thanks, - Kever > > > Heiko > >
Hi Kever, On 7 May 2017 at 19:45, Kever Yang <kever.yang@rock-chips.com> wrote: > Hi Heiko, > > > Thanks for your comments. > > > On 05/05/2017 09:10 PM, Heiko Stuebner wrote: >> >> Hi Kever, >> >> Am Freitag, 5. Mai 2017, 10:39:35 CEST schrieb Kever Yang: >>> >>> ARM64 is using 64bit address which address cell is 2 instead of 1, >>> update to support it when of-platdata enabled. >>> >>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >> >> This helps make OF_PLATDATA work on my firefly-rk3399 so yay :-), >> but I don't think it's that easy to solve, see below: >> >>> --- >>> >>> Changes in v3: >>> - move of_plat_get_number() into lib/of_plat.c >>> >>> Changes in v2: >>> - rename the fdtdec_get_number() to of_plat_get_number() >>> >>> drivers/core/regmap.c | 9 +++++++++ >>> include/of_plat.h | 22 ++++++++++++++++++++++ >>> lib/Makefile | 3 +++ >>> lib/of_plat.c | 17 +++++++++++++++++ >>> 4 files changed, 51 insertions(+) >>> create mode 100644 include/of_plat.h >>> create mode 100644 lib/of_plat.c >>> >>> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c >>> index 3bec3df..c03279e 100644 >>> --- a/drivers/core/regmap.c >>> +++ b/drivers/core/regmap.c >>> @@ -12,6 +12,7 @@ >>> #include <malloc.h> >>> #include <mapmem.h> >>> #include <regmap.h> >>> +#include <of_plat.h> >>> #include <asm/io.h> >>> @@ -49,11 +50,19 @@ int regmap_init_mem_platdata(struct udevice *dev, >>> u32 *reg, int count, >>> if (!map) >>> return -ENOMEM; >>> +#ifdef CONFIG_PHYS_64BIT >>> + map->base = of_plat_get_number(reg, 2); >>> + for (range = map->range; count > 0; reg += 4, range++, count--) { >>> + range->start = of_plat_get_number(reg, 2); >>> + range->size = of_plat_get_number(reg + 2, 2); >>> + } >> >> I may just be missing something, but how can you be sure that the >> cell-size >> is always 2? >> >> For example, there were discussions about 64bit platforms not really >> needing to add all the 0x0 elements, when the whole io-registers are well >> below the 4GB mark and for example at least one sunxi also uses this, see >> for example: >> allwinner/sun50i-a64.dtsi, altera/socfpga_stratix10.dtsi, >> broadcom/bcm283x.dtsi and problably more using #address-cells = <1>, >> #size-cells = <1> for their memory mapped io. >> >> And from what I've seen dtoc simply converts the reg property and just >> ignores #address-cells and #size-cells (or I'm overlooking something). >> >> Possible solutions that come to mind would be make dtoc also convert >> #address-cells and #size-cells, making regmap and everybody check it >> or alternatively make dtoc convert regs to cell-size 2 in all cases when >> CONFIG_PHYS_64BIT is set. > > > @Simon, could you take a look about this issue, I really not good at dtoc > and libfdt, I think maybe we can re-use fdtdec_get_number() if dtoc do not > do the fdt32_to_cpu() int dtoc? I think the best solution is the second one above. However dtoc is a bit of a pain to change at the moment. I've sent a series to clean it up. I'll hopefully take a look at the above in the next few weeks. Regards, Simon
Hi Kever, On 18 June 2017 at 22:11, Simon Glass <sjg@chromium.org> wrote: > > Hi Kever, > > On 7 May 2017 at 19:45, Kever Yang <kever.yang@rock-chips.com> wrote: > > Hi Heiko, > > > > > > Thanks for your comments. > > > > > > On 05/05/2017 09:10 PM, Heiko Stuebner wrote: > >> > >> Hi Kever, > >> > >> Am Freitag, 5. Mai 2017, 10:39:35 CEST schrieb Kever Yang: > >>> > >>> ARM64 is using 64bit address which address cell is 2 instead of 1, > >>> update to support it when of-platdata enabled. > >>> > >>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > >> > >> This helps make OF_PLATDATA work on my firefly-rk3399 so yay :-), > >> but I don't think it's that easy to solve, see below: > >> > >>> --- > >>> > >>> Changes in v3: > >>> - move of_plat_get_number() into lib/of_plat.c > >>> > >>> Changes in v2: > >>> - rename the fdtdec_get_number() to of_plat_get_number() > >>> > >>> drivers/core/regmap.c | 9 +++++++++ > >>> include/of_plat.h | 22 ++++++++++++++++++++++ > >>> lib/Makefile | 3 +++ > >>> lib/of_plat.c | 17 +++++++++++++++++ > >>> 4 files changed, 51 insertions(+) > >>> create mode 100644 include/of_plat.h > >>> create mode 100644 lib/of_plat.c > >>> > >>> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c > >>> index 3bec3df..c03279e 100644 > >>> --- a/drivers/core/regmap.c > >>> +++ b/drivers/core/regmap.c > >>> @@ -12,6 +12,7 @@ > >>> #include <malloc.h> > >>> #include <mapmem.h> > >>> #include <regmap.h> > >>> +#include <of_plat.h> > >>> #include <asm/io.h> > >>> @@ -49,11 +50,19 @@ int regmap_init_mem_platdata(struct udevice *dev, > >>> u32 *reg, int count, > >>> if (!map) > >>> return -ENOMEM; > >>> +#ifdef CONFIG_PHYS_64BIT > >>> + map->base = of_plat_get_number(reg, 2); > >>> + for (range = map->range; count > 0; reg += 4, range++, count--) { > >>> + range->start = of_plat_get_number(reg, 2); > >>> + range->size = of_plat_get_number(reg + 2, 2); > >>> + } > >> > >> I may just be missing something, but how can you be sure that the > >> cell-size > >> is always 2? > >> > >> For example, there were discussions about 64bit platforms not really > >> needing to add all the 0x0 elements, when the whole io-registers are well > >> below the 4GB mark and for example at least one sunxi also uses this, see > >> for example: > >> allwinner/sun50i-a64.dtsi, altera/socfpga_stratix10.dtsi, > >> broadcom/bcm283x.dtsi and problably more using #address-cells = <1>, > >> #size-cells = <1> for their memory mapped io. > >> > >> And from what I've seen dtoc simply converts the reg property and just > >> ignores #address-cells and #size-cells (or I'm overlooking something). > >> > >> Possible solutions that come to mind would be make dtoc also convert > >> #address-cells and #size-cells, making regmap and everybody check it > >> or alternatively make dtoc convert regs to cell-size 2 in all cases when > >> CONFIG_PHYS_64BIT is set. > > > > > > @Simon, could you take a look about this issue, I really not good at dtoc > > and libfdt, I think maybe we can re-use fdtdec_get_number() if dtoc do not > > do the fdt32_to_cpu() int dtoc? > > I think the best solution is the second one above. However dtoc is a > bit of a pain to change at the moment. I've sent a series to clean it > up. > > I'll hopefully take a look at the above in the next few weeks. Just to follow up, I sent a few patches for this yesterday. Regards, Simon
diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c index 3bec3df..c03279e 100644 --- a/drivers/core/regmap.c +++ b/drivers/core/regmap.c @@ -12,6 +12,7 @@ #include <malloc.h> #include <mapmem.h> #include <regmap.h> +#include <of_plat.h> #include <asm/io.h> @@ -49,11 +50,19 @@ int regmap_init_mem_platdata(struct udevice *dev, u32 *reg, int count, if (!map) return -ENOMEM; +#ifdef CONFIG_PHYS_64BIT + map->base = of_plat_get_number(reg, 2); + for (range = map->range; count > 0; reg += 4, range++, count--) { + range->start = of_plat_get_number(reg, 2); + range->size = of_plat_get_number(reg + 2, 2); + } +#else map->base = *reg; for (range = map->range; count > 0; reg += 2, range++, count--) { range->start = *reg; range->size = reg[1]; } +#endif *mapp = map; diff --git a/include/of_plat.h b/include/of_plat.h new file mode 100644 index 0000000..0badabb --- /dev/null +++ b/include/of_plat.h @@ -0,0 +1,22 @@ +/* + * (C) Copyright 2017 Rockchip Electronics Co., Ltd + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef __of_plat_h +#define __of_plat_h + +#include "libfdt_env.h" +/** + * Get a variable-sized number from a property + * + * This reads a number from one or more cells. + * + * @param ptr Pointer to property + * @param cells Number of cells containing the number + * @return the value in the cells + */ +u64 of_plat_get_number(const fdt32_t *ptr, unsigned int cells); + +#endif diff --git a/lib/Makefile b/lib/Makefile index 23e9f1e..a988965 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -55,6 +55,9 @@ obj-$(CONFIG_$(SPL_)OF_CONTROL) += fdtdec_common.o obj-$(CONFIG_$(SPL_)OF_CONTROL) += fdtdec.o endif +ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_SPL_OF_PLATDATA),yy) +obj-$(CONFIG_$(SPL_)OF_CONTROL) += of_plat.o +endif ifdef CONFIG_SPL_BUILD obj-$(CONFIG_SPL_YMODEM_SUPPORT) += crc16.o obj-$(CONFIG_SPL_NET_SUPPORT) += net_utils.o diff --git a/lib/of_plat.c b/lib/of_plat.c new file mode 100644 index 0000000..cf1cd1e --- /dev/null +++ b/lib/of_plat.c @@ -0,0 +1,17 @@ +/* + * (C) Copyright 2017 Rockchip Electronics Co., Ltd + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include "of_plat.h" + +u64 of_plat_get_number(const u32 *ptr, unsigned int cells) +{ + u64 number = 0; + + while (cells--) + number = (number << 32) | (*ptr++); + + return number; +}
ARM64 is using 64bit address which address cell is 2 instead of 1, update to support it when of-platdata enabled. Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- Changes in v3: - move of_plat_get_number() into lib/of_plat.c Changes in v2: - rename the fdtdec_get_number() to of_plat_get_number() drivers/core/regmap.c | 9 +++++++++ include/of_plat.h | 22 ++++++++++++++++++++++ lib/Makefile | 3 +++ lib/of_plat.c | 17 +++++++++++++++++ 4 files changed, 51 insertions(+) create mode 100644 include/of_plat.h create mode 100644 lib/of_plat.c