diff mbox series

[RFT,V2,09/10] net: dsa: microchip: Factor out regmap config generation into common header

Message ID 20181221015841.6992-10-marex@denx.de
State RFC, archived
Delegated to: David Miller
Headers show
Series net: dsa: microchip: Convert to regmap | expand

Commit Message

Marek Vasut Dec. 21, 2018, 1:58 a.m. UTC 
The regmap config tables are rather similar for various generations of
the KSZ8xxx/KSZ9xxx switches. Introduce a macro which allows generating
those tables without duplication. Note that $regalign parameter is not
used right now, but will be used in KSZ87xx series switches.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <Woojung.Huh@microchip.com>
---
V2: New patch
---
 drivers/net/dsa/microchip/ksz9477_spi.c | 28 ++---------------------
 drivers/net/dsa/microchip/ksz_common.h  | 30 +++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 26 deletions(-)

Comments

Tristram.Ha@microchip.com Dec. 21, 2018, 4:16 a.m. UTC | #1 
> +	{								\
> +		.val_bits = (width),					\
> +		.reg_stride = (width) / 8,				\
> +		.reg_bits = (regbits) + (regalign),			\
> +		.pad_bits = (regpad),					\
> +		.max_register = 0xF00,					\
> +		.cache_type = REGCACHE_NONE,
> 	\
> +		.read_flag_mask =					\
> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
> regpad), \
> +		.write_flag_mask =					\
> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
> regpad), \
> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
> 	\
> +		.val_format_endian = REGMAP_ENDIAN_BIG
> 	\
> +	}

max_registers for KSZ9477 should be 0x8000.

I found that the SPI access works with these settings:
reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask = KS_SPIOP_RD << 5.

I am using this in another driver running in 4.9.  Somehow the 8-bit write causes the
hardware to be in a wrong state.  This is a quick change so there may be bugs in the
driver.

As expected the access speed is a few microseconds slower than before.
Marek Vasut Dec. 21, 2018, 5:23 a.m. UTC | #2 
On 12/21/2018 05:16 AM, Tristram.Ha@microchip.com wrote:
>> +	{								\
>> +		.val_bits = (width),					\
>> +		.reg_stride = (width) / 8,				\
>> +		.reg_bits = (regbits) + (regalign),			\
>> +		.pad_bits = (regpad),					\
>> +		.max_register = 0xF00,					\
>> +		.cache_type = REGCACHE_NONE,
>> 	\
>> +		.read_flag_mask =					\
>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
>> regpad), \
>> +		.write_flag_mask =					\
>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
>> regpad), \
>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
>> 	\
>> +		.val_format_endian = REGMAP_ENDIAN_BIG
>> 	\
>> +	}
> 
> max_registers for KSZ9477 should be 0x8000.
> 
> I found that the SPI access works with these settings:
> reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask = KS_SPIOP_RD << 5.

This is wrong, val_bits must match the register width (8 for 8bit
regmap, 16 for 16bit regmap etc). Anyway, can you prepare a short diff
to give me an idea what needs to be changed ?

> I am using this in another driver running in 4.9.  Somehow the 8-bit write causes the
> hardware to be in a wrong state.  This is a quick change so there may be bugs in the
> driver.

Can you try this as-is on linux-next or net-next ?

> As expected the access speed is a few microseconds slower than before.

Why is it expected ?
Marek Vasut Jan. 5, 2019, 9:50 p.m. UTC | #3 
On 12/21/18 6:23 AM, Marek Vasut wrote:
> On 12/21/2018 05:16 AM, Tristram.Ha@microchip.com wrote:
>>> +	{								\
>>> +		.val_bits = (width),					\
>>> +		.reg_stride = (width) / 8,				\
>>> +		.reg_bits = (regbits) + (regalign),			\
>>> +		.pad_bits = (regpad),					\
>>> +		.max_register = 0xF00,					\
>>> +		.cache_type = REGCACHE_NONE,
>>> 	\
>>> +		.read_flag_mask =					\
>>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
>>> regpad), \
>>> +		.write_flag_mask =					\
>>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
>>> regpad), \
>>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
>>> 	\
>>> +		.val_format_endian = REGMAP_ENDIAN_BIG
>>> 	\
>>> +	}
>>
>> max_registers for KSZ9477 should be 0x8000.
>>
>> I found that the SPI access works with these settings:
>> reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask = KS_SPIOP_RD << 5.
> 
> This is wrong, val_bits must match the register width (8 for 8bit
> regmap, 16 for 16bit regmap etc). Anyway, can you prepare a short diff
> to give me an idea what needs to be changed ?
> 
>> I am using this in another driver running in 4.9.  Somehow the 8-bit write causes the
>> hardware to be in a wrong state.  This is a quick change so there may be bugs in the
>> driver.
> 
> Can you try this as-is on linux-next or net-next ?
> 
>> As expected the access speed is a few microseconds slower than before.
> 
> Why is it expected ?

