linux-kernel - Re: [PATCH v12 02/38] ARM: ep93xx: add regmap aux

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <93d6379423ab2cdaee66ba8f7d37d3ed85157479.camel@maquefel.me>
Date: Thu, 12 Sep 2024 10:47:44 +0300
From: Nikita Shubin <nikita.shubin@...uefel.me>
To: Stephen Boyd <sboyd@...nel.org>, Andy Shevchenko
 <andriy.shevchenko@...ux.intel.com>, Linus Walleij
 <linus.walleij@...aro.org>,  Nikita Shubin via B4 Relay
 <devnull+nikita.shubin.maquefel.me@...nel.org>, Thierry Reding
 <thierry.reding@...il.com>, Uwe Kleine-König
 <u.kleine-koenig@...gutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 02/38] ARM: ep93xx: add regmap aux_dev

Hello Stephen !

On Wed, 2024-09-11 at 15:46 -0700, Stephen Boyd wrote:
> Quoting Nikita Shubin via B4 Relay (2024-09-09 01:10:27)
> > diff --git a/include/linux/soc/cirrus/ep93xx.h
> > b/include/linux/soc/cirrus/ep93xx.h
> > index 56fbe2dc59b1..a27447971302 100644
> > --- a/include/linux/soc/cirrus/ep93xx.h
> > +++ b/include/linux/soc/cirrus/ep93xx.h
> > @@ -3,6 +3,18 @@
> >  #define _SOC_EP93XX_H
> >  
> >  struct platform_device;
> > +struct regmap;
> > +struct spinlock_t;
> > +
> > +enum ep93xx_soc_model {
> > +       EP93XX_9301_SOC,
> > +       EP93XX_9307_SOC,
> > +       EP93XX_9312_SOC,
> > +};
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/container_of.h>
> >  
> >  #define EP93XX_CHIP_REV_D0     3
> >  #define EP93XX_CHIP_REV_D1     4
> > @@ -10,6 +22,20 @@ struct platform_device;
> >  #define EP93XX_CHIP_REV_E1     6
> >  #define EP93XX_CHIP_REV_E2     7
> >  
> > +struct ep93xx_regmap_adev {
> > +       struct auxiliary_device adev;
> > +       struct regmap *map;
> > +       void __iomem *base;
> > +       spinlock_t *lock;
> > +       void (*write)(struct regmap *map, spinlock_t *lock,
> > unsigned int reg,
> > +                     unsigned int val);
> > +       void (*update_bits)(struct regmap *map, spinlock_t *lock,
> > +                           unsigned int reg, unsigned int mask,
> > unsigned int val);
> > +};
> 
> Why can't the single 'struct regmap' be passed as the auxiliary
> device's
> platform_data? The lock could be put into the regmap locking routines
> and the write/update_bits would be standard regmap API calls. Doing
> that
> would make the auxiliary device driver simpler because it wouldn't
> depend on this struct.
> 
> The device name could encode the SoC number as well so the auxiliary
> device driver could match different names and do different things.

Unfortunately ep93xx sw locked register are trickier than they appear
at first glance:

```
+ * Logic safeguards are included to condition the control signals for
+ * power connection to the matrix to prevent part damage. In addition,
a
+ * software lock register is included that must be written with 0xAA
+ * before each register write to change the values of the four switch
+ * matrix control registers.
```

- only a few registers are sw locked, so we are not using lock's
everywhere only when it is really needed
- lock is cleared on any write to this register, so we must ensure we
are performing 2 write operations, first writing 0xaa, second the
protected register itself under a single lock
- luckily we can read without clearing the lock

That's why we have such a funny ep93xx_regmap_update_bits() with forced
update_bits.

It was discussed in detail a five iteration ago or so with Andy
Shevchenko, when he was reviewing clk and pinctrl (and thank you once
again Andy).