Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Sat, 15 Jul 2023 09:58:57 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Naresh Solanki <naresh.solanki@9elements.com>
Cc:     devicetree@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
        Iwona Winiarska <iwona.winiarska@intel.com>,
        linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>
Subject: Re: [PATCH] hwmon: (dimmtemp) Support more than 32 DIMMs
Message-ID: <16080951-743c-4f3c-bec2-153ff38d922f@roeck-us.net>
References: <20230711152144.755177-1-Naresh.Solanki@9elements.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230711152144.755177-1-Naresh.Solanki@9elements.com>
Precedence: bulk

On Tue, Jul 11, 2023 at 05:21:43PM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> This patch introduces support for handling more than 32 DIMMs by
> utilizing bitmap operations. The changes ensure that the driver can
> handle a higher number of DIMMs efficiently.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>

Applied to hwmon-next.

Note: I got multiple versions of this patch. In the future
please version your patches and provide change logs.

Thanks,
Guenter

> ---
>  drivers/hwmon/peci/dimmtemp.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> 
> base-commit: 4dbbaf8fbdbd13adc80731b2452257857e4c2d8b
> 
> diff --git a/drivers/hwmon/peci/dimmtemp.c b/drivers/hwmon/peci/dimmtemp.c
> index ed968401f93c..ce89da3937a0 100644
> --- a/drivers/hwmon/peci/dimmtemp.c
> +++ b/drivers/hwmon/peci/dimmtemp.c
> @@ -219,19 +219,21 @@ static int check_populated_dimms(struct peci_dimmtemp *priv)
>  {
>  	int chan_rank_max = priv->gen_info->chan_rank_max;
>  	int dimm_idx_max = priv->gen_info->dimm_idx_max;
> -	u32 chan_rank_empty = 0;
> -	u32 dimm_mask = 0;
> -	int chan_rank, dimm_idx, ret;
> +	DECLARE_BITMAP(dimm_mask, DIMM_NUMS_MAX);
> +	DECLARE_BITMAP(chan_rank_empty, CHAN_RANK_MAX);
> +
> +	int chan_rank, dimm_idx, ret, i;
>  	u32 pcs;
>  
> -	BUILD_BUG_ON(BITS_PER_TYPE(chan_rank_empty) < CHAN_RANK_MAX);
> -	BUILD_BUG_ON(BITS_PER_TYPE(dimm_mask) < DIMM_NUMS_MAX);
>  	if (chan_rank_max * dimm_idx_max > DIMM_NUMS_MAX) {
>  		WARN_ONCE(1, "Unsupported number of DIMMs - chan_rank_max: %d, dimm_idx_max: %d",
>  			  chan_rank_max, dimm_idx_max);
>  		return -EINVAL;
>  	}
>  
> +	bitmap_zero(dimm_mask, DIMM_NUMS_MAX);
> +	bitmap_zero(chan_rank_empty, CHAN_RANK_MAX);
> +
>  	for (chan_rank = 0; chan_rank < chan_rank_max; chan_rank++) {
>  		ret = peci_pcs_read(priv->peci_dev, PECI_PCS_DDR_DIMM_TEMP, chan_rank, &pcs);
>  		if (ret) {
> @@ -242,7 +244,7 @@ static int check_populated_dimms(struct peci_dimmtemp *priv)
>  			 * detection to be performed at a later point in time.
>  			 */
>  			if (ret == -EINVAL) {
> -				chan_rank_empty |= BIT(chan_rank);
> +				bitmap_set(chan_rank_empty, chan_rank, 1);
>  				continue;
>  			}
>  
> @@ -251,7 +253,7 @@ static int check_populated_dimms(struct peci_dimmtemp *priv)
>  
>  		for (dimm_idx = 0; dimm_idx < dimm_idx_max; dimm_idx++)
>  			if (__dimm_temp(pcs, dimm_idx))
> -				dimm_mask |= BIT(chan_rank * dimm_idx_max + dimm_idx);
> +				bitmap_set(dimm_mask, chan_rank * dimm_idx_max + dimm_idx, 1);
>  	}
>  
>  	/*
> @@ -260,7 +262,7 @@ static int check_populated_dimms(struct peci_dimmtemp *priv)
>  	 * host platform boot. Retrying a couple of times lets us make sure
>  	 * that the state is persistent.
>  	 */
> -	if (chan_rank_empty == GENMASK(chan_rank_max - 1, 0)) {
> +	if (bitmap_full(chan_rank_empty, chan_rank_max)) {
>  		if (priv->no_dimm_retry_count < NO_DIMM_RETRY_COUNT_MAX) {
>  			priv->no_dimm_retry_count++;
>  
> @@ -274,14 +276,16 @@ static int check_populated_dimms(struct peci_dimmtemp *priv)
>  	 * It's possible that memory training is not done yet. In this case we
>  	 * defer the detection to be performed at a later point in time.
>  	 */
> -	if (!dimm_mask) {
> +	if (bitmap_empty(dimm_mask, DIMM_NUMS_MAX)) {
>  		priv->no_dimm_retry_count = 0;
>  		return -EAGAIN;
>  	}
>  
> -	dev_dbg(priv->dev, "Scanned populated DIMMs: %#x\n", dimm_mask);
> +	for_each_set_bit(i, dimm_mask, DIMM_NUMS_MAX) {
> +		dev_dbg(priv->dev, "Found DIMM%#x\n", i);
> +	}
>  
> -	bitmap_from_arr32(priv->dimm_mask, &dimm_mask, DIMM_NUMS_MAX);
> +	bitmap_copy(priv->dimm_mask, dimm_mask, DIMM_NUMS_MAX);
>  
>  	return 0;
>  }