Bump ?
Tristram.Ha@microchip.com Jan. 9, 2019, 7:08 p.m. UTC | #4 
> >>> +	{								\
> >>> +		.val_bits = (width),					\
> >>> +		.reg_stride = (width) / 8,				\
> >>> +		.reg_bits = (regbits) + (regalign),			\
> >>> +		.pad_bits = (regpad),					\
> >>> +		.max_register = 0xF00,					\
> >>> +		.cache_type = REGCACHE_NONE,
> >>> 	\
> >>> +		.read_flag_mask =					\
> >>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
> >>> regpad), \
> >>> +		.write_flag_mask =					\
> >>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
> >>> regpad), \
> >>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
> >>> 	\
> >>> +		.val_format_endian = REGMAP_ENDIAN_BIG
> >>> 	\
> >>> +	}
> >>
> >> max_registers for KSZ9477 should be 0x8000.
> >>
> >> I found that the SPI access works with these settings:
> >> reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask =
> KS_SPIOP_RD << 5.
> >
> > This is wrong, val_bits must match the register width (8 for 8bit
> > regmap, 16 for 16bit regmap etc). Anyway, can you prepare a short diff
> > to give me an idea what needs to be changed ?
> >
> 
> Bump ?
> 

This is the regmap_config I used in Linux 4.9:

	.reg_bits		= SPI_REGMAP_REG,
	.val_bits		= SPI_REGMAP_VAL,
	.pad_bits		= SPI_REGMAP_PAD,
	.read_flag_mask	= KS_SPIOP_RD << SPI_REGMAP_MASK_S,
	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,
	.reg_format_endian	= REGMAP_ENDIAN_BIG,
	.val_format_endian	= REGMAP_ENDIAN_BIG,

For KSZ9477:

SPI_CMD_LEN		4
SPI_REGMAP_PAD	SPI_TURNAROUND_SHIFT
SPI_REGMAP_VAL	8
SPI_REGMAP_REG	\
	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
SPI_REGMAP_MASK_S	\
	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))

For KSZ8795:

SPI_CMD_LEN		2
SPI_REGMAP_PAD	SPI_TURNAROUND_S
SPI_REGMAP_VAL	8
SPI_REGMAP_REG	\
	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_S)
SPI_REGMAP_MASK_S	\
	(SPI_ADDR_S + SPI_TURNAROUND_S - \
	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))

So the differences between KSZ9477 and KSZ8795 are SPI_CMD_LEN, SPI_ADDR_S, and SPI_TURNAROUND_S.

KSZ9477:

.reg_bits = 32 - 5 = 27
.val_bits = 8
.pad_bits = 5
.read_flag_mask = KS_SPIOP_RD << 5,

KSZ8795:

.reg_bits = 16 - 1 = 15
.val_bits = 8
.pad_bits = 1
.read_flag_mask = KS_SPIOP_RD << 5,

The regmap.c code uses reg_bits + reg_shift (which comes from pad_bits) to decide whether the register space is 16-bit or 32-bit.  The value space is always 8-bit.

