Re: [PATCH 05/28] mfd: zl3073x: Add components versions register defs

[Ivan Vecera <ivecera@redhat.com>]

On 10. 04. 25 12:42 odp., Krzysztof Kozlowski wrote:
> On 10/04/2025 12:23, Ivan Vecera wrote:
>>
>>
>> On 10. 04. 25 9:11 dop., Krzysztof Kozlowski wrote:
>>> On 09/04/2025 08:44, Ivan Vecera wrote:
>>>> On 07. 04. 25 11:09 odp., Andrew Lunn wrote:
>>>>> On Mon, Apr 07, 2025 at 07:28:32PM +0200, Ivan Vecera wrote:
>>>>>> Add register definitions for components versions and report them
>>>>>> during probe.
>>>>>>
>>>>>> Reviewed-by: Michal Schmidt <mschmidt@redhat.com>
>>>>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>>>>>> ---
>>>>>>     drivers/mfd/zl3073x-core.c | 35 +++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 35 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
>>>>>> index 39d4c8608a740..b3091b00cffa8 100644
>>>>>> --- a/drivers/mfd/zl3073x-core.c
>>>>>> +++ b/drivers/mfd/zl3073x-core.c
>>>>>> @@ -1,10 +1,19 @@
>>>>>>     // SPDX-License-Identifier: GPL-2.0-only
>>>>>>     
>>>>>> +#include <linux/bitfield.h>
>>>>>>     #include <linux/module.h>
>>>>>>     #include <linux/unaligned.h>
>>>>>>     #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
>>>>>>      */
>>>>>> @@ -159,10 +168,36 @@ EXPORT_SYMBOL_NS_GPL(zl3073x_dev_alloc, "ZL3073X");
>>>>>>     
>>>>>>     int zl3073x_dev_init(struct zl3073x_dev *zldev)
>>>>>>     {
>>>>>> +	u16 id, revision, fw_ver;
>>>>>>     	struct devlink *devlink;
>>>>>> +	u32 cfg_ver;
>>>>>> +	int rc;
>>>>>>     
>>>>>>     	devm_mutex_init(zldev->dev, &zldev->lock);
>>>>>>     
>>>>>> +	scoped_guard(zl3073x, zldev) {
>>>>>
>>>>> Why the scoped_guard? The locking scheme you have seems very opaque.
>>>>
>>>> We are read the HW registers in this block and the access is protected
>>>> by this device lock. Regmap locking will be disabled in v2 as this is
>>>
>>> Reading ID must be protected by mutex? Why and how?
>>
>> Yes, the ID is read from the hardware register and HW access functions
>> are protected by zl3073x_dev->lock. The access is not protected by
> 
> Please do not keep repeating the same. You again describe the code. We
> ask why do you implement that way?
> 
>> regmap locking schema. Set of registers are indirect and are accessed by
>> mailboxes where multiple register accesses need to be done atomically.
> 
> regmap handles that, but anyway, how multiple register access to ID
> registers happen? From what module? Which code does it? So they write
> here something in the middle and reading would be unsynced?

OK, I'm going to try to explain in detail...

The device have 16 register pages where each of them has 128 registers 
and register 0x7f on each page is a page selector.

Pages 0..9 contain direct registers that can be arbitrary read or 
written in any order. For these registers implicit regmap locking is 
sufficient.

Pages 10..16 contain indirect registers and these pages are called 
mailboxes. Each mailbox cover specific part of hardware (synth, DPLL 
channel, input ref, output...) and each of them contain mailbox_mask 
register and mailbox_sem register. The rest of registers in the 
particular page (mailbox) are latch registers.

Read operation (described in patch 8 in this v1 series):
E.g. driver needs to read frequency of input pin 4:
1) it set value of mailbox_mask (in input mailbox/page) to 4
2) it set mailbox_sem register (--"--) to read operation
3) it polls mailbox_sem to be cleared (firmware fills latch registers)
4) it reads frequency latch register (--"--) filled by FW

Write is similar but opposite:
1) it writes frequency to freq latch register (in input mb)
2) it set value of mailbox_mask
3) it set mailbox_sem to write operation
4) it polls mailbox_sem to be cleared (write was finished)

Steps 1-4 for both cases have to be done atomically - other reader 
cannot modify mailbox_sem prior step 4 is finished and other writer 
cannot touch latch registers prior step 4 is finished.

The module dpll_zl3073x (later in this series) and ptp_zl3073x (will be 
posted later) use this intensively from multiple contexts (DPLL core 
callbacks and monitoring threads).

So I have decided to use the custom locking scheme for accessing 
registers instead of regmap locking that cannot guarantee this atomicity.

Would it be better to leave implicit regmap locking scheme for direct 
registers and to have extra locking for mailboxes? If so, single mutex 
for all mailboxes or separate mutex for each mailbox type?

Thanks,
Ivan