Re: [U-Boot] [PATCH 1/9] Tegra: T30: Add include files
Hi Tom, On Thu, Sep 13, 2012 at 2:10 PM, Tom Warren twarren.nvi...@gmail.com wrote: Tom, On Thu, Sep 13, 2012 at 11:06 AM, Tom Rini tr...@ti.com wrote: On Wed, Sep 12, 2012 at 03:10:47PM -0700, Tom Warren wrote: Signed-off-by: Tom Warren twar...@nvidia.com A few things: - I see some #define FOO[space][space]val that should be [tab] Probably copied over from Tegra20 files. I'll turn on whitespace highlighting in my editor and fix 'em up. - I didn't checkpatch.pl this (nor the whole series) but please do and let us know if it's clean or why the warnings are false positives. I always run checkpath before submitting. I'll put a notice to that affect in the next version. Checkpatch ran clean w/only 1 false-positive about 'macros with complex values should be enclosed in parenthesis' for the #define CONFIG_DEFAULT_DEVICE_TREE tegra30-cardhu line in cardhu.h. - My preference is to bring in includes and C files and Makefiles and so on all at once, when each is needed / useful. YMMV and not a big deal. - But please make sure that you aren't adding defines / structs / etc that aren't used at some point by the end of the series at least. Removing (and correcting!) structs and defines was one of the things I had to do on am33xx. If it wasn't added until the corresponding driver work was being pushed, it'd have saved me some time. Sure, and that's good advice. I took a couple of passes during the port to try and remove vestigial and/or useless/unsupported files, features, and code, but I'm sure I missed some (as Stephen has already pointed out). I'll address those in V2. Congrats on getting this out. It is a lot of work! In the Chromium tree, we have an arch-tegra directory where a lot of the common code lives. It sounds like you are going to split that out a bit which is good. I suspect you may also want a few patches to move quite a bit of the code in arch/arm/include/arch-tegra20 to arch/arm/include/arch-tegra. Regards, Simon Thanks, Tom -- Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/9] Tegra: T30: Add include files
Simon, On Tue, Sep 18, 2012 at 12:29 PM, Simon Glass s...@chromium.org wrote: Hi Tom, On Thu, Sep 13, 2012 at 2:10 PM, Tom Warren twarren.nvi...@gmail.com wrote: Tom, On Thu, Sep 13, 2012 at 11:06 AM, Tom Rini tr...@ti.com wrote: On Wed, Sep 12, 2012 at 03:10:47PM -0700, Tom Warren wrote: Signed-off-by: Tom Warren twar...@nvidia.com A few things: - I see some #define FOO[space][space]val that should be [tab] Probably copied over from Tegra20 files. I'll turn on whitespace highlighting in my editor and fix 'em up. - I didn't checkpatch.pl this (nor the whole series) but please do and let us know if it's clean or why the warnings are false positives. I always run checkpath before submitting. I'll put a notice to that affect in the next version. Checkpatch ran clean w/only 1 false-positive about 'macros with complex values should be enclosed in parenthesis' for the #define CONFIG_DEFAULT_DEVICE_TREE tegra30-cardhu line in cardhu.h. - My preference is to bring in includes and C files and Makefiles and so on all at once, when each is needed / useful. YMMV and not a big deal. - But please make sure that you aren't adding defines / structs / etc that aren't used at some point by the end of the series at least. Removing (and correcting!) structs and defines was one of the things I had to do on am33xx. If it wasn't added until the corresponding driver work was being pushed, it'd have saved me some time. Sure, and that's good advice. I took a couple of passes during the port to try and remove vestigial and/or useless/unsupported files, features, and code, but I'm sure I missed some (as Stephen has already pointed out). I'll address those in V2. Congrats on getting this out. It is a lot of work! In the Chromium tree, we have an arch-tegra directory where a lot of the common code lives. It sounds like you are going to split that out a bit which is good. I suspect you may also want a few patches to move quite a bit of the code in arch/arm/include/arch-tegra20 to arch/arm/include/arch-tegra. As per a side-discussion with TomR and StephenW, I am in the process of recasting this T30 patchset as a move to common Tegra code. It'll mimic quite a lot of the Chromium U-Boot directory structure, I'm sure. Thanks, Tom Regards, Simon Thanks, Tom -- Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/9] Tegra: T30: Add include files
On Wed, Sep 12, 2012 at 03:10:47PM -0700, Tom Warren wrote: Signed-off-by: Tom Warren twar...@nvidia.com A few things: - I see some #define FOO[space][space]val that should be [tab] - I didn't checkpatch.pl this (nor the whole series) but please do and let us know if it's clean or why the warnings are false positives. - My preference is to bring in includes and C files and Makefiles and so on all at once, when each is needed / useful. YMMV and not a big deal. - But please make sure that you aren't adding defines / structs / etc that aren't used at some point by the end of the series at least. Removing (and correcting!) structs and defines was one of the things I had to do on am33xx. If it wasn't added until the corresponding driver work was being pushed, it'd have saved me some time. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/9] Tegra: T30: Add include files
On 09/12/2012 04:10 PM, Tom Warren wrote:
Signed-off-by: Tom Warren twar...@nvidia.com
Hmm. This is rather large to review, but I tried to at least glance
through it all and spot obvious issues...
diff --git a/arch/arm/include/asm/arch-tegra30/ap30.h
b/arch/arm/include/asm/arch-tegra30/ap30.h
+/* T30 Base physical address of SDRAM. */
+#define T30_BASE_PA_SDRAM 0x
That should be 0x8000. The fact anything still works with this issue
implies this constant isn't used anywhere? Aha, I see NV_PA_SDRAM_BASE
defined later with the correct value...
+/* T30 Base physical address of flash. */
+#define T30_BASE_PA_NOR_FLASH 0xD000
I don't know where NOR actually is mapped, but it can't be there; that's
in the middle of SDRAM (2G..4G-1)
diff --git a/arch/arm/include/asm/arch-tegra30/board.h
b/arch/arm/include/asm/arch-tegra30/board.h
+#ifndef _TEGRA_BOARD_H_
+#define _TEGRA_BOARD_H_
I wonder if include guards shouldn't say TEGRA30 not just TEGRA. That
would avoid any conflict between the different
arch/arm/include/asm/arch-tegra* directories. Sure, that's unlikely
right now, but if we really continue to push device tree, maybe we'll
end up with a unified Tegra20/Tegra30 bootloader image, and need to
include both.
diff --git a/arch/arm/include/asm/arch-tegra30/clk_rst.h
b/arch/arm/include/asm/arch-tegra30/clk_rst.h
+#ifndef _CLK_RST_H_
+#define _CLK_RST_H_
Hmm, and the naming format isn't consistent.
+/* The clocks supported by the hardware */
+enum periph_id {
+ PERIPH_ID_FIRST,
...
+/*
+ * Clock peripheral IDs which sadly don't match up with PERIPH_ID. we want
+ * callers to use the PERIPH_ID for all access to peripheral clocks to avoid
+ * confusion bewteen PERIPH_ID_... and PERIPHC_...
+ *
+ * We don't call this CLOCK_PERIPH_ID or PERIPH_CLOCK_ID as it would just be
+ * confusing.
+ */
+enum periphc_internal_id {
+ /* 0x00 */
+ PERIPHC_I2S1,
+ PERIPHC_I2S2,
+ PERIPHC_SPDIF_OUT,
+ PERIPHC_SPDIF_IN,
+ PERIPHC_PWM,
+ PERIPHC_05h,
+ PERIPHC_SBC2,
+ PERIPHC_SBC3,
...
Can you make sure that one/both (whichever is appropriate) of these line
up with the proposed Linux kernel Tegra30 clock binding clock IDs. I'm
not sure if the Tegra30 proposed binding has been published yet, but
Prashant Gaikwad can email you the patch off-list if you need.
+void clock_enable(enum periph_id clkid);
Hmm. I would have expected all the prototypes to be in a common header
between Tegra20 and Tegra30?
diff --git a/arch/arm/include/asm/arch-tegra30/funcmux.h
b/arch/arm/include/asm/arch-tegra30/funcmux.h
+/* Configs supported by the func mux */
+enum {
+ FUNCMUX_DEFAULT = 0,/* default config */
+
+ /* UART configs */
+ FUNCMUX_UART1_ULPI_UART2 = 0,
+ FUNCMUX_UART2_IRDA = 0,
+ FUNCMUX_UART4_GMC = 0,
+
+ /* I2C configs */
+ FUNCMUX_DVC_I2CP = 0,
+ FUNCMUX_I2C1_RM = 0,
+ FUNCMUX_I2C2_DDC = 0,
+ FUNCMUX_I2C2_PTA,
+ FUNCMUX_I2C3_DTF = 0,
+
+ /* SDMMC configs */
+ FUNCMUX_SDMMC1_SDIO1_4BIT = 0,
+ FUNCMUX_SDMMC2_DTA_DTD_8BIT = 0,
+ FUNCMUX_SDMMC3_SDB_4BIT = 0,
+ FUNCMUX_SDMMC3_SDB_SLXA_8BIT,
+ FUNCMUX_SDMMC4_ATC_ATD_8BIT = 0,
+ FUNCMUX_SDMMC4_ATB_GMA_4_BIT,
+ FUNCMUX_SDMMC4_ATB_GMA_GME_8_BIT,
+
+ /* USB configs */
+ FUNCMUX_USB2_ULPI = 0,
+
+ /* Serial Flash configs */
+ FUNCMUX_SPI1_GMC_GMD = 0,
+};
Those all look like Tegra20 options. Tegra30 doesn't have mux pin groups
(muxing is per-pin now), so names like DTA, DTD, SDB, SLXA, ATB, GMA,
... above don't make any sense (they're Tegra20 pin group names).
In turn, I wonder if a funcmux.h-style API even makes sense for Tegra30,
since the number of legal configurations is probably too large to
usefully enumerate, but that is perhaps another question.
diff --git a/arch/arm/include/asm/arch-tegra30/gp_padctrl.h
b/arch/arm/include/asm/arch-tegra30/gp_padctrl.h
+/* APB_MISC_GP and padctrl registers */
+struct apb_misc_gp_ctlr {
+ u32 modereg;/* 0x00: APB_MISC_GP_MODEREG */
+ u32 hidrev; /* 0x04: APB_MISC_GP_HIDREV */
+ u32 reserved0[22]; /* 0x08 - 0x5C: */
+ u32 emu_revid; /* 0x60: APB_MISC_GP_EMU_REVID */
+ u32 xactor_scratch; /* 0x64: APB_MISC_GP_XACTOR_SCRATCH */
+ u32 aocfg1; /* 0x68: APB_MISC_GP_AOCFG1PADCTRL */
+ u32 aocfg2; /* 0x6c: APB_MISC_GP_AOCFG2PADCTRL */
+ u32 atcfg1; /* 0x70: APB_MISC_GP_ATCFG1PADCTRL */
+ u32 atcfg2; /* 0x74: APB_MISC_GP_ATCFG2PADCTRL */
+ u32 cdevcfg1; /* 0x78: APB_MISC_GP_CDEV1CFGPADCTRL */
+ u32 cdevcfg2; /* 0x7C: APB_MISC_GP_CDEV2CFGPADCTRL */
+ u32 csuscfg;/* 0x80: APB_MISC_GP_CSUSCFGPADCTRL */
+ u32 dap1cfg;/* 0x84: APB_MISC_GP_DAP1CFGPADCTRL */
+ u32 dap2cfg;/* 0x88: APB_MISC_GP_DAP2CFGPADCTRL */
+
Re: [U-Boot] [PATCH 1/9] Tegra: T30: Add include files
Stephen,
On Thu, Sep 13, 2012 at 12:35 PM, Stephen Warren swar...@wwwdotorg.org wrote:
On 09/12/2012 04:10 PM, Tom Warren wrote:
Signed-off-by: Tom Warren twar...@nvidia.com
Hmm. This is rather large to review, but I tried to at least glance
through it all and spot obvious issues...
diff --git a/arch/arm/include/asm/arch-tegra30/ap30.h
b/arch/arm/include/asm/arch-tegra30/ap30.h
+/* T30 Base physical address of SDRAM. */
+#define T30_BASE_PA_SDRAM 0x
That should be 0x8000. The fact anything still works with this issue
implies this constant isn't used anywhere? Aha, I see NV_PA_SDRAM_BASE
defined later with the correct value...
Yep, T30_BASE_PA_SDRAM isn't used anywhere - just converted from
AP20_BASE_PA_SDRAM when I created ap30.h. I'll delete it.
+/* T30 Base physical address of flash. */
+#define T30_BASE_PA_NOR_FLASH 0xD000
I don't know where NOR actually is mapped, but it can't be there; that's
in the middle of SDRAM (2G..4G-1)
Again, copied when converting ap20.h to ap30.h. NOR Flash is @
0x4800: on T30. I'll just delete it, as no one uses it.
diff --git a/arch/arm/include/asm/arch-tegra30/board.h
b/arch/arm/include/asm/arch-tegra30/board.h
+#ifndef _TEGRA_BOARD_H_
+#define _TEGRA_BOARD_H_
I wonder if include guards shouldn't say TEGRA30 not just TEGRA. That
would avoid any conflict between the different
arch/arm/include/asm/arch-tegra* directories. Sure, that's unlikely
right now, but if we really continue to push device tree, maybe we'll
end up with a unified Tegra20/Tegra30 bootloader image, and need to
include both.
I could change it to _TEGRA30_BOARD_H_. I'd haven't considered ever
including both arch board.h files in a single build - as you say, it's
unlikely. But I'll change it.
diff --git a/arch/arm/include/asm/arch-tegra30/clk_rst.h
b/arch/arm/include/asm/arch-tegra30/clk_rst.h
+#ifndef _CLK_RST_H_
+#define _CLK_RST_H_
Hmm, and the naming format isn't consistent.
Copied from Tegra20. In the (near) future, I'll have a combined
clk_rst.h include for Tegra20/Tegra30.
+/* The clocks supported by the hardware */
+enum periph_id {
+ PERIPH_ID_FIRST,
...
+/*
+ * Clock peripheral IDs which sadly don't match up with PERIPH_ID. we want
+ * callers to use the PERIPH_ID for all access to peripheral clocks to avoid
+ * confusion bewteen PERIPH_ID_... and PERIPHC_...
+ *
+ * We don't call this CLOCK_PERIPH_ID or PERIPH_CLOCK_ID as it would just be
+ * confusing.
+ */
+enum periphc_internal_id {
+ /* 0x00 */
+ PERIPHC_I2S1,
+ PERIPHC_I2S2,
+ PERIPHC_SPDIF_OUT,
+ PERIPHC_SPDIF_IN,
+ PERIPHC_PWM,
+ PERIPHC_05h,
+ PERIPHC_SBC2,
+ PERIPHC_SBC3,
...
Can you make sure that one/both (whichever is appropriate) of these line
up with the proposed Linux kernel Tegra30 clock binding clock IDs. I'm
not sure if the Tegra30 proposed binding has been published yet, but
Prashant Gaikwad can email you the patch off-list if you need.
I'd rather leave it as is (copied from our internal T30 U-Boot code)
for now, especially for a 'proposed' kernel change. Let's get a basic,
working T30 build in there, and then we can make it match the kernel
IDs as needed.
+void clock_enable(enum periph_id clkid);
Hmm. I would have expected all the prototypes to be in a common header
between Tegra20 and Tegra30?
See my comment in another thread. My goal is to get a working T30
build in (to cmd prompt), and then I'll look at common-izing all of
the TegraXX code.
diff --git a/arch/arm/include/asm/arch-tegra30/funcmux.h
b/arch/arm/include/asm/arch-tegra30/funcmux.h
+/* Configs supported by the func mux */
+enum {
+ FUNCMUX_DEFAULT = 0,/* default config */
+
+ /* UART configs */
+ FUNCMUX_UART1_ULPI_UART2 = 0,
+ FUNCMUX_UART2_IRDA = 0,
+ FUNCMUX_UART4_GMC = 0,
+
+ /* I2C configs */
+ FUNCMUX_DVC_I2CP = 0,
+ FUNCMUX_I2C1_RM = 0,
+ FUNCMUX_I2C2_DDC = 0,
+ FUNCMUX_I2C2_PTA,
+ FUNCMUX_I2C3_DTF = 0,
+
+ /* SDMMC configs */
+ FUNCMUX_SDMMC1_SDIO1_4BIT = 0,
+ FUNCMUX_SDMMC2_DTA_DTD_8BIT = 0,
+ FUNCMUX_SDMMC3_SDB_4BIT = 0,
+ FUNCMUX_SDMMC3_SDB_SLXA_8BIT,
+ FUNCMUX_SDMMC4_ATC_ATD_8BIT = 0,
+ FUNCMUX_SDMMC4_ATB_GMA_4_BIT,
+ FUNCMUX_SDMMC4_ATB_GMA_GME_8_BIT,
+
+ /* USB configs */
+ FUNCMUX_USB2_ULPI = 0,
+
+ /* Serial Flash configs */
+ FUNCMUX_SPI1_GMC_GMD = 0,
+};
Those all look like Tegra20 options. Tegra30 doesn't have mux pin groups
(muxing is per-pin now), so names like DTA, DTD, SDB, SLXA, ATB, GMA,
... above don't make any sense (they're Tegra20 pin group names).
In turn, I wonder if a funcmux.h-style API even makes sense for Tegra30,
since the number of legal configurations is probably too large to
usefully enumerate, but that is perhaps another question.
Up to this point, there hasn't been much need for the funcmux stuff,
so I haven't looked at it too
Re: [U-Boot] [PATCH 1/9] Tegra: T30: Add include files
Tom, On Thu, Sep 13, 2012 at 11:06 AM, Tom Rini tr...@ti.com wrote: On Wed, Sep 12, 2012 at 03:10:47PM -0700, Tom Warren wrote: Signed-off-by: Tom Warren twar...@nvidia.com A few things: - I see some #define FOO[space][space]val that should be [tab] Probably copied over from Tegra20 files. I'll turn on whitespace highlighting in my editor and fix 'em up. - I didn't checkpatch.pl this (nor the whole series) but please do and let us know if it's clean or why the warnings are false positives. I always run checkpath before submitting. I'll put a notice to that affect in the next version. Checkpatch ran clean w/only 1 false-positive about 'macros with complex values should be enclosed in parenthesis' for the #define CONFIG_DEFAULT_DEVICE_TREE tegra30-cardhu line in cardhu.h. - My preference is to bring in includes and C files and Makefiles and so on all at once, when each is needed / useful. YMMV and not a big deal. - But please make sure that you aren't adding defines / structs / etc that aren't used at some point by the end of the series at least. Removing (and correcting!) structs and defines was one of the things I had to do on am33xx. If it wasn't added until the corresponding driver work was being pushed, it'd have saved me some time. Sure, and that's good advice. I took a couple of passes during the port to try and remove vestigial and/or useless/unsupported files, features, and code, but I'm sure I missed some (as Stephen has already pointed out). I'll address those in V2. Thanks, Tom -- Tom ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