The shift for _flag_mask turns out to be the same for both KSZ9477 and KSZ8795.
Tristram.Ha@microchip.com Jan. 9, 2019, 7:58 p.m. UTC | #5 
> This is the regmap_config I used in Linux 4.9:
> 
> 	.reg_bits		= SPI_REGMAP_REG,
> 	.val_bits		= SPI_REGMAP_VAL,
> 	.pad_bits		= SPI_REGMAP_PAD,
> 	.read_flag_mask	= KS_SPIOP_RD << SPI_REGMAP_MASK_S,
> 	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,
> 	.reg_format_endian	= REGMAP_ENDIAN_BIG,
> 	.val_format_endian	= REGMAP_ENDIAN_BIG,
> 
> For KSZ9477:
> 
> SPI_CMD_LEN		4
> SPI_REGMAP_PAD	SPI_TURNAROUND_SHIFT
> SPI_REGMAP_VAL	8
> SPI_REGMAP_REG	\
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
> SPI_REGMAP_MASK_S	\
> 	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> 
> For KSZ8795:
> 
> SPI_CMD_LEN		2
> SPI_REGMAP_PAD	SPI_TURNAROUND_S
> SPI_REGMAP_VAL	8
> SPI_REGMAP_REG	\
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_S)
> SPI_REGMAP_MASK_S	\
> 	(SPI_ADDR_S + SPI_TURNAROUND_S - \
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> 
> So the differences between KSZ9477 and KSZ8795 are SPI_CMD_LEN,
> SPI_ADDR_S, and SPI_TURNAROUND_S.
> 
> KSZ9477:
> 
> .reg_bits = 32 - 5 = 27
> .val_bits = 8
> .pad_bits = 5
> .read_flag_mask = KS_SPIOP_RD << 5,
> 
> KSZ8795:
> 
> .reg_bits = 16 - 1 = 15
> .val_bits = 8
> .pad_bits = 1
> .read_flag_mask = KS_SPIOP_RD << 5,
> 
> The regmap.c code uses reg_bits + reg_shift (which comes from pad_bits) to
> decide whether the register space is 16-bit or 32-bit.  The value space is
> always 8-bit.
> 
> The shift for _flag_mask turns out to be the same for both KSZ9477 and
> KSZ8795.

I just looked at your regmap code and you use 3 regmap pointers for specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.  It has automatic register increment so that you can access arbitrary length of registers.  The use of 16-bit and 32-bit accesses makes access efficient if it makes sense.

Most older switches define registers in 8-bit.  Exceptions are the default VID and indirect access.

A specific switch mostly defines registers in 16-bit because it shares the core design with an Ethernet controller.

KSZ9477 is the newer designed switch and it gets some designs from older switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register definitions.

In my code I just use regmap_bulk_read and regmap_bulk_write and still use the old spi access functions for specific 8-bit, 16-bit, and 32-bit accesses.

We can combine the logic of ksz_spi_read8 and others into ksz_read8 and so they can be used for both SPI and I2C accesses.
Marek Vasut Jan. 9, 2019, 9:56 p.m. UTC | #6 
On 1/9/19 8:08 PM, Tristram.Ha@microchip.com wrote:
>>>>> +	{								\
>>>>> +		.val_bits = (width),					\
>>>>> +		.reg_stride = (width) / 8,				\
>>>>> +		.reg_bits = (regbits) + (regalign),			\
>>>>> +		.pad_bits = (regpad),					\
>>>>> +		.max_register = 0xF00,					\
>>>>> +		.cache_type = REGCACHE_NONE,
>>>>> 	\
>>>>> +		.read_flag_mask =					\
>>>>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits,
>>>>> regpad), \
>>>>> +		.write_flag_mask =					\
>>>>> +			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits,
>>>>> regpad), \
>>>>> +		.reg_format_endian = REGMAP_ENDIAN_BIG,
>>>>> 	\
>>>>> +		.val_format_endian = REGMAP_ENDIAN_BIG
>>>>> 	\
>>>>> +	}
>>>>
>>>> max_registers for KSZ9477 should be 0x8000.
>>>>
>>>> I found that the SPI access works with these settings:
>>>> reg_bits = 32 - 5, val_bits = 8, pad_bits = 5, read_flag_mask =
>> KS_SPIOP_RD << 5.
>>>
>>> This is wrong, val_bits must match the register width (8 for 8bit
>>> regmap, 16 for 16bit regmap etc). Anyway, can you prepare a short diff
>>> to give me an idea what needs to be changed ?
>>>
>>
>> Bump ?
>>
> 
> This is the regmap_config I used in Linux 4.9:
> 
> 	.reg_bits		= SPI_REGMAP_REG,
> 	.val_bits		= SPI_REGMAP_VAL,
> 	.pad_bits		= SPI_REGMAP_PAD,
> 	.read_flag_mask	= KS_SPIOP_RD << SPI_REGMAP_MASK_S,
> 	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,
> 	.reg_format_endian	= REGMAP_ENDIAN_BIG,
> 	.val_format_endian	= REGMAP_ENDIAN_BIG,
> 
> For KSZ9477:
> 
> SPI_CMD_LEN		4
> SPI_REGMAP_PAD	SPI_TURNAROUND_SHIFT
> SPI_REGMAP_VAL	8
> SPI_REGMAP_REG	\
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
> SPI_REGMAP_MASK_S	\
> 	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> 
> For KSZ8795:
> 
> SPI_CMD_LEN		2
> SPI_REGMAP_PAD	SPI_TURNAROUND_S
> SPI_REGMAP_VAL	8
> SPI_REGMAP_REG	\
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_S)
> SPI_REGMAP_MASK_S	\
> 	(SPI_ADDR_S + SPI_TURNAROUND_S - \
> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> 
> So the differences between KSZ9477 and KSZ8795 are SPI_CMD_LEN, SPI_ADDR_S, and SPI_TURNAROUND_S.
> 
> KSZ9477:
> 
> .reg_bits = 32 - 5 = 27
> .val_bits = 8
> .pad_bits = 5
> .read_flag_mask = KS_SPIOP_RD << 5,
> 
> KSZ8795:
> 
> .reg_bits = 16 - 1 = 15
> .val_bits = 8
> .pad_bits = 1
> .read_flag_mask = KS_SPIOP_RD << 5,
> 
> The regmap.c code uses reg_bits + reg_shift (which comes from pad_bits) to decide whether the register space is 16-bit or 32-bit.  The value space is always 8-bit.
> 
> The shift for _flag_mask turns out to be the same for both KSZ9477 and KSZ8795.

Can you prepare a patch ? One which can apply on top of this series , so
we get the authorship information and stuff ?

Also, can you test this on net-next or next ?
Marek Vasut Jan. 9, 2019, 9:59 p.m. UTC | #7 
On 1/9/19 8:58 PM, Tristram.Ha@microchip.com wrote:
>> This is the regmap_config I used in Linux 4.9:
>>
>> 	.reg_bits		= SPI_REGMAP_REG,
>> 	.val_bits		= SPI_REGMAP_VAL,
>> 	.pad_bits		= SPI_REGMAP_PAD,
>> 	.read_flag_mask	= KS_SPIOP_RD << SPI_REGMAP_MASK_S,
>> 	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,
>> 	.reg_format_endian	= REGMAP_ENDIAN_BIG,
>> 	.val_format_endian	= REGMAP_ENDIAN_BIG,
>>
>> For KSZ9477:
>>
>> SPI_CMD_LEN		4
>> SPI_REGMAP_PAD	SPI_TURNAROUND_SHIFT
>> SPI_REGMAP_VAL	8
>> SPI_REGMAP_REG	\
>> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
>> SPI_REGMAP_MASK_S	\
>> 	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
>> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
>>
>> For KSZ8795:
>>
>> SPI_CMD_LEN		2
>> SPI_REGMAP_PAD	SPI_TURNAROUND_S
>> SPI_REGMAP_VAL	8
>> SPI_REGMAP_REG	\
>> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_S)
>> SPI_REGMAP_MASK_S	\
>> 	(SPI_ADDR_S + SPI_TURNAROUND_S - \
>> 	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
>>
>> So the differences between KSZ9477 and KSZ8795 are SPI_CMD_LEN,
>> SPI_ADDR_S, and SPI_TURNAROUND_S.
>>
>> KSZ9477:
>>
>> .reg_bits = 32 - 5 = 27
>> .val_bits = 8
>> .pad_bits = 5
>> .read_flag_mask = KS_SPIOP_RD << 5,
>>
>> KSZ8795:
>>
>> .reg_bits = 16 - 1 = 15
>> .val_bits = 8
>> .pad_bits = 1
>> .read_flag_mask = KS_SPIOP_RD << 5,
>>
>> The regmap.c code uses reg_bits + reg_shift (which comes from pad_bits) to
>> decide whether the register space is 16-bit or 32-bit.  The value space is
>> always 8-bit.
>>
>> The shift for _flag_mask turns out to be the same for both KSZ9477 and
>> KSZ8795.
> 
> I just looked at your regmap code and you use 3 regmap pointers for specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.  It has automatic register increment so that you can access arbitrary length of registers.  The use of 16-bit and 32-bit accesses makes access efficient if it makes sense.

Right, that's what happens here.

> Most older switches define registers in 8-bit.  Exceptions are the default VID and indirect access.
> 
> A specific switch mostly defines registers in 16-bit because it shares the core design with an Ethernet controller.
> 
> KSZ9477 is the newer designed switch and it gets some designs from older switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register definitions.

Right, that's quite horrible.

> In my code I just use regmap_bulk_read and regmap_bulk_write and still use the old spi access functions for specific 8-bit, 16-bit, and 32-bit accesses.

Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
stick to one, regmap.

> We can combine the logic of ksz_spi_read8 and others into ksz_read8 and so they can be used for both SPI and I2C accesses.

You can just use regmap_*() accessors and regmap will deal with i2c/spi
abstraction for you, that's the idea.
Tristram.Ha@microchip.com Jan. 10, 2019, 2:10 a.m. UTC | #8 
> > I just looked at your regmap code and you use 3 regmap pointers for
> specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.  It
> has automatic register increment so that you can access arbitrary length of
> registers.  The use of 16-bit and 32-bit accesses makes access efficient if it
> makes sense.
> 
> Right, that's what happens here.
> 
> > Most older switches define registers in 8-bit.  Exceptions are the default
> VID and indirect access.
> >
> > A specific switch mostly defines registers in 16-bit because it shares the
> core design with an Ethernet controller.
> >
> > KSZ9477 is the newer designed switch and it gets some designs from older
> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register
> definitions.
> 
> Right, that's quite horrible.
> 
> > In my code I just use regmap_bulk_read and regmap_bulk_write and still
> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit accesses.
> 
> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
> stick to one, regmap.
> 
> > We can combine the logic of ksz_spi_read8 and others into ksz_read8 and
> so they can be used for both SPI and I2C accesses.
> 
> You can just use regmap_*() accessors and regmap will deal with i2c/spi
> abstraction for you, that's the idea.
> 

What I meant is I use bulk_read as a base and modify it to access 16-bit and 32-bit instead of using regmap[1] and regmap[2].  We can keep regmap[2] for 32-bit access just for the regmap_update_bits function.

I intend to keep the 3 regmap pointers as a specific switch actually requires those specific accesses as it does not have automatic register increment.
Marek Vasut Jan. 10, 2019, 2:37 p.m. UTC | #9 
On 1/10/19 3:10 AM, Tristram.Ha@microchip.com wrote:
>>> I just looked at your regmap code and you use 3 regmap pointers for
>> specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.  It
>> has automatic register increment so that you can access arbitrary length of
>> registers.  The use of 16-bit and 32-bit accesses makes access efficient if it
>> makes sense.
>>
>> Right, that's what happens here.
>>
>>> Most older switches define registers in 8-bit.  Exceptions are the default
>> VID and indirect access.
>>>
>>> A specific switch mostly defines registers in 16-bit because it shares the
>> core design with an Ethernet controller.
>>>
>>> KSZ9477 is the newer designed switch and it gets some designs from older
>> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register
>> definitions.
>>
>> Right, that's quite horrible.
>>
>>> In my code I just use regmap_bulk_read and regmap_bulk_write and still
>> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit accesses.
>>
>> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
>> stick to one, regmap.
>>
>>> We can combine the logic of ksz_spi_read8 and others into ksz_read8 and
>> so they can be used for both SPI and I2C accesses.
>>
>> You can just use regmap_*() accessors and regmap will deal with i2c/spi
>> abstraction for you, that's the idea.
>>
> 
> What I meant is I use bulk_read as a base and modify it to access 16-bit and 32-bit instead of using regmap[1] and regmap[2].  We can keep regmap[2] for 32-bit access just for the regmap_update_bits function.
> 
> I intend to keep the 3 regmap pointers as a specific switch actually requires those specific accesses as it does not have automatic register increment.

I don't think the bulk read is a good idea due to register alignment.
You see, with bulk read, the user might try to read two bytes from the
middle of 4 byte register and I'm not sure how the HW would like that.
If we have 32bit regmap for 32bit registers, all behaves as expected.
Tristram.Ha@microchip.com Jan. 11, 2019, 4:04 a.m. UTC | #10 
> On 1/10/19 3:10 AM, Tristram.Ha@microchip.com wrote:
> >>> I just looked at your regmap code and you use 3 regmap pointers for
> >> specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.
> It
> >> has automatic register increment so that you can access arbitrary length of
> >> registers.  The use of 16-bit and 32-bit accesses makes access efficient if it
> >> makes sense.
> >>
> >> Right, that's what happens here.
> >>
> >>> Most older switches define registers in 8-bit.  Exceptions are the default
> >> VID and indirect access.
> >>>
> >>> A specific switch mostly defines registers in 16-bit because it shares the
> >> core design with an Ethernet controller.
> >>>
> >>> KSZ9477 is the newer designed switch and it gets some designs from
> older
> >> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register
> >> definitions.
> >>
> >> Right, that's quite horrible.
> >>
> >>> In my code I just use regmap_bulk_read and regmap_bulk_write and
> still
> >> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit
> accesses.
> >>
> >> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
> >> stick to one, regmap.
> >>
> >>> We can combine the logic of ksz_spi_read8 and others into ksz_read8
> and
> >> so they can be used for both SPI and I2C accesses.
> >>
> >> You can just use regmap_*() accessors and regmap will deal with i2c/spi
> >> abstraction for you, that's the idea.
> >>
> >
> > What I meant is I use bulk_read as a base and modify it to access 16-bit and
> 32-bit instead of using regmap[1] and regmap[2].  We can keep regmap[2]
> for 32-bit access just for the regmap_update_bits function.
> >
> > I intend to keep the 3 regmap pointers as a specific switch actually requires
> those specific accesses as it does not have automatic register increment.
> 
> I don't think the bulk read is a good idea due to register alignment.
> You see, with bulk read, the user might try to read two bytes from the
> middle of 4 byte register and I'm not sure how the HW would like that.
> If we have 32bit regmap for 32bit registers, all behaves as expected.

I just changed bulk_read to raw_read as it is more correct.

The switch access is 8-bit and so there is no restriction on accessing registers.
Of course some registers like PHY registers are better accessed in 16-bit, but you can write the high byte or low byte without problem.

Actually the hardware has some bugs that requires writing in 32-bit for some PHY related registers.  The driver has to make sure to write in the correct way to have the right result.

My point is the driver is the only one who is using these functions to write, so the developer does not try to write the register in the wrong way.

It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access functions does not work using the regmap mechanism without additional register manipulation, so we do not really need 3 regmap pointers.

One benefit of having 32-bit access is the register dump from debugfs can be much shorter than using 8-bit.
Marek Vasut Jan. 11, 2019, 4:49 a.m. UTC | #11 
On 1/11/19 5:04 AM, Tristram.Ha@microchip.com wrote:
>> On 1/10/19 3:10 AM, Tristram.Ha@microchip.com wrote:
>>>>> I just looked at your regmap code and you use 3 regmap pointers for
>>>> specific 8-bit, 16-bit, and 32-bit accesses.  The switch access is always 8-bit.
>> It
>>>> has automatic register increment so that you can access arbitrary length of
>>>> registers.  The use of 16-bit and 32-bit accesses makes access efficient if it
>>>> makes sense.
>>>>
>>>> Right, that's what happens here.
>>>>
>>>>> Most older switches define registers in 8-bit.  Exceptions are the default
>>>> VID and indirect access.
>>>>>
>>>>> A specific switch mostly defines registers in 16-bit because it shares the
>>>> core design with an Ethernet controller.
>>>>>
>>>>> KSZ9477 is the newer designed switch and it gets some designs from
>> older
>>>> switches and that is why it has a mix of 8-bit, 16-bit, and 32-bit register
>>>> definitions.
>>>>
>>>> Right, that's quite horrible.
>>>>
>>>>> In my code I just use regmap_bulk_read and regmap_bulk_write and
>> still
>>>> use the old spi access functions for specific 8-bit, 16-bit, and 32-bit
>> accesses.
>>>>
>>>> Let's not mix regmap and non-regmap accesses, that'd be a mess. Let's
>>>> stick to one, regmap.
>>>>
>>>>> We can combine the logic of ksz_spi_read8 and others into ksz_read8
>> and
>>>> so they can be used for both SPI and I2C accesses.
>>>>
>>>> You can just use regmap_*() accessors and regmap will deal with i2c/spi
>>>> abstraction for you, that's the idea.
>>>>
>>>
>>> What I meant is I use bulk_read as a base and modify it to access 16-bit and
>> 32-bit instead of using regmap[1] and regmap[2].  We can keep regmap[2]
>> for 32-bit access just for the regmap_update_bits function.
>>>
>>> I intend to keep the 3 regmap pointers as a specific switch actually requires
>> those specific accesses as it does not have automatic register increment.
>>
>> I don't think the bulk read is a good idea due to register alignment.
>> You see, with bulk read, the user might try to read two bytes from the
>> middle of 4 byte register and I'm not sure how the HW would like that.
>> If we have 32bit regmap for 32bit registers, all behaves as expected.
> 
> I just changed bulk_read to raw_read as it is more correct.
> 
> The switch access is 8-bit and so there is no restriction on accessing registers.
> Of course some registers like PHY registers are better accessed in 16-bit, but you can write the high byte or low byte without problem.
> 
> Actually the hardware has some bugs that requires writing in 32-bit for some PHY related registers.  The driver has to make sure to write in the correct way to have the right result.

OK, so there are clearly restrictions to what can be written and how.

> My point is the driver is the only one who is using these functions to write, so the developer does not try to write the register in the wrong way.
> 
> It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access functions does not work using the regmap mechanism without additional register manipulation, so we do not really need 3 regmap pointers.

Can you elaborate on this ?

> One benefit of having 32-bit access is the register dump from debugfs can be much shorter than using 8-bit.

You can constraint each regmap definition and have it allow only certain
types of accesses to a selected set of registers, so then your debugfs
regmap dump would match the register list in the datasheet. I didn't add
it into this initial series for simplicity's sake though.
Tristram.Ha@microchip.com Jan. 11, 2019, 6:56 p.m. UTC | #12 
> OK, so there are clearly restrictions to what can be written and how.
>

It is hardware bug.  You need to read those high PHY registers in 32-bit and modify them and write them back even though they are 16-bit.  The regular low PHY registers are not affected.

Another hardware bug with I2C access is an interrupt will be triggered whenever the PHY register write does not end at 32-bit boundary.  Right now that interrupt is not enabled, and this problem can be easily avoided by disabling a function.

These problems are for KSZ9477 only.
 
> > My point is the driver is the only one who is using these functions to write,
> so the developer does not try to write the register in the wrong way.
> >
> > It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access
> functions does not work using the regmap mechanism without additional
> register manipulation, so we do not really need 3 regmap pointers.
> 
> Can you elaborate on this ?
> 

This switch shares design with an Ethernet controller, and the register access uses byte enable.

There are 4 bits of byte enable indicating whether 1 byte, 2 bytes, 3 bytes, or 4 bytes are accessed.  Normally the 3 bytes option is not used.

The register address is then shifted right by 2.

0x40.1 -> 0x101
0x41.1 -> 0x102
0x42.1 -> 0x104
0x43.1 -> 0x108
0x40.2 -> 0x103
0x42.2 -> 0x10c
0x40.4 -> 0x10f
0x44.4 -> 0x11f

So the only option that works well with the regmap mechanism is 32-bit.

Problem is the register definitions are mostly 16-bit, while the switch also shares another switch design which uses 8-bit.
Marek Vasut Jan. 12, 2019, 12:11 a.m. UTC | #13 
On 1/11/19 7:56 PM, Tristram.Ha@microchip.com wrote:
>> OK, so there are clearly restrictions to what can be written and how.
>>
> 
> It is hardware bug.  You need to read those high PHY registers in 32-bit and modify them and write them back even though they are 16-bit.  The regular low PHY registers are not affected.
> 
> Another hardware bug with I2C access is an interrupt will be triggered whenever the PHY register write does not end at 32-bit boundary.  Right now that interrupt is not enabled, and this problem can be easily avoided by disabling a function.
> 
> These problems are for KSZ9477 only.

The regmap constraints can easily deal with this :-)

