[PATCH net-next v3 0/5] dpll: add phase-offset and phase-adjust

[PATCH net-next v3 0/5] dpll: add phase-offset and phase-adjust

[Arkadiusz Kubalewski]

Improve monitoring and control over dpll devices.
Allow user to receive measurement of phase difference between signals on
pin and dpll (phase-offset).
Allow user to receive and control adjustable value of pin's signal
phase (phase-adjust).

v2->v3:
- do not increase do version of uAPI header as it is not needed

v1->v2:
- improve handling for error case of requesting the phase adjust set
- align handling for error case of frequency set request with the
approach introduced for phase adjust

Arkadiusz Kubalewski (5):
  dpll: docs: add support for pin signal phase offset/adjust
  dpll: spec: add support for pin-dpll signal phase offset/adjust
  dpll: netlink/core: add support for pin-dpll signal phase
    offset/adjust
  ice: dpll: implement phase related callbacks
  dpll: netlink/core: change pin frequency set behavior

 Documentation/driver-api/dpll.rst         |  53 ++++-
 Documentation/netlink/specs/dpll.yaml     |  33 +++-
 drivers/dpll/dpll_netlink.c               | 180 +++++++++++++++--
 drivers/dpll/dpll_nl.c                    |   8 +-
 drivers/dpll/dpll_nl.h                    |   2 +-
 drivers/net/ethernet/intel/ice/ice_dpll.c | 224 +++++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_dpll.h |  10 +-
 include/linux/dpll.h                      |  18 ++
 include/uapi/linux/dpll.h                 |   8 +-
 9 files changed, 514 insertions(+), 22 deletions(-)

-- 
2.38.1

[PATCH net-next v3 1/5] dpll: docs: add support for pin signal phase offset/adjust

[Arkadiusz Kubalewski]

Add documentation on:
- measurment of phase of signal between pin and dpll
- adjustment of pin signal phase

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 Documentation/driver-api/dpll.rst | 53 ++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
index 69670deb8c4e..564997373437 100644
--- a/Documentation/driver-api/dpll.rst
+++ b/Documentation/driver-api/dpll.rst
@@ -173,6 +173,47 @@ in order to configure active input of a MUX-type pin, the user needs to
 request desired pin state of the child pin on the parent pin,
 as described in the ``MUX-type pins`` chapter.
 
+Phase offset measurement and adjustment
+========================================
+
+Device may provide ability to measure a phase difference between signals
+on a pin and its parent dpll device. If pin-dpll phase offset measurement
+is supported, it shall be provided with ``DPLL_A_PIN_PHASE_OFFSET``
+attribute for each parent dpll device.
+
+Device may also provide ability to adjust a signal phase on a pin.
+If pin phase adjustment is supported, minimal and maximal values that pin
+handle shall be provide to the user on ``DPLL_CMD_PIN_GET`` respond
+with ``DPLL_A_PIN_PHASE_ADJUST_MIN`` and ``DPLL_A_PIN_PHASE_ADJUST_MAX``
+attributes. Configured phase adjust value is provided with
+``DPLL_A_PIN_PHASE_ADJUST`` attribute of a pin, and value change can be
+requested with the same attribute with ``DPLL_CMD_PIN_SET`` command.
+
+  =============================== ======================================
+  ``DPLL_A_PIN_ID``               configured pin id
+  ``DPLL_A_PIN_PHASE_ADJUST_MIN`` attr minimum value of phase adjustment
+  ``DPLL_A_PIN_PHASE_ADJUST_MAX`` attr maximum value of phase adjustment
+  ``DPLL_A_PIN_PHASE_ADJUST``     attr configured value of phase
+                                  adjustment on parent dpll device
+  ``DPLL_A_PIN_PARENT_DEVICE``    nested attribute for requesting
+                                  configuration on given parent dpll
+                                  device
+    ``DPLL_A_PIN_PARENT_ID``      parent dpll device id
+    ``DPLL_A_PIN_PHASE_OFFSET``   attr measured phase difference
+                                  between a pin and parent dpll device
+  =============================== ======================================
+
+All phase related values are provided in pico seconds, which represents
+time differnece between signals phase. The negative value means that
+phase of signal on pin is earlier in time than dpll's signal. Positive
+value means that phase of signal on pin is later in time than signal of
+a dpll.
+
+Phase adjust (also min and max) values are integers, but measured phase
+offset values are fractional with 3-digit decimal places and shell be
+divided with ``DPLL_PIN_PHASE_OFFSET_DIVIDER`` to get integer part and
+modulo divided to get fractional part.
+
 Configuration commands group
 ============================
 
@@ -263,6 +304,12 @@ according to attribute purpose.
                                        frequencies
       ``DPLL_A_PIN_ANY_FREQUENCY_MIN`` attr minimum value of frequency
       ``DPLL_A_PIN_ANY_FREQUENCY_MAX`` attr maximum value of frequency
+    ``DPLL_A_PIN_PHASE_ADJUST_MIN``    attr minimum value of phase
+                                       adjustment
+    ``DPLL_A_PIN_PHASE_ADJUST_MAX``    attr maximum value of phase
+                                       adjustment
+    ``DPLL_A_PIN_PHASE_ADJUST``        attr configured value of phase
+                                       adjustment on parent device
     ``DPLL_A_PIN_PARENT_DEVICE``       nested attr for each parent device
                                        the pin is connected with
       ``DPLL_A_PIN_PARENT_ID``         attr parent dpll device id
@@ -270,8 +317,10 @@ according to attribute purpose.
                                        dpll device
       ``DPLL_A_PIN_STATE``             attr state of pin on the parent
                                        dpll device
-     ``DPLL_A_PIN_DIRECTION``          attr direction of a pin on the
+      ``DPLL_A_PIN_DIRECTION``         attr direction of a pin on the
                                        parent dpll device
+      ``DPLL_A_PIN_PHASE_OFFSET``      attr measured phase difference
+                                       between a pin and parent dpll
     ``DPLL_A_PIN_PARENT_PIN``          nested attr for each parent pin
                                        the pin is connected with
       ``DPLL_A_PIN_PARENT_ID``         attr parent pin id
@@ -284,6 +333,8 @@ according to attribute purpose.
   ``DPLL_CMD_PIN_SET``                 command to set pins configuration
     ``DPLL_A_PIN_ID``                  attr unique a pin ID
     ``DPLL_A_PIN_FREQUENCY``           attr requested frequency of a pin
+    ``DPLL_A_PIN_PHASE_ADJUST``        attr requested value of phase
+                                       adjustment on parent device
     ``DPLL_A_PIN_PARENT_DEVICE``       nested attr for each parent dpll
                                        device configuration request
       ``DPLL_A_PIN_PARENT_ID``         attr parent dpll device id
-- 
2.38.1

[PATCH net-next v3 2/5] dpll: spec: add support for pin-dpll signal phase offset/adjust

[Arkadiusz Kubalewski]

Add attributes for providing the user with:
- measurement of signals phase offset between pin and dpll
- ability to adjust the phase of pin signal

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 Documentation/netlink/specs/dpll.yaml | 33 ++++++++++++++++++++++++++-
 drivers/dpll/dpll_nl.c                |  8 ++++---
 drivers/dpll/dpll_nl.h                |  2 +-
 include/uapi/linux/dpll.h             |  8 ++++++-
 4 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
index 8b86b28b47a6..dc057494101f 100644
--- a/Documentation/netlink/specs/dpll.yaml
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
 
 name: dpll
-
+version: 2
 doc: DPLL subsystem.
 
 definitions:
@@ -164,6 +164,18 @@ definitions:
       -
         name: state-can-change
         doc: pin state can be changed
+  -
+    type: const
+    name: phase-offset-divider
+    value: 1000
+    doc: |
+      phase offset divider allows userspace to calculate a value of
+      measured signal phase difference between a pin and dpll device
+      as a fractional value with three digit decimal precision.
+      Value of (DPLL_A_PHASE_OFFSET / DPLL_PHASE_OFFSET_DIVIDER) is an
+      integer part of a measured phase offest value.
+      Value of (DPLL_A_PHASE_OFFSET % DPLL_PHASE_OFFSET_DIVIDER) is a
+      fractional part of a measured phase offest value.
 
 attribute-sets:
   -
@@ -272,6 +284,18 @@ attribute-sets:
         type: nest
         multi-attr: true
         nested-attributes: pin-parent-pin
+      -
+        name: phase-adjust-min
+        type: s32
+      -
+        name: phase-adjust-max
+        type: s32
+      -
+        name: phase-adjust
+        type: s32
+      -
+        name: phase-offset
+        type: s64
   -
     name: pin-parent-device
     subset-of: pin
@@ -288,6 +312,9 @@ attribute-sets:
       -
         name: state
         type: u32
