On 20/03/2020 07:14, Zhang, Qi Z wrote:
> Hi Kevin:
>
>> -----Original Message-----
>> From: Kevin Traynor <ktray...@redhat.com>
>> Sent: Monday, March 9, 2020 11:45 PM
>> To: Zhang, Qi Z <qi.z.zh...@intel.com>; Yang, Qiming
>> <qiming.y...@intel.com>; Xing, Beilei <beilei.x...@intel.com>
>> Cc: Ye, Xiaolong <xiaolong...@intel.com>; dev@dpdk.org; Shelton, Benjamin H
>> <benjamin.h.shel...@intel.com>; Stillwell Jr, Paul M
>> <paul.m.stillwell...@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 04/28] net/ice/base: read PSM clock frequency
>> from register
>>
>> On 09/03/2020 11:43, Qi Zhang wrote:
>>> Read the GLGEN_CLKSTAT_SRC register to determine which PSM clock
>>> frequency is selected. This ensures that the rate limiter profile
>>> calculations will be correct.
>>>
>>
>> This seems to be a fix whereby a default and possibly incorrect frequency was
>> used previously. In that case, it should have a Fixes tag (and probably
>> stable tag
>> too)
>
> OK, will add fixline
>>
>>> Signed-off-by: Ben Shelton <benjamin.h.shel...@intel.com>
>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell...@intel.com>
>>> Signed-off-by: Qi Zhang <qi.z.zh...@intel.com>
>>> ---
>>> drivers/net/ice/base/ice_common.c | 1 +
>>> drivers/net/ice/base/ice_sched.c | 59
>>> +++++++++++++++++++++++++++++++++------
>>> drivers/net/ice/base/ice_sched.h | 7 ++++-
>>> drivers/net/ice/base/ice_type.h | 4 ++-
>>> 4 files changed, 61 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/ice/base/ice_common.c
>>> b/drivers/net/ice/base/ice_common.c
>>> index 786e99d21..9ef1aeef2 100644
>>> --- a/drivers/net/ice/base/ice_common.c
>>> +++ b/drivers/net/ice/base/ice_common.c
>>> @@ -672,6 +672,7 @@ enum ice_status ice_init_hw(struct ice_hw *hw)
>>> "Failed to get scheduler allocated resources\n");
>>> goto err_unroll_alloc;
>>> }
>>> + ice_sched_get_psm_clk_freq(hw);
>>>
>>> /* Initialize port_info struct with scheduler data */
>>> status = ice_sched_init_port(hw->port_info);
>>> diff --git a/drivers/net/ice/base/ice_sched.c
>>> b/drivers/net/ice/base/ice_sched.c
>>> index 553fc28ff..26c4ba36f 100644
>>> --- a/drivers/net/ice/base/ice_sched.c
>>> +++ b/drivers/net/ice/base/ice_sched.c
>>> @@ -1369,6 +1369,46 @@ enum ice_status
>>> ice_sched_query_res_alloc(struct ice_hw *hw) }
>>>
>>> /**
>>> + * ice_sched_get_psm_clk_freq - determine the PSM clock frequency
>>> + * @hw: pointer to the HW struct
>>> + *
>>> + * Determine the PSM clock frequency and store in HW struct */ void
>>> +ice_sched_get_psm_clk_freq(struct ice_hw *hw) {
>>> + u32 val, clk_src;
>>> +
>>> + val = rd32(hw, GLGEN_CLKSTAT_SRC);
>>> + clk_src = (val & GLGEN_CLKSTAT_SRC_PSM_CLK_SRC_M) >>
>>> + GLGEN_CLKSTAT_SRC_PSM_CLK_SRC_S;
>>> +
>>> +#define PSM_CLK_SRC_367_MHZ 0x0
>>> +#define PSM_CLK_SRC_416_MHZ 0x1
>>> +#define PSM_CLK_SRC_446_MHZ 0x2
>>> +#define PSM_CLK_SRC_390_MHZ 0x3
>>> +
>>> + switch (clk_src) {
>>> + case PSM_CLK_SRC_367_MHZ:
>>> + hw->psm_clk_freq = ICE_PSM_CLK_367MHZ_IN_HZ;
>>> + break;
>>> + case PSM_CLK_SRC_416_MHZ:
>>> + hw->psm_clk_freq = ICE_PSM_CLK_416MHZ_IN_HZ;
>>> + break;
>>> + case PSM_CLK_SRC_446_MHZ:
>>> + hw->psm_clk_freq = ICE_PSM_CLK_446MHZ_IN_HZ;
>>> + break;
>>> + case PSM_CLK_SRC_390_MHZ:
>>> + hw->psm_clk_freq = ICE_PSM_CLK_390MHZ_IN_HZ;
>>> + break;
>>> + default:
>>> + ice_debug(hw, ICE_DBG_SCHED, "PSM clk_src unexpected %u\n",
>>> + clk_src);
>>> + /* fall back to a safe default */
>>> + hw->psm_clk_freq = ICE_PSM_CLK_446MHZ_IN_HZ;
>>> + }
>>> +}
>>> +
>>> +/**
>>> * ice_sched_find_node_in_subtree - Find node in part of base node
>> subtree
>>> * @hw: pointer to the HW struct
>>> * @base: pointer to the base node
>>> @@ -2867,7 +2907,7 @@ ice_sched_update_elem(struct ice_hw *hw, struct
>> ice_sched_node *node,
>>> */
>>> static enum ice_status
>>> ice_sched_cfg_node_bw_alloc(struct ice_hw *hw, struct ice_sched_node
>> *node,
>>> - enum ice_rl_type rl_type, u8 bw_alloc)
>>> + enum ice_rl_type rl_type, u16 bw_alloc)
>>
>> This fix looks unrelated to the commit message
>
> Yes, will move this fix to patch that cover all the minor fix.
>>
>>> {
>>> struct ice_aqc_txsched_elem_data buf;
>>> struct ice_aqc_txsched_elem *data;
>>> @@ -3671,11 +3711,12 @@ ice_cfg_agg_bw_alloc(struct ice_port_info *pi,
>>> u32 agg_id, u8 ena_tcmap,
>>>
>>> /**
>>> * ice_sched_calc_wakeup - calculate RL profile wakeup parameter
>>> + * @hw: pointer to the HW struct
>>> * @bw: bandwidth in Kbps
>>> *
>>> * This function calculates the wakeup parameter of RL profile.
>>> */
>>> -static u16 ice_sched_calc_wakeup(s32 bw)
>>> +static u16 ice_sched_calc_wakeup(struct ice_hw *hw, s32 bw)
>>> {
>>> s64 bytes_per_sec, wakeup_int, wakeup_a, wakeup_b, wakeup_f;
>>> s32 wakeup_f_int;
>>> @@ -3683,7 +3724,7 @@ static u16 ice_sched_calc_wakeup(s32 bw)
>>>
>>> /* Get the wakeup integer value */
>>> bytes_per_sec = DIV_64BIT(((s64)bw * 1000), BITS_PER_BYTE);
>>> - wakeup_int = DIV_64BIT(ICE_RL_PROF_FREQUENCY, bytes_per_sec);
>>> + wakeup_int = DIV_64BIT(hw->psm_clk_freq, bytes_per_sec);
>>> if (wakeup_int > 63) {
>>> wakeup = (u16)((1 << 15) | wakeup_int);
>>> } else {
>>> @@ -3692,7 +3733,7 @@ static u16 ice_sched_calc_wakeup(s32 bw)
>>> */
>>> wakeup_b = (s64)ICE_RL_PROF_MULTIPLIER * wakeup_int;
>>> wakeup_a = DIV_64BIT((s64)ICE_RL_PROF_MULTIPLIER *
>>> - ICE_RL_PROF_FREQUENCY, bytes_per_sec);
>>> + hw->psm_clk_freq, bytes_per_sec);
>>>
>>> /* Get Fraction value */
>>> wakeup_f = wakeup_a - wakeup_b;
>>> @@ -3712,13 +3753,15 @@ static u16 ice_sched_calc_wakeup(s32 bw)
>>>
>>> /**
>>> * ice_sched_bw_to_rl_profile - convert BW to profile parameters
>>> + * @hw: pointer to the HW struct
>>> * @bw: bandwidth in Kbps
>>> * @profile: profile parameters to return
>>> *
>>> * This function converts the BW to profile structure format.
>>> */
>>> static enum ice_status
>>> -ice_sched_bw_to_rl_profile(u32 bw, struct ice_aqc_rl_profile_elem
>>> *profile)
>>> +ice_sched_bw_to_rl_profile(struct ice_hw *hw, u32 bw,
>>> + struct ice_aqc_rl_profile_elem *profile)
>>> {
>>> enum ice_status status = ICE_ERR_PARAM;
>>> s64 bytes_per_sec, ts_rate, mv_tmp;
>>> @@ -3738,7 +3781,7 @@ ice_sched_bw_to_rl_profile(u32 bw, struct
>> ice_aqc_rl_profile_elem *profile)
>>> for (i = 0; i < 64; i++) {
>>> u64 pow_result = BIT_ULL(i);
>>>
>>> - ts_rate = DIV_64BIT((s64)ICE_RL_PROF_FREQUENCY,
>>> + ts_rate = DIV_64BIT((s64)hw->psm_clk_freq,
>>> pow_result * ICE_RL_PROF_TS_MULTIPLIER);
>>> if (ts_rate <= 0)
>>> continue;
>>> @@ -3762,7 +3805,7 @@ ice_sched_bw_to_rl_profile(u32 bw, struct
>> ice_aqc_rl_profile_elem *profile)
>>> if (found) {
>>> u16 wm;
>>>
>>> - wm = ice_sched_calc_wakeup(bw);
>>> + wm = ice_sched_calc_wakeup(hw, bw);
>>> profile->rl_multiply = CPU_TO_LE16(mv);
>>> profile->wake_up_calc = CPU_TO_LE16(wm);
>>> profile->rl_encode = CPU_TO_LE16(encode); @@ -3831,7 +3874,7
>> @@
>>> ice_sched_add_rl_profile(struct ice_port_info *pi,
>>> if (!rl_prof_elem)
>>> return NULL;
>>>
>>> - status = ice_sched_bw_to_rl_profile(bw, &rl_prof_elem->profile);
>>> + status = ice_sched_bw_to_rl_profile(hw, bw, &rl_prof_elem->profile);
>>> if (status != ICE_SUCCESS)
>>> goto exit_add_rl_prof;
>>>
>>> diff --git a/drivers/net/ice/base/ice_sched.h
>>> b/drivers/net/ice/base/ice_sched.h
>>> index d6b467477..1a8549931 100644
>>> --- a/drivers/net/ice/base/ice_sched.h
>>> +++ b/drivers/net/ice/base/ice_sched.h
>>> @@ -25,12 +25,16 @@
>>> ((BIT(11) - 1) * 64) /* In Bytes */
>>> #define ICE_MAX_BURST_SIZE_KBYTE_GRANULARITY
>> ICE_MAX_BURST_SIZE_ALLOWED
>>>
>>> -#define ICE_RL_PROF_FREQUENCY 446000000 #define
>>> ICE_RL_PROF_ACCURACY_BYTES 128 #define ICE_RL_PROF_MULTIPLIER
>> 10000
>>> #define ICE_RL_PROF_TS_MULTIPLIER 32 #define ICE_RL_PROF_FRACTION
>> 512
>>>
>>> +#define ICE_PSM_CLK_367MHZ_IN_HZ 367647059 #define
>>> +ICE_PSM_CLK_416MHZ_IN_HZ 416666667 #define
>> ICE_PSM_CLK_446MHZ_IN_HZ
>>> +446428571 #define ICE_PSM_CLK_390MHZ_IN_HZ 390625000
>>> +
>>> struct rl_profile_params {
>>> u32 bw; /* in Kbps */
>>> u16 rl_multiplier;
>>> @@ -83,6 +87,7 @@ ice_aq_query_sched_elems(struct ice_hw *hw, u16
>> elems_req,
>>> u16 *elems_ret, struct ice_sq_cd *cd); enum ice_status
>>> ice_sched_init_port(struct ice_port_info *pi); enum ice_status
>>> ice_sched_query_res_alloc(struct ice_hw *hw);
>>> +void ice_sched_get_psm_clk_freq(struct ice_hw *hw);
>>>
>>> /* Functions to cleanup scheduler SW DB */ void
>>> ice_sched_clear_port(struct ice_port_info *pi); diff --git
>>> a/drivers/net/ice/base/ice_type.h b/drivers/net/ice/base/ice_type.h
>>> index 9773a549f..237220ee8 100644
>>> --- a/drivers/net/ice/base/ice_type.h
>>> +++ b/drivers/net/ice/base/ice_type.h
>>> @@ -524,7 +524,7 @@ struct ice_sched_node {
>>> #define ICE_TXSCHED_GET_EIR_BWALLOC(x) \
>>> LE16_TO_CPU((x)->info.eir_bw.bw_alloc)
>>>
>>> -struct ice_sched_rl_profle {
>>> +struct ice_sched_rl_profile {
>>
>> Is this used somewhere?
>
> The base code is shared by linux/dpdk/windows, the consideration here is
> 1) we try to make code identical, otherwise a lot of compile option is
> involved to strip out code for different usage.
ok, I can see the argument for keeping the base code common and not
customizing. Thanks.
> 2) some code may not be used on specific platform currently, but might be
> active in future.
>
> Thanks
> Qi
>
>>
>>> u32 rate; /* In Kbps */
>>> struct ice_aqc_rl_profile_elem info; }; @@ -741,6 +741,8 @@ struct
>>> ice_hw {
>>> struct ice_sched_rl_profile **cir_profiles;
>>> struct ice_sched_rl_profile **eir_profiles;
>>> struct ice_sched_rl_profile **srl_profiles;
>>> + /* PSM clock frequency for calculating RL profile params */
>>> + u32 psm_clk_freq;
>>> u64 debug_mask; /* BITMAP for debug mask */
>>> enum ice_mac_type mac_type;
>>>
>>>
>