>>> My point is the driver is the only one who is using these functions to write,
>> so the developer does not try to write the register in the wrong way.
>>>
>>> It turns out the switch that requires exact 8-bit, 16-bit, and 32-bit access
>> functions does not work using the regmap mechanism without additional
>> register manipulation, so we do not really need 3 regmap pointers.
>>
>> Can you elaborate on this ?
>>
> 
> This switch shares design with an Ethernet controller, and the register access uses byte enable.
> 
> There are 4 bits of byte enable indicating whether 1 byte, 2 bytes, 3 bytes, or 4 bytes are accessed.  Normally the 3 bytes option is not used.
> 
> The register address is then shifted right by 2.
> 
> 0x40.1 -> 0x101
> 0x41.1 -> 0x102
> 0x42.1 -> 0x104
> 0x43.1 -> 0x108
> 0x40.2 -> 0x103
> 0x42.2 -> 0x10c
> 0x40.4 -> 0x10f
> 0x44.4 -> 0x11f
> 
> So the only option that works well with the regmap mechanism is 32-bit.
> 
> Problem is the register definitions are mostly 16-bit, while the switch also shares another switch design which uses 8-bit.

So we can have a regmap for each "chunk" of the address space, which as
the correct width, that's fine.

Can you try this series on net-next on the KSZ9477 , fix it up where
needed to make it work with that switch and send out the changes that
were needed ?