+      -
+        name: phase-offset
+        type: s64
   -
     name: pin-parent-pin
     subset-of: pin
@@ -439,6 +466,9 @@ operations:
             - capabilities
             - parent-device
             - parent-pin
+            - phase-adjust-min
+            - phase-adjust-max
+            - phase-adjust
 
       dump:
         pre: dpll-lock-dumpit
@@ -466,6 +496,7 @@ operations:
             - state
             - parent-device
             - parent-pin
+            - phase-adjust
     -
       name: pin-create-ntf
       doc: Notification about pin appearing
diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
index 14064c8c783b..eaee5be7aa64 100644
--- a/drivers/dpll/dpll_nl.c
+++ b/drivers/dpll/dpll_nl.c
@@ -11,11 +11,12 @@
 #include <uapi/linux/dpll.h>
 
 /* Common nested types */
-const struct nla_policy dpll_pin_parent_device_nl_policy[DPLL_A_PIN_STATE + 1] = {
+const struct nla_policy dpll_pin_parent_device_nl_policy[DPLL_A_PIN_PHASE_OFFSET + 1] = {
 	[DPLL_A_PIN_PARENT_ID] = { .type = NLA_U32, },
 	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2),
 	[DPLL_A_PIN_PRIO] = { .type = NLA_U32, },
 	[DPLL_A_PIN_STATE] = NLA_POLICY_RANGE(NLA_U32, 1, 3),
+	[DPLL_A_PIN_PHASE_OFFSET] = { .type = NLA_S64, },
 };
 
 const struct nla_policy dpll_pin_parent_pin_nl_policy[DPLL_A_PIN_STATE + 1] = {
@@ -61,7 +62,7 @@ static const struct nla_policy dpll_pin_get_dump_nl_policy[DPLL_A_PIN_ID + 1] =
 };
 
 /* DPLL_CMD_PIN_SET - do */
-static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_PIN + 1] = {
+static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + 1] = {
 	[DPLL_A_PIN_ID] = { .type = NLA_U32, },
 	[DPLL_A_PIN_FREQUENCY] = { .type = NLA_U64, },
 	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2),
@@ -69,6 +70,7 @@ static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_PIN + 1]
 	[DPLL_A_PIN_STATE] = NLA_POLICY_RANGE(NLA_U32, 1, 3),
 	[DPLL_A_PIN_PARENT_DEVICE] = NLA_POLICY_NESTED(dpll_pin_parent_device_nl_policy),
 	[DPLL_A_PIN_PARENT_PIN] = NLA_POLICY_NESTED(dpll_pin_parent_pin_nl_policy),
+	[DPLL_A_PIN_PHASE_ADJUST] = { .type = NLA_S32, },
 };
 
 /* Ops table for dpll */
@@ -140,7 +142,7 @@ static const struct genl_split_ops dpll_nl_ops[] = {
 		.doit		= dpll_nl_pin_set_doit,
 		.post_doit	= dpll_pin_post_doit,
 		.policy		= dpll_pin_set_nl_policy,
-		.maxattr	= DPLL_A_PIN_PARENT_PIN,
+		.maxattr	= DPLL_A_PIN_PHASE_ADJUST,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
 };
diff --git a/drivers/dpll/dpll_nl.h b/drivers/dpll/dpll_nl.h
index 1f67aaed4742..92d4c9c4f788 100644
--- a/drivers/dpll/dpll_nl.h
+++ b/drivers/dpll/dpll_nl.h
@@ -12,7 +12,7 @@
 #include <uapi/linux/dpll.h>
 
 /* Common nested types */
-extern const struct nla_policy dpll_pin_parent_device_nl_policy[DPLL_A_PIN_STATE + 1];
+extern const struct nla_policy dpll_pin_parent_device_nl_policy[DPLL_A_PIN_PHASE_OFFSET + 1];
 extern const struct nla_policy dpll_pin_parent_pin_nl_policy[DPLL_A_PIN_STATE + 1];
 
 int dpll_lock_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
index 20ef0718f8dc..050f51b48ef8 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -7,7 +7,7 @@
 #define _UAPI_LINUX_DPLL_H
 
 #define DPLL_FAMILY_NAME	"dpll"
-#define DPLL_FAMILY_VERSION	1
+#define DPLL_FAMILY_VERSION	2
 
 /**
  * enum dpll_mode - working modes a dpll can support, differentiates if and how
@@ -138,6 +138,8 @@ enum dpll_pin_capabilities {
 	DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE = 4,
 };
 
+#define DPLL_PHASE_OFFSET_DIVIDER	1000
+
 enum dpll_a {
 	DPLL_A_ID = 1,
 	DPLL_A_MODULE_NAME,
@@ -173,6 +175,10 @@ enum dpll_a_pin {
 	DPLL_A_PIN_CAPABILITIES,
 	DPLL_A_PIN_PARENT_DEVICE,
 	DPLL_A_PIN_PARENT_PIN,
+	DPLL_A_PIN_PHASE_ADJUST_MIN,
+	DPLL_A_PIN_PHASE_ADJUST_MAX,
+	DPLL_A_PIN_PHASE_ADJUST,
+	DPLL_A_PIN_PHASE_OFFSET,
 
 	__DPLL_A_PIN_MAX,
 	DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
-- 
2.38.1

Re: [PATCH net-next v3 2/5] dpll: spec: add support for pin-dpll signal phase offset/adjust

[Jiri Pirko]

Fri, Oct 06, 2023 at 01:40:58PM CEST, arkadiusz.kubalewski@intel.com wrote:
>Add attributes for providing the user with:
>- measurement of signals phase offset between pin and dpll
>- ability to adjust the phase of pin signal
>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> Documentation/netlink/specs/dpll.yaml | 33 ++++++++++++++++++++++++++-
> drivers/dpll/dpll_nl.c                |  8 ++++---
> drivers/dpll/dpll_nl.h                |  2 +-
> include/uapi/linux/dpll.h             |  8 ++++++-
> 4 files changed, 45 insertions(+), 6 deletions(-)
>
>diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
>index 8b86b28b47a6..dc057494101f 100644
>--- a/Documentation/netlink/specs/dpll.yaml
>+++ b/Documentation/netlink/specs/dpll.yaml
>@@ -1,7 +1,7 @@
> # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
> 
> name: dpll
>-
>+version: 2

I'm confused. Didn't you say you'll remove this? If not, my question
from v1 still stands.

Re: [PATCH net-next v3 2/5] dpll: spec: add support for pin-dpll signal phase offset/adjust

[Jakub Kicinski]

On Fri, 6 Oct 2023 14:30:00 +0200 Jiri Pirko wrote:
> >+version: 2  
> 
> I'm confused. Didn't you say you'll remove this? If not, my question
> from v1 still stands.

Perhaps we should dis-allow setting version in non-genetlink-legacy
specs? I thought it may be a useful thing to someone, at some point,
but so far the scoreboard is: legit uses: 0, confused uses: 1 :S

Thoughts?

Re: [PATCH net-next v3 2/5] dpll: spec: add support for pin-dpll signal phase offset/adjust

[Jiri Pirko]

Fri, Oct 06, 2023 at 04:55:36PM CEST, kuba@kernel.org wrote:
>On Fri, 6 Oct 2023 14:30:00 +0200 Jiri Pirko wrote:
>> >+version: 2  
>> 
>> I'm confused. Didn't you say you'll remove this? If not, my question
>> from v1 still stands.
>
>Perhaps we should dis-allow setting version in non-genetlink-legacy
>specs? I thought it may be a useful thing to someone, at some point,
>but so far the scoreboard is: legit uses: 0, confused uses: 1 :S
>
>Thoughts?

I don't know what the meaning of version is. I just never saw that being
touched. Is there any semantics documented for it?

Kuba, any opinion?

Re: [PATCH net-next v3 2/5] dpll: spec: add support for pin-dpll signal phase offset/adjust

[Kuba Kicinski]

On Fri, 6 Oct 2023 18:53:04 +0200 Jiri Pirko wrote:
> Fri, Oct 06, 2023 at 04:55:36PM CEST, kuba@kernel.org wrote:
> >> I'm confused. Didn't you say you'll remove this? If not, my question
> >> from v1 still stands.  
> >
> >Perhaps we should dis-allow setting version in non-genetlink-legacy
> >specs? I thought it may be a useful thing to someone, at some point,
> >but so far the scoreboard is: legit uses: 0, confused uses: 1 :S
> >
> >Thoughts?  
> 
> I don't know what the meaning of version is. I just never saw that being
> touched. Is there any semantics documented for it?
> 
> Kuba, any opinion?

/me switches the first name in From :P

I think it basically predates the op / policy introspection,
and allows people to break backward compat.

drop_monitor bumped to 2 in 2009:

  683703a26e46 ("drop_monitor: Update netlink protocol to include
netlink attribute header in alert message")

which breaks backward compat.

genetlink ctrl went to 2 in 2006:

  334c29a64507 ("[GENETLINK]: Move command capabilities to flags.")

which moves some info around in attrs, also breaks backward compat
if someone depended on the old placement.

ovs did it in 2013:

  44da5ae5fbea ("openvswitch: Drop user features if old user space
attempted to create datapath")

again, breaks backwards compat.


I guess it may still make one day to bump the version for some proto
which has very tight control over the user space. But it hasn't
happened for 10 years.

Re: [PATCH net-next v3 2/5] dpll: spec: add support for pin-dpll signal phase offset/adjust

[Jiri Pirko]

Fri, Oct 06, 2023 at 09:44:57PM CEST, kuba@kernel.org wrote:
>On Fri, 6 Oct 2023 18:53:04 +0200 Jiri Pirko wrote:
>> Fri, Oct 06, 2023 at 04:55:36PM CEST, kuba@kernel.org wrote:
>> >> I'm confused. Didn't you say you'll remove this? If not, my question
>> >> from v1 still stands.  
>> >
>> >Perhaps we should dis-allow setting version in non-genetlink-legacy
>> >specs? I thought it may be a useful thing to someone, at some point,
>> >but so far the scoreboard is: legit uses: 0, confused uses: 1 :S
>> >
>> >Thoughts?  
>> 
>> I don't know what the meaning of version is. I just never saw that being
>> touched. Is there any semantics documented for it?
>> 
>> Kuba, any opinion?
>
>/me switches the first name in From :P

I messed up a bit. Kuba* confusion, sorry :)


>
>I think it basically predates the op / policy introspection,
>and allows people to break backward compat.
>
>drop_monitor bumped to 2 in 2009:
>
>  683703a26e46 ("drop_monitor: Update netlink protocol to include
>netlink attribute header in alert message")
>
>which breaks backward compat.
>
>genetlink ctrl went to 2 in 2006:
>
>  334c29a64507 ("[GENETLINK]: Move command capabilities to flags.")
>
>which moves some info around in attrs, also breaks backward compat
>if someone depended on the old placement.
>
>ovs did it in 2013:
>
>  44da5ae5fbea ("openvswitch: Drop user features if old user space
>attempted to create datapath")
>
>again, breaks backwards compat.
>
>
>I guess it may still make one day to bump the version for some proto
>which has very tight control over the user space. But it hasn't
>happened for 10 years.

But since by the policy we cannot break uapi compat, version should be
never bumped. I wonder howcome it is legit in the examples you listed
above...

Let's forbid that in genetlink.yaml. I have a patch ready, please ack
this approach.

Thx!

Re: [PATCH net-next v3 2/5] dpll: spec: add support for pin-dpll signal phase offset/adjust

[Jakub Kicinski]

On Sat, 7 Oct 2023 12:29:47 +0200 Jiri Pirko wrote:
> But since by the policy we cannot break uapi compat, version should be
> never bumped. I wonder howcome it is legit in the examples you listed
> above...

Yes, even it's the 0.0001% of the time when "breaking' uAPI is fine,
the change in the family spec can these days be much more precisely
detected using policy dump.

> Let's forbid that in genetlink.yaml. I have a patch ready, please ack
> this approach.

Ack, please remember to move version into the # Start genetlink-legacy
section in the genetlink-legacy schema.

RE: [PATCH net-next v3 2/5] dpll: spec: add support for pin-dpll signal phase offset/adjust

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Friday, October 6, 2023 2:30 PM
>
>Fri, Oct 06, 2023 at 01:40:58PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>Add attributes for providing the user with:
>>- measurement of signals phase offset between pin and dpll
>>- ability to adjust the phase of pin signal
>>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>> Documentation/netlink/specs/dpll.yaml | 33 ++++++++++++++++++++++++++-
>> drivers/dpll/dpll_nl.c                |  8 ++++---
>> drivers/dpll/dpll_nl.h                |  2 +-
>> include/uapi/linux/dpll.h             |  8 ++++++-
>> 4 files changed, 45 insertions(+), 6 deletions(-)
>>
>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>b/Documentation/netlink/specs/dpll.yaml
>>index 8b86b28b47a6..dc057494101f 100644
>>--- a/Documentation/netlink/specs/dpll.yaml
>>+++ b/Documentation/netlink/specs/dpll.yaml
>>@@ -1,7 +1,7 @@
>> # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR
>>BSD-3-Clause)
>>
>> name: dpll
>>-
>>+version: 2
>
>I'm confused. Didn't you say you'll remove this? If not, my question from
>v1 still stands.

I am just moron who have sent wrong patch :s
Now is fixed in v4.

Thank you,
Arkadiusz

[PATCH net-next v3 3/5] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust

[Arkadiusz Kubalewski]

Add callback ops for pin-dpll phase measurment.
Add callback for pin signal phase adjustment.
Add min and max phase adjustment values to pin proprties.
Invoke callbacks in dpll_netlink.c when filling the pin details to
provide user with phase related attribute values.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/dpll/dpll_netlink.c | 130 +++++++++++++++++++++++++++++++++++-
 include/linux/dpll.h        |  18 +++++
 2 files changed, 147 insertions(+), 1 deletion(-)

diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index e20daba6896a..97319a9e4667 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -212,6 +212,53 @@ dpll_msg_add_pin_direction(struct sk_buff *msg, struct dpll_pin *pin,
 	return 0;
 }
 
+static int
+dpll_msg_add_pin_phase_adjust(struct sk_buff *msg, struct dpll_pin *pin,
+			      struct dpll_pin_ref *ref,
+			      struct netlink_ext_ack *extack)
+{
+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
+	struct dpll_device *dpll = ref->dpll;
+	s32 phase_adjust;
+	int ret;
+
+	if (!ops->phase_adjust_get)
+		return 0;
+	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				    dpll, dpll_priv(dpll),
+				    &phase_adjust, extack);
+	if (ret)
+		return ret;
+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST, phase_adjust))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int
+dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin,
+			  struct dpll_pin_ref *ref,
+			  struct netlink_ext_ack *extack)
+{
+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
+	struct dpll_device *dpll = ref->dpll;
+	s64 phase_offset;
+	int ret;
+
+	if (!ops->phase_offset_get)
+		return 0;
+	ret = ops->phase_offset_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				    dpll, dpll_priv(dpll), &phase_offset,
+				    extack);
+	if (ret)
+		return ret;
+	if (nla_put_64bit(msg, DPLL_A_PIN_PHASE_OFFSET, sizeof(phase_offset),
+			  &phase_offset, DPLL_A_PIN_PAD))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
 static int
 dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
 		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
@@ -330,6 +377,9 @@ dpll_msg_add_pin_dplls(struct sk_buff *msg, struct dpll_pin *pin,
 		if (ret)
 			goto nest_cancel;
 		ret = dpll_msg_add_pin_direction(msg, pin, ref, extack);
+		if (ret)
+			goto nest_cancel;
+		ret = dpll_msg_add_phase_offset(msg, pin, ref, extack);
 		if (ret)
 			goto nest_cancel;
 		nla_nest_end(msg, attr);
@@ -377,6 +427,15 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
 	if (nla_put_u32(msg, DPLL_A_PIN_CAPABILITIES, prop->capabilities))
 		return -EMSGSIZE;
 	ret = dpll_msg_add_pin_freq(msg, pin, ref, extack);
+	if (ret)
+		return ret;
+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN,
+			prop->phase_range.min))
+		return -EMSGSIZE;
+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MAX,
+			prop->phase_range.max))
+		return -EMSGSIZE;
+	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
 	if (ret)
 		return ret;
 	if (xa_empty(&pin->parent_refs))
@@ -416,7 +475,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
 	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
 		return -EMSGSIZE;
 
-	return ret;
+	return 0;
 }
 
 static int
@@ -705,6 +764,70 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct dpll_device *dpll,
 	return 0;
 }
 
+static int
+dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
+		       struct netlink_ext_ack *extack)
+{
+	struct dpll_pin_ref *ref, *failed;
+	const struct dpll_pin_ops *ops;
+	s32 phase_adj, old_phase_adj;
+	struct dpll_device *dpll;
+	unsigned long i;
+	int ret;
+
+	phase_adj = nla_get_s32(phase_adj_attr);
+	if (phase_adj > pin->prop->phase_range.max ||
+	    phase_adj < pin->prop->phase_range.min) {
+		NL_SET_ERR_MSG(extack, "phase adjust value not supported");
+		return -EINVAL;
+	}
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		ops = dpll_pin_ops(ref);
+		if (!ops->phase_adjust_set || !ops->phase_adjust_get)
+			return -EOPNOTSUPP;
+	}
+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
+	ops = dpll_pin_ops(ref);
+	dpll = ref->dpll;
+	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				    dpll, dpll_priv(dpll), &old_phase_adj,
+				    extack);
+	if (ret) {
+		NL_SET_ERR_MSG(extack, "unable to get old phase adjust value");
+		return ret;
+	}
+	if (phase_adj == old_phase_adj)
+		return 0;
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		ops = dpll_pin_ops(ref);
+		dpll = ref->dpll;
+		ret = ops->phase_adjust_set(pin,
+					    dpll_pin_on_dpll_priv(dpll, pin),
+					    dpll, dpll_priv(dpll), phase_adj,
+					    extack);
+		if (ret) {
+			failed = ref;
+			goto rollback;
+		}
+	}
+	__dpll_pin_change_ntf(pin);
+
+	return 0;
+
+rollback:
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		if (ref == failed)
+			break;
+		ops = dpll_pin_ops(ref);
+		dpll = ref->dpll;
+		if (ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
+					  dpll, dpll_priv(dpll), old_phase_adj,
+					  extack))
+			NL_SET_ERR_MSG(extack, "set phase adjust rollback failed");
+	}
+	return ret;
+}
+
 static int
 dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr *parent_nest,
 			   struct netlink_ext_ack *extack)
@@ -793,6 +916,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
 			if (ret)
 				return ret;
 			break;
+		case DPLL_A_PIN_PHASE_ADJUST:
+			ret = dpll_pin_phase_adj_set(pin, a, info->extack);
+			if (ret)
+				return ret;
+			break;
 		case DPLL_A_PIN_PARENT_DEVICE:
 			ret = dpll_pin_parent_device_set(pin, a, info->extack);
 			if (ret)
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index bbc480cd2932..578fc5fa3750 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -68,6 +68,18 @@ struct dpll_pin_ops {
 	int (*prio_set)(const struct dpll_pin *pin, void *pin_priv,
 			const struct dpll_device *dpll, void *dpll_priv,
 			const u32 prio, struct netlink_ext_ack *extack);
+	int (*phase_offset_get)(const struct dpll_pin *pin, void *pin_priv,
+				const struct dpll_device *dpll, void *dpll_priv,
+				s64 *phase_offset,
+				struct netlink_ext_ack *extack);
+	int (*phase_adjust_get)(const struct dpll_pin *pin, void *pin_priv,
+				const struct dpll_device *dpll, void *dpll_priv,
+				s32 *phase_adjust,
+				struct netlink_ext_ack *extack);
+	int (*phase_adjust_set)(const struct dpll_pin *pin, void *pin_priv,
+				const struct dpll_device *dpll, void *dpll_priv,
+				const s32 phase_adjust,
+				struct netlink_ext_ack *extack);
 };
 
 struct dpll_pin_frequency {
@@ -91,6 +103,11 @@ struct dpll_pin_frequency {
 #define DPLL_PIN_FREQUENCY_DCF77 \
 	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ)
 
+struct dpll_pin_phase_adjust_range {
+	s32 min;
+	s32 max;
+};
+
 struct dpll_pin_properties {
 	const char *board_label;
 	const char *panel_label;
@@ -99,6 +116,7 @@ struct dpll_pin_properties {
 	unsigned long capabilities;
 	u32 freq_supported_num;
 	struct dpll_pin_frequency *freq_supported;
+	struct dpll_pin_phase_adjust_range phase_range;
 };
 
 #if IS_ENABLED(CONFIG_DPLL)
-- 
2.38.1

Re: [PATCH net-next v3 3/5] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust

[Jiri Pirko]

Fri, Oct 06, 2023 at 01:40:59PM CEST, arkadiusz.kubalewski@intel.com wrote:
>Add callback ops for pin-dpll phase measurment.
>Add callback for pin signal phase adjustment.
>Add min and max phase adjustment values to pin proprties.
>Invoke callbacks in dpll_netlink.c when filling the pin details to
>provide user with phase related attribute values.
>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> drivers/dpll/dpll_netlink.c | 130 +++++++++++++++++++++++++++++++++++-
> include/linux/dpll.h        |  18 +++++
> 2 files changed, 147 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index e20daba6896a..97319a9e4667 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -212,6 +212,53 @@ dpll_msg_add_pin_direction(struct sk_buff *msg, struct dpll_pin *pin,
> 	return 0;
> }
> 
>+static int
>+dpll_msg_add_pin_phase_adjust(struct sk_buff *msg, struct dpll_pin *pin,
>+			      struct dpll_pin_ref *ref,
>+			      struct netlink_ext_ack *extack)
>+{
>+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>+	struct dpll_device *dpll = ref->dpll;
>+	s32 phase_adjust;
>+	int ret;
>+
>+	if (!ops->phase_adjust_get)
>+		return 0;
>+	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+				    dpll, dpll_priv(dpll),
>+				    &phase_adjust, extack);
>+	if (ret)
>+		return ret;
>+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST, phase_adjust))
>+		return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
>+static int
>+dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin,
>+			  struct dpll_pin_ref *ref,
>+			  struct netlink_ext_ack *extack)
>+{
>+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>+	struct dpll_device *dpll = ref->dpll;
>+	s64 phase_offset;
>+	int ret;
>+
>+	if (!ops->phase_offset_get)
>+		return 0;
>+	ret = ops->phase_offset_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+				    dpll, dpll_priv(dpll), &phase_offset,
>+				    extack);
>+	if (ret)
>+		return ret;
>+	if (nla_put_64bit(msg, DPLL_A_PIN_PHASE_OFFSET, sizeof(phase_offset),
>+			  &phase_offset, DPLL_A_PIN_PAD))
>+		return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
> static int
> dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
> 		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
>@@ -330,6 +377,9 @@ dpll_msg_add_pin_dplls(struct sk_buff *msg, struct dpll_pin *pin,
> 		if (ret)
> 			goto nest_cancel;
> 		ret = dpll_msg_add_pin_direction(msg, pin, ref, extack);
>+		if (ret)
>+			goto nest_cancel;
>+		ret = dpll_msg_add_phase_offset(msg, pin, ref, extack);
> 		if (ret)
> 			goto nest_cancel;
> 		nla_nest_end(msg, attr);
>@@ -377,6 +427,15 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
> 	if (nla_put_u32(msg, DPLL_A_PIN_CAPABILITIES, prop->capabilities))
> 		return -EMSGSIZE;
> 	ret = dpll_msg_add_pin_freq(msg, pin, ref, extack);
>+	if (ret)
>+		return ret;
>+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN,
>+			prop->phase_range.min))
>+		return -EMSGSIZE;
>+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MAX,
>+			prop->phase_range.max))
>+		return -EMSGSIZE;
>+	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
> 	if (ret)
> 		return ret;
> 	if (xa_empty(&pin->parent_refs))
>@@ -416,7 +475,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
> 	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
> 		return -EMSGSIZE;
> 
>-	return ret;
>+	return 0;
> }
> 
> static int
>@@ -705,6 +764,70 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct dpll_device *dpll,
> 	return 0;
> }
> 
>+static int
>+dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
>+		       struct netlink_ext_ack *extack)
>+{
>+	struct dpll_pin_ref *ref, *failed;
>+	const struct dpll_pin_ops *ops;
>+	s32 phase_adj, old_phase_adj;
>+	struct dpll_device *dpll;
>+	unsigned long i;
>+	int ret;
>+
>+	phase_adj = nla_get_s32(phase_adj_attr);
>+	if (phase_adj > pin->prop->phase_range.max ||
>+	    phase_adj < pin->prop->phase_range.min) {
>+		NL_SET_ERR_MSG(extack, "phase adjust value not supported");
>+		return -EINVAL;
>+	}
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		ops = dpll_pin_ops(ref);
>+		if (!ops->phase_adjust_set || !ops->phase_adjust_get)

Extack msg.


>+			return -EOPNOTSUPP;
>+	}
>+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>+	ops = dpll_pin_ops(ref);
>+	dpll = ref->dpll;
>+	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+				    dpll, dpll_priv(dpll), &old_phase_adj,
>+				    extack);
>+	if (ret) {
>+		NL_SET_ERR_MSG(extack, "unable to get old phase adjust value");
>+		return ret;
>+	}
>+	if (phase_adj == old_phase_adj)
>+		return 0;
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		ops = dpll_pin_ops(ref);
>+		dpll = ref->dpll;
>+		ret = ops->phase_adjust_set(pin,
>+					    dpll_pin_on_dpll_priv(dpll, pin),
>+					    dpll, dpll_priv(dpll), phase_adj,
>+					    extack);
>+		if (ret) {
>+			failed = ref;

Extack msg.

>+			goto rollback;
>+		}
>+	}
>+	__dpll_pin_change_ntf(pin);
>+
>+	return 0;
>+
>+rollback:
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		if (ref == failed)
>+			break;
>+		ops = dpll_pin_ops(ref);
>+		dpll = ref->dpll;
>+		if (ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+					  dpll, dpll_priv(dpll), old_phase_adj,
>+					  extack))
>+			NL_SET_ERR_MSG(extack, "set phase adjust rollback failed");
>+	}
>+	return ret;
>+}
>+
> static int
> dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr *parent_nest,
> 			   struct netlink_ext_ack *extack)
>@@ -793,6 +916,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
> 			if (ret)
> 				return ret;
> 			break;
>+		case DPLL_A_PIN_PHASE_ADJUST:
>+			ret = dpll_pin_phase_adj_set(pin, a, info->extack);
>+			if (ret)
>+				return ret;
>+			break;
> 		case DPLL_A_PIN_PARENT_DEVICE:
> 			ret = dpll_pin_parent_device_set(pin, a, info->extack);
> 			if (ret)
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index bbc480cd2932..578fc5fa3750 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -68,6 +68,18 @@ struct dpll_pin_ops {
> 	int (*prio_set)(const struct dpll_pin *pin, void *pin_priv,
> 			const struct dpll_device *dpll, void *dpll_priv,
> 			const u32 prio, struct netlink_ext_ack *extack);
>+	int (*phase_offset_get)(const struct dpll_pin *pin, void *pin_priv,
>+				const struct dpll_device *dpll, void *dpll_priv,
>+				s64 *phase_offset,
>+				struct netlink_ext_ack *extack);
>+	int (*phase_adjust_get)(const struct dpll_pin *pin, void *pin_priv,
>+				const struct dpll_device *dpll, void *dpll_priv,
>+				s32 *phase_adjust,
>+				struct netlink_ext_ack *extack);
>+	int (*phase_adjust_set)(const struct dpll_pin *pin, void *pin_priv,
>+				const struct dpll_device *dpll, void *dpll_priv,
>+				const s32 phase_adjust,
>+				struct netlink_ext_ack *extack);
> };
> 
> struct dpll_pin_frequency {
>@@ -91,6 +103,11 @@ struct dpll_pin_frequency {
> #define DPLL_PIN_FREQUENCY_DCF77 \
> 	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ)
> 
>+struct dpll_pin_phase_adjust_range {
>+	s32 min;
>+	s32 max;
>+};
>+
> struct dpll_pin_properties {
> 	const char *board_label;
> 	const char *panel_label;
>@@ -99,6 +116,7 @@ struct dpll_pin_properties {
> 	unsigned long capabilities;
> 	u32 freq_supported_num;
> 	struct dpll_pin_frequency *freq_supported;
>+	struct dpll_pin_phase_adjust_range phase_range;
> };
> 
> #if IS_ENABLED(CONFIG_DPLL)
>-- 
>2.38.1
>

RE: [PATCH net-next v3 3/5] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Friday, October 6, 2023 2:38 PM
>
>Fri, Oct 06, 2023 at 01:40:59PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>Add callback ops for pin-dpll phase measurment.
>>Add callback for pin signal phase adjustment.
>>Add min and max phase adjustment values to pin proprties.
>>Invoke callbacks in dpll_netlink.c when filling the pin details to
>>provide user with phase related attribute values.
>>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>> drivers/dpll/dpll_netlink.c | 130 +++++++++++++++++++++++++++++++++++-
>> include/linux/dpll.h        |  18 +++++
>> 2 files changed, 147 insertions(+), 1 deletion(-)
>>
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index e20daba6896a..97319a9e4667 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -212,6 +212,53 @@ dpll_msg_add_pin_direction(struct sk_buff *msg,
>>struct dpll_pin *pin,
>> 	return 0;
>> }
>>
>>+static int
>>+dpll_msg_add_pin_phase_adjust(struct sk_buff *msg, struct dpll_pin *pin,
>>+			      struct dpll_pin_ref *ref,
>>+			      struct netlink_ext_ack *extack)
>>+{
>>+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>>+	struct dpll_device *dpll = ref->dpll;
>>+	s32 phase_adjust;
>>+	int ret;
>>+
>>+	if (!ops->phase_adjust_get)
>>+		return 0;
>>+	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>>+				    dpll, dpll_priv(dpll),
>>+				    &phase_adjust, extack);
>>+	if (ret)
>>+		return ret;
>>+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST, phase_adjust))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>>+static int
>>+dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin,
>>+			  struct dpll_pin_ref *ref,
>>+			  struct netlink_ext_ack *extack)
>>+{
>>+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>>+	struct dpll_device *dpll = ref->dpll;
>>+	s64 phase_offset;
>>+	int ret;
>>+
>>+	if (!ops->phase_offset_get)
>>+		return 0;
>>+	ret = ops->phase_offset_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>>+				    dpll, dpll_priv(dpll), &phase_offset,
>>+				    extack);
>>+	if (ret)
>>+		return ret;
>>+	if (nla_put_64bit(msg, DPLL_A_PIN_PHASE_OFFSET, sizeof(phase_offset),
>>+			  &phase_offset, DPLL_A_PIN_PAD))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>> static int
>> dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
>> 		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
>>@@ -330,6 +377,9 @@ dpll_msg_add_pin_dplls(struct sk_buff *msg, struct
>>dpll_pin *pin,
>> 		if (ret)
>> 			goto nest_cancel;
>> 		ret = dpll_msg_add_pin_direction(msg, pin, ref, extack);
>>+		if (ret)
>>+			goto nest_cancel;
>>+		ret = dpll_msg_add_phase_offset(msg, pin, ref, extack);
>> 		if (ret)
>> 			goto nest_cancel;
>> 		nla_nest_end(msg, attr);
>>@@ -377,6 +427,15 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct
>>dpll_pin *pin,
>> 	if (nla_put_u32(msg, DPLL_A_PIN_CAPABILITIES, prop->capabilities))
>> 		return -EMSGSIZE;
>> 	ret = dpll_msg_add_pin_freq(msg, pin, ref, extack);
>>+	if (ret)
>>+		return ret;
>>+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN,
>>+			prop->phase_range.min))
>>+		return -EMSGSIZE;
>>+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MAX,
>>+			prop->phase_range.max))
>>+		return -EMSGSIZE;
>>+	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
>> 	if (ret)
>> 		return ret;
>> 	if (xa_empty(&pin->parent_refs))
>>@@ -416,7 +475,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>sk_buff *msg,
>> 	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>> 		return -EMSGSIZE;
>>
>>-	return ret;
>>+	return 0;
>> }
>>
>> static int
>>@@ -705,6 +764,70 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct
>>dpll_device *dpll,
>> 	return 0;
>> }
>>
>>+static int
>>+dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
>>*phase_adj_attr,
>>+		       struct netlink_ext_ack *extack)
>>+{
>>+	struct dpll_pin_ref *ref, *failed;
>>+	const struct dpll_pin_ops *ops;
>>+	s32 phase_adj, old_phase_adj;
>>+	struct dpll_device *dpll;
>>+	unsigned long i;
>>+	int ret;
>>+
>>+	phase_adj = nla_get_s32(phase_adj_attr);
>>+	if (phase_adj > pin->prop->phase_range.max ||
>>+	    phase_adj < pin->prop->phase_range.min) {
>>+		NL_SET_ERR_MSG(extack, "phase adjust value not supported");
>>+		return -EINVAL;
>>+	}
>>+	xa_for_each(&pin->dpll_refs, i, ref) {
>>+		ops = dpll_pin_ops(ref);
>>+		if (!ops->phase_adjust_set || !ops->phase_adjust_get)
>
>Extack msg.
>

Fixed.

>
>>+			return -EOPNOTSUPP;
>>+	}
>>+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>>+	ops = dpll_pin_ops(ref);
>>+	dpll = ref->dpll;
>>+	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>>+				    dpll, dpll_priv(dpll), &old_phase_adj,
>>+				    extack);
>>+	if (ret) {
>>+		NL_SET_ERR_MSG(extack, "unable to get old phase adjust value");
>>+		return ret;
>>+	}
>>+	if (phase_adj == old_phase_adj)
>>+		return 0;
>>+	xa_for_each(&pin->dpll_refs, i, ref) {
>>+		ops = dpll_pin_ops(ref);
>>+		dpll = ref->dpll;
>>+		ret = ops->phase_adjust_set(pin,
>>+					    dpll_pin_on_dpll_priv(dpll, pin),
>>+					    dpll, dpll_priv(dpll), phase_adj,
>>+					    extack);
>>+		if (ret) {
>>+			failed = ref;
>
>Extack msg.
>

Fixed.

Thank you!
Arkadiusz

>>+			goto rollback;
>>+		}
>>+	}
>>+	__dpll_pin_change_ntf(pin);
>>+
>>+	return 0;
>>+
>>+rollback:
>>+	xa_for_each(&pin->dpll_refs, i, ref) {
>>+		if (ref == failed)
>>+			break;
>>+		ops = dpll_pin_ops(ref);
>>+		dpll = ref->dpll;
>>+		if (ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll,
>>pin),
>>+					  dpll, dpll_priv(dpll), old_phase_adj,
>>+					  extack))
>>+			NL_SET_ERR_MSG(extack, "set phase adjust rollback
>>failed");
>>+	}
>>+	return ret;
>>+}
>>+
>> static int
>> dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr
>>*parent_nest,
>> 			   struct netlink_ext_ack *extack)
>>@@ -793,6 +916,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct
>>genl_info *info)
>> 			if (ret)
>> 				return ret;
>> 			break;
>>+		case DPLL_A_PIN_PHASE_ADJUST:
>>+			ret = dpll_pin_phase_adj_set(pin, a, info->extack);
>>+			if (ret)
>>+				return ret;
>>+			break;
>> 		case DPLL_A_PIN_PARENT_DEVICE:
>> 			ret = dpll_pin_parent_device_set(pin, a, info->extack);
>> 			if (ret)
>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>index bbc480cd2932..578fc5fa3750 100644
>>--- a/include/linux/dpll.h
>>+++ b/include/linux/dpll.h
>>@@ -68,6 +68,18 @@ struct dpll_pin_ops {
>> 	int (*prio_set)(const struct dpll_pin *pin, void *pin_priv,
>> 			const struct dpll_device *dpll, void *dpll_priv,
>> 			const u32 prio, struct netlink_ext_ack *extack);
>>+	int (*phase_offset_get)(const struct dpll_pin *pin, void *pin_priv,
>>+				const struct dpll_device *dpll, void *dpll_priv,
>>+				s64 *phase_offset,
>>+				struct netlink_ext_ack *extack);
>>+	int (*phase_adjust_get)(const struct dpll_pin *pin, void *pin_priv,
>>+				const struct dpll_device *dpll, void *dpll_priv,
>>+				s32 *phase_adjust,
>>+				struct netlink_ext_ack *extack);
>>+	int (*phase_adjust_set)(const struct dpll_pin *pin, void *pin_priv,
>>+				const struct dpll_device *dpll, void *dpll_priv,
>>+				const s32 phase_adjust,
>>+				struct netlink_ext_ack *extack);
>> };
>>
>> struct dpll_pin_frequency {
>>@@ -91,6 +103,11 @@ struct dpll_pin_frequency {
>> #define DPLL_PIN_FREQUENCY_DCF77 \
>> 	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ)
>>
>>+struct dpll_pin_phase_adjust_range {
>>+	s32 min;
>>+	s32 max;
>>+};
>>+
>> struct dpll_pin_properties {
>> 	const char *board_label;
>> 	const char *panel_label;
>>@@ -99,6 +116,7 @@ struct dpll_pin_properties {
>> 	unsigned long capabilities;
>> 	u32 freq_supported_num;
>> 	struct dpll_pin_frequency *freq_supported;
>>+	struct dpll_pin_phase_adjust_range phase_range;
>> };
>>
>> #if IS_ENABLED(CONFIG_DPLL)
>>--
>>2.38.1
>>

[PATCH net-next v3 4/5] ice: dpll: implement phase related callbacks

[Arkadiusz Kubalewski]

Implement new callback ops related to measurment and adjustment of
signal phase for pin-dpll in ice driver.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_dpll.c | 224 +++++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_dpll.h |  10 +-
 2 files changed, 230 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 1faee9cb944d..6f7a2916f6c2 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -878,6 +878,203 @@ ice_dpll_output_direction(const struct dpll_pin *pin, void *pin_priv,
 	return 0;
 }
 
+/**
+ * ice_dpll_pin_phase_adjust_get - callback for get pin phase adjust value
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @phase_adjust: on success holds pin phase_adjust value
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Handler for getting phase adjust value of a pin.
+ *
+ * Context: Acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_pin_phase_adjust_get(const struct dpll_pin *pin, void *pin_priv,
+			      const struct dpll_device *dpll, void *dpll_priv,
+			      s32 *phase_adjust,
+			      struct netlink_ext_ack *extack)
+{
+	struct ice_dpll_pin *p = pin_priv;
+	struct ice_pf *pf = p->pf;
+
+	mutex_lock(&pf->dplls.lock);
+	*phase_adjust = p->phase_adjust;
+	mutex_unlock(&pf->dplls.lock);
+
+	return 0;
+}
+
+/**
+ * ice_dpll_pin_phase_adjust_set - helper for setting a pin phase adjust value
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @phase_adjust: phase_adjust to be set
+ * @extack: error reporting
+ * @type: type of a pin
+ *
+ * Helper for dpll subsystem callback. Handler for setting phase adjust value
+ * of a pin.
+ *
+ * Context: Acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_pin_phase_adjust_set(const struct dpll_pin *pin, void *pin_priv,
+			      const struct dpll_device *dpll, void *dpll_priv,
+			      s32 phase_adjust,
+			      struct netlink_ext_ack *extack,
+			      enum ice_dpll_pin_type type)
+{
+	struct ice_dpll_pin *p = pin_priv;
+	struct ice_dpll *d = dpll_priv;
+	struct ice_pf *pf = d->pf;
+	u8 flag, flags_en = 0;
+	int ret;
+
+	mutex_lock(&pf->dplls.lock);
+	if (phase_adjust == p->phase_adjust) {
+		mutex_unlock(&pf->dplls.lock);
+		return 0;
+	}
+	switch (type) {
+	case ICE_DPLL_PIN_TYPE_INPUT:
+		flag = ICE_AQC_SET_CGU_IN_CFG_FLG1_UPDATE_DELAY;
+		if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)
+			flags_en |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
+		if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_INPUT_EN)
+			flags_en |= ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN;
+		ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, flag, flags_en,
+					       0, phase_adjust);
+		break;
+	case ICE_DPLL_PIN_TYPE_OUTPUT:
+		flag = ICE_AQC_SET_CGU_OUT_CFG_UPDATE_PHASE;
+		if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_OUT_EN)
+			flag |= ICE_AQC_SET_CGU_OUT_CFG_OUT_EN;
+		if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)
+			flag |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
+		ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flag, 0, 0,
+						phase_adjust);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	if (!ret)
+		p->phase_adjust = phase_adjust;
+	mutex_unlock(&pf->dplls.lock);
+	if (ret)
+		NL_SET_ERR_MSG_FMT(extack,
+				   "err:%d %s failed to set pin phase_adjust:%d for pin:%u on dpll:%u\n",
+				   ret,
+				   ice_aq_str(pf->hw.adminq.sq_last_status),
+				   phase_adjust, p->idx, d->dpll_idx);
+
+	return ret;
+}
+
+/**
+ * ice_dpll_input_phase_adjust_set - callback for set input pin phase adjust
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @phase_adjust: phase_adjust to be set
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Wraps a handler for setting phase adjust on input
+ * pin.
+ *
+ * Context: Calls a function which acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_input_phase_adjust_set(const struct dpll_pin *pin, void *pin_priv,
+				const struct dpll_device *dpll, void *dpll_priv,
+				s32 phase_adjust,
+				struct netlink_ext_ack *extack)
+{
+	return ice_dpll_pin_phase_adjust_set(pin, pin_priv, dpll, dpll_priv,
+					     phase_adjust, extack,
+					     ICE_DPLL_PIN_TYPE_INPUT);
+}
+
+/**
+ * ice_dpll_output_phase_adjust_set - callback for set output pin phase adjust
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @phase_adjust: phase_adjust to be set
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Wraps a handler for setting phase adjust on output
+ * pin.
+ *
+ * Context: Calls a function which acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_output_phase_adjust_set(const struct dpll_pin *pin, void *pin_priv,
+				 const struct dpll_device *dpll, void *dpll_priv,
+				 s32 phase_adjust,
+				 struct netlink_ext_ack *extack)
+{
+	return ice_dpll_pin_phase_adjust_set(pin, pin_priv, dpll, dpll_priv,
+					     phase_adjust, extack,
+					     ICE_DPLL_PIN_TYPE_OUTPUT);
+}
+
+#define ICE_DPLL_PHASE_OFFSET_DIVIDER	100
+#define ICE_DPLL_PHASE_OFFSET_FACTOR		\
+	(DPLL_PHASE_OFFSET_DIVIDER / ICE_DPLL_PHASE_OFFSET_DIVIDER)
+/**
+ * ice_dpll_phase_offset_get - callback for get dpll phase shift value
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @phase_adjust: on success holds pin phase_adjust value
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Handler for getting phase shift value between
+ * dpll's input and output.
+ *
+ * Context: Acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
+			  const struct dpll_device *dpll, void *dpll_priv,
+			  s64 *phase_offset, struct netlink_ext_ack *extack)
+{
+	struct ice_dpll *d = dpll_priv;
+	struct ice_pf *pf = d->pf;
+
+	mutex_lock(&pf->dplls.lock);
+	if (d->active_input == pin)
+		*phase_offset = d->phase_offset * ICE_DPLL_PHASE_OFFSET_FACTOR;
+	else
+		*phase_offset = 0;
+	mutex_unlock(&pf->dplls.lock);
+
+	return 0;
+}
+
 /**
  * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin
  * @pin: pointer to a pin
@@ -993,6 +1190,9 @@ static const struct dpll_pin_ops ice_dpll_input_ops = {
 	.prio_get = ice_dpll_input_prio_get,
 	.prio_set = ice_dpll_input_prio_set,
 	.direction_get = ice_dpll_input_direction,
+	.phase_adjust_get = ice_dpll_pin_phase_adjust_get,
+	.phase_adjust_set = ice_dpll_input_phase_adjust_set,
+	.phase_offset_get = ice_dpll_phase_offset_get,
 };
 
 static const struct dpll_pin_ops ice_dpll_output_ops = {
@@ -1001,6 +1201,8 @@ static const struct dpll_pin_ops ice_dpll_output_ops = {
 	.state_on_dpll_get = ice_dpll_output_state_get,
 	.state_on_dpll_set = ice_dpll_output_state_set,
 	.direction_get = ice_dpll_output_direction,
+	.phase_adjust_get = ice_dpll_pin_phase_adjust_get,
+	.phase_adjust_set = ice_dpll_output_phase_adjust_set,
 };
 
 static const struct dpll_device_ops ice_dpll_ops = {
@@ -1031,6 +1233,8 @@ static u64 ice_generate_clock_id(struct ice_pf *pf)
  */
 static void ice_dpll_notify_changes(struct ice_dpll *d)
 {
+	bool pin_notified = false;
+
 	if (d->prev_dpll_state != d->dpll_state) {
 		d->prev_dpll_state = d->dpll_state;
 		dpll_device_change_ntf(d->dpll);
@@ -1039,7 +1243,14 @@ static void ice_dpll_notify_changes(struct ice_dpll *d)
 		if (d->prev_input)
 			dpll_pin_change_ntf(d->prev_input);
 		d->prev_input = d->active_input;
-		if (d->active_input)
+		if (d->active_input) {
+			dpll_pin_change_ntf(d->active_input);
+			pin_notified = true;
+		}
+	}
+	if (d->prev_phase_offset != d->phase_offset) {
+		d->prev_phase_offset = d->phase_offset;
+		if (!pin_notified && d->active_input)
 			dpll_pin_change_ntf(d->active_input);
 	}
 }
