socfpga: Adding Scan Manager driver

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 <[email protected]>

Cc Dinh Nguyen <[email protected]>

Cc Wolfgang Denk <[email protected]>

CC Pavel Machek <[email protected]>

Cc Tom Rini <[email protected]>

Cc Albert Aribaud <[email protected]>

---
Changes for v6
- Fixed various coding style issue
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          |  211 +++++++
 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 +

Quoted 22 lines · Chin Liang See

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 <[email protected]>

Cc Dinh Nguyen <[email protected]>

Cc Wolfgang Denk <[email protected]>

CC Pavel Machek <[email protected]>

Cc Tom Rini <[email protected]>

Cc Albert Aribaud <[email protected]>

---
Changes for v6
- Fixed various coding style issue
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

Quoted 8 lines

---
 arch/arm/cpu/armv7/socfpga/Makefile                |    2 +-
 arch/arm/cpu/armv7/socfpga/scan_manager.c          |  211 +++++++
 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 +

I still have problem with content of these two files.
In iocsr_config.c is ~600 lines which targets just one specific hardware design configuration. I can't see any reason why this should go to mainline and stay there. Because it brings no value.

I would recommend you just to define that arrays like this

const unsigned long iocsr_scan_chain0_table[];
const unsigned long iocsr_scan_chain0_table[];
...

+ in header

#define CONFIG_HPS_IOCSR_SCANCHAIN0_LENGTH        0
#define CONFIG_HPS_IOCSR_SCANCHAIN1_LENGTH        0
#define CONFIG_HPS_IOCSR_SCANCHAIN2_LENGTH        0
#define CONFIG_HPS_IOCSR_SCANCHAIN3_LENGTH        0

and write these 2 files by hand. Then your users will just replace them by hand for specific board/design.

Thanks,
Michal

Hi Michal,

Quoted 50 lines · Michal Simek

Quoted 22 lines · Chin Liang See

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 <[email protected]>

Cc Dinh Nguyen <[email protected]>

Cc Wolfgang Denk <[email protected]>

CC Pavel Machek <[email protected]>

Cc Tom Rini <[email protected]>

Cc Albert Aribaud <[email protected]>

---
Changes for v6
- Fixed various coding style issue
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

Quoted 8 lines

---
 arch/arm/cpu/armv7/socfpga/Makefile                |    2 +-
 arch/arm/cpu/armv7/socfpga/scan_manager.c          |  211 +++++++
 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 +

I still have problem with content of these two files.
In iocsr_config.c is ~600 lines which targets just one specific hardware design configuration. I can't see any reason why this should go to mainline and stay there. Because it brings no value.

I would recommend you just to define that arrays like this

const unsigned long iocsr_scan_chain0_table[];
const unsigned long iocsr_scan_chain0_table[];
...

+ in header

#define CONFIG_HPS_IOCSR_SCANCHAIN0_LENGTH        0
#define CONFIG_HPS_IOCSR_SCANCHAIN1_LENGTH        0
#define CONFIG_HPS_IOCSR_SCANCHAIN2_LENGTH        0
#define CONFIG_HPS_IOCSR_SCANCHAIN3_LENGTH        0

Quoted 2 lines

and write these 2 files by hand. Then your users will just replace them by hand for specific board/design.

Actually the intention is that user can pull the code from git and build it. We want to avoid any tools dependency here.

At same time, these files are located inside board folders. If user have different boards, they will have new set of folders here their own handoff files. From there, there won't the need to regenerate everytime.

Also I expect that you can change all pins for uarts/ethernets/spi/i2c/etc that's why there is no golden configuration for socfpga that's why it is better to keep it empty just to compile it.

Quoted 3 lines

At same time, these files are located inside board folders. If user have different boards, they will have new set of folders here their own handoff files. From there, there won't the need to regenerate everytime.

Please explain me one thing how many users will use this configuration? Especially these ~600 lines?

Hi!

Quoted 1 line

Also I expect that you can change all pins for

uarts/ethernets/spi/i2c/etc

Quoted 2 lines

that's why there is no golden configuration for socfpga that's why it is better to keep it empty just to compile it.

Well, there are some development boards around... and having u-boot able to boot on the devel board by default would be very very useful.

Quoted 2 lines

Quoted 2 lines

At same time, these files are located inside board folders. If user

have

Quoted 2 lines

