Dear Stefano Babic,
In message <1295513194-16158-2-git-send-email-sba...@denx.de> you wrote:
> The patch adds basic support for the Freescale's i.MX35
> (arm1136 based) processor.
>
> Signed-off-by: Stefano Babic <sba...@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();
> + else
> + pll = 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
--
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
The human race has one really effective weapon, and that is laughter.
- Mark Twain
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
