linux-kernel - Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 11 May 2020 18:06:47 +0800
From:   Dilip Kota <eswara.kota@...ux.intel.com>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     linux-kernel@...r.kernel.org, kishon@...com,
        devicetree@...r.kernel.org, lee.jones@...aro.org, arnd@...db.de,
        robh@...nel.org, andriy.shevchenko@...el.com,
        cheol.yong.kim@...el.com, chuanhua.lei@...ux.intel.com,
        qi-ming.wu@...el.com, yixin.zhu@...el.com
Subject: Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

Hi Vinod,

On 5/5/2020 3:54 PM, Dilip Kota wrote:
>
> On 5/5/2020 1:21 PM, Vinod Koul wrote:
>> On 04-05-20, 17:32, Dilip Kota wrote:
>>> On 5/4/2020 5:20 PM, Vinod Koul wrote:
>>>> On 04-05-20, 16:26, Dilip Kota wrote:
>>>>> On 5/4/2020 3:29 PM, Vinod Koul wrote:
>>>>>> On 30-04-20, 15:15, Dilip Kota wrote:
>>>>>>
>>>>>>> +                      u32 mask, u32 val)
>>>>>>> +{
>>>>>>> +    u32 reg_val;
>>>>>>> +
>>>>>>> +    reg_val = readl(base + reg);
>>>>>>> +    reg_val &= ~mask;
>>>>>>> +    reg_val |= FIELD_PREP(mask, val);
>>>>>>> +    writel(reg_val, base + reg);
>>>>>> bypassing regmap here... why?
>>>>> It is not regmap address, one of the below two addresses are 
>>>>> passed to this
>>>>> function.
>>>> okay, perhaps add a comment somewhere that regmap is not used for this
>>>> base?
>>> I dont see a need of adding a comment, describing don't do regmap here.
>> Driver uses regmap except here, which seems odd hence explanation
>> required for this.
> During the driver Probe, the register phandles are stored in regmap 
> datatype variables and PHY core addresses are stored in iomem datatype.
> Since then, regmap access is performed for the regmap datatype 
> variables and readl/writel access is performed on the iomem datatype 
> variables. And nowhere in the driver iomem datatype address are 
> converted to regmap address and performed regmap access.
>
> Driver is not doing any 'regmap_init' on any physical address. Driver 
> is getting the register address phandle from the device tree node and 
> performing the regmap access.
> ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL, 
> 1, 0, &ref);
> [...]
> cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode));
>
> [...]
> ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 
> 1, 0, &ref);
> [...]
>
> cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode));
>
> [...]
> cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app");
>  [...]
> cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core");
>
> The DT parsing logic in the driver is explaining why the PHY driver 
> should do regmap access and to whom should be done. For this reason i 
> am a bit puzzled to what more is needed to explain in the comments and 
> where to add it.
> Please let me know your view.
>
Gentle Reminder!
Could you please update on this.

Regards,
Dilip

> Regards,
> Dilip