Quoted 2 lines

different boards, they will have new set of folders here their own handoff files. From there, there won't the need to regenerate

everytime.

Quoted 3 lines

Please explain me one thing how many users will use this configuration? Especially these ~600 lines?

Pretty much everyone when they test-boot their system...?

Hi Pavel,

2014-03-05 17:13 GMT+01:00 Pavel Machek <[email protected]>:
Hi!

> Also I expect that you can change all pins for uarts/ethernets/spi/i2c/etc
> that's why there is no golden configuration for socfpga that's why
> it is better to keep it empty just to compile it.

Well, there are some development boards around... and having u-boot
able to boot on the devel board by default would be very very useful.

I believe you but the point is somewhere else if you can reach it by universal

target.

> > At same time, these files are located inside board folders. If user have
> > different boards, they will have new set of folders here their own
> > handoff files. From there, there won't the need to regenerate everytime.
>
> Please explain me one thing how many users will use this configuration?
> Especially these ~600 lines?

Pretty much everyone when they test-boot their system...?

Really? It means that all development boards/custom boards with socfpga

use the same configuration which seems to me weird.

If yes, then why these files are autogenerated if the same config can be use for
all boards?

Thanks,

Michal

Quoted 38 lines · Michal Simek

Hi Pavel,

2014-03-05 17:13 GMT+01:00 Pavel Machek <[email protected]>:
Hi!

> Also I expect that you can change all pins for uarts/ethernets/spi/i2c/etc
> that's why there is no golden configuration for socfpga that's why
> it is better to keep it empty just to compile it.

Well, there are some development boards around... and having u-boot
able to boot on the devel board by default would be very very useful.

I believe you but the point is somewhere else if you can reach it by universal

target.

> > At same time, these files are located inside board folders. If user have
> > different boards, they will have new set of folders here their own
> > handoff files. From there, there won't the need to regenerate everytime.
>
> Please explain me one thing how many users will use this configuration?
> Especially these ~600 lines?

Quoted 6 lines

Pretty much everyone when they test-boot their system...?

Really? It means that all development boards/custom boards with socfpga

I believe you confuse with arch and board. These files are located inside board. For board/altera/socfpga, its referring to Altera dev kit. When another vendor, example like Xilinx, fabricated a board with SOCFPGA chip, then it will have another folder called board/xilinx/socfpga.

Quoted 6 lines

use the same configuration which seems to me weird.

If yes, then why these files are autogenerated if the same config can be use for
all boards?

Probably an example will clear your doubt. When the board is fabricated, the pin mux used will be fixed. You won't need to change it until you fabricated a new board.

Quoted 46 lines · Chin Liang See

Quoted 44 lines · Michal Simek

Hi Pavel,

2014-03-05 17:13 GMT+01:00 Pavel Machek <[email protected]>:
Hi!

> Also I expect that you can change all pins for uarts/ethernets/spi/i2c/etc
> that's why there is no golden configuration for socfpga that's why
> it is better to keep it empty just to compile it.

Well, there are some development boards around... and having u-boot
able to boot on the devel board by default would be very very useful.

I believe you but the point is somewhere else if you can reach it by universal

target.

> > At same time, these files are located inside board folders. If user have
> > different boards, they will have new set of folders here their own
> > handoff files. From there, there won't the need to regenerate everytime.
>
> Please explain me one thing how many users will use this configuration?
> Especially these ~600 lines?

Pretty much everyone when they test-boot their system...?

Really? It means that all development boards/custom boards with socfpga

Quoted 5 lines

I believe you confuse with arch and board. These files are located inside board. For board/altera/socfpga, its referring to Altera dev kit. When another vendor, example like Xilinx, fabricated a board with SOCFPGA chip, then it will have another folder called board/xilinx/socfpga.

That explains a lot. Why isn't call devkit then?

Quoted 11 lines

Quoted 8 lines

use the same configuration which seems to me weird.

If yes, then why these files are autogenerated if the same config can be use for
all boards?

Probably an example will clear your doubt. When the board is fabricated, the pin mux used will be fixed. You won't need to change it until you fabricated a new board.

I understand that you want to have something which you can just compile and boot. But only pins are fixed not internal connection inside chip. Also I expect you have connectors on the board which you can use for add-on cards.
My doubts are just based on my experience with zynq that not everything what it is generated by tools is really needed to get board boot. Also boards have just fixed pins but not internal connections to these pins. I believe that this is just the same for your dev kit.
Also I hardly believe that you need all 600lines just to have something to boot.

