[Intel-wired-lan] [PATCH v10 iwl-next 03/11] virtchnl: add PTP virtchnl definitions

[Olech, Milena]

On 4/9/2025 8:12 PM, Jacob Keller wrote:

>On 4/9/2025 4:51 AM, Olech, Milena wrote:
>> On 4/8/2025 11:12 PM, Jacob Keller wrote:
>>> On 4/8/2025 3:30 AM, Milena Olech wrote:
>>>> +/**
>>>> + * struct virtchnl2_ptp_adj_dev_clk_fine: Associated with message
>>>> + *					  VIRTCHNL2_OP_PTP_ADJ_DEV_CLK_FINE.
>>>> + * @incval: Source timer increment value per clock cycle
>>>> + *
>>>> + * PF/VF sends this message to adjust the frequency of the main timer by the
>>>> + * indicated scaled ppm.
>>>> + */
>
>This comment should be rephrased then. The text implies the value being
>sent is the scaled PPM value.

Ok I see, I will update the comment in the next version.

Milena

>
>>>
>>> Do we want to encode scaled_ppm here in the virtchnl interface? I
>>> suppose its not that big a deal but it is kind of an implementation
>>> quirk of the Linux APIs. We could use parts per trillion or something
>>> similar..
>>>
>>> I suppose there is little value in translating from scaled_ppm to some
>>> other format, due to accumulated error, and scaled_ppm is higher
>>> precision than ppb. Ok.
>> 
>> I'm not sure I fully understand your concern, but you think that we
>> could use another naming convention, or provide to control plane raw
>> scaled ppm value?
>> 
>> Please notice that in current shape, we negotiate also basic increment
>> value in PTP capabilities, to adjust scaled ppm - as it is done in any
>> other product - and then the diff is sent through virtchnl message.
>> 
>
>No. What I am trying to get at is that i don't think it makes sense to
>encode the use of scaled_ppm in the virtchnl message. You didn't do that
>which is good. But the comment makes it seem like you did, because it
>seems like the message itself adjusts the main timer by the scaled PPM
>indicated within the message. In fact the driver calculates the new
>invcalue and sends it.
>
>Its not a big deal either way to me, I just misinterpreted the meaning
>of the comment.
>
>> Thanks,
>> Milena
>