* Re: [PATCH 0/1] Add support for mx31moboard
[not found] <1393509826-19549-1-git-send-email-philippe.retornaz@epfl.ch>
@ 2014-02-27 20:24 ` Sascha Hauer
[not found] ` <1393509826-19549-2-git-send-email-philippe.retornaz@epfl.ch>
1 sibling, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2014-02-27 20:24 UTC (permalink / raw)
To: Philippe Rétornaz; +Cc: barebox
On Thu, Feb 27, 2014 at 03:03:44PM +0100, Philippe Rétornaz wrote:
> Hello
>
> This patch adds support for the mx31moboard board to bareboot.
> I've based this on the pcm037 board with some minor modifications
> in the clock and SDRAM initialisation.
>
> We were using redboot until now, barebox ease of use is quite a
> shock :)
Yeah, redboot is not exactly famous for good usability ;)
>
> BTW, I suspect there is a typo in the clock initialisation while
> writing the CCM_PDR0 register on the pcm037 board:
>
> writel(MX31_PDR0_CSI_PODF(0xff1) | \
> MX31_PDR0_PER_PODF(7) | \
> MX31_PDR0_HSP_PODF(3) | \
> MX31_PDR0_NFC_PODF(5) | \
> MX31_PDR0_IPG_PODF(1) | \
> MX31_PDR0_MAX_PODF(3) | \
> MX31_PDR0_MCU_PODF(0), \
> MX31_CCM_BASE_ADDR + MX31_CCM_PDR0);
>
> The CSI_PODF field is only 9 bits wide, thus, MX31_PDR0_CSI_PODF(0xff1)
> should better be MX31_PDR0_CSI_PODF(0x1ff).
Thanks for noting. This indeed looks buggy.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ARM: i.MX31: Add support for mx31moboard board
[not found] ` <1393509826-19549-2-git-send-email-philippe.retornaz@epfl.ch>
@ 2014-02-27 20:36 ` Sascha Hauer
2014-02-28 8:21 ` Philippe Rétornaz
0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2014-02-27 20:36 UTC (permalink / raw)
To: Philippe Rétornaz; +Cc: barebox
Looks mostly good, some remarks inline.
On Thu, Feb 27, 2014 at 03:03:45PM +0100, Philippe Rétornaz wrote:
> +lwl-y += lowlevel.o
> +obj-y += mx31moboard.o
> diff --git a/arch/arm/boards/mx31moboard/env/boot/net b/arch/arm/boards/mx31moboard/env/boot/net
> new file mode 100644
> index 0000000..e81f2cd
> --- /dev/null
> +++ b/arch/arm/boards/mx31moboard/env/boot/net
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +
> +# No network card support on this board
Are you sure you want to drop this? You could attach a USB ethernet
dongle.
> diff --git a/arch/arm/boards/mx31moboard/env/boot/sd b/arch/arm/boards/mx31moboard/env/boot/sd
> new file mode 100644
> index 0000000..f96633f
> --- /dev/null
> +++ b/arch/arm/boards/mx31moboard/env/boot/sd
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +
> +if [ "$1" = menu ]; then
> + boot-menu-add-entry "$0" "SD Boot"
> + exit
> +fi
> +
> +global.bootm.image=/mnt/sd/boot/zImage
> +
> +if [ -e /mnt/sd/boot/oftree ]; then
> + global.bootm.oftree=/mnt/sd/boot/oftree
> +fi
> +
> +if [ -e /mnt/sd/boot/initrd.img ]; then
> + global.bootm.initrd=/mnt/sd/boot/initrd.img
> +fi
> +
> +global.linux.bootargs.dyn.root="root=/dev/mmcblk0p1 rootwait"
Consider using bootloader spec entries, see
http://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/
Basically you put a config file under /loader/entries/ in your rootfs.
The config file describes where your kernel/initrd/oftree is. Under
barebox you just have to 'boot mci0' without further configuration.
> +void __bare_init __naked barebox_arm_reset_vector(void)
> +{
> + uint32_t r;
> +
> + arm_cpu_lowlevel_init();
> +
> + /* Enable IPU Display interface */
> + writel(1 << 6, MX31_IPU_CTRL_BASE_ADDR);
> +
> + writel(0x074B0BF5, MX31_CCM_BASE_ADDR + MX31_CCM_CCMR);
> +
> + asm volatile("1:\n\t"
> + "SUBS %0, %0, #1 \n\t"
> + "BNE 1b \n\t"
> + : : "r" (0x4000) : "cc");
You can write a delay loop in c with:
volatile int c;
for (c = 0; c < 0x4000; c++)
(the volatile prevents the compiler from optimizing the loop away)
> +#include <asm/mmu.h>
> +#include <partition.h>
> +#include <generated/mach-types.h>
> +#include <asm/barebox-arm.h>
> +#include <mach/devices-imx31.h>
> +
> +#ifdef CONFIG_USB
> +#define USBH2_EN_B IOMUX_TO_GPIO(MX31_PIN_SCK6)
> +#define USB_RESET_B IOMUX_TO_GPIO(MX31_PIN_GPIO1_0)
> +
> +static void mx31moboard_usb_init(void)
> +{
> + u32 tmp;
> +
Instead of ifdeffing the code you can write:
if (!IS_ENABLED(CONFIG_USB))
return;
The compiler is clever enough to optimize the unused code away, still
you get better compile coverage.
> +static void mx31moboard_add_leds(void)
> +{
> + int i;
> +
Same here:
if (!IS_ENABLED(CONFIG_LED_GPIO))
return;
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ARM: i.MX31: Add support for mx31moboard board
2014-02-27 20:36 ` [PATCH 1/1] ARM: i.MX31: Add support for mx31moboard board Sascha Hauer
@ 2014-02-28 8:21 ` Philippe Rétornaz
2014-02-28 8:37 ` Sascha Hauer
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Rétornaz @ 2014-02-28 8:21 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
Le 27/02/2014 21:36, Sascha Hauer a écrit :
> On Thu, Feb 27, 2014 at 03:03:45PM +0100, Philippe Rétornaz wrote:
>> +lwl-y += lowlevel.o
>> +obj-y += mx31moboard.o
>> diff --git a/arch/arm/boards/mx31moboard/env/boot/net b/arch/arm/boards/mx31moboard/env/boot/net
>> new file mode 100644
>> index 0000000..e81f2cd
>> --- /dev/null
>> +++ b/arch/arm/boards/mx31moboard/env/boot/net
>> @@ -0,0 +1,3 @@
>> +#!/bin/sh
>> +
>> +# No network card support on this board
>
> Are you sure you want to drop this? You could attach a USB ethernet
> dongle.
Ok, I will remove this file.
>> diff --git a/arch/arm/boards/mx31moboard/env/boot/sd b/arch/arm/boards/mx31moboard/env/boot/sd
>> new file mode 100644
>> index 0000000..f96633f
>> --- /dev/null
>> +++ b/arch/arm/boards/mx31moboard/env/boot/sd
>> @@ -0,0 +1,18 @@
>> +#!/bin/sh
>> +
>> +if [ "$1" = menu ]; then
>> + boot-menu-add-entry "$0" "SD Boot"
>> + exit
>> +fi
>> +
>> +global.bootm.image=/mnt/sd/boot/zImage
>> +
>> +if [ -e /mnt/sd/boot/oftree ]; then
>> + global.bootm.oftree=/mnt/sd/boot/oftree
>> +fi
>> +
>> +if [ -e /mnt/sd/boot/initrd.img ]; then
>> + global.bootm.initrd=/mnt/sd/boot/initrd.img
>> +fi
>> +
>> +global.linux.bootargs.dyn.root="root=/dev/mmcblk0p1 rootwait"
>
> Consider using bootloader spec entries, see
> http://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/
> Basically you put a config file under /loader/entries/ in your rootfs.
> The config file describes where your kernel/initrd/oftree is. Under
> barebox you just have to 'boot mci0' without further configuration.
Seems nice, I'll have a look at it.
>> +void __bare_init __naked barebox_arm_reset_vector(void)
>> +{
>> + uint32_t r;
>> +
>> + arm_cpu_lowlevel_init();
>> +
>> + /* Enable IPU Display interface */
>> + writel(1 << 6, MX31_IPU_CTRL_BASE_ADDR);
>> +
>> + writel(0x074B0BF5, MX31_CCM_BASE_ADDR + MX31_CCM_CCMR);
>> +
>> + asm volatile("1:\n\t"
>> + "SUBS %0, %0, #1 \n\t"
>> + "BNE 1b \n\t"
>> + : : "r" (0x4000) : "cc");
>
> You can write a delay loop in c with:
>
> volatile int c;
>
> for (c = 0; c < 0x4000; c++)
Well, no, at least not on my toolchain. Because the volatile ask gcc to
not optimize the variable, it then put it on the stack. But the stack
pointer is not yet initialized, so it crashes. I've tried with a
barrier() instead of the volatile, but it leads to the same assembly
(which is not surprising). Here is the compiled code with your suggestion:
ldr r2, .L9+8
b .L2
.L3:
ldr r3, [sp, #4]
add r3, r3, #1
str r3, [sp, #4]
.L2:
ldr r3, [sp, #4]
cmp r3, r2
ble .L3
With L9+8:
.word 16383
But the stack pointer is initialised only in barebox_arm_entry() which
is called later. So I decided that a two instructions assembly loop was
the simplest solution.
>
>> +#include <asm/mmu.h>
>> +#include <partition.h>
>> +#include <generated/mach-types.h>
>> +#include <asm/barebox-arm.h>
>> +#include <mach/devices-imx31.h>
>> +
>> +#ifdef CONFIG_USB
>> +#define USBH2_EN_B IOMUX_TO_GPIO(MX31_PIN_SCK6)
>> +#define USB_RESET_B IOMUX_TO_GPIO(MX31_PIN_GPIO1_0)
>> +
>> +static void mx31moboard_usb_init(void)
>> +{
>> + u32 tmp;
>> +
>
> Instead of ifdeffing the code you can write:
>
> if (!IS_ENABLED(CONFIG_USB))
> return;
>
> The compiler is clever enough to optimize the unused code away, still
> you get better compile coverage.
>
>> +static void mx31moboard_add_leds(void)
>> +{
>> + int i;
>> +
>
> Same here:
>
> if (!IS_ENABLED(CONFIG_LED_GPIO))
> return;
>
Ok, will do.
Thanks for this review, I will post a V2 with the required changes.
Philippe
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ARM: i.MX31: Add support for mx31moboard board
2014-02-28 8:21 ` Philippe Rétornaz
@ 2014-02-28 8:37 ` Sascha Hauer
2014-02-28 13:10 ` Philippe Rétornaz
0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2014-02-28 8:37 UTC (permalink / raw)
To: Philippe Rétornaz; +Cc: barebox
On Fri, Feb 28, 2014 at 09:21:40AM +0100, Philippe Rétornaz wrote:
> Le 27/02/2014 21:36, Sascha Hauer a écrit :
> >On Thu, Feb 27, 2014 at 03:03:45PM +0100, Philippe Rétornaz wrote:
> >>+lwl-y += lowlevel.o
> >>+obj-y += mx31moboard.o
> >>diff --git a/arch/arm/boards/mx31moboard/env/boot/net b/arch/arm/boards/mx31moboard/env/boot/net
> >>new file mode 100644
> >>index 0000000..e81f2cd
> >>--- /dev/null
> >>+++ b/arch/arm/boards/mx31moboard/env/boot/net
> >>@@ -0,0 +1,3 @@
> >>+#!/bin/sh
> >>+
> >>+# No network card support on this board
> >
> >Are you sure you want to drop this? You could attach a USB ethernet
> >dongle.
>
> Ok, I will remove this file.
>
> >>diff --git a/arch/arm/boards/mx31moboard/env/boot/sd b/arch/arm/boards/mx31moboard/env/boot/sd
> >>new file mode 100644
> >>index 0000000..f96633f
> >>--- /dev/null
> >>+++ b/arch/arm/boards/mx31moboard/env/boot/sd
> >>@@ -0,0 +1,18 @@
> >>+#!/bin/sh
> >>+
> >>+if [ "$1" = menu ]; then
> >>+ boot-menu-add-entry "$0" "SD Boot"
> >>+ exit
> >>+fi
> >>+
> >>+global.bootm.image=/mnt/sd/boot/zImage
> >>+
> >>+if [ -e /mnt/sd/boot/oftree ]; then
> >>+ global.bootm.oftree=/mnt/sd/boot/oftree
> >>+fi
> >>+
> >>+if [ -e /mnt/sd/boot/initrd.img ]; then
> >>+ global.bootm.initrd=/mnt/sd/boot/initrd.img
> >>+fi
> >>+
> >>+global.linux.bootargs.dyn.root="root=/dev/mmcblk0p1 rootwait"
> >
> >Consider using bootloader spec entries, see
> >http://www.freedesktop.org/wiki/Specifications/BootLoaderSpec/
> >Basically you put a config file under /loader/entries/ in your rootfs.
> >The config file describes where your kernel/initrd/oftree is. Under
> >barebox you just have to 'boot mci0' without further configuration.
>
> Seems nice, I'll have a look at it.
>
> >>+void __bare_init __naked barebox_arm_reset_vector(void)
> >>+{
> >>+ uint32_t r;
> >>+
> >>+ arm_cpu_lowlevel_init();
> >>+
> >>+ /* Enable IPU Display interface */
> >>+ writel(1 << 6, MX31_IPU_CTRL_BASE_ADDR);
> >>+
> >>+ writel(0x074B0BF5, MX31_CCM_BASE_ADDR + MX31_CCM_CCMR);
> >>+
> >>+ asm volatile("1:\n\t"
> >>+ "SUBS %0, %0, #1 \n\t"
> >>+ "BNE 1b \n\t"
> >>+ : : "r" (0x4000) : "cc");
> >
> >You can write a delay loop in c with:
> >
> > volatile int c;
> >
> > for (c = 0; c < 0x4000; c++)
>
> Well, no, at least not on my toolchain. Because the volatile ask gcc to
> not optimize the variable, it then put it on the stack. But the stack
> pointer is not yet initialized, so it crashes. I've tried with a
> barrier() instead of the volatile, but it leads to the same assembly
> (which is not surprising). Here is the compiled code with your suggestion:
>
> ldr r2, .L9+8
> b .L2
> .L3:
> ldr r3, [sp, #4]
> add r3, r3, #1
> str r3, [sp, #4]
> .L2:
> ldr r3, [sp, #4]
> cmp r3, r2
> ble .L3
>
> With L9+8:
> .word 16383
>
> But the stack pointer is initialised only in barebox_arm_entry() which
> is called later. So I decided that a two instructions assembly loop was
> the simplest solution.
This may happen because the function gets too complex and gcc starts
using the stack in this case.
Try rewriting the lowlevel stuff as:
static void __noinline mx31_moboard_startup(void)
{
/* Put setup here */
}
void __bare_init __naked barebox_arm_reset_vector(void)
{
arm_cpu_lowlevel_init();
arm_setup_stack(MX31_IRAM_BASE_ADDR + MX31_IRAM_SIZE - 12);
mx31_moboard_startup();
}
With this you can use the stack and should be on the safe side.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ARM: i.MX31: Add support for mx31moboard board
2014-02-28 8:37 ` Sascha Hauer
@ 2014-02-28 13:10 ` Philippe Rétornaz
2014-02-28 13:37 ` Sascha Hauer
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Rétornaz @ 2014-02-28 13:10 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
Le 28/02/2014 09:37, Sascha Hauer a écrit :
> On Fri, Feb 28, 2014 at 09:21:40AM +0100, Philippe Rétornaz wrote:
>> Le 27/02/2014 21:36, Sascha Hauer a écrit :
>>> On Thu, Feb 27, 2014 at 03:03:45PM +0100, Philippe Rétornaz wrote:
>>>> +void __bare_init __naked barebox_arm_reset_vector(void)
>>>> +{
>>>> + uint32_t r;
>>>> +
>>>> + arm_cpu_lowlevel_init();
>>>> +
>>>> + /* Enable IPU Display interface */
>>>> + writel(1 << 6, MX31_IPU_CTRL_BASE_ADDR);
>>>> +
>>>> + writel(0x074B0BF5, MX31_CCM_BASE_ADDR + MX31_CCM_CCMR);
>>>> +
>>>> + asm volatile("1:\n\t"
>>>> + "SUBS %0, %0, #1 \n\t"
>>>> + "BNE 1b \n\t"
>>>> + : : "r" (0x4000) : "cc");
>>>
>>> You can write a delay loop in c with:
>>>
>>> volatile int c;
>>>
>>> for (c = 0; c < 0x4000; c++)
>>
>> Well, no, at least not on my toolchain. Because the volatile ask gcc to
>> not optimize the variable, it then put it on the stack. But the stack
>> pointer is not yet initialized, so it crashes. I've tried with a
>> barrier() instead of the volatile, but it leads to the same assembly
>> (which is not surprising). Here is the compiled code with your suggestion:
>>
>> ldr r2, .L9+8
>> b .L2
>> .L3:
>> ldr r3, [sp, #4]
>> add r3, r3, #1
>> str r3, [sp, #4]
>> .L2:
>> ldr r3, [sp, #4]
>> cmp r3, r2
>> ble .L3
>>
>> With L9+8:
>> .word 16383
>>
>> But the stack pointer is initialised only in barebox_arm_entry() which
>> is called later. So I decided that a two instructions assembly loop was
>> the simplest solution.
>
> This may happen because the function gets too complex and gcc starts
> using the stack in this case.
>
> Try rewriting the lowlevel stuff as:
>
> static void __noinline mx31_moboard_startup(void)
> {
> /* Put setup here */
> }
>
> void __bare_init __naked barebox_arm_reset_vector(void)
> {
> arm_cpu_lowlevel_init();
>
> arm_setup_stack(MX31_IRAM_BASE_ADDR + MX31_IRAM_SIZE - 12);
>
> mx31_moboard_startup();
> }
>
> With this you can use the stack and should be on the safe side.
Ok, I tried and it works fine so I will do this.
BTW, any reason for the -12, why not -8 ?
Thanks,
Philippe
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ARM: i.MX31: Add support for mx31moboard board
2014-02-28 13:10 ` Philippe Rétornaz
@ 2014-02-28 13:37 ` Sascha Hauer
0 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2014-02-28 13:37 UTC (permalink / raw)
To: Philippe Rétornaz; +Cc: barebox
On Fri, Feb 28, 2014 at 02:10:59PM +0100, Philippe Rétornaz wrote:
> Le 28/02/2014 09:37, Sascha Hauer a écrit :
> >This may happen because the function gets too complex and gcc starts
> >using the stack in this case.
> >
> >Try rewriting the lowlevel stuff as:
> >
> >static void __noinline mx31_moboard_startup(void)
> >{
> > /* Put setup here */
> >}
> >
> >void __bare_init __naked barebox_arm_reset_vector(void)
> >{
> > arm_cpu_lowlevel_init();
> >
> > arm_setup_stack(MX31_IRAM_BASE_ADDR + MX31_IRAM_SIZE - 12);
> >
> > mx31_moboard_startup();
> >}
> >
> >With this you can use the stack and should be on the safe side.
>
> Ok, I tried and it works fine so I will do this.
> BTW, any reason for the -12, why not -8 ?
Probably historic reasons. I don't know exactly.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-02-28 13:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1393509826-19549-1-git-send-email-philippe.retornaz@epfl.ch>
2014-02-27 20:24 ` [PATCH 0/1] Add support for mx31moboard Sascha Hauer
[not found] ` <1393509826-19549-2-git-send-email-philippe.retornaz@epfl.ch>
2014-02-27 20:36 ` [PATCH 1/1] ARM: i.MX31: Add support for mx31moboard board Sascha Hauer
2014-02-28 8:21 ` Philippe Rétornaz
2014-02-28 8:37 ` Sascha Hauer
2014-02-28 13:10 ` Philippe Rétornaz
2014-02-28 13:37 ` Sascha Hauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox