[U-Boot] [PATCH v5] socfpga: Adding Scan Manager driver

[Wolfgang Denk <wd@denx.de>]

Dear Chin Liang See,

In message <1393022790-9296-1-git-send-email-clsee@altera.com> you wrote:
> Scan Manager driver will be called to configure the IOCSR
> scan chain. This configuration will setup the IO buffer settings
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@altera.com>
> Cc: Wolfgang Denk <wd@denx.de>
> CC: Pavel Machek <pavel@denx.de>
> Cc: Tom Rini <trini@ti.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> ---
> Changes for v5
> - Removal of additional blank line
> - Added comment for magic number
> Changes for v4
> - avoid code duplication by add goto error
> - include underscore to variables name
> Changes for v3
> - merge the handoff file and driver into single patch
> Changes for v2
> - rebase with latest v2014.01-rc1
> ---
>  arch/arm/cpu/armv7/socfpga/Makefile                |    2 +-
>  arch/arm/cpu/armv7/socfpga/scan_manager.c          |  214 +++++++
>  arch/arm/cpu/armv7/socfpga/spl.c                   |    4 +
>  arch/arm/include/asm/arch-socfpga/scan_manager.h   |   96 +++
>  .../include/asm/arch-socfpga/socfpga_base_addrs.h  |    1 +
>  board/altera/socfpga/iocsr_config.c                |  657 ++++++++++++++++++++
>  board/altera/socfpga/iocsr_config.h                |   17 +
>  include/configs/socfpga_cyclone5.h                 |    1 +
>  8 files changed, 991 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/cpu/armv7/socfpga/scan_manager.c
>  create mode 100644 arch/arm/include/asm/arch-socfpga/scan_manager.h
>  create mode 100644 board/altera/socfpga/iocsr_config.c
>  create mode 100644 board/altera/socfpga/iocsr_config.h
> 
> diff --git a/arch/arm/cpu/armv7/socfpga/Makefile b/arch/arm/cpu/armv7/socfpga/Makefile
> index 3e84a0c..4edc5d4 100644
> --- a/arch/arm/cpu/armv7/socfpga/Makefile
> +++ b/arch/arm/cpu/armv7/socfpga/Makefile
> @@ -9,4 +9,4 @@
>  
>  obj-y	:= lowlevel_init.o
>  obj-y	+= misc.o timer.o reset_manager.o system_manager.o
> -obj-$(CONFIG_SPL_BUILD) += spl.o freeze_controller.o
> +obj-$(CONFIG_SPL_BUILD) += spl.o freeze_controller.o scan_manager.o
> diff --git a/arch/arm/cpu/armv7/socfpga/scan_manager.c b/arch/arm/cpu/armv7/socfpga/scan_manager.c
> new file mode 100644
> index 0000000..6e6f372
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/socfpga/scan_manager.c
> @@ -0,0 +1,214 @@
> +/*
> + *  Copyright (C) 2013 Altera Corporation <www.altera.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/arch/freeze_controller.h>
> +#include <asm/arch/scan_manager.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static const struct socfpga_scan_manager *scan_manager_base =
> +		(void *)(SOCFPGA_SCANMGR_ADDRESS);
> +static const struct socfpga_freeze_controller *freeze_controller_base =
> +		(void *)(SOCFPGA_SYSMGR_ADDRESS + SYSMGR_FRZCTRL_ADDRESS);
> +
> +/*
> + * Function to check IO scan chain engine status and wait if the engine is
> + * is active. Poll the IO scan chain engine till maximum iteration reached.
> + */
> +static inline uint32_t scan_mgr_io_scan_chain_engine_is_idle(uint32_t max_iter)
> +{
> +	uint32_t scanmgr_status;
> +
> +	scanmgr_status = readl(&scan_manager_base->stat);
> +
> +	/* Poll the engine until the scan engine is inactive */
> +	while (SCANMGR_STAT_ACTIVE_GET(scanmgr_status)
> +		|| (SCANMGR_STAT_WFIFOCNT_GET(scanmgr_status) > 0)) {
> +		max_iter--;

This is difficult to read due to unlucky indentation.  I suggest to
rewrite like this:


	while (SCANMGR_STAT_ACTIVE_GET(scanmgr_status) ||
	       (SCANMGR_STAT_WFIFOCNT_GET(scanmgr_status) > 0)) {

or even

	while (SCANMGR_STAT_ACTIVE_GET(scanmgr_status) ||
	       (SCANMGR_STAT_WFIFOCNT_GET(scanmgr_status) > 0)
	      ) {

> +			scanmgr_status = readl(&scan_manager_base->stat);
> +		else
> +			return SCAN_MGR_IO_SCAN_ENGINE_STATUS_ACTIVE;
> +	}
> +	return SCAN_MGR_IO_SCAN_ENGINE_STATUS_IDLE;
> +}

You decrement the parameter "max_iter" in the loop - I guess this was
intended to make sure this is not an endless loop.  But there is no
test anywhere for it becoming 0 or so - which makes the parameter
unused.  I guess this needs fixing?

> +	if (SCAN_MGR_IO_SCAN_ENGINE_STATUS_IDLE
> +		!= scan_mgr_io_scan_chain_engine_is_idle(
> +		MAX_WAITING_DELAY_IO_SCAN_ENGINE))
> +		return 1;

See above.  This is not readable.

Blame yourself that you have such problems - why do you cose so long
identifiers as "SCAN_MGR_IO_SCAN_ENGINE_STATUS_IDLE" or
"scan_mgr_io_scan_chain_engine_is_idle" - when you cannot put two
names on a single line without breaking the 80 character limit, you
are doing something wrong.

> +		if (SCAN_MGR_IO_SCAN_ENGINE_STATUS_IDLE
> +			!= scan_mgr_io_scan_chain_engine_is_idle(
> +			MAX_WAITING_DELAY_IO_SCAN_ENGINE))
> +			goto error;

Same here.  Fix the indentation.

Even better, fix these unwieldy names.

> +	/* Final TDI_TDO packet if any */
> +	if (0 != io_scan_chain_data_residual) {

Actually I would prefer if you wrote all this in the natural order,
i. e. as

	if (io_scan_chain_data_residual != 0)

With != it's just a nuisance, but with < or > it requires the reader
to think backwards which makes the code hard to read and increases the
error rate.

> +		tdi_tdo_header	= TDI_TDO_HEADER_FIRST_BYTE |
> +			((io_scan_chain_data_residual - 1)
> +			<< TDI_TDO_HEADER_SECOND_BYTE_SHIFT);

Again, this is hard to read.  Please fix globally.  Do not start a new
line with an operation - put this in the previous line where it serves
as a mark that the line will need a continuation.

> +		for (i = 0; i < io_program_iter; i++)
> +			/*
> +			 * write remaining scan chain data into scan
> +			 * manager WFIFO with 4 bytes write
> +			*/
> +			writel(iocsr_scan_chain[index + i],
> +				&scan_manager_base->fifo_quad_byte);

Incorrect multiline comment style, fix the last line.

Second, even if checkpatch lats this pass, we don't.  This is a
multi-line statement (even without the comment), and as such it
requires braces.

> +		if (IO_SCAN_CHAIN_PAYLOAD_24BIT < residual)
> +			/*
> +			 * write the last 4B scan chain data
> +			 * into scan manager WFIFO
> +			 */
> +			writel(iocsr_scan_chain[index],
> +				&scan_manager_base->fifo_quad_byte);
> +		else {

Braces needed - first because it is multiline, second because the esle
branch has braces.

> +			for (i = 0; i < residual; i += 8)
> +				writel(((iocsr_scan_chain[index] >> i)
> +					& IO_SCAN_CHAIN_BYTE_MASK),
> +					&scan_manager_base->fifo_single_byte);

Braces needed.

> +		if (SCAN_MGR_IO_SCAN_ENGINE_STATUS_IDLE
> +			!=  scan_mgr_io_scan_chain_engine_is_idle(
> +			MAX_WAITING_DELAY_IO_SCAN_ENGINE))
> +			goto error;

fix indentation.,


> +#define IO_SCAN_CHAIN_128BIT_SHIFT		(7)

Remove parens around simple values - please fix globally.

> +++ b/board/altera/socfpga/iocsr_config.h
> @@ -0,0 +1,17 @@
> +/*
> + * Copyright Altera Corporation (C) 2012-2014. All rights reserved
> + *
> + * SPDX-License-Identifier:    BSD-3-Clause

Why is this not under GPL?  New code that gets added to U-Boot shall
be GPL-2.0+


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
Every solution breeds new problems.