regmap: mmio: regression in pre-v4.6-rc1

DISCUSSION Alexander Stein 2 participants 7 messages Mar 23, 2016

Share & tools

Alexander Stein

author

2016-03-23 08:49 UTC #0 lore raw

Hi,

I'm currently trying to get PCIe working on LS1021A (little-endian ARM). For link-detection I need access to a syscon perpheral (SCFG) which is attched to CPU as big-endian. The corresponding DT part is:

		scfg: scfg@1570000 {
			compatible = "fsl,ls1021a-scfg", "syscon";
			reg = <0x0 0x1570000 0x0 0x10000>;
			big-endian;
		};

Based on current linus's master (a24e3d414e59ac765, "Merge branch 'akpm' (patches from Andrew)") I noticed the access is actually done as little-endian. I could track it down to commit 922a9f936e40001f ("regmap: mmio: Convert to regmap_bus and fix accessor usage"). Reverting it, the access is fine now and I get my PCIe link.

Best regards,
Alexander

Alexander Stein

author

2016-03-23 09:43 UTC #1 lore raw

Quoted 11 lines · Alexander Stein

I'm currently trying to get PCIe working on LS1021A (little-endian ARM). For link-detection I need access to a syscon perpheral (SCFG) which is attched to CPU as big-endian. The corresponding DT part is:
		scfg: scfg@1570000 {
			compatible = "fsl,ls1021a-scfg", "syscon";
			reg = <0x0 0x1570000 0x0 0x10000>;
			big-endian;
		};
Based on current linus's master (a24e3d414e59ac765, "Merge branch 'akpm' (patches from Andrew)") I noticed the access is actually done as little-endian. I could track it down to commit 922a9f936e40001f ("regmap: mmio: Convert to regmap_bus and fix accessor usage"). Reverting it, the access is fine now and I get my PCIe link.

Just for the records, this also affects spi-fsl-dspi which is also attached in big-endian.

Best regards,
Alexander

Mark Brown

2016-03-23 10:34 UTC #2 lore raw

In reply to Alexander Stein

On Wed, Mar 23, 2016 at 09:48:42AM +0100, Alexander Stein wrote:

Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.

Quoted 3 lines

I'm currently trying to get PCIe working on LS1021A (little-endian ARM). For link-detection I need access to a syscon perpheral (SCFG) which is attched to CPU as big-endian.

Are you *sure* that this is actually big endian? Are you basing this on documentation or on what happened to work for you in the past.

Quoted 5 lines

Based on current linus's master (a24e3d414e59ac765, "Merge branch 'akpm' (patches from Andrew)") I noticed the access is actually done as little-endian. I could track it down to commit 922a9f936e40001f ("regmap: mmio: Convert to regmap_bus and fix accessor usage"). Reverting it, the access is fine now and I get my PCIe link.

Have you tried tracing through the code to see what ends up happening to the I/O? It should come out using your architecture's big endian accessors.

Alexander Stein

author

2016-03-23 11:16 UTC #3 lore raw

Quoted 6 lines · Mark Brown

Quoted 3 lines
I'm currently trying to get PCIe working on LS1021A (little-endian ARM). For link-detection I need access to a syscon perpheral (SCFG) which is attched to CPU as big-endian.
Are you *sure* that this is actually big endian? Are you basing this on documentation or on what happened to work for you in the past.

Please refer to QorIQ LS1021A Reference Manual (REV 0) table 2.2 (CCSR block base address map) which states that this peripheral (among _most_ but not all) requires byte swapping. Same for DSPI.
Yeah, it sounds strange.

Quoted 9 lines

Quoted 5 lines
Based on current linus's master (a24e3d414e59ac765, "Merge branch 'akpm' (patches from Andrew)") I noticed the access is actually done as little-endian. I could track it down to commit 922a9f936e40001f ("regmap: mmio: Convert to regmap_bus and fix accessor usage"). Reverting it, the access is fine now and I get my PCIe link.
Have you tried tracing through the code to see what ends up happening to the I/O? It should come out using your architecture's big endian accessors.

