>From: Paul Menzel <pmen...@molgen.mpg.de>
>Sent: Wednesday, September 11, 2024 8:41 AM
>
>Dear Arkadiusz,
>
>
>Thank you for your patch. It’d be great if you used a more specific
>summary. Maybe:
>
>ice: Allow full frequency range of 1 Hz–25 MHz for dpll pins
>
>Some more nits below:
>
>Am 11.09.24 um 01:29 schrieb Arkadiusz Kubalewski:
>> Dpll's pins frequencies were hardcoded in the driver to the 1Hz/10MHz.
>
>1. Is “Dpll’s pins frequencies” a common term, or would “Dpll pin
>frequencies” also work?
>2. I’d use present tense: *are* hardcoded
>3. Remove *the*: to 1 Hz/10 MHz
>
>> Fix it be allowing users to set full range of supported frequencies
>> which is a range 1Hz-25MHz.
>
>the range …
>
>Where is the range documented? Please reference the source like the
>datasheet.
>
Dear Paul,
I wanted to fix other comments and send the follow up, fortunately your
last comment made me to look for user documentation, and it seems it has
changed from the time I seen it, there is some extra mechanic described
on this now (previously there was AnyFreq field in related AQ messages,
which indicated if any dpll supported frequency can be used, but it was
always enabled for ice supported dplls).
Now, with the new specification, I will have to actually rethink this
solution. Most probably introduce new AQ command and make the supported
frequencies to be obtained dynamically on init phase from firmware.
Please drop this patch for now, I will redesign this and send a new one.
Thank you for the review!
Arkadiusz
>> Fixes: 8a3a565ff210 ("ice: add admin commands to access cgu configuration")
>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktio...@intel.com>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalew...@intel.com>
>> ---
>> drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 11 +++++------
>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>>b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index 3a33e6b9b313..496bd588525b 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> @@ -9,8 +9,7 @@
>> #include "ice_cgu_regs.h"
>>
>> static struct dpll_pin_frequency ice_cgu_pin_freq_common[] = {
>> - DPLL_PIN_FREQUENCY_1PPS,
>> - DPLL_PIN_FREQUENCY_10MHZ,
>> + DPLL_PIN_FREQUENCY_RANGE(1, 25000000),
>> };
>>
>> static struct dpll_pin_frequency ice_cgu_pin_freq_1_hz[] = {
>> @@ -63,9 +62,9 @@ static const struct ice_cgu_pin_desc
>>ice_e810t_sfp_cgu_outputs[] = {
>> { "PHY-CLK", ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
>> { "MAC-CLK", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
>> { "CVL-SDP21", ZL_OUT4, DPLL_PIN_TYPE_EXT,
>> - ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> { "CVL-SDP23", ZL_OUT5, DPLL_PIN_TYPE_EXT,
>> - ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> };
>>
>> static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_outputs[] = {
>> @@ -77,9 +76,9 @@ static const struct ice_cgu_pin_desc
>>ice_e810t_qsfp_cgu_outputs[] = {
>> { "PHY2-CLK", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
>> { "MAC-CLK", ZL_OUT4, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
>> { "CVL-SDP21", ZL_OUT5, DPLL_PIN_TYPE_EXT,
>> - ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> { "CVL-SDP23", ZL_OUT6, DPLL_PIN_TYPE_EXT,
>> - ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
>> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
>> };
>>
>> static const struct ice_cgu_pin_desc ice_e823_si_cgu_inputs[] = {
>
>Reviewed-by: Paul Menzel <pmen...@molgen.mpg.de>
>
>
>Kind regards,
>
>Paul
