Message-ID: <71192366-4d12-ffe1-8665-1f0ef2f40d67@denx.de>
Date:   Fri, 11 Jan 2019 05:49:58 +0100
From:   Marek Vasut <marex@...x.de>
To:     Tristram.Ha@...rochip.com
Cc:     f.fainelli@...il.com, andrew@...n.ch, Woojung.Huh@...rochip.com,
        netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [RFT][PATCH V2 09/10] net: dsa: microchip: Factor out regmap
 config generation into common header

On 1/11/19 5:04 AM, Tristram.Ha@...rochip.com wrote:
>> On 1/10/19 3:10 AM, Tristram.Ha@...rochip.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.

-- 
Best regards,
Marek Vasut

Powered by blists - more mailing lists