Message-ID: <8899fbf306051fa3cdd8bde92634de8134bce0fb.camel@svanheule.net>
Date:   Fri, 04 Jun 2021 20:16:53 +0200
From:   Sander Vanheule <sander@...nheule.net>
To:     Mark Brown <broonie@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

Hi Mark,

On Fri, 2021-06-04 at 18:25 +0100, Mark Brown wrote:
> On Thu, Jun 03, 2021 at 08:25:08PM +0200, Sander Vanheule wrote:
> 
> > 1. I've opted to just ignore any bits that lie beyond the allowed address
> >    width. Would it be cleaner to raise an error instead?
> 
> It doesn't *really* matter, enforcement is probably a bit better as it
> might catch bugs.

Agreed.

> > 2. Packing of the clause-45 register addresses (16 bit) and adressed device
> >    type (5 bit) is the same as in the mdio subsystem, resulting in a 21 bit
> >    address. Is this an appropriate way to pack this information into one
> >    address for the regmap interface?
> 
> Either that or pass the type in when instantiating the regmap (it sounds
> like it should be the same for all registers on the device?).

I think the 'device type' field should be viewed more as a paging index. A phy
will have PMA/PMD ("generic phy") features, but will likely also have status and
settings in the AN (auto-negotiation) page. I'm sure Andrew knows a lot more
about this than I do.

> 
> > The reasoning behind (1) is to allow the regmap user to use extra bits
> > as a way to virtually extend the address space. Note that this actually
> > results in aliasing. This can be useful if the data read from to a
> > register doesn't have the same meaning as the data written to it
> > (e.g. GPIO pin input and output data). An alternative solution to this
> > would be some concept of "aliased registers" in regmap -- like volatile or
> > precious registers.
> 
> I think these registers are in practice going to either need to be
> volatile (how most of them work at the minute) or otherwise handled in
> regmap (eg, the page support we've got).  Having two different names for
> the same register feels like it's asking for bugs if any of the higher
> level functions of regmap get used.

This is actually an issue with a GPIO chip that I'm trying to implement [1]. To
set an output, data is written to the register. To get an input value, data is
read from the register. Since a register contains data for 16 GPIO lines, a
regular read-modify-write could erroneously overwrite output values. A pin
outside of the RMW mask could've changed to an input, and may now be reading a
different value. The issue I was running into, had to do with a RMW not being
written because the pin value apparently hadn't changed.

To work around the issue, I created a "virtual page" by adding an extra bit [2].
On reads and writes, they are aliased to the same actual register. However, by
having two different addresses, one can be marked as "volatile and read-only",
while the other is "non-volatile and write-only". The latter allows for caching,
ensuring that a RMW will use the (correct) cached value to calculate the updated
register value.

I didn't use the existing paging mechanism for this, since (I think) then I
would need to specify a register that contains the page index. But as I don't
have an actual page register, I would have to specify another existing register
with an empty mask. This could lead to useless bus activity if I accidentally
chose a volatile register.

[1] https://lore.kernel.org/lkml/84352c93f27d7c8b7afea54f3932020e9cd97d02.camel@svanheule.net/
[2] https://lore.kernel.org/lkml/56fb027587fa067a249237ecaf40828cd508cdcc.1622713678.git.sander@svanheule.net/


Best,
Sander

Powered by blists - more mailing lists