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@xxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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
