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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Sent: Monday, October 2, 2023 5:04 PM
>
>Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote:
>>>From: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
>>>Sent: Wednesday, September 27, 2023 8:09 PM
>>>
>>>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
>>>> Add callback op (get) for pin-dpll phase-offset measurment.
>>>> Add callback ops (get/set) for pin signal phase adjustment.
>>>> Add min and max phase adjustment values to pin proprties.
>>>> Invoke get callbacks when filling up the pin details to provide user
>>>> with phase related attribute values.
>>>> Invoke phase-adjust set callback when phase-adjust value is provided
>>>> for
>>>> pin-set request.
>>>>
>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>>>
>>>[...]
>>>
>>>> +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;
>>>> +	unsigned long i;
>>>> +	s32 phase_adj;
>>>> +	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) {
>>>> +		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>>>> +		struct dpll_device *dpll = ref->dpll;
>>>> +
>>>> +		if (!ops->phase_adjust_set)
>>>> +			return -EOPNOTSUPP;
>>>
>>>I'm thinking about this part. We can potentially have dpll devices with
>>>different expectations on phase adjustments, right? And if one of them
>>>won't be able to adjust phase (or will fail in the next line), then
>>>netlink will return EOPNOTSUPP while _some_ of the devices will be
>>>adjusted. Doesn't look great. Can we think about different way to apply
>>>the change?
>>>
>>
>>Well makes sense to me.
>>
>>Does following makes sense as a fix?
>>We would call op for all devices which has been provided with the op.
>>If device has no op -> add extack error, continue
>
>Is it real to expect some of the device support this and others don't?
>Is it true for ice?
>If not, I would got for all-or-nothing here.
>

Let's step back a bit.
The op itself is introduced as per pin-dpll tuple.. did this intentionally,
to inform each dpll that the offset has been changed - in case dplls are
controlled by separated driver/firmware instances but still sharing the pin.
Same way a pin frequency is being set, from user perspective on a pin, but
callback is called for each dpll the pin was registered with.
Whatever we do here, it shall be probably done for frequency_set() callback as
well.

The answers:
So far I don't know the device that might do it this way, it rather supports
phase_adjust or not. In theory we allow such behavior to be implemented, i.e.
pin is registered with 2 dplls, one has the callback, second not.
Current hardware of ice sets phase offset for a pin no matter on which dpll
device callback was invoked.
"all-or-nothing" - do you mean to check all callback returns and then decide
if it was successful?

Thank you!
Arkadiusz

>
>>If device fails to set -> add extack error, continue
>>Function always returns 0.
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>> +		ret = ops->phase_adjust_set(pin,
>>>> +					    dpll_pin_on_dpll_priv(dpll, pin),
>>>> +					    dpll, dpll_priv(dpll), phase_adj,
>>>> +					    extack);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +	__dpll_pin_change_ntf(pin);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux