>From: Intel-wired-lan <intel-wired-lan-boun...@osuosl.org> On Behalf Of
>Simon Horman
>Sent: Tuesday, December 3, 2024 11:05 AM
>
>On Wed, Nov 20, 2024 at 08:51:12AM +0100, Arkadiusz Kubalewski wrote:
>> Mask admin command returned max phase adjust value for both input and
>> output pins. Only 31 bits are relevant, last released data sheet wrongly
>> points that 32 bits are valid - see [1] 3.2.6.4.1 Get CCU Capabilities
>> Command for reference. Fix of the datasheet itself is in progress.
>>
>> Fix the min/max assignment logic, previously the value was wrongly
>> considered as negative value due to most significant bit being set.
>
>Thanks Arkadiusz,
>
>I understand the most-significant-bit issue and see that is addressed
>through the use of ICE_AQC_GET_CGU_MAX_PHASE_ADJ. I also agree that this is
>a fix.
>
>But, although I like simplification afforded ice_dpll_phase_range_set()
>I'm not convinced it is a part of the fix. Does the code behave correctly
>without those changes? If so, I'm wondering if that part should be broken
>out into a separate follow-up patch for iwl.
>
Hi Simon,
Thank you for the review!
Well, the extra helper function was introduced as part of review.
But the logic shall be fixed anyway (negative is min/positive max),
as implemented within the new function - different then original code.
So yes, we could remove addition of the helper function from this patch,
and just fix the logic in 2 lines function is called.
I believe having it is simpler to maintain for the future. But won't argue
about, please just let me know what you think, if you still want it
separated, will do.
>>
>> Example of previous broken behavior:
>> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
>> --do pin-get --json '{"id":1}'| grep phase-adjust
>> 'phase-adjust': 0,
>> 'phase-adjust-max': 16723,
>> 'phase-adjust-min': -16723,
>
>I'm curious to know if the values for max and min above are inverted.
>I.e. if, sude to the most-significant-bit issue they are:
>
Yes, initially they were wrongly inverted in driver, since the driver was
also using the most significant bit made - the value was negative.
Thank you!
Arkadiusz
> 'phase-adjust-max': -16723,
> 'phase-adjust-min': 16723,
>
>>
>> Correct behavior with the fix:
>> $ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
>> --do pin-get --json '{"id":1}'| grep phase-adjust
>> 'phase-adjust': 0,
>> 'phase-adjust-max': 2147466925,
>> 'phase-adjust-min': -2147466925,
>>
>> [1] https://cdrdv2.intel.com/v1/dl/getContent/613875?explicitVersion=true
>>
>> Fixes: 90e1c90750d7 ("ice: dpll: implement phase related callbacks")
>> Reviewed-by: Przemek Kitszel <przemyslaw.kits...@intel.com>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalew...@intel.com>
>
>...
