Pin-level attributes (frequency, phase adjust, embedded sync, reference sync) are properties of the pin itself, not of a particular DPLL device. The get callbacks already use only the pin owner's DPLL reference (via dpll_pin_own_dpll_ref_first()), but the set callbacks iterate over all registered DPLL references and invoke the set operation on each one. This is redundant because a pin is a single physical entity - setting its frequency or phase adjust once through the owner's ops is sufficient. Calling set on every registered DPLL just results in duplicate HW writes for drivers that share a pin across multiple DPLL devices (e.g. ice registers each input pin with both the EEC and PPS DPLL, zl3073x registers input pins with every DPLL channel). Simplify dpll_pin_freq_set(), dpll_pin_esync_set(), dpll_pin_ref_sync_state_set() and dpll_pin_phase_adj_set() to call the set callback only through the owner's DPLL reference, matching the existing get-side behavior. This removes the xa_for_each iteration loops, the now-unnecessary rollback logic, and several local variables. The -EOPNOTSUPP validation loop, which checked ops support across all owner-matching references, is replaced with a direct check on the single owner reference returned by dpll_pin_own_dpll_ref_first(). The documentation in dpll.rst is updated to reflect that pin-level attributes are set through the pin owner's dpll reference only. No existing driver is affected: - ptp_ocp and mlx5 register each pin with a single DPLL. - ice registers input pins with two DPLLs (EEC and PPS) using identical ops and pin_priv; the set callbacks address the HW by pin index, not by DPLL, so the second call was a no-op. - zl3073x registers input pins with every DPLL channel; the set callbacks address HW by pin/ref ID regardless of DPLL. The ref_sync_set callback was the only one with per-channel behavior, addressed by the preceding patch. Signed-off-by: Ivan Vecera --- Documentation/driver-api/dpll.rst | 11 +- drivers/dpll/dpll_netlink.c | 213 +++++++----------------------- 2 files changed, 56 insertions(+), 168 deletions(-) diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst index f83150917814e2..7c117ae37cc1bc 100644 --- a/Documentation/driver-api/dpll.rst +++ b/Documentation/driver-api/dpll.rst @@ -116,8 +116,9 @@ Shared pins A single pin object can be attached to multiple dpll devices. Then there are two groups of configuration knobs: -1) Set on a pin - the configuration affects all dpll devices pin is - registered to (i.e., ``DPLL_A_PIN_FREQUENCY``), +1) Set on a pin - the configuration is a property of the pin itself and + applies to all dpll devices the pin is registered with + (i.e., ``DPLL_A_PIN_FREQUENCY``), 2) Set on a pin-dpll tuple - the configuration affects only selected dpll device (i.e., ``DPLL_A_PIN_PRIO``, ``DPLL_A_PIN_STATE``, ``DPLL_A_PIN_DIRECTION``). @@ -507,9 +508,9 @@ as well as parameter being configured (``DPLL_A_MODE``). ``DPLL_CMD_PIN_SET`` - to target a pin user must provide a ``DPLL_A_PIN_ID``, which is unique identifier of a pin in the system. Also configured pin parameters must be added. -If ``DPLL_A_PIN_FREQUENCY`` is configured, this affects all the dpll -devices that are connected with the pin, that is why frequency attribute -shall not be enclosed in ``DPLL_A_PIN_PARENT_DEVICE``. +If ``DPLL_A_PIN_FREQUENCY`` is configured, it is a property of the pin +itself and applies to all dpll devices the pin is registered with, so the +frequency attribute shall not be enclosed in ``DPLL_A_PIN_PARENT_DEVICE``. Other attributes: ``DPLL_A_PIN_PRIO``, ``DPLL_A_PIN_STATE`` or ``DPLL_A_PIN_DIRECTION`` must be enclosed in ``DPLL_A_PIN_PARENT_DEVICE`` as their configuration relates to only one diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index afb31c0040382c..a909cd4451b008 100644 --- a/drivers/dpll/dpll_netlink.c +++ b/drivers/dpll/dpll_netlink.c @@ -1079,10 +1079,9 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a, struct netlink_ext_ack *extack) { u64 freq = nla_get_u64(a), old_freq; - struct dpll_pin_ref *ref, *failed; const struct dpll_pin_ops *ops; + struct dpll_pin_ref *ref; struct dpll_device *dpll; - unsigned long i; int ret; if (!dpll_pin_is_freq_supported(pin, freq)) { @@ -1090,22 +1089,17 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a, return -EINVAL; } - xa_for_each(&pin->dpll_refs, i, ref) { - ops = dpll_pin_ops(ref); - if ((!ops->frequency_set || !ops->frequency_get) && - ref->dpll->module == pin->module && - ref->dpll->clock_id == pin->clock_id) { - NL_SET_ERR_MSG(extack, - "frequency set not supported by the device"); - return -EOPNOTSUPP; - } - } ref = dpll_pin_own_dpll_ref_first(pin); if (!ref) { NL_SET_ERR_MSG(extack, "pin owner dpll not found"); return -ENODEV; } ops = dpll_pin_ops(ref); + if (!ops->frequency_set || !ops->frequency_get) { + NL_SET_ERR_MSG(extack, + "frequency set not supported by the device"); + return -EOPNOTSUPP; + } dpll = ref->dpll; ret = ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, dpll_priv(dpll), &old_freq, extack); @@ -1116,68 +1110,42 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a, if (freq == old_freq) return 0; - xa_for_each(&pin->dpll_refs, i, ref) { - ops = dpll_pin_ops(ref); - if (!ops->frequency_set) - continue; - dpll = ref->dpll; - ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin), - dpll, dpll_priv(dpll), freq, extack); - if (ret) { - failed = ref; - NL_SET_ERR_MSG_FMT(extack, "frequency set failed for dpll_id:%u", - dpll->id); - goto rollback; - } + ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin), + dpll, dpll_priv(dpll), freq, extack); + if (ret) { + NL_SET_ERR_MSG_FMT(extack, + "frequency set failed for dpll_id:%u", + dpll->id); + return ret; } __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); - if (!ops->frequency_set) - continue; - 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 dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a, struct netlink_ext_ack *extack) { - struct dpll_pin_ref *ref, *failed; const struct dpll_pin_ops *ops; struct dpll_pin_esync esync; u64 freq = nla_get_u64(a); + struct dpll_pin_ref *ref; struct dpll_device *dpll; bool supported = false; - unsigned long i; - int ret; + int ret, i; - xa_for_each(&pin->dpll_refs, i, ref) { - ops = dpll_pin_ops(ref); - if ((!ops->esync_set || !ops->esync_get) && - ref->dpll->module == pin->module && - ref->dpll->clock_id == pin->clock_id) { - NL_SET_ERR_MSG(extack, - "embedded sync feature is not supported by this device"); - return -EOPNOTSUPP; - } - } ref = dpll_pin_own_dpll_ref_first(pin); if (!ref) { NL_SET_ERR_MSG(extack, "pin owner dpll not found"); return -ENODEV; } ops = dpll_pin_ops(ref); + if (!ops->esync_set || !ops->esync_get) { + NL_SET_ERR_MSG(extack, + "embedded sync feature is not supported by this device"); + return -EOPNOTSUPP; + } dpll = ref->dpll; ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, dpll_priv(dpll), &esync, extack); @@ -1196,44 +1164,17 @@ dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a, return -EINVAL; } - xa_for_each(&pin->dpll_refs, i, ref) { - void *pin_dpll_priv; - - ops = dpll_pin_ops(ref); - if (!ops->esync_set) - continue; - dpll = ref->dpll; - pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin); - ret = ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll), - freq, extack); - if (ret) { - failed = ref; - NL_SET_ERR_MSG_FMT(extack, - "embedded sync frequency set failed for dpll_id: %u", - dpll->id); - goto rollback; - } + ret = ops->esync_set(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, + dpll_priv(dpll), freq, extack); + if (ret) { + NL_SET_ERR_MSG_FMT(extack, + "embedded sync frequency set failed for dpll_id: %u", + dpll->id); + return ret; } __dpll_pin_change_ntf(pin); return 0; - -rollback: - xa_for_each(&pin->dpll_refs, i, ref) { - void *pin_dpll_priv; - - if (ref == failed) - break; - ops = dpll_pin_ops(ref); - if (!ops->esync_set) - continue; - dpll = ref->dpll; - pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin); - if (ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll), - esync.freq, extack)) - NL_SET_ERR_MSG(extack, "set embedded sync frequency rollback failed"); - } - return ret; } static int @@ -1241,14 +1182,12 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin, unsigned long ref_sync_pin_idx, const enum dpll_pin_state state, struct netlink_ext_ack *extack) - { - struct dpll_pin_ref *ref, *failed; const struct dpll_pin_ops *ops; enum dpll_pin_state old_state; struct dpll_pin *ref_sync_pin; + struct dpll_pin_ref *ref; struct dpll_device *dpll; - unsigned long i; int ret; ref_sync_pin = xa_find(&pin->ref_sync_pins, &ref_sync_pin_idx, @@ -1282,42 +1221,20 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin, } if (state == old_state) return 0; - xa_for_each(&pin->dpll_refs, i, ref) { - ops = dpll_pin_ops(ref); - if (!ops->ref_sync_set) - continue; - dpll = ref->dpll; - ret = ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin), - ref_sync_pin, - dpll_pin_on_dpll_priv(dpll, - ref_sync_pin), - state, extack); - if (ret) { - failed = ref; - NL_SET_ERR_MSG_FMT(extack, "reference sync set failed for dpll_id:%u", - dpll->id); - goto rollback; - } + + ret = ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin), + ref_sync_pin, + dpll_pin_on_dpll_priv(dpll, ref_sync_pin), + state, extack); + if (ret) { + NL_SET_ERR_MSG_FMT(extack, + "reference sync set failed for dpll_id:%u", + dpll->id); + return ret; } __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); - if (!ops->ref_sync_set) - continue; - dpll = ref->dpll; - if (ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin), - ref_sync_pin, - dpll_pin_on_dpll_priv(dpll, ref_sync_pin), - old_state, extack)) - NL_SET_ERR_MSG(extack, "set reference sync rollback failed"); - } - return ret; } static int @@ -1478,11 +1395,10 @@ 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_pin_ref *ref; struct dpll_device *dpll; - unsigned long i; int ret; phase_adj = nla_get_s32(phase_adj_attr); @@ -1499,21 +1415,16 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr, return -EINVAL; } - xa_for_each(&pin->dpll_refs, i, ref) { - ops = dpll_pin_ops(ref); - if ((!ops->phase_adjust_set || !ops->phase_adjust_get) && - ref->dpll->module == pin->module && - ref->dpll->clock_id == pin->clock_id) { - NL_SET_ERR_MSG(extack, "phase adjust not supported"); - return -EOPNOTSUPP; - } - } ref = dpll_pin_own_dpll_ref_first(pin); if (!ref) { NL_SET_ERR_MSG(extack, "pin owner dpll not found"); return -ENODEV; } ops = dpll_pin_ops(ref); + if (!ops->phase_adjust_set || !ops->phase_adjust_get) { + NL_SET_ERR_MSG(extack, "phase adjust not supported"); + return -EOPNOTSUPP; + } dpll = ref->dpll; ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, dpll_priv(dpll), &old_phase_adj, @@ -1525,41 +1436,17 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr, if (phase_adj == old_phase_adj) return 0; - xa_for_each(&pin->dpll_refs, i, ref) { - ops = dpll_pin_ops(ref); - if (!ops->phase_adjust_set) - continue; - 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; - NL_SET_ERR_MSG_FMT(extack, - "phase adjust set failed for dpll_id:%u", - dpll->id); - goto rollback; - } + ret = ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll, pin), + dpll, dpll_priv(dpll), phase_adj, extack); + if (ret) { + NL_SET_ERR_MSG_FMT(extack, + "phase adjust set failed for dpll_id:%u", + dpll->id); + return ret; } __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); - if (!ops->phase_adjust_set) - continue; - 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 -- 2.54.0