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

Chin Liang See clsee at altera.com
Thu Feb 27 17:03:39 CET 2014


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-clsee at 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 at altera.com>
> > Cc: Dinh Nguyen <dinguyen at altera.com>
> > Cc: Wolfgang Denk <wd at denx.de>
> > CC: Pavel Machek <pavel at denx.de>
> > Cc: Tom Rini <trini at ti.com>
> > Cc: Albert Aribaud <albert.u.boot at 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
> 




More information about the U-Boot mailing list