Dear Wolfgang,
On Sat, 2014-02-22 at 09:42 +0100, ZY - wd wrote:
> 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)
> ) {
>
Fixed
> > + 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?
Actually this is checked at later line of code where the function will
exit if max_iter equal to zero or less.
>
> > + 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.
Actually the variables are generated by tools. But I agree that it
create unnecessary length which make the code hard to read. With that, I
fixed all these variables.
>
> > + 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.
Fixed
>
> > + /* 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.
Fixed by simplifying it to if (io_scan_chain_data_residual)
>
> > + 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.
Fixed
>
> > + 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.
Fixed
>
> 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.
Yup, good to note this. Fixed
>
> > + 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.
Fixed
>
> > + 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.
Fixed
>
> > + 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.,
Fixed
>
>
> > +#define IO_SCAN_CHAIN_128BIT_SHIFT (7)
>
> Remove parens around simple values - please fix globally.
Good to know this. Fixed
>
> > +++ 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+
Actually this is tools generated which will be consumed by non GPL
software too. This file doesn't contain any source but just array with
values. Hope this is ok.
>
>
> Best regards,
>
> Wolfgang Denk
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