In regmap_mmio_gen_context ctx->reg_read is set to regmap_mmio_read32le and ctx->reg_write to regmap_mmio_write32le respectively.

I noticed that before that change map->reg_read = _regmap_bus_read and map-

Quoted 1 line

reg_write = _regmap_bus_raw_write. After that change it is map->reg_read =

_regmap_bus_reg_read resp. map->reg_write = _regmap_bus_reg_write. I hope this description is not that confusing.

Best regards,
Alexander

Mark Brown

2016-03-23 11:39 UTC #4 lore raw

Quoted 9 lines · Alexander Stein

Quoted 2 lines · Mark Brown
Are you *sure* that this is actually big endian? Are you basing this on documentation or on what happened to work for you in the past.
Please refer to QorIQ LS1021A Reference Manual (REV 0) table 2.2 (CCSR block base address map) which states that this peripheral (among _most_ but not all) requires byte swapping. Same for DSPI.
Yeah, it sounds strange.

I don't have that document.

Quoted 6 lines

Quoted 3 lines
Have you tried tracing through the code to see what ends up happening to the I/O? It should come out using your architecture's big endian accessors.
In regmap_mmio_gen_context ctx->reg_read is set to regmap_mmio_read32le and ctx->reg_write to regmap_mmio_write32le respectively.

So how does that happen then? We set these values if the bus is default, little or native endian but if it's big endian we go into a completely different case...

Alexander Stein

author

2016-03-23 11:51 UTC #5 lore raw

Quoted 6 lines · Mark Brown

Quoted 4 lines · Alexander Stein
Quoted 2 lines · Mark Brown
Are you *sure* that this is actually big endian? Are you basing this on documentation or on what happened to work for you in the past.

Quoted 6 lines

Quoted 4 lines
Please refer to QorIQ LS1021A Reference Manual (REV 0) table 2.2 (CCSR block base address map) which states that this peripheral (among _most_ but not all) requires byte swapping. Same for DSPI.
Yeah, it sounds strange.
I don't have that document.

Nothing wrong with that, I just wanted to state where it is actually documented.

Quoted 10 lines

Quoted 6 lines
Quoted 3 lines
Have you tried tracing through the code to see what ends up happening to the I/O? It should come out using your architecture's big endian accessors.
In regmap_mmio_gen_context ctx->reg_read is set to regmap_mmio_read32le and ctx->reg_write to regmap_mmio_write32le respectively.
So how does that happen then? We set these values if the bus is default, little or native endian but if it's big endian we go into a completely different case...

Well, in regmap_mmio_gen_context config->reg_format_endian is still set to REGMAP_ENDIAN_DEFAULT. of_syscon_register sets config.val_format_endian (notice val_ instead of reg_) depending on "big-endian" (or "little-endian") property. I'm kinda confused regarding reg_format_endian and val_format_endian. Dunno what should be set in which way.

Best regards,
Alexander

Mark Brown

2016-03-23 12:05 UTC #6 lore raw

On Wed, Mar 23, 2016 at 12:50:53PM +0100, Alexander Stein wrote:

As I said previously please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.

Quoted 7 lines

Quoted 6 lines
Quoted 2 lines
In regmap_mmio_gen_context ctx->reg_read is set to regmap_mmio_read32le and ctx->reg_write to regmap_mmio_write32le respectively.
So how does that happen then? We set these values if the bus is default, little or native endian but if it's big endian we go into a completely different case...

Quoted 6 lines

Well, in regmap_mmio_gen_context config->reg_format_endian is still set to REGMAP_ENDIAN_DEFAULT. of_syscon_register sets config.val_format_endian (notice val_ instead of reg_) depending on "big-endian" (or "little-endian") property. I'm kinda confused regarding reg_format_endian and val_format_endian. Dunno what should be set in which way.

Ah, I see. That's definitely broken - if we're changing the value format we should be checking the value.