Message-ID: <SN1PR11MB04469F92EE4779B2D3BB0A0BEC850@SN1PR11MB0446.namprd11.prod.outlook.com>
Date:   Fri, 11 Jan 2019 04:04:26 +0000
From:   <Tristram.Ha@...rochip.com>
To:     <marex@...x.de>
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/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.

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.

Powered by blists - more mailing lists