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;
Subject: Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel,
 vm-map" not defined
Message-ID: <2f5c5828-87b9-f3d2-e3d3-0200adbe830c@amazon.com>
Date:   Thu, 1 Sep 2022 11:39:58 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Content-Language: en-US
To:     Guenter Roeck <linux@roeck-us.net>, <jdelvare@suse.com>,
        <robh+dt@kernel.org>, <p.zabel@pengutronix.de>,
        <rtanwar@maxlinear.com>, <linux-hwmon@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
CC:     <talel@amazon.com>, <hhhawa@amazon.com>, <jonnyc@amazon.com>,
        <hanochu@amazon.com>, <ronenk@amazon.com>, <itamark@amazon.com>,
        <shellykz@amazon.com>, <shorer@amazon.com>, <amitlavi@amazon.com>,
        <almogbs@amazon.com>, <dkl@amazon.com>,
        <andriy.shevchenko@intel.com>, "Farber, Eliav" <farbere@amazon.com>
References: <20220830192212.28570-1-farbere@amazon.com>
 <20220830192212.28570-3-farbere@amazon.com>
 <cddebb5a-3b83-e89d-db00-9a59ddbd6741@roeck-us.net>
 <84a68eff-be64-71ce-1533-1e228d3da2a4@amazon.com>
 <71d6d57c-2165-5fe3-515d-9395022921e2@roeck-us.net>
From:   "Farber, Eliav" <farbere@amazon.com>
In-Reply-To: <71d6d57c-2165-5fe3-515d-9395022921e2@roeck-us.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8/31/2022 2:48 PM, Guenter Roeck wrote:
> On 8/30/22 22:49, Farber, Eliav wrote:
>> On 8/31/2022 8:36 AM, Guenter Roeck wrote:
>>> On 8/30/22 12:21, Eliav Farber wrote:
>>>> Bug fix - in case "intel,vm-map" is missing in device-tree ,'num' 
>>>> is set
>>>> to 0, and no voltage channel infos are allocated.
>>>>
>>>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>>>> ---
>>>>   drivers/hwmon/mr75203.c | 28 ++++++++++++----------------
>>>>   1 file changed, 12 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
>>>> index 046523d47c29..0e29877a1a9c 100644
>>>> --- a/drivers/hwmon/mr75203.c
>>>> +++ b/drivers/hwmon/mr75203.c
>>>> @@ -580,8 +580,6 @@ static int mr75203_probe(struct platform_device 
>>>> *pdev)
>>>>       }
>>>>
>>>>       if (vm_num) {
>>>> -             u32 num = vm_num;
>>>> -
>>>>               ret = pvt_get_regmap(pdev, "vm", pvt);
>>>>               if (ret)
>>>>                       return ret;
>>>> @@ -594,30 +592,28 @@ static int mr75203_probe(struct 
>>>> platform_device *pdev)
>>>>               ret = device_property_read_u8_array(dev, "intel,vm-map",
>>>> pvt->vm_idx, vm_num);
>>>>               if (ret) {
>>>> -                     num = 0;
>>>> +                     /*
>>>> +                      * Incase intel,vm-map property is not 
>>>> defined, we
>>>> +                      * assume incremental channel numbers.
>>>> +                      */
>>>> +                     for (i = 0; i < vm_num; i++)
>>>> +                             pvt->vm_idx[i] = i;
>>>>               } else {
>>>>                       for (i = 0; i < vm_num; i++)
>>>>                               if (pvt->vm_idx[i] >= vm_num ||
>>>> -                                 pvt->vm_idx[i] == 0xff) {
>>>> -                                     num = i;
>>>> +                                 pvt->vm_idx[i] == 0xff)
>>>>                                       break;
>>>
>>> So all vm_idx values from 0x00 to 0xfe would be acceptable ?
>>> Does the chip really have that many registers (0x200 + 0x40 + 0x200 
>>> * 0xfe) ?
>>> Is that documented somewhere ?
>> According to the code vm_num is limited to 32 because the mask is
>> only 5 bits:
>>
>> #define VM_NUM_MSK    GENMASK(20, 16)
>> #define VM_NUM_SFT    16
>> vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>
>> In practice according to the data sheet I have:
>> 0 <= VM instances <= 8
>>
> Sorry, my bad. I misread the patch and thought the first part of
> the if statement was removed.
>
> Anyway, what is the difference between specifying an vm_idx value of
> 0xff and not specifying anything ? Or, in other words, taking the dt
> example, the difference between
>        intel,vm-map = [03 01 04 ff ff];
> and
>        intel,vm-map = [03 01 04]; 

The actual number of VMs is read from a HW register:
     ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
     ...
     vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;

Also, using:
     ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
                         vm_num);
in the driver will fail if vm_num > sizeof array in device-tree.

So, if for example vm_num = 5, but you will want to map only 3 of them
you most set property to be:
     intel,vm-map = [03 01 04 ff ff];
otherwise if you set:
     intel,vm-map = [03 01 04];
it will assume the property doesn't, and will continue the flow in code
as if it doesn’t exist (which is not what the user wanted, and before my
fix also has a bug).

--
Regards, Eliav