@@ -1065,7 +1276,7 @@ ice_dpll_update_state(struct ice_pf *pf, struct ice_dpll *d, bool init)
 
 	ret = ice_get_cgu_state(&pf->hw, d->dpll_idx, d->prev_dpll_state,
 				&d->input_idx, &d->ref_state, &d->eec_mode,
-				&d->phase_shift, &d->dpll_state);
+				&d->phase_offset, &d->dpll_state);
 
 	dev_dbg(ice_pf_to_dev(pf),
 		"update dpll=%d, prev_src_idx:%u, src_idx:%u, state:%d, prev:%d mode:%d\n",
@@ -1656,6 +1867,15 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
 				return ret;
 			pins[i].prop.capabilities |=
 				DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE;
+			pins[i].prop.phase_range.min =
+				pf->dplls.input_phase_adj_max;
+			pins[i].prop.phase_range.max =
+				-pf->dplls.input_phase_adj_max;
+		} else {
+			pins[i].prop.phase_range.min =
+				pf->dplls.output_phase_adj_max,
+			pins[i].prop.phase_range.max =
+				-pf->dplls.output_phase_adj_max;
 		}
 		pins[i].prop.capabilities |=
 			DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
index 2dfe764b81e1..bb32b6d88373 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
@@ -19,6 +19,7 @@
  * @state: state of a pin
  * @prop: pin properties
  * @freq: current frequency of a pin
