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 <mschm...@redhat.com>
Signed-off-by: Ivan Vecera <ivec...@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
