Phase-adjust values are currently limited by a min-max range. Some
hardware requires, for certain pin types, that values be multiples of
a specific granularity, as in the zl3073x driver.
Add a `phase-adjust-gran` pin attribute and an appropriate field in
dpll_pin_properties. If set by the driver, use its value to validate
user-provided phase-adjust values.
Reviewed-by: Michal Schmidt <mschmidt@redhat.com>
Reviewed-by: Petr Oros <poros@redhat.com>
Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
Documentation/driver-api/dpll.rst | 36 +++++++++++++++------------
Documentation/netlink/specs/dpll.yaml | 7 ++++++
drivers/dpll/dpll_netlink.c | 12 ++++++++-
include/linux/dpll.h | 1 +
include/uapi/linux/dpll.h | 1 +
5 files changed, 40 insertions(+), 17 deletions(-)
diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
index be1fc643b645e..83118c728ed90 100644
--- a/Documentation/driver-api/dpll.rst
+++ b/Documentation/driver-api/dpll.rst
@@ -198,26 +198,28 @@ be requested with the same attribute with ``DPLL_CMD_DEVICE_SET`` command.
================================== ======================================
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``
+If pin phase adjustment is supported, minimal and maximal values and
+granularity that pin handle shall be provided to the user on
+``DPLL_CMD_PIN_GET`` respond with ``DPLL_A_PIN_PHASE_ADJUST_MIN``,
+``DPLL_A_PIN_PHASE_ADJUST_MAX`` and ``DPLL_A_PIN_PHASE_ADJUST_GRAN``
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
- =============================== ======================================
+ ================================ ==========================================
+ ``DPLL_A_PIN_ID`` configured pin id
+ ``DPLL_A_PIN_PHASE_ADJUST_GRAN`` attr granularity of phase adjustment value
+ ``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 difference between signals phase. The negative value means that
@@ -384,6 +386,8 @@ 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_GRAN`` attr granularity of phase
+ adjustment value
``DPLL_A_PIN_PHASE_ADJUST_MIN`` attr minimum value of phase
adjustment
``DPLL_A_PIN_PHASE_ADJUST_MAX`` attr maximum value of phase
diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
index cafb4ec20447e..e9b476b5db1b4 100644
--- a/Documentation/netlink/specs/dpll.yaml
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -440,6 +440,12 @@ attribute-sets:
doc: |
Capable pin provides list of pins that can be bound to create a
reference-sync pin pair.
+ -
+ name: phase-adjust-gran
+ type: s32
+ doc: |
+ Granularity of phase adjustment, in picoseconds. The value of
+ phase adjustment must be a multiple of this granularity.
-
name: pin-parent-device
@@ -614,6 +620,7 @@ operations:
- capabilities
- parent-device
- parent-pin
+ - phase-adjust-gran
- phase-adjust-min
- phase-adjust-max
- phase-adjust
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 74c1f0ca95f24..25d3a46e889b5 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -637,6 +637,10 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
ret = dpll_msg_add_pin_freq(msg, pin, ref, extack);
if (ret)
return ret;
+ if (prop->phase_gran &&
+ nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_GRAN,
+ prop->phase_gran))
+ return -EMSGSIZE;
if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN,
prop->phase_range.min))
return -EMSGSIZE;
@@ -1261,7 +1265,13 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
if (phase_adj > pin->prop.phase_range.max ||
phase_adj < pin->prop.phase_range.min) {
NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
- "phase adjust value not supported");
+ "phase adjust value of out range");
+ return -EINVAL;
+ }
+ if (pin->prop.phase_gran && phase_adj % pin->prop.phase_gran) {
+ NL_SET_ERR_MSG_ATTR_FMT(extack, phase_adj_attr,
+ "phase adjust value not multiple of %u",
+ pin->prop.phase_gran);
return -EINVAL;
}
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index 25be745bf41f1..4455d095925e9 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -163,6 +163,7 @@ struct dpll_pin_properties {
u32 freq_supported_num;
struct dpll_pin_frequency *freq_supported;
struct dpll_pin_phase_adjust_range phase_range;
+ s32 phase_gran;
};
#if IS_ENABLED(CONFIG_DPLL)
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
index ab1725a954d74..69d35570ac4f1 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -251,6 +251,7 @@ enum dpll_a_pin {
DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED,
DPLL_A_PIN_ESYNC_PULSE,
DPLL_A_PIN_REFERENCE_SYNC,
+ DPLL_A_PIN_PHASE_ADJUST_GRAN,
__DPLL_A_PIN_MAX,
DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
--
2.51.0On 24/10/2025 15:49, Ivan Vecera wrote:
> Phase-adjust values are currently limited by a min-max range. Some
> hardware requires, for certain pin types, that values be multiples of
> a specific granularity, as in the zl3073x driver.
>
> Add a `phase-adjust-gran` pin attribute and an appropriate field in
> dpll_pin_properties. If set by the driver, use its value to validate
> user-provided phase-adjust values.
>
> Reviewed-by: Michal Schmidt <mschmidt@redhat.com>
> Reviewed-by: Petr Oros <poros@redhat.com>
> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> Documentation/driver-api/dpll.rst | 36 +++++++++++++++------------
> Documentation/netlink/specs/dpll.yaml | 7 ++++++
> drivers/dpll/dpll_netlink.c | 12 ++++++++-
> include/linux/dpll.h | 1 +
> include/uapi/linux/dpll.h | 1 +
> 5 files changed, 40 insertions(+), 17 deletions(-)
>
> @@ -1261,7 +1265,13 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
> if (phase_adj > pin->prop.phase_range.max ||
> phase_adj < pin->prop.phase_range.min) {
> NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
> - "phase adjust value not supported");
> + "phase adjust value of out range");
> + return -EINVAL;
> + }
> + if (pin->prop.phase_gran && phase_adj % pin->prop.phase_gran) {
> + NL_SET_ERR_MSG_ATTR_FMT(extack, phase_adj_attr,
> + "phase adjust value not multiple of %u",
> + pin->prop.phase_gran);
That is pretty strict on the uAPI input. Maybe it's better to allow any
value, but report back the one that is really applied based on driver's
understanding of hardware? I mean the driver is doing some math before
applying the math, it can potentially round any value to the closest
acceptable and report it back?
> return -EINVAL;
> }
>
On 10/29/25 11:20 AM, Vadim Fedorenko wrote:
> On 24/10/2025 15:49, Ivan Vecera wrote:
>> Phase-adjust values are currently limited by a min-max range. Some
>> hardware requires, for certain pin types, that values be multiples of
>> a specific granularity, as in the zl3073x driver.
>>
>> Add a `phase-adjust-gran` pin attribute and an appropriate field in
>> dpll_pin_properties. If set by the driver, use its value to validate
>> user-provided phase-adjust values.
>>
>> Reviewed-by: Michal Schmidt <mschmidt@redhat.com>
>> Reviewed-by: Petr Oros <poros@redhat.com>
>> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>> Documentation/driver-api/dpll.rst | 36 +++++++++++++++------------
>> Documentation/netlink/specs/dpll.yaml | 7 ++++++
>> drivers/dpll/dpll_netlink.c | 12 ++++++++-
>> include/linux/dpll.h | 1 +
>> include/uapi/linux/dpll.h | 1 +
>> 5 files changed, 40 insertions(+), 17 deletions(-)
>>
>> @@ -1261,7 +1265,13 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin,
>> struct nlattr *phase_adj_attr,
>> if (phase_adj > pin->prop.phase_range.max ||
>> phase_adj < pin->prop.phase_range.min) {
>> NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
>> - "phase adjust value not supported");
>> + "phase adjust value of out range");
>> + return -EINVAL;
>> + }
>> + if (pin->prop.phase_gran && phase_adj % pin->prop.phase_gran) {
>> + NL_SET_ERR_MSG_ATTR_FMT(extack, phase_adj_attr,
>> + "phase adjust value not multiple of %u",
>> + pin->prop.phase_gran);
>
> That is pretty strict on the uAPI input. Maybe it's better to allow any
> value, but report back the one that is really applied based on driver's
> understanding of hardware? I mean the driver is doing some math before
> applying the math, it can potentially round any value to the closest
> acceptable and report it back?
I’d prefer to use the same approach as for phase-adjust-{min,max}.
Because we could treat them the same way - the user sets a value
above/below the max/min, and the driver clamps it.
Would it be better? I don't think so.
Let’s say the granularity is 1000, and the user sets a value of 499...
then the driver rounds it and sets it to 0. The user then reads the
current value via pin-get and finds that it hasn’t changed - quite
confusing, I’d say. If the user knows the granularity in advance,
they can adjust accordingly.
IMHO, strict behavior is better than smart behavior.
Ivan
On Fri, 24 Oct 2025 16:49:26 +0200 Ivan Vecera wrote: > + - > + name: phase-adjust-gran > + type: s32 > + doc: | > + Granularity of phase adjustment, in picoseconds. The value of > + phase adjustment must be a multiple of this granularity. Do we need this to be signed?
Hi Kuba, On 10/29/25 2:39 AM, Jakub Kicinski wrote: > On Fri, 24 Oct 2025 16:49:26 +0200 Ivan Vecera wrote: >> + - >> + name: phase-adjust-gran >> + type: s32 >> + doc: | >> + Granularity of phase adjustment, in picoseconds. The value of >> + phase adjustment must be a multiple of this granularity. > > Do we need this to be signed? > To have it unsigned brings a need to use explicit type casting in the core and driver's code. The phase adjustment can be both positive and negative it has to be signed. The granularity specifies that adjustment has to be multiple of granularity value so the core checks for zero remainder (this patch) and the driver converts the given adjustment value using division by the granularity. If we would have phase-adjust-gran and corresponding structure fields defined as u32 then we have to explicitly cast the granularity to s32 because for: <snip> s32 phase_adjust, remainder; u32 phase_gran = 1000; phase_adjust = 5000; remainder = phase_adjust % phase_gran; /* remainder = 0 -> OK for positive adjust */ phase_adjust = -5000; remainder = phase_adjust % phase_gran; /* remainder = 296 * Wrong for negative adjustment because phase_adjust is casted to u32 * prior division -> 2^32 - 5000 = 4294962296. * 4294962296 % 1000 = 296 */ remainder = phase_adjust % (s32)phase_gran; /* remainder = 0 * Now OK because phase_adjust remains to be s32 */ </snip> Similarly for division in the driver code if the granularity would be u32. So I have proposed phase adjustment granularity to be s32 to avoid these explicit type castings and potential bugs in drivers. Thanks, Ivan
Wed, Oct 29, 2025 at 08:44:52AM +0100, ivecera@redhat.com wrote: >Hi Kuba, > >On 10/29/25 2:39 AM, Jakub Kicinski wrote: >> On Fri, 24 Oct 2025 16:49:26 +0200 Ivan Vecera wrote: >> > + - >> > + name: phase-adjust-gran >> > + type: s32 >> > + doc: | >> > + Granularity of phase adjustment, in picoseconds. The value of >> > + phase adjustment must be a multiple of this granularity. >> >> Do we need this to be signed? >> >To have it unsigned brings a need to use explicit type casting in the core >and driver's code. The phase adjustment can be both positive and >negative it has to be signed. The granularity specifies that adjustment >has to be multiple of granularity value so the core checks for zero >remainder (this patch) and the driver converts the given adjustment >value using division by the granularity. > >If we would have phase-adjust-gran and corresponding structure fields >defined as u32 then we have to explicitly cast the granularity to s32 >because for: I prefer cast. The uapi should be clear. There is not point of having negative granularity. > ><snip> >s32 phase_adjust, remainder; >u32 phase_gran = 1000; > >phase_adjust = 5000; >remainder = phase_adjust % phase_gran; >/* remainder = 0 -> OK for positive adjust */ > >phase_adjust = -5000; >remainder = phase_adjust % phase_gran; >/* remainder = 296 > * Wrong for negative adjustment because phase_adjust is casted to u32 > * prior division -> 2^32 - 5000 = 4294962296. > * 4294962296 % 1000 = 296 > */ > > remainder = phase_adjust % (s32)phase_gran; > /* remainder = 0 > * Now OK because phase_adjust remains to be s32 > */ ></snip> > >Similarly for division in the driver code if the granularity would be >u32. > >So I have proposed phase adjustment granularity to be s32 to avoid these >explicit type castings and potential bugs in drivers. Cast in dpll core, no? > >Thanks, >Ivan >
On 10/29/25 3:20 PM, Jiri Pirko wrote: > Wed, Oct 29, 2025 at 08:44:52AM +0100, ivecera@redhat.com wrote: >> Hi Kuba, >> >> On 10/29/25 2:39 AM, Jakub Kicinski wrote: >>> On Fri, 24 Oct 2025 16:49:26 +0200 Ivan Vecera wrote: >>>> + - >>>> + name: phase-adjust-gran >>>> + type: s32 >>>> + doc: | >>>> + Granularity of phase adjustment, in picoseconds. The value of >>>> + phase adjustment must be a multiple of this granularity. >>> >>> Do we need this to be signed? >>> >> To have it unsigned brings a need to use explicit type casting in the core >> and driver's code. The phase adjustment can be both positive and >> negative it has to be signed. The granularity specifies that adjustment >> has to be multiple of granularity value so the core checks for zero >> remainder (this patch) and the driver converts the given adjustment >> value using division by the granularity. >> >> If we would have phase-adjust-gran and corresponding structure fields >> defined as u32 then we have to explicitly cast the granularity to s32 >> because for: > > I prefer cast. The uapi should be clear. There is not point of having > negative granularity. > > I will use u32 for phase-adjust-gran and dpll_pin_properties.phase_gran. OK? >> <snip> >> s32 phase_adjust, remainder; >> u32 phase_gran = 1000; >> >> phase_adjust = 5000; >> remainder = phase_adjust % phase_gran; >> /* remainder = 0 -> OK for positive adjust */ >> >> phase_adjust = -5000; >> remainder = phase_adjust % phase_gran; >> /* remainder = 296 >> * Wrong for negative adjustment because phase_adjust is casted to u32 >> * prior division -> 2^32 - 5000 = 4294962296. >> * 4294962296 % 1000 = 296 >> */ >> >> remainder = phase_adjust % (s32)phase_gran; >> /* remainder = 0 >> * Now OK because phase_adjust remains to be s32 >> */ >> </snip> >> >> Similarly for division in the driver code if the granularity would be >> u32. >> >> So I have proposed phase adjustment granularity to be s32 to avoid these >> explicit type castings and potential bugs in drivers. > > Cast in dpll core, no? Depends... if the driver will use s32 (sse patch 2) then no castings are necessary. Thanks, Ivan
© 2016 - 2026 Red Hat, Inc.