+ * @phase_adjust: current phase adjust value
  */
 struct ice_dpll_pin {
 	struct dpll_pin *pin;
@@ -30,6 +31,7 @@ struct ice_dpll_pin {
 	u8 state[ICE_DPLL_RCLK_NUM_MAX];
 	struct dpll_pin_properties prop;
 	u32 freq;
+	s32 phase_adjust;
 };
 
 /** ice_dpll - store info required for DPLL control
@@ -40,7 +42,8 @@ struct ice_dpll_pin {
  * @prev_input_idx: previously selected input index
  * @ref_state: state of dpll reference signals
  * @eec_mode: eec_mode dpll is configured for
- * @phase_shift: phase shift delay of a dpll
+ * @phase_offset: phase offset of active pin vs dpll signal
+ * @prev_phase_offset: previous phase offset of active pin vs dpll signal
  * @input_prio: priorities of each input
  * @dpll_state: current dpll sync state
  * @prev_dpll_state: last dpll sync state
@@ -55,7 +58,8 @@ struct ice_dpll {
 	u8 prev_input_idx;
 	u8 ref_state;
 	u8 eec_mode;
-	s64 phase_shift;
+	s64 phase_offset;
+	s64 prev_phase_offset;
 	u8 *input_prio;
 	enum dpll_lock_status dpll_state;
 	enum dpll_lock_status prev_dpll_state;
@@ -78,6 +82,8 @@ struct ice_dpll {
  * @cgu_state_acq_err_num: number of errors returned during periodic work
  * @base_rclk_idx: idx of first pin used for clock revocery pins
  * @clock_id: clock_id of dplls
+ * @input_phase_adj_max: max phase adjust value for an input pins
+ * @output_phase_adj_max: max phase adjust value for an output pins
  */
 struct ice_dplls {
 	struct kthread_worker *kworker;
-- 
2.38.1

Re: [PATCH net-next v3 4/5] ice: dpll: implement phase related callbacks

[Simon Horman]

On Fri, Oct 06, 2023 at 01:41:00PM +0200, Arkadiusz Kubalewski wrote:
> Implement new callback ops related to measurment and adjustment of
> signal phase for pin-dpll in ice driver.
> 
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Hi Arkadiusz,

some minor feedback from my side.

If you do end up re-spinning the series, please consider
running checkpatch.pl --codespell.

> ---
>  drivers/net/ethernet/intel/ice/ice_dpll.c | 224 +++++++++++++++++++++-
>  drivers/net/ethernet/intel/ice/ice_dpll.h |  10 +-
>  2 files changed, 230 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c

...

> +/**
> + * ice_dpll_phase_offset_get - callback for get dpll phase shift value
> + * @pin: pointer to a pin
> + * @pin_priv: private data pointer passed on pin registration
> + * @dpll: registered dpll pointer
> + * @dpll_priv: private data pointer passed on dpll registration
> + * @phase_adjust: on success holds pin phase_adjust value

nit: The parameter is called phase_offset, not phase_adjust in the code below

> + * @extack: error reporting
> + *
> + * Dpll subsystem callback. Handler for getting phase shift value between
> + * dpll's input and output.
> + *
> + * Context: Acquires pf->dplls.lock
> + * Return:
> + * * 0 - success
> + * * negative - error
> + */
> +static int
> +ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
> +			  const struct dpll_device *dpll, void *dpll_priv,
> +			  s64 *phase_offset, struct netlink_ext_ack *extack)
> +{
> +	struct ice_dpll *d = dpll_priv;
> +	struct ice_pf *pf = d->pf;
> +
> +	mutex_lock(&pf->dplls.lock);
> +	if (d->active_input == pin)
> +		*phase_offset = d->phase_offset * ICE_DPLL_PHASE_OFFSET_FACTOR;
> +	else
> +		*phase_offset = 0;
> +	mutex_unlock(&pf->dplls.lock);
> +
> +	return 0;
> +}
> +
>  /**
>   * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin
>   * @pin: pointer to a pin

...

> @@ -1656,6 +1867,15 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
>  				return ret;
>  			pins[i].prop.capabilities |=
>  				DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE;
> +			pins[i].prop.phase_range.min =
> +				pf->dplls.input_phase_adj_max;
> +			pins[i].prop.phase_range.max =
> +				-pf->dplls.input_phase_adj_max;
> +		} else {
> +			pins[i].prop.phase_range.min =
> +				pf->dplls.output_phase_adj_max,

nit: It probably doesn't make any difference, but perhaps ',' should be ';'.

As flagged by clang-16 with -Wcomma

> +			pins[i].prop.phase_range.max =
> +				-pf->dplls.output_phase_adj_max;
>  		}
>  		pins[i].prop.capabilities |=
>  			DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;

...

Re: [PATCH net-next v3 4/5] ice: dpll: implement phase related callbacks

[Jiri Pirko]

Fri, Oct 06, 2023 at 02:33:34PM CEST, horms@kernel.org wrote:
>On Fri, Oct 06, 2023 at 01:41:00PM +0200, Arkadiusz Kubalewski wrote:
>> Implement new callback ops related to measurment and adjustment of
>> signal phase for pin-dpll in ice driver.
>> 
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>
>Hi Arkadiusz,
>
>some minor feedback from my side.
>
>If you do end up re-spinning the series, please consider
>running checkpatch.pl --codespell.
>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_dpll.c | 224 +++++++++++++++++++++-
>>  drivers/net/ethernet/intel/ice/ice_dpll.h |  10 +-
>>  2 files changed, 230 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
>
>...
>
>> +/**
>> + * ice_dpll_phase_offset_get - callback for get dpll phase shift value
>> + * @pin: pointer to a pin
>> + * @pin_priv: private data pointer passed on pin registration
>> + * @dpll: registered dpll pointer
>> + * @dpll_priv: private data pointer passed on dpll registration
>> + * @phase_adjust: on success holds pin phase_adjust value
>
>nit: The parameter is called phase_offset, not phase_adjust in the code below

Yeah, the non-sense static function docs and how buggy they are. Nobody
reads them anyway. Same old story for ice I guess....


>
>> + * @extack: error reporting
>> + *
>> + * Dpll subsystem callback. Handler for getting phase shift value between
>> + * dpll's input and output.
>> + *
>> + * Context: Acquires pf->dplls.lock
>> + * Return:
>> + * * 0 - success
>> + * * negative - error
>> + */
>> +static int
>> +ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
>> +			  const struct dpll_device *dpll, void *dpll_priv,
>> +			  s64 *phase_offset, struct netlink_ext_ack *extack)
>> +{
>> +	struct ice_dpll *d = dpll_priv;
>> +	struct ice_pf *pf = d->pf;
>> +
>> +	mutex_lock(&pf->dplls.lock);
>> +	if (d->active_input == pin)
>> +		*phase_offset = d->phase_offset * ICE_DPLL_PHASE_OFFSET_FACTOR;
>> +	else
>> +		*phase_offset = 0;
>> +	mutex_unlock(&pf->dplls.lock);
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin
>>   * @pin: pointer to a pin
>
>...
>
>> @@ -1656,6 +1867,15 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
>>  				return ret;
>>  			pins[i].prop.capabilities |=
>>  				DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE;
>> +			pins[i].prop.phase_range.min =
>> +				pf->dplls.input_phase_adj_max;
>> +			pins[i].prop.phase_range.max =
>> +				-pf->dplls.input_phase_adj_max;
>> +		} else {
>> +			pins[i].prop.phase_range.min =
>> +				pf->dplls.output_phase_adj_max,
>
>nit: It probably doesn't make any difference, but perhaps ',' should be ';'.
>
>As flagged by clang-16 with -Wcomma
>
>> +			pins[i].prop.phase_range.max =
>> +				-pf->dplls.output_phase_adj_max;
>>  		}
>>  		pins[i].prop.capabilities |=
>>  			DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
>
>...