Thanks !
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index b440641f4898..29b51524cae9 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -14,36 +14,12 @@ 
 #include <linux/spi/spi.h>
 
 #include "ksz_priv.h"
+#include "ksz_common.h"
 
 #define SPI_ADDR_SHIFT			24
 #define SPI_TURNAROUND_SHIFT		5
 
-/* SPI frame opcodes */
-#define KS_SPIOP_RD			3
-#define KS_SPIOP_WR			2
-
-#define KS_SPIOP_FLAG_MASK(opcode)		\
-	cpu_to_be16((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
-
-#define KSZ_REGMAP_COMMON(width)					\
-	{								\
-		.val_bits = (width),					\
-		.reg_stride = (width) / 8,				\
-		.reg_bits = SPI_ADDR_SHIFT,				\
-		.pad_bits = SPI_TURNAROUND_SHIFT,			\
-		.max_register = 0xF00,					\
-		.cache_type = REGCACHE_NONE,				\
-		.read_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_RD),	\
-		.write_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_WR),	\
-		.reg_format_endian = REGMAP_ENDIAN_BIG,			\
-		.val_format_endian = REGMAP_ENDIAN_BIG			\
-	}
-
-static const struct regmap_config ksz9477_regmap_config[] = {
-	KSZ_REGMAP_COMMON(8),
-	KSZ_REGMAP_COMMON(16),
-	KSZ_REGMAP_COMMON(32),
-};
+KSZ_REGMAP_TABLE(ksz9477, SPI_ADDR_SHIFT, SPI_TURNAROUND_SHIFT, 0);
 
 static int ksz9477_spi_probe(struct spi_device *spi)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 903e3e39bfd4..4d30a67c14a3 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -114,4 +114,34 @@  static inline void ksz_pwrite32(struct ksz_device *dev, int port, int offset,
 	ksz_write32(dev, dev->dev_ops->get_port_addr(port, offset), data);
 }
 
