| Message ID | 20190101233745.16433-8-lukma@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | dm: Convert TPC70 to use DM and DTS in SPL and u-boot proper | expand |
On 1/2/19 12:37 AM, Lukasz Majewski wrote: > This commit is necessary to be able to re-use the pinctrl code in early > SPL to properly configure pins. > > The problem is that those "static" structures are placed in the SDRAM > area, which corresponds to u-boot proper (not even SPL). Hence, when > one wants to configure pins before relocation via DTS/DM, the board > hangs (imx6q SoC powered one) as only OCRAM area is available (0x009xxxxx). > > It is also safe to use calloc in this case, as the early SPL code provides > it - either full or trimmed version. The allocated memory is also correct > in respect to the memory area in which the SoC is currently running (OCRAM > vs. SDRAM). You should be able to access static data in early environment, many other drivers have no problem with it. I believe there is some more fundamental problem that needs to be fixed, hacking around it like this is just hiding the real issue. > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > > drivers/pinctrl/nxp/pinctrl-imx6.c | 39 +++++++++++++++++++------------------- > 1 file changed, 19 insertions(+), 20 deletions(-) > > diff --git a/drivers/pinctrl/nxp/pinctrl-imx6.c b/drivers/pinctrl/nxp/pinctrl-imx6.c > index d7c95bb738..876049b39b 100644 > --- a/drivers/pinctrl/nxp/pinctrl-imx6.c > +++ b/drivers/pinctrl/nxp/pinctrl-imx6.c > @@ -10,34 +10,33 @@ > > #include "pinctrl-imx.h" > > -static struct imx_pinctrl_soc_info imx6_pinctrl_soc_info; > - > -/* FIXME Before reloaction, BSS is overlapped with DT area */ > -static struct imx_pinctrl_soc_info imx6ul_pinctrl_soc_info = { > - .flags = ZERO_OFFSET_VALID, > -}; > - > -static struct imx_pinctrl_soc_info imx6_snvs_pinctrl_soc_info = { > - .flags = ZERO_OFFSET_VALID, > -}; > - > static int imx6_pinctrl_probe(struct udevice *dev) > { > struct imx_pinctrl_soc_info *info = > - (struct imx_pinctrl_soc_info *)dev_get_driver_data(dev); > + calloc(1, sizeof(struct imx_pinctrl_soc_info)); > + > + if (!info) { > + printf("%s: Not enough memory!\n", __func__); > + return -ENOMEM; > + } > + > + if (device_is_compatible(dev, "fsl,imx6sll-iomuxc-snvs") || > + device_is_compatible(dev, "fsl,imx6ull-iomuxc-snvs") || > + device_is_compatible(dev, "fsl,imx6ul-iomuxc")) > + info->flags = ZERO_OFFSET_VALID; > > return imx_pinctrl_probe(dev, info); > } > > static const struct udevice_id imx6_pinctrl_match[] = { > - { .compatible = "fsl,imx6q-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, > - { .compatible = "fsl,imx6dl-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, > - { .compatible = "fsl,imx6sl-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, > - { .compatible = "fsl,imx6sll-iomuxc-snvs", .data = (ulong)&imx6_snvs_pinctrl_soc_info }, > - { .compatible = "fsl,imx6sll-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, > - { .compatible = "fsl,imx6sx-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, > - { .compatible = "fsl,imx6ul-iomuxc", .data = (ulong)&imx6ul_pinctrl_soc_info }, > - { .compatible = "fsl,imx6ull-iomuxc-snvs", .data = (ulong)&imx6_snvs_pinctrl_soc_info }, > + { .compatible = "fsl,imx6q-iomuxc" }, > + { .compatible = "fsl,imx6dl-iomuxc" }, > + { .compatible = "fsl,imx6sl-iomuxc" }, > + { .compatible = "fsl,imx6sll-iomuxc-snvs" }, > + { .compatible = "fsl,imx6sll-iomuxc" }, > + { .compatible = "fsl,imx6sx-iomuxc" }, > + { .compatible = "fsl,imx6ul-iomuxc" }, > + { .compatible = "fsl,imx6ull-iomuxc-snvs" }, > { /* sentinel */ } > }; > >
Hi Marek, > On 1/2/19 12:37 AM, Lukasz Majewski wrote: > > This commit is necessary to be able to re-use the pinctrl code in > > early SPL to properly configure pins. > > > > The problem is that those "static" structures are placed in the > > SDRAM area, which corresponds to u-boot proper (not even SPL). > > Hence, when one wants to configure pins before relocation via > > DTS/DM, the board hangs (imx6q SoC powered one) as only OCRAM area > > is available (0x009xxxxx). > > > > It is also safe to use calloc in this case, as the early SPL code > > provides it - either full or trimmed version. The allocated memory > > is also correct in respect to the memory area in which the SoC is > > currently running (OCRAM vs. SDRAM). > > You should be able to access static data in early environment, many > other drivers have no problem with it. Have you used pinctrl IMX6Q driver with early DM at SPL? > I believe there is some more > fundamental problem that needs to be fixed, With the current code - which was not used in OCRAM only available memory - the statics are placed in the SDRAM area (0x178x xxxx). Instead of changing linker script (and validate it for all drivers and boards for SPL and u-boot proper) - the code has been rewritten to use malloc (which is available in the early code - either full blown or simple version). > hacking around it like > this is just hiding the real issue. The real issue is that IMX6Q uses SDRAM memory for statics/globals from the very beginning. > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > > > drivers/pinctrl/nxp/pinctrl-imx6.c | 39 > > +++++++++++++++++++------------------- 1 file changed, 19 > > insertions(+), 20 deletions(-) > > > > diff --git a/drivers/pinctrl/nxp/pinctrl-imx6.c > > b/drivers/pinctrl/nxp/pinctrl-imx6.c index d7c95bb738..876049b39b > > 100644 --- a/drivers/pinctrl/nxp/pinctrl-imx6.c > > +++ b/drivers/pinctrl/nxp/pinctrl-imx6.c > > @@ -10,34 +10,33 @@ > > > > #include "pinctrl-imx.h" > > > > -static struct imx_pinctrl_soc_info imx6_pinctrl_soc_info; > > - > > -/* FIXME Before reloaction, BSS is overlapped with DT area */ > > -static struct imx_pinctrl_soc_info imx6ul_pinctrl_soc_info = { > > - .flags = ZERO_OFFSET_VALID, > > -}; > > - > > -static struct imx_pinctrl_soc_info imx6_snvs_pinctrl_soc_info = { > > - .flags = ZERO_OFFSET_VALID, > > -}; > > - > > static int imx6_pinctrl_probe(struct udevice *dev) > > { > > struct imx_pinctrl_soc_info *info = > > - (struct imx_pinctrl_soc_info > > *)dev_get_driver_data(dev); > > + calloc(1, sizeof(struct imx_pinctrl_soc_info)); > > + > > + if (!info) { > > + printf("%s: Not enough memory!\n", __func__); > > + return -ENOMEM; > > + } > > + > > + if (device_is_compatible(dev, "fsl,imx6sll-iomuxc-snvs") || > > + device_is_compatible(dev, "fsl,imx6ull-iomuxc-snvs") || > > + device_is_compatible(dev, "fsl,imx6ul-iomuxc")) > > + info->flags = ZERO_OFFSET_VALID; > > > > return imx_pinctrl_probe(dev, info); > > } > > > > static const struct udevice_id imx6_pinctrl_match[] = { > > - { .compatible = "fsl,imx6q-iomuxc", .data = > > (ulong)&imx6_pinctrl_soc_info }, > > - { .compatible = "fsl,imx6dl-iomuxc", .data = > > (ulong)&imx6_pinctrl_soc_info }, > > - { .compatible = "fsl,imx6sl-iomuxc", .data = > > (ulong)&imx6_pinctrl_soc_info }, > > - { .compatible = "fsl,imx6sll-iomuxc-snvs", .data = > > (ulong)&imx6_snvs_pinctrl_soc_info }, > > - { .compatible = "fsl,imx6sll-iomuxc", .data = > > (ulong)&imx6_pinctrl_soc_info }, > > - { .compatible = "fsl,imx6sx-iomuxc", .data = > > (ulong)&imx6_pinctrl_soc_info }, > > - { .compatible = "fsl,imx6ul-iomuxc", .data = > > (ulong)&imx6ul_pinctrl_soc_info }, > > - { .compatible = "fsl,imx6ull-iomuxc-snvs", .data = > > (ulong)&imx6_snvs_pinctrl_soc_info }, > > + { .compatible = "fsl,imx6q-iomuxc" }, > > + { .compatible = "fsl,imx6dl-iomuxc" }, > > + { .compatible = "fsl,imx6sl-iomuxc" }, > > + { .compatible = "fsl,imx6sll-iomuxc-snvs" }, > > + { .compatible = "fsl,imx6sll-iomuxc" }, > > + { .compatible = "fsl,imx6sx-iomuxc" }, > > + { .compatible = "fsl,imx6ul-iomuxc" }, > > + { .compatible = "fsl,imx6ull-iomuxc-snvs" }, > > { /* sentinel */ } > > }; > > > > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 1/2/19 9:26 AM, Lukasz Majewski wrote: > Hi Marek, > >> On 1/2/19 12:37 AM, Lukasz Majewski wrote: >>> This commit is necessary to be able to re-use the pinctrl code in >>> early SPL to properly configure pins. >>> >>> The problem is that those "static" structures are placed in the >>> SDRAM area, which corresponds to u-boot proper (not even SPL). >>> Hence, when one wants to configure pins before relocation via >>> DTS/DM, the board hangs (imx6q SoC powered one) as only OCRAM area >>> is available (0x009xxxxx). >>> >>> It is also safe to use calloc in this case, as the early SPL code >>> provides it - either full or trimmed version. The allocated memory >>> is also correct in respect to the memory area in which the SoC is >>> currently running (OCRAM vs. SDRAM). >> >> You should be able to access static data in early environment, many >> other drivers have no problem with it. > > Have you used pinctrl IMX6Q driver with early DM at SPL? > >> I believe there is some more >> fundamental problem that needs to be fixed, > > With the current code - which was not used in OCRAM only available > memory - the statics are placed in the SDRAM area (0x178x xxxx). > > Instead of changing linker script (and validate it for all drivers and > boards for SPL and u-boot proper) - the code has been rewritten to use > malloc (which is available in the early code - either full blown or > simple version). > >> hacking around it like >> this is just hiding the real issue. > > The real issue is that IMX6Q uses SDRAM memory for statics/globals from > the very beginning. Fix it ? What you're implying here is that rodata (preinitialized static variables) are misplaced.
On Wed, 2 Jan 2019 15:14:22 +0100 Marek Vasut <marex@denx.de> wrote: > On 1/2/19 9:26 AM, Lukasz Majewski wrote: > > Hi Marek, > > > >> On 1/2/19 12:37 AM, Lukasz Majewski wrote: > >>> This commit is necessary to be able to re-use the pinctrl code in > >>> early SPL to properly configure pins. > >>> > >>> The problem is that those "static" structures are placed in the > >>> SDRAM area, which corresponds to u-boot proper (not even SPL). > >>> Hence, when one wants to configure pins before relocation via > >>> DTS/DM, the board hangs (imx6q SoC powered one) as only OCRAM area > >>> is available (0x009xxxxx). > >>> > >>> It is also safe to use calloc in this case, as the early SPL code > >>> provides it - either full or trimmed version. The allocated memory > >>> is also correct in respect to the memory area in which the SoC is > >>> currently running (OCRAM vs. SDRAM). > >> > >> You should be able to access static data in early environment, many > >> other drivers have no problem with it. > > > > Have you used pinctrl IMX6Q driver with early DM at SPL? > > > >> I believe there is some more > >> fundamental problem that needs to be fixed, > > > > With the current code - which was not used in OCRAM only available > > memory - the statics are placed in the SDRAM area (0x178x xxxx). > > > > Instead of changing linker script (and validate it for all drivers > > and boards for SPL and u-boot proper) - the code has been rewritten > > to use malloc (which is available in the early code - either full > > blown or simple version). > > > >> hacking around it like > >> this is just hiding the real issue. > > > > The real issue is that IMX6Q uses SDRAM memory for statics/globals > > from the very beginning. > Fix it ? What you're implying here is that rodata (preinitialized > static variables) are misplaced. > The not initialized (as going to be zeroed) static struct imx_pinctrl_soc_info imx6_pinctrl_soc_info; goes to bss section, which is located in SDRAM (not available yet). .bss.imx6_pinctrl_soc_info 0x0000000018200050 0x10 drivers/built-in.o The other structs in this file, as they have assigned values, are placed correctly in data (OCRAM area). I could add __attribute__((section("data"))) in front of this definition, but this may be a problematic for u-boot proper where this code is reused (and correctly this data is placed in bss). Hence, the idea to use calloc explicitly - and with it the correct (available in the moment) memory is provided. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/drivers/pinctrl/nxp/pinctrl-imx6.c b/drivers/pinctrl/nxp/pinctrl-imx6.c index d7c95bb738..876049b39b 100644 --- a/drivers/pinctrl/nxp/pinctrl-imx6.c +++ b/drivers/pinctrl/nxp/pinctrl-imx6.c @@ -10,34 +10,33 @@ #include "pinctrl-imx.h" -static struct imx_pinctrl_soc_info imx6_pinctrl_soc_info; - -/* FIXME Before reloaction, BSS is overlapped with DT area */ -static struct imx_pinctrl_soc_info imx6ul_pinctrl_soc_info = { - .flags = ZERO_OFFSET_VALID, -}; - -static struct imx_pinctrl_soc_info imx6_snvs_pinctrl_soc_info = { - .flags = ZERO_OFFSET_VALID, -}; - static int imx6_pinctrl_probe(struct udevice *dev) { struct imx_pinctrl_soc_info *info = - (struct imx_pinctrl_soc_info *)dev_get_driver_data(dev); + calloc(1, sizeof(struct imx_pinctrl_soc_info)); + + if (!info) { + printf("%s: Not enough memory!\n", __func__); + return -ENOMEM; + } + + if (device_is_compatible(dev, "fsl,imx6sll-iomuxc-snvs") || + device_is_compatible(dev, "fsl,imx6ull-iomuxc-snvs") || + device_is_compatible(dev, "fsl,imx6ul-iomuxc")) + info->flags = ZERO_OFFSET_VALID; return imx_pinctrl_probe(dev, info); } static const struct udevice_id imx6_pinctrl_match[] = { - { .compatible = "fsl,imx6q-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, - { .compatible = "fsl,imx6dl-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, - { .compatible = "fsl,imx6sl-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, - { .compatible = "fsl,imx6sll-iomuxc-snvs", .data = (ulong)&imx6_snvs_pinctrl_soc_info }, - { .compatible = "fsl,imx6sll-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, - { .compatible = "fsl,imx6sx-iomuxc", .data = (ulong)&imx6_pinctrl_soc_info }, - { .compatible = "fsl,imx6ul-iomuxc", .data = (ulong)&imx6ul_pinctrl_soc_info }, - { .compatible = "fsl,imx6ull-iomuxc-snvs", .data = (ulong)&imx6_snvs_pinctrl_soc_info }, + { .compatible = "fsl,imx6q-iomuxc" }, + { .compatible = "fsl,imx6dl-iomuxc" }, + { .compatible = "fsl,imx6sl-iomuxc" }, + { .compatible = "fsl,imx6sll-iomuxc-snvs" }, + { .compatible = "fsl,imx6sll-iomuxc" }, + { .compatible = "fsl,imx6sx-iomuxc" }, + { .compatible = "fsl,imx6ul-iomuxc" }, + { .compatible = "fsl,imx6ull-iomuxc-snvs" }, { /* sentinel */ } };
This commit is necessary to be able to re-use the pinctrl code in early SPL to properly configure pins. The problem is that those "static" structures are placed in the SDRAM area, which corresponds to u-boot proper (not even SPL). Hence, when one wants to configure pins before relocation via DTS/DM, the board hangs (imx6q SoC powered one) as only OCRAM area is available (0x009xxxxx). It is also safe to use calloc in this case, as the early SPL code provides it - either full or trimmed version. The allocated memory is also correct in respect to the memory area in which the SoC is currently running (OCRAM vs. SDRAM). Signed-off-by: Lukasz Majewski <lukma@denx.de> --- drivers/pinctrl/nxp/pinctrl-imx6.c | 39 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 20 deletions(-)