[PATCH net-next v3 5/5] dpll: netlink/core: change pin frequency set behavior

[Arkadiusz Kubalewski]

Align the aproach of pin frequency set behavior with the approach
introduced with pin phase adjust set.
Fail the request if any of devices did not registered the callback ops.
If callback op on any pin's registered device fails, return error and
rollback the value to previous one.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/dpll/dpll_netlink.c | 50 +++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 97319a9e4667..8e5fea74aec1 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -615,30 +615,60 @@ static int
 dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 		  struct netlink_ext_ack *extack)
 {
-	u64 freq = nla_get_u64(a);
-	struct dpll_pin_ref *ref;
+	u64 freq = nla_get_u64(a), old_freq;
+	struct dpll_pin_ref *ref, *failed;
+	const struct dpll_pin_ops *ops;
+	struct dpll_device *dpll;
 	unsigned long i;
 	int ret;
 
 	if (!dpll_pin_is_freq_supported(pin, freq)) {
-		NL_SET_ERR_MSG_ATTR(extack, a, "frequency is not supported by the device");
+		NL_SET_ERR_MSG_ATTR(extack, a,
+				    "frequency is not supported by the device");
 		return -EINVAL;
 	}
-
 	xa_for_each(&pin->dpll_refs, i, ref) {
-		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
-		struct dpll_device *dpll = ref->dpll;
-
-		if (!ops->frequency_set)
+		ops = dpll_pin_ops(ref);
+		if (!ops->frequency_set || !ops->frequency_get)
 			return -EOPNOTSUPP;
+	}
+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
+	ops = dpll_pin_ops(ref);
+	dpll = ref->dpll;
+	ret = ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
+				 dpll_priv(dpll), &old_freq, extack);
+	if (ret) {
+		NL_SET_ERR_MSG(extack, "unable to get old frequency value");
+		return ret;
+	}
+	if (freq == old_freq)
+		return 0;
+
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		ops = dpll_pin_ops(ref);
+		dpll = ref->dpll;
 		ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
 					 dpll, dpll_priv(dpll), freq, extack);
-		if (ret)
-			return ret;
+		if (ret) {
+			failed = ref;
+			goto rollback;
+		}
 	}
 	__dpll_pin_change_ntf(pin);
 
 	return 0;
+
+rollback:
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		if (ref == failed)
+			break;
+		ops = dpll_pin_ops(ref);
+		dpll = ref->dpll;
+		if (ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				       dpll, dpll_priv(dpll), old_freq, extack))
+			NL_SET_ERR_MSG(extack, "set frequency rollback failed");
+	}
+	return ret;
 }
 
 static int
-- 
2.38.1

Re: [PATCH net-next v3 5/5] dpll: netlink/core: change pin frequency set behavior

[Jiri Pirko]

Fri, Oct 06, 2023 at 01:41:01PM CEST, arkadiusz.kubalewski@intel.com wrote:
>Align the aproach of pin frequency set behavior with the approach
>introduced with pin phase adjust set.
>Fail the request if any of devices did not registered the callback ops.
>If callback op on any pin's registered device fails, return error and
>rollback the value to previous one.
>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> drivers/dpll/dpll_netlink.c | 50 +++++++++++++++++++++++++++++--------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index 97319a9e4667..8e5fea74aec1 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -615,30 +615,60 @@ static int
> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
> 		  struct netlink_ext_ack *extack)
> {
>-	u64 freq = nla_get_u64(a);
>-	struct dpll_pin_ref *ref;
>+	u64 freq = nla_get_u64(a), old_freq;
>+	struct dpll_pin_ref *ref, *failed;
>+	const struct dpll_pin_ops *ops;
>+	struct dpll_device *dpll;
> 	unsigned long i;
> 	int ret;
> 
> 	if (!dpll_pin_is_freq_supported(pin, freq)) {
>-		NL_SET_ERR_MSG_ATTR(extack, a, "frequency is not supported by the device");
>+		NL_SET_ERR_MSG_ATTR(extack, a,
>+				    "frequency is not supported by the device");

No need for this wrapping. Seems unrelated to the rest of the patch
anyway.


> 		return -EINVAL;
> 	}
>-

No need for this too.



> 	xa_for_each(&pin->dpll_refs, i, ref) {
>-		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>-		struct dpll_device *dpll = ref->dpll;
>-
>-		if (!ops->frequency_set)
>+		ops = dpll_pin_ops(ref);
>+		if (!ops->frequency_set || !ops->frequency_get)
> 			return -EOPNOTSUPP;

Add an extack msg while you are at it - could be a separate patch.


>+	}
>+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>+	ops = dpll_pin_ops(ref);
>+	dpll = ref->dpll;
>+	ret = ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+				 dpll_priv(dpll), &old_freq, extack);
>+	if (ret) {
>+		NL_SET_ERR_MSG(extack, "unable to get old frequency value");
>+		return ret;
>+	}
>+	if (freq == old_freq)
>+		return 0;
>+
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		ops = dpll_pin_ops(ref);
>+		dpll = ref->dpll;
> 		ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
> 					 dpll, dpll_priv(dpll), freq, extack);
>-		if (ret)
>-			return ret;
>+		if (ret) {
>+			failed = ref;

Extack msg.


>+			goto rollback;
>+		}
> 	}
> 	__dpll_pin_change_ntf(pin);
> 
> 	return 0;
>+
>+rollback:
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		if (ref == failed)
>+			break;
>+		ops = dpll_pin_ops(ref);
>+		dpll = ref->dpll;
>+		if (ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+				       dpll, dpll_priv(dpll), old_freq, extack))
>+			NL_SET_ERR_MSG(extack, "set frequency rollback failed");
>+	}
>+	return ret;
> }
> 
> static int
>-- 
>2.38.1
>