Re: [PATCH 02/12] drm/i915: Extract functions to derive SKL+ DIMM info

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the code less repetitive by extracting a few small helpers.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 68 +++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 48c6bc44072d..b94bf475b04c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1068,16 +1068,42 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv)
>  	intel_gvt_sanitize_options(dev_priv);
>  }
>  
> -static int skl_get_dimm_ranks(u8 size, u32 rank)
> +static int skl_get_dimm_size(u16 val)
>  {
> -	if (size == 0)
> +	return val & SKL_DRAM_SIZE_MASK;
> +}
> +
> +static int skl_get_dimm_width(u16 val)
> +{
> +	if (skl_get_dimm_size(val) == 0)
>  		return 0;
> -	if (rank == SKL_DRAM_RANK_SINGLE)
> -		return 1;
> -	else if (rank == SKL_DRAM_RANK_DUAL)
> -		return 2;
>  
> -	return 0;
> +	switch (val & SKL_DRAM_WIDTH_MASK) {
> +	case SKL_DRAM_WIDTH_X8:
> +	case SKL_DRAM_WIDTH_X16:
> +	case SKL_DRAM_WIDTH_X32:
> +		val = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> +		return 8 << val;
> +	default:
> +		MISSING_CASE(val);
> +		return 0;
> +	}
> +}
> +
> +static int skl_get_dimm_ranks(u16 val)
> +{
> +	if (skl_get_dimm_size(val) == 0)
> +		return 0;
> +
> +	switch (val & SKL_DRAM_RANK_MASK) {
> +	case SKL_DRAM_RANK_SINGLE:
> +	case SKL_DRAM_RANK_DUAL:
> +		val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> +		return val + 1;
> +	default:
> +		MISSING_CASE(val);
> +		return 0;
> +	}

I don't much care for this dual use of both the macro and then the
calculation. I'd either just calculate, or return pre-calculated values
from the cases, not both. The missing cases can also never happen.

But it all checks out, so

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  }
>  
>  static bool
> @@ -1098,30 +1124,22 @@ skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
>  static int
>  skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
>  {
> -	u32 tmp_l, tmp_s;
> -	u32 s_val = val >> SKL_DRAM_S_SHIFT;
> +	u16 tmp_l, tmp_s;
>  
> -	if (!val)
> -		return -EINVAL;
> +	tmp_l = val & 0xffff;
> +	tmp_s = val >> 16;
>  
> -	tmp_l = val & SKL_DRAM_SIZE_MASK;
> -	tmp_s = s_val & SKL_DRAM_SIZE_MASK;
> +	ch->l_info.size = skl_get_dimm_size(tmp_l);
> +	ch->s_info.size = skl_get_dimm_size(tmp_s);
>  
> -	if (tmp_l == 0 && tmp_s == 0)
> +	if (ch->l_info.size == 0 && ch->s_info.size == 0)
>  		return -EINVAL;
>  
> -	ch->l_info.size = tmp_l;
> -	ch->s_info.size = tmp_s;
> -
> -	tmp_l = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> -	tmp_s = (s_val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> -	ch->l_info.width = (1 << tmp_l) * 8;
> -	ch->s_info.width = (1 << tmp_s) * 8;
> +	ch->l_info.width = skl_get_dimm_width(tmp_l);
> +	ch->s_info.width = skl_get_dimm_width(tmp_s);
>  
> -	tmp_l = val & SKL_DRAM_RANK_MASK;
> -	tmp_s = s_val & SKL_DRAM_RANK_MASK;
> -	ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
> -	ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
> +	ch->l_info.ranks = skl_get_dimm_ranks(tmp_l);
> +	ch->s_info.ranks = skl_get_dimm_ranks(tmp_s);
>  
>  	if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
>  		ch->ranks = 2;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx