Re: [U-Boot] [PATCH v2] ARMV7: Add support For Logic OMAP35x/DM37x modules
Dear Igor Grinberg, In message <4eedaba8.4010...@compulab.co.il> you wrote: > > > As for the function, in arch/arm/cpu/armv7/omap3/board.c there's already > > a checkboard() function that is called early (before relocation) that And keep in mind: before relocation means that the BSS segment is not available, and data segment is read-only. [You may be lucky on some systems that things appear to be different, but this is nothing you should take granted, and it is definitely nothing that should be used for common code.] > > code aborts when it tries to set gd->bd->bi_arch_number. I then tried > > storing the value in a global and then set gd->bd->bi_arch_number in > > board_init(), but between the call to checkboard() and board_init() the > > BSS section is zeroed. If I stored/load the computed bi_arch_number > > into a non-zero static value it works, but I feel that is really fragile. It is not only fragile, but broken. See above: the data ssegment must not be written before relocation. > This make me wonder, should we move the checkboard() call further > in the init sequence? No, it should not. It is part of the very early init code, and should remain where it is. Just don't add any code that does not belong there. > Also, the struct bd_info (bd_t) should have the board data and > ironically it is not available in checkboard() function. I don't see what is ironically about thaat. bd_info does not exist at that point of time. You misinterpret the purpose of the checkboard() function, it seems. > So, Albert, Wolfgang, > the question is, should we move the checkboard() call after > the gd->bd pointer initialization or are there any cases > where it is not appropriate? Please leave as is, it is perfectly intentional. Just don't place any code in checkboard() which does not fit there. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de A stone was placed at a ford in a river with the inscription: "When this stone is covered it is dangerous to ford here." ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ARMV7: Add support For Logic OMAP35x/DM37x modules
Hi Peter, On 12/16/11 08:09, Peter Barada wrote: > On 12/15/2011 01:30 PM, Tom Rini wrote: >> On Thu, Dec 15, 2011 at 10:15 AM, Peter Barada >> wrote: >>> This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo >>> reference boards. It assumes U-boot is loaded to SDRAM with the >>> help of another small bootloader (x-load) running from SRAM. [...] > As for the function, in arch/arm/cpu/armv7/omap3/board.c there's already > a checkboard() function that is called early (before relocation) that > prints out the banner. I tried to make that a weak alias and override > it in my board file, but when its called, gd->bd is not setup so that > code aborts when it tries to set gd->bd->bi_arch_number. I then tried > storing the value in a global and then set gd->bd->bi_arch_number in > board_init(), but between the call to checkboard() and board_init() the > BSS section is zeroed. If I stored/load the computed bi_arch_number > into a non-zero static value it works, but I feel that is really fragile. This make me wonder, should we move the checkboard() call further in the init sequence? Because, IIRC, it stayed in the same place, where it was, before the relocation feature was introduced and board_init() was called before the checkboard() (as board_init_f() does now). Also, the struct bd_info (bd_t) should have the board data and ironically it is not available in checkboard() function. So, Albert, Wolfgang, the question is, should we move the checkboard() call after the gd->bd pointer initialization or are there any cases where it is not appropriate? -- Regards, Igor. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ARMV7: Add support For Logic OMAP35x/DM37x modules
On Thu, Dec 15, 2011 at 11:09 PM, Peter Barada wrote:
> On 12/15/2011 01:30 PM, Tom Rini wrote:
>> On Thu, Dec 15, 2011 at 10:15 AM, Peter Barada
>> wrote:
>>> This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo
>>> reference boards. It assumes U-boot is loaded to SDRAM with the
>>> help of another small bootloader (x-load) running from SRAM.
>> #if 0'd code isn't allowed for merging so I assume this is an RFC :)
> My bad - sent the wrong patch. I'll update and send again.
>>> + /* Let it soak for a bit */
>>> + for (i = 0; i < 0x100; ++i)
>>> + asm("nop");
>> Can we just use a sdelay(...) here? And in the whole function you
>> haven't addressed Igor's comment (unless it's incoming and I just
>> haven't gotten the email yet) about this just being checkboard().
> Done - "sdelay(0x100)" instead of the delay loop.
Great, thanks.
> As for the function, in arch/arm/cpu/armv7/omap3/board.c there's already
> a checkboard() function that is called early (before relocation) that
> prints out the banner. I tried to make that a weak alias and override
> it in my board file, but when its called, gd->bd is not setup so that
> code aborts when it tries to set gd->bd->bi_arch_number. I then tried
> storing the value in a global and then set gd->bd->bi_arch_number in
> board_init(), but between the call to checkboard() and board_init() the
> BSS section is zeroed. If I stored/load the computed bi_arch_number
> into a non-zero static value it works, but I feel that is really fragile.
>
> Remember that identify_board() not only prints a banner but also
> determines one of four machine IDs passed to the linux kernel to
> identify which of the variants its running on.
[snip]
> So the big question before I submit V3 of this patch is whether to
> include overriding OMAP3's checkboard() function or to use
> identify_board() as is to compute bi_arch_number... I can do it either
> way, but don't want to waste anyone's time (with the wrong method).
OK, after re-reading (and mentally applying the changes you said
you've got) the patches, along with Igor's comments on v1, just inline
your identify logic to board_init()
--
Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ARMV7: Add support For Logic OMAP35x/DM37x modules
On 12/15/2011 01:30 PM, Tom Rini wrote:
> On Thu, Dec 15, 2011 at 10:15 AM, Peter Barada
> wrote:
>> This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo
>> reference boards. It assumes U-boot is loaded to SDRAM with the
>> help of another small bootloader (x-load) running from SRAM.
> #if 0'd code isn't allowed for merging so I assume this is an RFC :)
My bad - sent the wrong patch. I'll update and send again.
>> + /* Let it soak for a bit */
>> + for (i = 0; i < 0x100; ++i)
>> + asm("nop");
> Can we just use a sdelay(...) here? And in the whole function you
> haven't addressed Igor's comment (unless it's incoming and I just
> haven't gotten the email yet) about this just being checkboard().
Done - "sdelay(0x100)" instead of the delay loop.
As for the function, in arch/arm/cpu/armv7/omap3/board.c there's already
a checkboard() function that is called early (before relocation) that
prints out the banner. I tried to make that a weak alias and override
it in my board file, but when its called, gd->bd is not setup so that
code aborts when it tries to set gd->bd->bi_arch_number. I then tried
storing the value in a global and then set gd->bd->bi_arch_number in
board_init(), but between the call to checkboard() and board_init() the
BSS section is zeroed. If I stored/load the computed bi_arch_number
into a non-zero static value it works, but I feel that is really fragile.
Remember that identify_board() not only prints a banner but also
determines one of four machine IDs passed to the linux kernel to
identify which of the variants its running on.
>> +/* GPMC CS1 settings for Logic SOM LV/Torpedo LAN92xx Ethernet chip */
>> +#define LOGIC_NET_GPMC_CONFIG1 0x1000
>> +#define LOGIC_NET_GPMC_CONFIG2 0x00080801
>> +#define LOGIC_NET_GPMC_CONFIG3 0x
>> +#define LOGIC_NET_GPMC_CONFIG4 0x08010801
>> +#define LOGIC_NET_GPMC_CONFIG5 0x00080a0a
>> +#define LOGIC_NET_GPMC_CONFIG6 0x03000280
>> +#define LOGIC_NET_GPMC_CONFIG7 0x0848
>> +
>> +/*
>> + * Routine: setup_net_chip
>> + * Description: Setting up the configuration GPMC registers specific to the
>> + * Ethernet hardware.
>> + */
>> +static void setup_net_chip(void)
>> +{
>> + struct ctrl *ctrl_base = (struct ctrl *)OMAP34XX_CTRL_BASE;
>> +
>> + /* Configure GPMC registers */
>> + writel(LOGIC_NET_GPMC_CONFIG1, &gpmc_cfg->cs[1].config1);
>> + writel(LOGIC_NET_GPMC_CONFIG2, &gpmc_cfg->cs[1].config2);
>> + writel(LOGIC_NET_GPMC_CONFIG3, &gpmc_cfg->cs[1].config3);
>> + writel(LOGIC_NET_GPMC_CONFIG4, &gpmc_cfg->cs[1].config4);
>> + writel(LOGIC_NET_GPMC_CONFIG5, &gpmc_cfg->cs[1].config5);
>> + writel(LOGIC_NET_GPMC_CONFIG6, &gpmc_cfg->cs[1].config6);
>> + writel(LOGIC_NET_GPMC_CONFIG7, &gpmc_cfg->cs[1].config7);
>> +
>> + /* Enable off mode for NWE in PADCONF_GPMC_NWE register */
>> + writew(readw(&ctrl_base->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe);
>> + /* Enable off mode for NOE in PADCONF_GPMC_NADV_ALE register */
>> + writew(readw(&ctrl_base->gpmc_noe) | 0x0E00, &ctrl_base->gpmc_noe);
>> + /* Enable off mode for ALE in PADCONF_GPMC_NADV_ALE register */
>> + writew(readw(&ctrl_base->gpmc_nadv_ale) | 0x0E00,
>> + &ctrl_base->gpmc_nadv_ale);
>> +
>> +}
> Or this just being an enable_gpmc_cs_config call.
Done.
>> +int misc_init_r(void)
>> +{
>> +
>> + dieid_num_r();
>> +
>> + return 0;
>> +}
> Extra whitespace. I think checkpatch.pl will catch this so please run
> your patch through checkpatch.pl, roughly like this:
> $ git format-patch -1
> $ ./tools/checkpatch.pl 0001-whatever-its-called.patch
I rand checkpatch.pl over the result of "git diff --cached" and it
didn't complain about the emty line after the open brace. I've removed it.
> [snip]
>> +#define CONFIG_L2_OFF /* Keep L2 Cache Disabled */
> Why do we want to do this?
This is a holdover from a previous verison ofu-boot (2010.06) where I
added LCD/HDMI support for up to a 720p 32bpp display and when I did
that the kernel wouldn't start w/o disabling L2 in u-boot (possibly
because the reserved memory for the display in u-boot alone is larger
than the L2 cache in a OMAP35x/DM37x), but I never figured out why it
failed. I've removed it and will readdress when I add in the LCD/HDMI
support.
> [snip]
>> +#define CONFIG_SYS_MAXARGS 64 /* max number of command
>> args */
> This is very very large and probably doesn't need to be. This is the
> number of arguments to the u-boot commands, not to the linux kernel.
You're right, yet another holdover from an older version of u-boot.
I've reduced it 16 to match up with most of the other boards.
So the big question before I submit V3 of this patch is whether to
include overriding OMAP3's checkboard() function or to use
identify_board() as is to compute bi_arch_number... I can do it either
way, but don't want to waste any
Re: [U-Boot] [PATCH v2] ARMV7: Add support For Logic OMAP35x/DM37x modules
On Thu, Dec 15, 2011 at 10:15 AM, Peter Barada wrote:
> This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo
> reference boards. It assumes U-boot is loaded to SDRAM with the
> help of another small bootloader (x-load) running from SRAM.
#if 0'd code isn't allowed for merging so I assume this is an RFC :)
> + /* Let it soak for a bit */
> + for (i = 0; i < 0x100; ++i)
> + asm("nop");
Can we just use a sdelay(...) here? And in the whole function you
haven't addressed Igor's comment (unless it's incoming and I just
haven't gotten the email yet) about this just being checkboard().
> +/* GPMC CS1 settings for Logic SOM LV/Torpedo LAN92xx Ethernet chip */
> +#define LOGIC_NET_GPMC_CONFIG1 0x1000
> +#define LOGIC_NET_GPMC_CONFIG2 0x00080801
> +#define LOGIC_NET_GPMC_CONFIG3 0x
> +#define LOGIC_NET_GPMC_CONFIG4 0x08010801
> +#define LOGIC_NET_GPMC_CONFIG5 0x00080a0a
> +#define LOGIC_NET_GPMC_CONFIG6 0x03000280
> +#define LOGIC_NET_GPMC_CONFIG7 0x0848
> +
> +/*
> + * Routine: setup_net_chip
> + * Description: Setting up the configuration GPMC registers specific to the
> + * Ethernet hardware.
> + */
> +static void setup_net_chip(void)
> +{
> + struct ctrl *ctrl_base = (struct ctrl *)OMAP34XX_CTRL_BASE;
> +
> + /* Configure GPMC registers */
> + writel(LOGIC_NET_GPMC_CONFIG1, &gpmc_cfg->cs[1].config1);
> + writel(LOGIC_NET_GPMC_CONFIG2, &gpmc_cfg->cs[1].config2);
> + writel(LOGIC_NET_GPMC_CONFIG3, &gpmc_cfg->cs[1].config3);
> + writel(LOGIC_NET_GPMC_CONFIG4, &gpmc_cfg->cs[1].config4);
> + writel(LOGIC_NET_GPMC_CONFIG5, &gpmc_cfg->cs[1].config5);
> + writel(LOGIC_NET_GPMC_CONFIG6, &gpmc_cfg->cs[1].config6);
> + writel(LOGIC_NET_GPMC_CONFIG7, &gpmc_cfg->cs[1].config7);
> +
> + /* Enable off mode for NWE in PADCONF_GPMC_NWE register */
> + writew(readw(&ctrl_base->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe);
> + /* Enable off mode for NOE in PADCONF_GPMC_NADV_ALE register */
> + writew(readw(&ctrl_base->gpmc_noe) | 0x0E00, &ctrl_base->gpmc_noe);
> + /* Enable off mode for ALE in PADCONF_GPMC_NADV_ALE register */
> + writew(readw(&ctrl_base->gpmc_nadv_ale) | 0x0E00,
> + &ctrl_base->gpmc_nadv_ale);
> +
> +}
Or this just being an enable_gpmc_cs_config call.
> +int misc_init_r(void)
> +{
> +
> + dieid_num_r();
> +
> + return 0;
> +}
Extra whitespace. I think checkpatch.pl will catch this so please run
your patch through checkpatch.pl, roughly like this:
$ git format-patch -1
$ ./tools/checkpatch.pl 0001-whatever-its-called.patch
[snip]
> +#define CONFIG_L2_OFF /* Keep L2 Cache Disabled */
Why do we want to do this?
[snip]
> +#define CONFIG_SYS_MAXARGS 64 /* max number of command args
> */
This is very very large and probably doesn't need to be. This is the
number of arguments to the u-boot commands, not to the linux kernel.
Thanks.
--
Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2] ARMV7: Add support For Logic OMAP35x/DM37x modules
This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo
reference boards. It assumes U-boot is loaded to SDRAM with the
help of another small bootloader (x-load) running from SRAM.
Signed-off-by: Peter Barada
Cc: Tom Rini
Cc: Igor Grinberg
---
Changes for V2:
Rework logic_identify() into identify_board() - can't use checkboard()
since its enabled by CONFIG_DISPLAY_BOARDINFO
Properly indent comments in set_muxconf_regs()
Move setup_net_chip() call from misc_init_r to board_eth_init()
Remove triple empty line spacing
Pass gpio_request(189) non-empty description
Remove board/logicpd/omap3som/config.mk
Remove clean/distclean from board/logicpd/omap3som/Makefile
Modify board_mmc_init() to be one line function
Modify include/configs/omap3_logic.h to use on/off #defines
board/logicpd/omap3som/Makefile | 42 +++
board/logicpd/omap3som/omap3logic.c | 582 +++
board/logicpd/omap3som/omap3logic.h | 35 ++
boards.cfg |1 +
include/configs/omap3_logic.h | 359 +
5 files changed, 1019 insertions(+), 0 deletions(-)
create mode 100644 board/logicpd/omap3som/Makefile
create mode 100644 board/logicpd/omap3som/omap3logic.c
create mode 100644 board/logicpd/omap3som/omap3logic.h
create mode 100644 include/configs/omap3_logic.h
diff --git a/board/logicpd/omap3som/Makefile b/board/logicpd/omap3som/Makefile
new file mode 100644
index 000..75e237b
--- /dev/null
+++ b/board/logicpd/omap3som/Makefile
@@ -0,0 +1,42 @@
+#
+# (C) Copyright 2000, 2001, 2002
+# Wolfgang Denk, DENX Software Engineering, w...@denx.de.
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#
+
+include $(TOPDIR)/config.mk
+
+LIB= $(obj)lib$(BOARD).o
+
+COBJS-y:= omap3logic.o
+
+COBJS := $(sort $(COBJS-y))
+SRCS := $(COBJS:.o=.c)
+OBJS := $(addprefix $(obj),$(COBJS))
+
+$(LIB):$(obj).depend $(OBJS)
+ $(call cmd_link_o_target, $(OBJS))
+
+#
+
+# defines $(obj).depend target
+include $(SRCTREE)/rules.mk
+
+sinclude $(obj).depend
diff --git a/board/logicpd/omap3som/omap3logic.c
b/board/logicpd/omap3som/omap3logic.c
new file mode 100644
index 000..0411adb
--- /dev/null
+++ b/board/logicpd/omap3som/omap3logic.c
@@ -0,0 +1,582 @@
+/*
+ * (C) Copyright 2011
+ * Logic Product Development
+ *
+ * Author :
+ * Peter Barada
+ *
+ * Derived from Beagle Board and 3430 SDP code by
+ * Richard Woodruff
+ * Syed Mohammed Khasim
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "omap3logic.h"
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/* Mux hsusb0_data5 as gpio_189 */
+#define MUX_LOGIC_HSUSB0_DATA5_GPIO_MUX() \
+ MUX_VAL(CP(HSUSB0_DATA5), (IEN | PTD | DIS | M4))
+
+/* Mux hsusb0_data5 as hasusb0_data5 */
+#define MUX_LOGIC_HSUSB0_DATA5_DATA5() \
+ MUX_VAL(CP(HSUSB0_DATA5), (IEN | PTD | DIS | M0))
+
+/* two dimensional array of Linux machine IDs; row it selected based on CPU,
+ * column is slected based on hsusb0_data5 having pulldown resistor */
+static struct board_id {
+ char *name;
+ int machine_id;
+} boards[2][2] = {
+ {
+ {
+