+/* Regmap tables generation */
+#define KSZ_SPI_OP_RD		3
+#define KSZ_SPI_OP_WR		2
+
+#define KSZ_SPI_OP_FLAG_MASK(opcode, regbits, regpad)			\
+	cpu_to_be16((opcode) << ((regbits) + (regpad)))
+
+#define KSZ_REGMAP_ENTRY(width, regbits, regpad, regalign)		\
+	{								\
+		.val_bits = (width),					\
+		.reg_stride = (width) / 8,				\
+		.reg_bits = (regbits) + (regalign),			\
+		.pad_bits = (regpad),					\
+		.max_register = 0xF00,					\
+		.cache_type = REGCACHE_NONE,				\
+		.read_flag_mask =					\
+			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_RD, regbits, regpad), \
+		.write_flag_mask =					\
+			KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, regbits, regpad), \
+		.reg_format_endian = REGMAP_ENDIAN_BIG,			\
+		.val_format_endian = REGMAP_ENDIAN_BIG			\
+	}
+
+#define KSZ_REGMAP_TABLE(ksz, regbits, regpad, regalign)		\
+	static const struct regmap_config ksz##_regmap_config[] = {	\
+		KSZ_REGMAP_ENTRY(8, (regbits), (regpad), (regalign)),	\
+		KSZ_REGMAP_ENTRY(16, (regbits), (regpad), (regalign)),	\
+		KSZ_REGMAP_ENTRY(32, (regbits), (regpad), (regalign)),	\
+	}
+
 #endif