Dear Chin Liang See,
In message <1393022790-9296-1-git-send-email-cl...@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 <cl...@altera.com>
> Cc: Dinh Nguyen <dingu...@altera.com>
> Cc: Wolfgang Denk <w...@denx.de>
> CC: Pavel Machek <pa...@denx.de>
> Cc: Tom Rini <tr...@ti.com>
> Cc: Albert Aribaud <albert.u.b...@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: w...@denx.de
Every solution breeds new problems.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
