[U-Boot] [PATCH V2 01/11] Add support for MX35 processor

[Wolfgang Denk <wd@denx.de>]

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();
> +		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: wd at denx.de
The human race has one really effective weapon, and that is laughter.
                                                         - Mark Twain