Dear Stefano Babic,
In message 1295513194-16158-2-git-send-email-sbabic@denx.de you wrote:
The patch adds basic support for the Freescale's i.MX35 (arm1136 based) processor.
Signed-off-by: Stefano Babic sbabic@denx.de
checkpatch says:
[U-Boot] [PATCH V2 01/11] Add support for MX35 processor total: 7 errors, 8 warnings, 2109 lines checked
Please fix.
+u32 get_cpu_rev(void) +{
- int reg;
- struct iim_regs *iim =
(struct iim_regs *)IIM_BASE_ADDR;- reg = readl(&iim->iim_srev);
- if (!reg) {
reg = readw(ROMPATCH_REV);reg <<= 4;- } else
reg += CHIP_REV_1_0;
If there are braces for the "if", then there shall also be braces for the "else" ("Use braces in both branches.")
+u32 imx_get_uartclk(void) +{
- u32 freq;
- struct ccm_regs *ccm =
(struct ccm_regs *)IMX_CCM_BASE;- u32 pdr4 = readl(&ccm->pdr4);
- if (readl(&ccm->pdr3) & MXC_CCM_PDR3_UART_M_U)
freq = get_mcu_main_clk();- else
freq = decode_pll(readl(&ccm->ppctl),CONFIG_MX35_HCLK_FREQ);
Braces needed.
- case USB_CLK:
usb_prdf = (reg4 >> 25) & 0x7;usb_podf = (reg4 >> 22) & 0x7;if (reg4 & 0x200)pll = get_mcu_main_clk();elsepll = decode_pll(readl(&ccm->ppctl),CONFIG_MX35_HCLK_FREQ);
Ditto. Please fix globally.
index 0000000..c1dc62a --- /dev/null +++ b/arch/arm/include/asm/arch-mx35/asm-offsets.h @@ -0,0 +1,21 @@ +/*
- needed for lowlevel_init.S
- These should be auto-generated
- */
+/* CCM */ +#define CLKCTL_CCMR 0x00 +#define CLKCTL_PDR0 0x04 +#define CLKCTL_PDR1 0x08 +#define CLKCTL_PDR2 0x0C +#define CLKCTL_PDR3 0x10 +#define CLKCTL_PDR4 0x14 +#define CLKCTL_RCSR 0x18 +#define CLKCTL_MPCTL 0x1C +#define CLKCTL_PPCTL 0x20 +#define CLKCTL_ACMR 0x24 +#define CLKCTL_COSR 0x28 +#define CLKCTL_CGR0 0x2C +#define CLKCTL_CGR1 0x30 +#define CLKCTL_CGR2 0x34 +#define CLKCTL_CGR3 0x38
Indeed they should. Why don't you autogenerate these, then?
We have all the tools in place, use them.
+#define NFC_BUF_SIZE 0x1000 +#define NFC_BUFSIZE_REG_OFF (0 + 0x00) +#define RAM_BUFFER_ADDRESS_REG_OFF (0 + 0x04) +#define NAND_FLASH_ADD_REG_OFF (0 + 0x06) +#define NAND_FLASH_CMD_REG_OFF (0 + 0x08) +#define NFC_CONFIGURATION_REG_OFF (0 + 0x0A) +#define ECC_STATUS_RESULT_REG_OFF (0 + 0x0C) +#define ECC_RSLT_MAIN_AREA_REG_OFF (0 + 0x0E) +#define ECC_RSLT_SPARE_AREA_REG_OFF (0 + 0x10) +#define NF_WR_PROT_REG_OFF (0 + 0x12) +#define NAND_FLASH_WR_PR_ST_REG_OFF (0 + 0x18) +#define NAND_FLASH_CONFIG1_REG_OFF (0 + 0x1A) +#define NAND_FLASH_CONFIG2_REG_OFF (0 + 0x1C) +#define UNLOCK_START_BLK_ADD_REG_OFF (0 + 0x20) +#define UNLOCK_END_BLK_ADD_REG_OFF (0 + 0x22)
Why has this not been converted into a C struct?
...
+typedef enum iomux_pin_config {
- MUX_CONFIG_FUNC = 0, /*!< used as function */
- MUX_CONFIG_ALT1, /*!< used as alternate function 1 */
- MUX_CONFIG_ALT2, /*!< used as alternate function 2 */
- MUX_CONFIG_ALT3, /*!< used as alternate function 3 */
- MUX_CONFIG_ALT4, /*!< used as alternate function 4 */
- MUX_CONFIG_ALT5, /*!< used as alternate function 5 */
- MUX_CONFIG_ALT6, /*!< used as alternate function 6 */
- MUX_CONFIG_ALT7, /*!< used as alternate function 7 */
- MUX_CONFIG_SION = 0x1 << 4, /*!< used as LOOPBACK:MUX SION bit */
- MUX_CONFIG_GPIO = MUX_CONFIG_ALT5, /*!< used as GPIO */
+} iomux_pin_cfg_t;
/*!< ???
+/*!
/*! ???
...
+/*!
- Request ownership for an IO pin. This function has to be the first one
- being called before that pin is used. The caller has to check the
- return value to make sure it returns 0.
- @param pin a name defined by \b iomux_pin_name_t
- @param cfg an input function as defined in \b #iomux_pin_cfg_t
\b ???
- @param pin a name defined by \b iomux_pin_name_t
- @param cfg an input function as defined in \b #iomux_pin_cfg_t
"iomux_pin_name_t", but "#iomux_pin_cfg_t" ???
...
+typedef enum iomux_pins {
...
- MX35_PIN_A0 = _MXC_BUILD_NON_GPIO_PIN(0x28, 0x368),
- MX35_PIN_A1 = _MXC_BUILD_NON_GPIO_PIN(0x2C, 0x36C),
- MX35_PIN_A2 = _MXC_BUILD_NON_GPIO_PIN(0x30, 0x370),
- MX35_PIN_A3 = _MXC_BUILD_NON_GPIO_PIN(0x34, 0x374),
- MX35_PIN_A4 = _MXC_BUILD_NON_GPIO_PIN(0x38, 0x378),
- MX35_PIN_A5 = _MXC_BUILD_NON_GPIO_PIN(0x3C, 0x37C),
- MX35_PIN_A6 = _MXC_BUILD_NON_GPIO_PIN(0x40, 0x380),
- MX35_PIN_A7 = _MXC_BUILD_NON_GPIO_PIN(0x44, 0x384),
- MX35_PIN_A8 = _MXC_BUILD_NON_GPIO_PIN(0x48, 0x388),
- MX35_PIN_A9 = _MXC_BUILD_NON_GPIO_PIN(0x4C, 0x38C),
- MX35_PIN_A10 = _MXC_BUILD_NON_GPIO_PIN(0x50, 0x390),
- MX35_PIN_MA10 = _MXC_BUILD_NON_GPIO_PIN(0x54, 0x394),
- MX35_PIN_A11 = _MXC_BUILD_NON_GPIO_PIN(0x58, 0x398),
- MX35_PIN_A12 = _MXC_BUILD_NON_GPIO_PIN(0x5C, 0x39C),
- MX35_PIN_A13 = _MXC_BUILD_NON_GPIO_PIN(0x60, 0x3A0),
- MX35_PIN_A14 = _MXC_BUILD_NON_GPIO_PIN(0x64, 0x3A4),
- MX35_PIN_A15 = _MXC_BUILD_NON_GPIO_PIN(0x68, 0x3A8),
- MX35_PIN_A16 = _MXC_BUILD_NON_GPIO_PIN(0x6C, 0x3AC),
- MX35_PIN_A17 = _MXC_BUILD_NON_GPIO_PIN(0x70, 0x3B0),
- MX35_PIN_A18 = _MXC_BUILD_NON_GPIO_PIN(0x74, 0x3B4),
- MX35_PIN_A19 = _MXC_BUILD_NON_GPIO_PIN(0x78, 0x3B8),
- MX35_PIN_A20 = _MXC_BUILD_NON_GPIO_PIN(0x7C, 0x3BC),
- MX35_PIN_A21 = _MXC_BUILD_NON_GPIO_PIN(0x80, 0x3C0),
- MX35_PIN_A22 = _MXC_BUILD_NON_GPIO_PIN(0x84, 0x3C4),
- MX35_PIN_A23 = _MXC_BUILD_NON_GPIO_PIN(0x88, 0x3C8),
- MX35_PIN_A24 = _MXC_BUILD_NON_GPIO_PIN(0x8C, 0x3CC),
- MX35_PIN_A25 = _MXC_BUILD_NON_GPIO_PIN(0x90, 0x3D0),
Note: the following remark is a question, NOT a change request:
Would it not be possible to reduce all these terrible lists? As far as I can tell, the list is built sequentially, with both arguments to _MXC_BUILD_NON_GPIO_PIN() being incremented by 4 for the next register. This begs for automatic code generation, doesn't it?
Best regards,
Wolfgang Denk