Add register definitions for components versions and report them
during probe.
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
drivers/mfd/zl3073x-core.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index f0d85f77a7a76..28f28d00da1cc 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -1,7 +1,9 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/array_size.h>
+#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <linux/cleanup.h>
#include <linux/dev_printk.h>
#include <linux/device.h>
#include <linux/export.h>
@@ -13,6 +15,14 @@
#include <net/devlink.h>
#include "zl3073x.h"
+/*
+ * Register Map Page 0, General
+ */
+ZL3073X_REG16_DEF(id, 0x0001);
+ZL3073X_REG16_DEF(revision, 0x0003);
+ZL3073X_REG16_DEF(fw_ver, 0x0005);
+ZL3073X_REG32_DEF(custom_config_ver, 0x0007);
+
/*
* Regmap ranges
*/
@@ -196,7 +206,9 @@ static void zl3073x_devlink_unregister(void *ptr)
*/
int zl3073x_dev_init(struct zl3073x_dev *zldev)
{
+ u16 id, revision, fw_ver;
struct devlink *devlink;
+ u32 cfg_ver;
int rc;
rc = devm_mutex_init(zldev->dev, &zldev->lock);
@@ -205,6 +217,30 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
return rc;
}
+ /* Take device lock */
+ scoped_guard(zl3073x, zldev) {
+ rc = zl3073x_read_id(zldev, &id);
+ if (rc)
+ return rc;
+ rc = zl3073x_read_revision(zldev, &revision);
+ if (rc)
+ return rc;
+ rc = zl3073x_read_fw_ver(zldev, &fw_ver);
+ if (rc)
+ return rc;
+ rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
+ if (rc)
+ return rc;
+ }
+
+ dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
+ id, revision, fw_ver);
+ dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
+ FIELD_GET(GENMASK(31, 24), cfg_ver),
+ FIELD_GET(GENMASK(23, 16), cfg_ver),
+ FIELD_GET(GENMASK(15, 8), cfg_ver),
+ FIELD_GET(GENMASK(7, 0), cfg_ver));
+
devlink = priv_to_devlink(zldev);
devlink_register(devlink);
--
2.48.1On Wed, Apr 9, 2025 at 5:43 PM Ivan Vecera <ivecera@redhat.com> wrote: > > Add register definitions for components versions and report them > during probe. JFYI: disabling regmap lock (independently of having an additional one or not) is not recommended. With that you actually disable the useful debugging feature of regmap, your device will not be present in the (regmap) debugfs after that.
On 10. 04. 25 7:50 odp., Andy Shevchenko wrote:
> On Wed, Apr 9, 2025 at 5:43 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>
>> Add register definitions for components versions and report them
>> during probe.
>
> JFYI: disabling regmap lock (independently of having an additional one
> or not) is not recommended. With that you actually disable the useful
> debugging feature of regmap, your device will not be present in the
> (regmap) debugfs after that.
>
I will follow Andrew's recommendation:
1st regmap for direct registers (pages 0-9) with config like:
regmap_config {
...
.lock = mutex_lock,
.unlock = mutex_unlock,
.lock_arg = &zl3073x_dev->lock
...
};
2nd regmap for indirect registers (mailboxes) (pages 10-15) with
disabled locking:
regmap_config {
...
.disable_lock = true,
...
};
For direct registers the lock will be handled automatically by regmap 1.
For indirect registers the lock will be managed explicitly by the driver
to ensure atomic access to mailbox.
The range for regmap 1: (registers 0x000-0x4FF)
regmap_range_cfg {
.range_min = 0,
.range_max = 10 * 128 - 1, /* 10 pages, 128 registers each */
.selector_reg = 0x7f, /* page selector at each page */
.selector_shift = 0, /* no shift in page selector */
.selector_mask = GENMASK(3, 0), /* 4 bits for page sel */
.window_start = 0, /* 128 regs from 0x00-0x7f */
.window_len = 128,
};
The range for regmap 2: (registers 0x500-0x77F)
regmap_range_cfg {
.range_min = 10 * 128,
.range_max = 15 * 128 - 1, /* 5 pages, 128 registers each */
.selector_reg = 0x7f, /* page selector at each page */
.selector_shift = 0, /* no shift in page selector */
.selector_mask = GENMASK(3, 0), /* 4 bits for page sel */
.window_start = 0, /* 128 regs from 0x00-0x7f */
.window_len = 128,
};
Is it now OK?
Thanks,
Ivan
On 11. 04. 25 1:19 odp., Ivan Vecera wrote:
> The range for regmap 1: (registers 0x000-0x4FF)
> regmap_range_cfg {
> .range_min = 0,
> .range_max = 10 * 128 - 1, /* 10 pages, 128 registers each */
> .selector_reg = 0x7f, /* page selector at each page */
> .selector_shift = 0, /* no shift in page selector */
> .selector_mask = GENMASK(3, 0), /* 4 bits for page sel */
> .window_start = 0, /* 128 regs from 0x00-0x7f */
> .window_len = 128,
> };
>
> The range for regmap 2: (registers 0x500-0x77F)
> regmap_range_cfg {
> .range_min = 10 * 128,
> .range_max = 15 * 128 - 1, /* 5 pages, 128 registers each */
> .selector_reg = 0x7f, /* page selector at each page */
> .selector_shift = 0, /* no shift in page selector */
> .selector_mask = GENMASK(3, 0), /* 4 bits for page sel */
> .window_start = 0, /* 128 regs from 0x00-0x7f */
> .window_len = 128,
> };
>
> Is it now OK?
No this is not good... I cannot use 2 ranges.
This is not safe... if the caller use regmap 2 to read/write something
below 0x500 (by mistake), no mapping is applied and value is directly
used as register number that's wrong :-(.
Should I use rather single mapping range to cover all pages and ensure
at driver level that regmap 2 is not used for regs < 0x500?
-or-
what would be the best approach?
I.
61;8000;1cOn Fri, Apr 11, 2025 at 03:17:14PM +0200, Ivan Vecera wrote:
> On 11. 04. 25 1:19 odp., Ivan Vecera wrote:
> > The range for regmap 1: (registers 0x000-0x4FF)
> > regmap_range_cfg {
> > .range_min = 0,
> > .range_max = 10 * 128 - 1, /* 10 pages, 128 registers each */
> > .selector_reg = 0x7f, /* page selector at each page */
> > .selector_shift = 0, /* no shift in page selector */
> > .selector_mask = GENMASK(3, 0), /* 4 bits for page sel */
> > .window_start = 0, /* 128 regs from 0x00-0x7f */
> > .window_len = 128,
> > };
> >
> > The range for regmap 2: (registers 0x500-0x77F)
> > regmap_range_cfg {
> > .range_min = 10 * 128,
> > .range_max = 15 * 128 - 1, /* 5 pages, 128 registers each */
> > .selector_reg = 0x7f, /* page selector at each page */
> > .selector_shift = 0, /* no shift in page selector */
> > .selector_mask = GENMASK(3, 0), /* 4 bits for page sel */
> > .window_start = 0, /* 128 regs from 0x00-0x7f */
> > .window_len = 128,
> > };
> >
> > Is it now OK?
>
> No this is not good... I cannot use 2 ranges.
>
> This is not safe... if the caller use regmap 2 to read/write something below
> 0x500 (by mistake), no mapping is applied and value is directly used as
> register number that's wrong :-(.
>
> Should I use rather single mapping range to cover all pages and ensure at
> driver level that regmap 2 is not used for regs < 0x500?
I don't know regmap too well, but cannot your mailbox regmap have a
reg_base of 10 * 128. Going blow that would then require a negative
reg, but they are unsigned int.
One of that things the core MFD driver is about is giving you safe
access to shared registers on some sort of bus. So it could well be
your MFD exports an higher level API for mailboxs, a mailbox read and
mailbox write, etc. The regmap below it is not exposed outside of the
MFD core. And the MFD core does all the locking.
Andrew
> 2nd regmap for indirect registers (mailboxes) (pages 10-15) with disabled
> locking:
>
> regmap_config {
> ...
> .disable_lock = true,
> ...
> };
Do all registers in pages 10-15 need special locking? Or just a
subset?
Andrew
On 11. 04. 25 2:31 odp., Andrew Lunn wrote:
>> 2nd regmap for indirect registers (mailboxes) (pages 10-15) with disabled
>> locking:
>>
>> regmap_config {
>> ...
>> .disable_lock = true,
>> ...
>> };
>
> Do all registers in pages 10-15 need special locking? Or just a
> subset?
>
> Andrew
>
All of them... 1 page (>=10) == 1 mailbox.
I.
On 09/04/2025 16:42, Ivan Vecera wrote:
> Add register definitions for components versions and report them
> during probe.
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/mfd/zl3073x-core.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
> index f0d85f77a7a76..28f28d00da1cc 100644
> --- a/drivers/mfd/zl3073x-core.c
> +++ b/drivers/mfd/zl3073x-core.c
> @@ -1,7 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0-only
>
> #include <linux/array_size.h>
> +#include <linux/bitfield.h>
> #include <linux/bits.h>
> +#include <linux/cleanup.h>
> #include <linux/dev_printk.h>
> #include <linux/device.h>
> #include <linux/export.h>
> @@ -13,6 +15,14 @@
> #include <net/devlink.h>
> #include "zl3073x.h"
>
> +/*
> + * Register Map Page 0, General
> + */
> +ZL3073X_REG16_DEF(id, 0x0001);
> +ZL3073X_REG16_DEF(revision, 0x0003);
> +ZL3073X_REG16_DEF(fw_ver, 0x0005);
> +ZL3073X_REG32_DEF(custom_config_ver, 0x0007);
> +
> /*
> * Regmap ranges
> */
> @@ -196,7 +206,9 @@ static void zl3073x_devlink_unregister(void *ptr)
> */
> int zl3073x_dev_init(struct zl3073x_dev *zldev)
> {
> + u16 id, revision, fw_ver;
> struct devlink *devlink;
> + u32 cfg_ver;
> int rc;
>
> rc = devm_mutex_init(zldev->dev, &zldev->lock);
> @@ -205,6 +217,30 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
> return rc;
> }
>
> + /* Take device lock */
What is a device lock? Why do you need to comment standard guards/mutexes?
> + scoped_guard(zl3073x, zldev) {
> + rc = zl3073x_read_id(zldev, &id);
> + if (rc)
> + return rc;
> + rc = zl3073x_read_revision(zldev, &revision);
> + if (rc)
> + return rc;
> + rc = zl3073x_read_fw_ver(zldev, &fw_ver);
> + if (rc)
> + return rc;
> + rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
> + if (rc)
> + return rc;
> + }
Nothing improved here. Andrew comments are still valid and do not send
v3 before the discussion is resolved.
> +
> + dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
> + id, revision, fw_ver);
> + dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
> + FIELD_GET(GENMASK(31, 24), cfg_ver),
> + FIELD_GET(GENMASK(23, 16), cfg_ver),
> + FIELD_GET(GENMASK(15, 8), cfg_ver),
> + FIELD_GET(GENMASK(7, 0), cfg_ver));
Both should be dev_dbg. Your driver should be silent on success.
> +
> devlink = priv_to_devlink(zldev);
> devlink_register(devlink);
>
Best regards,
Krzysztof
On 10. 04. 25 9:13 dop., Krzysztof Kozlowski wrote:
> On 09/04/2025 16:42, Ivan Vecera wrote:
>> Add register definitions for components versions and report them
>> during probe.
>>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>> drivers/mfd/zl3073x-core.c | 36 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
>> index f0d85f77a7a76..28f28d00da1cc 100644
>> --- a/drivers/mfd/zl3073x-core.c
>> +++ b/drivers/mfd/zl3073x-core.c
>> @@ -1,7 +1,9 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>>
>> #include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>> #include <linux/bits.h>
>> +#include <linux/cleanup.h>
>> #include <linux/dev_printk.h>
>> #include <linux/device.h>
>> #include <linux/export.h>
>> @@ -13,6 +15,14 @@
>> #include <net/devlink.h>
>> #include "zl3073x.h"
>>
>> +/*
>> + * Register Map Page 0, General
>> + */
>> +ZL3073X_REG16_DEF(id, 0x0001);
>> +ZL3073X_REG16_DEF(revision, 0x0003);
>> +ZL3073X_REG16_DEF(fw_ver, 0x0005);
>> +ZL3073X_REG32_DEF(custom_config_ver, 0x0007);
>> +
>> /*
>> * Regmap ranges
>> */
>> @@ -196,7 +206,9 @@ static void zl3073x_devlink_unregister(void *ptr)
>> */
>> int zl3073x_dev_init(struct zl3073x_dev *zldev)
>> {
>> + u16 id, revision, fw_ver;
>> struct devlink *devlink;
>> + u32 cfg_ver;
>> int rc;
>>
>> rc = devm_mutex_init(zldev->dev, &zldev->lock);
>> @@ -205,6 +217,30 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
>> return rc;
>> }
>>
>> + /* Take device lock */
>
> What is a device lock? Why do you need to comment standard guards/mutexes?
Just to inform code reader, this is a section that accesses device
registers that are protected by this zl3073x device lock.
>> + scoped_guard(zl3073x, zldev) {
>> + rc = zl3073x_read_id(zldev, &id);
>> + if (rc)
>> + return rc;
>> + rc = zl3073x_read_revision(zldev, &revision);
>> + if (rc)
>> + return rc;
>> + rc = zl3073x_read_fw_ver(zldev, &fw_ver);
>> + if (rc)
>> + return rc;
>> + rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
>> + if (rc)
>> + return rc;
>> + }
>
> Nothing improved here. Andrew comments are still valid and do not send
> v3 before the discussion is resolved.
I'm accessing device registers here and they are protected by the device
lock. I have to take the lock, register access functions expect this by
lockdep_assert.
>> +
>> + dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
>> + id, revision, fw_ver);
>> + dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
>> + FIELD_GET(GENMASK(31, 24), cfg_ver),
>> + FIELD_GET(GENMASK(23, 16), cfg_ver),
>> + FIELD_GET(GENMASK(15, 8), cfg_ver),
>> + FIELD_GET(GENMASK(7, 0), cfg_ver));
>
>
> Both should be dev_dbg. Your driver should be silent on success.
+1
will change.
>> +
>> devlink = priv_to_devlink(zldev);
>> devlink_register(devlink);
>>
>
>
> Best regards,
> Krzysztof
>
> > > + /* Take device lock */
> >
> > What is a device lock? Why do you need to comment standard guards/mutexes?
>
> Just to inform code reader, this is a section that accesses device registers
> that are protected by this zl3073x device lock.
I didn't see a reply to my question about the big picture. Why is the
regmap lock not sufficient. Why do you need a device lock.
>
> > > + scoped_guard(zl3073x, zldev) {
> > > + rc = zl3073x_read_id(zldev, &id);
> > > + if (rc)
> > > + return rc;
> > > + rc = zl3073x_read_revision(zldev, &revision);
> > > + if (rc)
> > > + return rc;
> > > + rc = zl3073x_read_fw_ver(zldev, &fw_ver);
> > > + if (rc)
> > > + return rc;
> > > + rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
> > > + if (rc)
> > > + return rc;
> > > + }
> >
> > Nothing improved here. Andrew comments are still valid and do not send
> > v3 before the discussion is resolved.
>
> I'm accessing device registers here and they are protected by the device
> lock. I have to take the lock, register access functions expect this by
> lockdep_assert.
Because lockdep asserts is not a useful answer. Locks are there to
protect you from something. What are you protecting yourself from? If
you cannot answer that question, your locking scheme is built on sand
and very probably broken.
Andrew
On 10. 04. 25 7:41 odp., Andrew Lunn wrote:
>>>> + /* Take device lock */
>>>
>>> What is a device lock? Why do you need to comment standard guards/mutexes?
>>
>> Just to inform code reader, this is a section that accesses device registers
>> that are protected by this zl3073x device lock.
>
> I didn't see a reply to my question about the big picture. Why is the
> regmap lock not sufficient. Why do you need a device lock.
>
>>
>>>> + scoped_guard(zl3073x, zldev) {
>>>> + rc = zl3073x_read_id(zldev, &id);
>>>> + if (rc)
>>>> + return rc;
>>>> + rc = zl3073x_read_revision(zldev, &revision);
>>>> + if (rc)
>>>> + return rc;
>>>> + rc = zl3073x_read_fw_ver(zldev, &fw_ver);
>>>> + if (rc)
>>>> + return rc;
>>>> + rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
>>>> + if (rc)
>>>> + return rc;
>>>> + }
>>>
>>> Nothing improved here. Andrew comments are still valid and do not send
>>> v3 before the discussion is resolved.
>>
>> I'm accessing device registers here and they are protected by the device
>> lock. I have to take the lock, register access functions expect this by
>> lockdep_assert.
>
> Because lockdep asserts is not a useful answer. Locks are there to
> protect you from something. What are you protecting yourself from? If
> you cannot answer that question, your locking scheme is built on sand
> and very probably broken.
>
> Andrew
Hi Andrew,
I have described the locking requirements under different message [v1
05/28]. Could you please take a look?
Thanks,
Ivan
On Thu, Apr 10, 2025 at 08:44:43PM +0200, Ivan Vecera wrote:
>
>
> On 10. 04. 25 7:41 odp., Andrew Lunn wrote:
> > > > > + /* Take device lock */
> > > >
> > > > What is a device lock? Why do you need to comment standard guards/mutexes?
> > >
> > > Just to inform code reader, this is a section that accesses device registers
> > > that are protected by this zl3073x device lock.
> >
> > I didn't see a reply to my question about the big picture. Why is the
> > regmap lock not sufficient. Why do you need a device lock.
> >
> > >
> > > > > + scoped_guard(zl3073x, zldev) {
> > > > > + rc = zl3073x_read_id(zldev, &id);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > + rc = zl3073x_read_revision(zldev, &revision);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > + rc = zl3073x_read_fw_ver(zldev, &fw_ver);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > + rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > + }
> > > >
> > > > Nothing improved here. Andrew comments are still valid and do not send
> > > > v3 before the discussion is resolved.
> > >
> > > I'm accessing device registers here and they are protected by the device
> > > lock. I have to take the lock, register access functions expect this by
> > > lockdep_assert.
> >
> > Because lockdep asserts is not a useful answer. Locks are there to
> > protect you from something. What are you protecting yourself from? If
> > you cannot answer that question, your locking scheme is built on sand
> > and very probably broken.
> >
> > Andrew
>
> Hi Andrew,
> I have described the locking requirements under different message [v1
> 05/28]. Could you please take a look?
So a small number of registers in the regmap need special locking. It
was not clear to me what exactly those locking requirements are,
because they don't appear to be described.
But when i look at the code above, the scoped guard gives the
impression that i have to read id, revision, fw_vr and cfg_ver all in
one go without any other reads/writes happening. I strongly suspect
that impression is wrong. The question then becomes, how can i tell
apart reads/writes which do need to be made as one group, form others
which can be arbitrarily ordered with other read/writes.
What i suggest you do is try to work out how to push the locking down
as low as possible. Make the lock cover only what it needs to cover.
Probably for 95% of the registers, the regmap lock is sufficient.
Just throwing out ideas, i've no idea if they are good or not. Create
two regmaps onto your i2c device, covering different register
ranges. The 'normal' one uses standard regmap locking, the second
'special' one has locking disabled. You additionally provide your own
lock functions to the 'normal' one, so you have access to the
lock. When you need to access the mailboxes, take the lock, so you
know the 'normal' regmap cannot access anything, and then use the
'special' regmap to do what you need to do. A structure like this
should help explain what the special steps are for those special
registers, while not scattering wrong ideas about what the locking
scheme actually is all over the code.
Andrew
On 10. 04. 25 11:54 odp., Andrew Lunn wrote: > ... > > So a small number of registers in the regmap need special locking. It > was not clear to me what exactly those locking requirements are, > because they don't appear to be described. > > But when i look at the code above, the scoped guard gives the > impression that i have to read id, revision, fw_vr and cfg_ver all in > one go without any other reads/writes happening. I strongly suspect > that impression is wrong. The question then becomes, how can i tell > apart reads/writes which do need to be made as one group, form others > which can be arbitrarily ordered with other read/writes. > > What i suggest you do is try to work out how to push the locking down > as low as possible. Make the lock cover only what it needs to cover. > > Probably for 95% of the registers, the regmap lock is sufficient. > > Just throwing out ideas, i've no idea if they are good or not. Create > two regmaps onto your i2c device, covering different register > ranges. The 'normal' one uses standard regmap locking, the second > 'special' one has locking disabled. You additionally provide your own > lock functions to the 'normal' one, so you have access to the > lock. When you need to access the mailboxes, take the lock, so you > know the 'normal' regmap cannot access anything, and then use the > 'special' regmap to do what you need to do. A structure like this > should help explain what the special steps are for those special > registers, while not scattering wrong ideas about what the locking > scheme actually is all over the code. Hi Andrew, the idea looks interesting but there are some caveats and disadvantages. I thought about it but the idea with two regmaps (one for simple registers and one for mailboxes) where the simple one uses implicit locking and mailbox one has locking disabled with explicit locking requirement. There are two main problems: 1) Regmap cache has to be disabled as it cannot be shared between multiple regmaps... so also page selector cannot be cached. 2) You cannot mix access to mailbox registers and to simple registers. This means that mailbox accesses have to be wrapped e.g. inside scoped_guard() The first problem is really pain as I would like to extend later the driver with proper caching (page selector for now). The second one brings only confusions for a developer how to properly access different types of registers. I think the best approach would be to use just single regmap for all registers with implicit locking enabled and have extra mailbox mutex to protect mailbox registers and ensure atomic operations with them. This will allow to use regmap cache and also intermixing mailbox and simple registers' accesses won't be an issue. @Andy Shevchenko, wdym about it? Thanks, Ivan
> Hi Andrew, > the idea looks interesting but there are some caveats and disadvantages. > I thought about it but the idea with two regmaps (one for simple registers > and one for mailboxes) where the simple one uses implicit locking and > mailbox one has locking disabled with explicit locking requirement. There > are two main problems: > > 1) Regmap cache has to be disabled as it cannot be shared between multiple > regmaps... so also page selector cannot be cached. > > 2) You cannot mix access to mailbox registers and to simple registers. This > means that mailbox accesses have to be wrapped e.g. inside scoped_guard() > > The first problem is really pain as I would like to extend later the driver > with proper caching (page selector for now). > The second one brings only confusions for a developer how to properly access > different types of registers. > > I think the best approach would be to use just single regmap for all > registers with implicit locking enabled and have extra mailbox mutex to > protect mailbox registers and ensure atomic operations with them. > This will allow to use regmap cache and also intermixing mailbox and simple > registers' accesses won't be an issue. As i said, it was just an idea, i had no idea if it was a good idea. What is important is that the scope of the locking becomes clear, unlike what the first version had. So locking has to be pushed down to the lower levels so you lock a single register access, or you lock an mailbox access. Also, you say this is an MFD partially because GPIOs could be added later. I assume that GPIO code would have the same locking issue, which suggests the locking should be in the MFD core, not the individual drivers stacked on top of it. Andrew
On 15. 04. 25 2:57 odp., Andrew Lunn wrote:
>> Hi Andrew,
>> the idea looks interesting but there are some caveats and disadvantages.
>> I thought about it but the idea with two regmaps (one for simple registers
>> and one for mailboxes) where the simple one uses implicit locking and
>> mailbox one has locking disabled with explicit locking requirement. There
>> are two main problems:
>>
>> 1) Regmap cache has to be disabled as it cannot be shared between multiple
>> regmaps... so also page selector cannot be cached.
>>
>> 2) You cannot mix access to mailbox registers and to simple registers. This
>> means that mailbox accesses have to be wrapped e.g. inside scoped_guard()
>>
>> The first problem is really pain as I would like to extend later the driver
>> with proper caching (page selector for now).
>> The second one brings only confusions for a developer how to properly access
>> different types of registers.
>>
>> I think the best approach would be to use just single regmap for all
>> registers with implicit locking enabled and have extra mailbox mutex to
>> protect mailbox registers and ensure atomic operations with them.
>> This will allow to use regmap cache and also intermixing mailbox and simple
>> registers' accesses won't be an issue.
>
> As i said, it was just an idea, i had no idea if it was a good idea.
>
> What is important is that the scope of the locking becomes clear,
> unlike what the first version had. So locking has to be pushed down to
> the lower levels so you lock a single register access, or you lock an
> mailbox access.
>
> Also, you say this is an MFD partially because GPIOs could be added
> later. I assume that GPIO code would have the same locking issue,
> which suggests the locking should be in the MFD core, not the
> individual drivers stacked on top of it.
To work with GPIO block inside chip you use just simple registers not
mailboxes. But it does not matter. The approach above exposes for
individual sub-drivers an API (not direct usage of regmap (all registers
are exposed by function helpers), helpers to enter/leave "mailbox mode"
that locks/unlocks mailbox mutex)...
Something like this
{
...
rc = zl3073x_read_id(zldev, &id); // locked implicitly
...
scoped_guard(zl3073x_mailbox)(zldev) { // enter mailbox 'mode'
rc = zl3073x_mb_ref_set(zldev, ref_idx);
rc = zl3073x_mb_ref_op(zldev, REF_OP_READ);
regmap_wait_poll_timeout(...);
rc = zl3073x_mb_ref_freq(zldev, &freq);
} // leave mailbox access 'mode'
...
rc = zl3073x_write_blahblah(zldev, value);
}
Thanks,
Ivan
On Tue, Apr 15, 2025 at 12:01:43PM +0200, Ivan Vecera wrote: > On 10. 04. 25 11:54 odp., Andrew Lunn wrote: ... > > So a small number of registers in the regmap need special locking. It > > was not clear to me what exactly those locking requirements are, > > because they don't appear to be described. > > > > But when i look at the code above, the scoped guard gives the > > impression that i have to read id, revision, fw_vr and cfg_ver all in > > one go without any other reads/writes happening. I strongly suspect > > that impression is wrong. The question then becomes, how can i tell > > apart reads/writes which do need to be made as one group, form others > > which can be arbitrarily ordered with other read/writes. > > > > What i suggest you do is try to work out how to push the locking down > > as low as possible. Make the lock cover only what it needs to cover. > > > > Probably for 95% of the registers, the regmap lock is sufficient. > > > > Just throwing out ideas, i've no idea if they are good or not. Create > > two regmaps onto your i2c device, covering different register > > ranges. The 'normal' one uses standard regmap locking, the second > > 'special' one has locking disabled. You additionally provide your own > > lock functions to the 'normal' one, so you have access to the > > lock. When you need to access the mailboxes, take the lock, so you > > know the 'normal' regmap cannot access anything, and then use the > > 'special' regmap to do what you need to do. A structure like this > > should help explain what the special steps are for those special > > registers, while not scattering wrong ideas about what the locking > > scheme actually is all over the code. > > Hi Andrew, > the idea looks interesting but there are some caveats and disadvantages. > I thought about it but the idea with two regmaps (one for simple registers > and one for mailboxes) where the simple one uses implicit locking and > mailbox one has locking disabled with explicit locking requirement. There > are two main problems: > > 1) Regmap cache has to be disabled as it cannot be shared between multiple > regmaps... so also page selector cannot be cached. > > 2) You cannot mix access to mailbox registers and to simple registers. This > means that mailbox accesses have to be wrapped e.g. inside scoped_guard() > > The first problem is really pain as I would like to extend later the driver > with proper caching (page selector for now). > The second one brings only confusions for a developer how to properly access > different types of registers. > > I think the best approach would be to use just single regmap for all > registers with implicit locking enabled and have extra mailbox mutex to > protect mailbox registers and ensure atomic operations with them. > This will allow to use regmap cache and also intermixing mailbox and simple > registers' accesses won't be an issue. > > @Andy Shevchenko, wdym about it? Sounds like a good plan to me, but I'm not in the exact area of this driver's interest, so others may have better suggestions. -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.