| 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: <93c0c4d7-1e3a-56d6-de22-aa2c948403e2@linaro.org>
Date: Thu, 28 Apr 2022 09:23:13 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Rex-BC Chen <rex-bc.chen@...iatek.com>,
"mturquette@...libre.com" <mturquette@...libre.com>,
"sboyd@...nel.org" <sboyd@...nel.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>
Cc: "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"angelogioacchino.delregno@...labora.com"
<angelogioacchino.delregno@...labora.com>,
Chun-Jie Chen (陳浚桀)
<Chun-Jie.Chen@...iatek.com>,
"wenst@...omium.org" <wenst@...omium.org>,
Runyang Chen (陈润洋)
<Runyang.Chen@...iatek.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [PATCH V3 12/17] dt-binding: mt8192: Add infra_ao reset bit
On 28/04/2022 08:48, Rex-BC Chen wrote:
> On Thu, 2022-04-28 at 14:40 +0800, Krzysztof Kozlowski wrote:
>> On 26/04/2022 10:23, Rex-BC Chen wrote:
>>> On Mon, 2022-04-25 at 15:52 +0800, Krzysztof Kozlowski wrote:
>>>> On 25/04/2022 07:01, Rex-BC Chen wrote:
>>>>> On Sat, 2022-04-23 at 18:28 +0800, Krzysztof Kozlowski wrote:
>>>>>> On 22/04/2022 08:01, Rex-BC Chen wrote:
>>>>>>> To support reset of infra_ao, add the bit definition for
>>>>>>> thermal/PCIe/SVS.
>>>>>>>
>>>>>>> Signed-off-by: Rex-BC Chen <rex-bc.chen@...iatek.com>
>>>>>>> ---
>>>>>>> include/dt-bindings/reset/mt8192-resets.h | 10 ++++++++++
>>>>>>> 1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/dt-bindings/reset/mt8192-resets.h
>>>>>>> b/include/dt-bindings/reset/mt8192-resets.h
>>>>>>> index be9a7ca245b9..d5f3433175c1 100644
>>>>>>> --- a/include/dt-bindings/reset/mt8192-resets.h
>>>>>>> +++ b/include/dt-bindings/reset/mt8192-resets.h
>>>>>>> @@ -27,4 +27,14 @@
>>>>>>>
>>>>>>> #define MT8192_TOPRGU_SW_RST_NUM
>>>>>>> 23
>>>>>>>
>>>>>>> +/* INFRA RST0 */
>>>>>>> +#define MT8192_INFRA_RST0_LVTS_AP_RST
>>>>>>>
>>>>>>> 0
>>>>>>> +/* INFRA RST2 */
>>>>>>> +#define MT8192_INFRA_RST2_PCIE_PHY_RST
>>>>>>>
>>>>>>> 15
>>>>>>> +/* INFRA RST3 */
>>>>>>> +#define MT8192_INFRA_RST3_PTP_RST
>>>>>>> 5
>>>>>>> +/* INFRA RST4 */
>>>>>>> +#define MT8192_INFRA_RST4_LVTS_MCU
>>>>>>> 12
>>>>>>> +#define MT8192_INFRA_RST4_PCIE_TOP
>>>>>>> 1
>>>>>>
>>>>>> These should be the IDs of reset, not some register
>>>>>> values/offsets.
>>>>>> Therefore it is expected to have them incremented by 1.
>>>>>>
>>>>>>
>>>>>
>>>>> Hello Krzysztof,
>>>>>
>>>>> This is define bit.
>>>>>
>>>>> There is serveral reset set for infra_ao while it's not serial.
>>>>> For MT8192, it's 0x120/0x130/0x140/0x150/0x730.
>>>>> We are implement #reset-cells = <2>, and we can use this reset
>>>>> drive
>>>>> more easier.
>>>>>
>>>>> For example, in dts, we can define
>>>>> infra_ao: syscon {
>>>>> compatible = "mediatek,mt8192-infracfg", "syscon";
>>>>> reg = <0 0x10001000 0 0x1000>;
>>>>> #clock-cells = <1>;
>>>>> #reset-cells = <2>;
>>>>> };
>>>>>
>>>>> thermal {
>>>>> ...
>>>>> resets = <&infra_ao 0x730 MT8192_INFRA_RST4_LVTS_MCU>;
>>>>> ...
>>>>> };
>>>>>
>>>>> If it's acceptabel, I can update all bit difinition from 0 to
>>>>> 15
>>>>> for
>>>>> all reset set.
>>>>
>>>> Bits are not acceptable, because you embed specific device
>>>> programming
>>>> model (register bits) into the binding.
>>>>
>>>> These should be IDs, so decimal numbers incremented from 0, so:
>>>> #define MT8192_INFRA_RST0_LVTS_AP_RST
>>>> 0
>>>> #define MT8192_INFRA_RST4_LVTS_MCU
>>>> 1
>>>> #define MT8192_INFRA_RST4_PCIE_TOP
>>>> 2
>>>>
>>>> And what is 0x730 in your example? It does not look like ID of a
>>>> reset...
>>>>
>>>> Entire changeset look wrong from DT point of view.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> Hello Krzysztof,
>>>
>>> Got it. I will modify them to reset index.
>>> And the dts in my next version would somthing like this:
>>>
>>> ----
>>> #define MT8192_INFRA_THERMAL_CTRL_RST 0
>>> #define MT8192_INFRA_PEXTP_PHY_RST 79
>>> #define MT8192_INFRA_PTP_RST 101
>>> #define MT8192_INFRA_RST4_PCIE_TOP 129
>>> #define MT8192_INFRA_THERMAL_CTRL_MCU_RST 140
>>
>> These are still not IDs, incremented by one.
>>
>> So again from beginning:
>> 0
>> 1
>> 2
>> ...
>>
>> Do not encode hardware register bits into the binding.
>>
>> Best regards,
>> Krzysztof
>
> Hello Krzysztof,
>
> It's not bit definiton, and it's index for our reset.
> We have 32*5 reset bits for infra.
> But we only use these 5 index currently, I do not list all of them.
You do not have to list all of them. You can list three, e.g.:
#define MT8192_INFRA_THERMAL_CTRL_RST 0
#define MT8192_INFRA_PEXTP_PHY_RST 1
#define MT8192_INFRA_PTP_RST 2
and you will add all further later. This is how all dt-binding headers
are created.
>
> The implementation is in [1].
> -----
> static int mtk_reset_update_set_clr(struct reset_controller_dev *rcdev,
> unsigned int deassert_ofs = deassert ? 0x4 : 0;
>
> return regmap_write(data->regmap,
> data->desc->rst_bank_ofs[id /
> RST_NR_PER_BANK] +
> deassert_ofs,
> BIT(id % RST_NR_PER_BANK));
> }
Exactly, you hard-code the hardware programming model - register
values/bits/whatever - in the ID, which is not correct. Additionally,
bindings are (mostly) independent of Linux implementation.
Best regards,
Krzysztof
Powered by blists - more mailing lists