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

From: Ivan Vecera
Date: Thu Apr 10 2025 - 08:06:36 EST



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@xxxxxxxxxx>
Signed-off-by: Ivan Vecera <ivecera@xxxxxxxxxx>
---
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