Thanks,
Michal

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 <[email protected]>

Cc Dinh Nguyen <[email protected]>

Cc Wolfgang Denk <[email protected]>

CC Pavel Machek <[email protected]>

Cc Tom Rini <[email protected]>

Cc Albert Aribaud <[email protected]>

arch/arm/cpu/armv7/socfpga/scan_manager.c new file +7 -0

+static inline uint32_t 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 */

arch/arm/cpu/armv7/socfpga/scan_manager.c new file +6 -0

+/* Program HPS IO Scan Chain */
+uint32_t scan_mgr_io_scan_chain_prg(
+	uint32_t io_scan_chain_id,
+	uint32_t io_scan_chain_len_in_bits,
+	const uint32_t *iocsr_scan_chain)
+{

Hi!

Quoted 23 lines

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 <[email protected]>

Cc Dinh Nguyen <[email protected]>

Cc Wolfgang Denk <[email protected]>

CC Pavel Machek <[email protected]>

Cc Tom Rini <[email protected]>

Cc Albert Aribaud <[email protected]>

arch/arm/cpu/armv7/socfpga/scan_manager.c new file +13 -0

+static inline uint32_t 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--;
+		if (max_iter > 0)
+			scanmgr_status = readl(&scan_manager_base->stat);
+		else

arch/arm/cpu/armv7/socfpga/scan_manager.c line 39

Quoted 4 lines

arch/arm/cpu/armv7/socfpga/scan_manager.c new file +4 -0

+			return SCAN_MGR_STATUS_ACTIVE;
+	}
+	return SCAN_MGR_STATUS_IDLE;
+}

The function is named _is_idle, but returns 1 if it is _active_. I'd get rid of SCAN_MGR_STATUS_* defines (they make it harder to read) and reverse the logic...

Quoted 7 lines

arch/arm/cpu/armv7/socfpga/scan_manager.c new file +6 -0

+/* Program HPS IO Scan Chain */
+uint32_t scan_mgr_io_scan_chain_prg(
+	uint32_t io_scan_chain_id,
+	uint32_t io_scan_chain_len_in_bits,
+	const uint32_t *iocsr_scan_chain)
+{

...

arch/arm/cpu/armv7/socfpga/scan_manager.c line 67

Quoted 6 lines

arch/arm/cpu/armv7/socfpga/scan_manager.c new file +6 -0

+	/*
+	 * Check if the scan chain engine is inactive and the
+	 * WFIFO is empty before enabling the IO scan chain
+	 */
+	if (scan_chain_engine_is_idle(SCAN_MAX_DELAY) != SCAN_MGR_STATUS_IDLE)
+		return 1;

Maybe it would be better to return <0 numbers on error?

Should the function be static? Then it could have shorter name...

arch/arm/cpu/armv7/socfpga/scan_manager.c line 187

Quoted 1 line

arch/arm/cpu/armv7/socfpga/scan_manager.c new file +1 -0

+	/* Disable IO Scan chain when configuration done*/

I'd put space before end of comment.

arch/arm/include/asm/arch-socfpga/scan_manager.h line 85

Quoted 10 lines

arch/arm/include/asm/arch-socfpga/scan_manager.h new file +10 -0

+/*
+ * Program HPS IO Scan Chain
+ * io_scan_chain_id - IO scan chain ID
+ * io_scan_chain_len_in_bits - IO scan chain length in bits
+ * iocsr_scan_chain - IO scan chain table
+ */
+uint32_t scan_mgr_io_scan_chain_prg(
+	uint32_t io_scan_chain_id,
+	uint32_t io_scan_chain_len_in_bits,
+	const uint32_t *iocsr_scan_chain);

...and you can probably drop the prototype...

Thanks,
Pavel

I changed that to if (!scan_chain_engine_is_idle(SCAN_MAX_DELAY)) It should much better now

Removed. Thanks

Hi!

Quoted 2 lines

I changed that to if (!scan_chain_engine_is_idle(SCAN_MAX_DELAY)) It should much better now

Yes, thanks.

Quoted 1 line

Removed. Thanks

Thanks for all the changes, it looks better now, so you have my Acked-by in another thread.

Pavel