On Mon, 25 Feb 2019, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Reduce the code duplication a bit by sharing the same
> code for parsing both DIMMs on a channel.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 44 ++++++++++++++++++---------------
> 1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9c1ff3eb5775..3d6a08e907e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1112,25 +1112,30 @@ skl_is_16gb_dimm(const struct dram_dimm_info *dimm)
> return dimm->size * dimm->width / (8 * dimm->ranks ?: 1) == 16;
> }
>
> -static int
> -skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
> +static void
> +skl_dram_get_dimm_info(struct dram_dimm_info *dimm,
> + int channel, char dimm_name, u16 val)
Personally I think using chars like this for dimm_name is an unnecessary
micro-optimization for const char * of length 1. But *shrug*.
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
PS. What a horrible, horrible diff to review this change produced!
> {
> - u16 tmp_l, tmp_s;
> + dimm->size = skl_get_dimm_size(val);
> + dimm->width = skl_get_dimm_width(val);
> + dimm->ranks = skl_get_dimm_ranks(val);
>
> - tmp_l = val & 0xffff;
> - tmp_s = val >> 16;
> + DRM_DEBUG_KMS("CH%d DIMM %c size: %d GB, width: X%d, ranks: %d, 16Gb DIMMs: %s\n",
> + channel, dimm_name, dimm->size, dimm->width, dimm->ranks,
> + yesno(skl_is_16gb_dimm(dimm)));
> +}
>
> - ch->l_info.size = skl_get_dimm_size(tmp_l);
> - ch->s_info.size = skl_get_dimm_size(tmp_s);
> +static int
> +skl_dram_get_channel_info(struct dram_channel_info *ch,
> + int channel, u32 val)
> +{
> + skl_dram_get_dimm_info(&ch->l_info, channel, 'L', val & 0xffff);
> + skl_dram_get_dimm_info(&ch->s_info, channel, 'S', val >> 16);
>
> - if (ch->l_info.size == 0 && ch->s_info.size == 0)
> + if (ch->l_info.size == 0 && ch->s_info.size == 0) {
> + DRM_DEBUG_KMS("CH%d not populated\n", channel);
> return -EINVAL;
> -
> - ch->l_info.width = skl_get_dimm_width(tmp_l);
> - ch->s_info.width = skl_get_dimm_width(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;
> @@ -1143,9 +1148,8 @@ skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
> skl_is_16gb_dimm(&ch->l_info) ||
> skl_is_16gb_dimm(&ch->s_info);
>
> - DRM_DEBUG_KMS("(size:width:ranks) L(%dGB:X%d:%d) S(%dGB:X%d:%d)\n",
> - ch->l_info.size, ch->l_info.width, ch->l_info.ranks,
> - ch->s_info.size, ch->s_info.width, ch->s_info.ranks);
> + DRM_DEBUG_KMS("CH%d ranks: %d, 16Gb DIMMs: %s\n",
> + channel, ch->ranks, yesno(ch->is_16gb_dimm));
>
> return 0;
> }
> @@ -1165,17 +1169,17 @@ static int
> skl_dram_get_channels_info(struct drm_i915_private *dev_priv)
> {
> struct dram_info *dram_info = &dev_priv->dram_info;
> - struct dram_channel_info ch0, ch1;
> + struct dram_channel_info ch0 = {}, ch1 = {};
> u32 val_ch0, val_ch1;
> int ret;
>
> val_ch0 = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> - ret = skl_dram_get_channel_info(&ch0, val_ch0);
> + ret = skl_dram_get_channel_info(&ch0, 0, val_ch0);
> if (ret == 0)
> dram_info->num_channels++;
>
> val_ch1 = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> - ret = skl_dram_get_channel_info(&ch1, val_ch1);
> + ret = skl_dram_get_channel_info(&ch1, 1, val_ch1);
> if (ret == 0)
> dram_info->num_channels++;
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
