| 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
| ||
|
Message-ID: <c2d1f933-0bb8-4aee-acd3-af4246f66913@collabora.com>
Date: Wed, 13 Nov 2024 09:54:12 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Zoie Lin (林禹妡) <Zoie.Lin@...iatek.com>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
Qii Wang (王琪) <Qii.Wang@...iatek.com>,
"andi.shyti@...nel.org" <andi.shyti@...nel.org>
Cc: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
Teddy Chen (陳乾元) <Teddy.Chen@...iatek.com>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [v2,1/1] i2c: mediatek: add runtime PM operations and bus
regulator control
Il 07/11/24 16:19, Zoie Lin (林禹妡) ha scritto:
> Hi Angelo,
>
> On Thu, 2024-11-07 at 11:13 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Il 06/11/24 13:52, Zoie Lin ha scritto:
>>> This commit introduces support for runtime PM operations in
>>> the I2C driver, enabling runtime suspend and resume functionality.
>>>
>>> Although in the most platforms, the bus power of i2c are always
>>> on, some platforms disable the i2c bus power in order to meet
>>> low power request.
>>>
>>> This implementation includes bus regulator control to facilitate
>>> proper handling of the bus power based on platform requirements.
>>>
>>> Signed-off-by: Zoie Lin <zoie.lin@...iatek.com>
>>> ---
>>> drivers/i2c/busses/i2c-mt65xx.c | 77
>>> ++++++++++++++++++++++++++++-----
>>> 1 file changed, 65 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-mt65xx.c
>>> b/drivers/i2c/busses/i2c-mt65xx.c
>>> index 5bd342047d59..4209daec1efa 100644
>>> --- a/drivers/i2c/busses/i2c-mt65xx.c
>>> +++ b/drivers/i2c/busses/i2c-mt65xx.c
>>
>> ..snip..
>>
>>> @@ -1370,6 +1373,40 @@ static int mtk_i2c_parse_dt(struct
>>> device_node *np, struct mtk_i2c *i2c)
>>> return 0;
>>> }
>>>
>>> +static int mtk_i2c_runtime_suspend(struct device *dev)
>>> +{
>>> + struct mtk_i2c *i2c = dev_get_drvdata(dev);
>>> +
>>> + clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
>>> + if (i2c->adap.bus_regulator)
>>> + regulator_disable(i2c->adap.bus_regulator);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int mtk_i2c_runtime_resume(struct device *dev)
>>> +{
>>> + int ret = 0;
>>> + struct mtk_i2c *i2c = dev_get_drvdata(dev);
>>
>> struct mtk_i2c *i2c = dev_get_drvdata(dev);
>> int ret;
>>
>>> +
>>> + if (i2c->adap.bus_regulator) {
>>> + ret = regulator_enable(i2c->adap.bus_regulator);
>>> + if (ret) {
>>> + dev_err(dev, "enable regulator failed!\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
>>> + if (ret) {
>>> + if (i2c->adap.bus_regulator)
>>> + regulator_disable(i2c->adap.bus_regulator);
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int mtk_i2c_probe(struct platform_device *pdev)
>>> {
>>> int ret = 0;
>>> @@ -1472,13 +1509,18 @@ static int mtk_i2c_probe(struct
>>> platform_device *pdev)
>>> }
>>> }
>>>
>>> - ret = clk_bulk_prepare_enable(I2C_MT65XX_CLK_MAX, i2c-
>>>> clocks);
>>> + ret = clk_bulk_prepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
>>> if (ret) {
>>> - dev_err(&pdev->dev, "clock enable failed!\n");
>>> return ret;
>>> }
>>> +
>>> + platform_set_drvdata(pdev, i2c);
>>> +
>>> + ret = mtk_i2c_runtime_resume(i2c->dev);
>>> + if (ret < 0)
>>> + goto err_clk_bulk_unprepare;
>>> mtk_i2c_init_hw(i2c);
>>> - clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
>>> + mtk_i2c_runtime_suspend(i2c->dev);
>>>
>>> ret = devm_request_irq(&pdev->dev, irq, mtk_i2c_irq,
>>> IRQF_NO_SUSPEND | IRQF_TRIGGER_NONE,
>>> @@ -1486,19 +1528,22 @@ static int mtk_i2c_probe(struct
>>> platform_device *pdev)
>>> if (ret < 0) {
>>> dev_err(&pdev->dev,
>>> "Request I2C IRQ %d fail\n", irq);
>>> - goto err_bulk_unprepare;
>>> + goto err_clk_bulk_unprepare;
>>> }
>>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>>
>> You had comments from me and from Andi on this delay, and you
>> completely ignored
>> both of us.
>>
>> We're still waiting for an answer to our question.
>
> I am sorry for any confusion caused by my previous response.
> The response to your question was included in the cover letter, which
> might not have been very noticeable.
>
> The delay before runtime_put_autosuspend() actually executes
> mtk_i2c_runtime_suspend() depends on the frequency of I2C usage by the
> devices attached to this bus. A 1000ms delay is a balanced value for
> latency and power metrics based on the MTK platform.
>
Can you please write down "the numbers" into the commit description?
As in, to justify why 1000ms is a balanced value for latency and power, so,
write down a small table containing both values for various delays, like:
Delay(ms) Latency(us) Power(uW)
200 999 9999
500 9999 99999
1000 99999 999999
Please also say what is the minimum acceptable latency.
Thanks,
Angelo
>
> Thank you.
>
> Best regards,
> Zoie
>>
>>
>>> + pm_runtime_use_autosuspend(&pdev->dev);
>>> + pm_runtime_enable(&pdev->dev);
>>
>> devm_pm_runtime_enable() please.
>>
>>>
>>> i2c_set_adapdata(&i2c->adap, i2c);
>>> ret = i2c_add_adapter(&i2c->adap);
>>> if (ret)
>>> - goto err_bulk_unprepare;
>>> -
>>> - platform_set_drvdata(pdev, i2c);
>>> + goto err_pm_runtime_disable;
>>>
>>> return 0;
>>>
>>> -err_bulk_unprepare:
>>> +err_pm_runtime_disable:
>>> + pm_runtime_disable(&pdev->dev);
>>> +err_clk_bulk_unprepare:
>>> clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
>>>
>>> return ret;
>>> @@ -1510,6 +1555,7 @@ static void mtk_i2c_remove(struct
>>> platform_device *pdev)
>>>
>>> i2c_del_adapter(&i2c->adap);
>>>
>>> + pm_runtime_disable(&pdev->dev);
>>> clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
>>> }
>>>
>>> @@ -1518,6 +1564,10 @@ static int mtk_i2c_suspend_noirq(struct
>>> device *dev)
>>> struct mtk_i2c *i2c = dev_get_drvdata(dev);
>>>
>>> i2c_mark_adapter_suspended(&i2c->adap);
>>> +
>>> + if (!pm_runtime_status_suspended(dev))
>>> + mtk_i2c_runtime_suspend(dev);
>>> +
>>> clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
>>>
>>> return 0;
>>> @@ -1536,7 +1586,8 @@ static int mtk_i2c_resume_noirq(struct device
>>> *dev)
>>>
>>> mtk_i2c_init_hw(i2c);
>>>
>>> - clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
>>> + if (pm_runtime_status_suspended(dev))
>>> + mtk_i2c_runtime_suspend(dev);
>>
>> You want to resume, not to suspend, in a resume handler.
>>
>>>
>>> i2c_mark_adapter_resumed(&i2c->adap);
>>>
>>> @@ -1546,6 +1597,8 @@ static int mtk_i2c_resume_noirq(struct device
>>> *dev)
>>> static const struct dev_pm_ops mtk_i2c_pm = {
>>> NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_i2c_suspend_noirq,
>>> mtk_i2c_resume_noirq)
>>> + SET_RUNTIME_PM_OPS(mtk_i2c_runtime_suspend,
>>> mtk_i2c_runtime_resume,
>>> + NULL)
>>> };
>>>
>>> static struct platform_driver mtk_i2c_driver = {
>>
>>
>>
Powered by blists - more mailing lists