| Message ID | 20220422060152.13534-13-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Cleanup MediaTek clk reset drivers and support MT8192/MT8195 | expand |
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@mediatek.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. > + > #endif /* _DT_BINDINGS_RESET_CONTROLLER_MT8192 */ Best regards, Krzysztof
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@mediatek.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. BRs, Rex > > + > > #endif /* _DT_BINDINGS_RESET_CONTROLLER_MT8192 */ > > > Best regards, > Krzysztof
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@mediatek.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
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@mediatek.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 ---- infra_ao: syscon { compatible = "mediatek,mt8192-infracfg", "syscon"; reg = <0 0x10001000 0 0x1000>; #clock-cells = <1>; #reset-cells = <1>; }; thermal { ... resets = <&infra_ao MT8192_INFRA_THERMAL_CTRL_MCU_RST>; ... }; BRs, Rex
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@mediatek.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
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@mediatek.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. 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)); } ----- [1]: https://lore.kernel.org/all/20220427030950.23395-8-rex-bc.chen@mediatek.com/ BRs, Rex
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@mediatek.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
On Thu, 2022-04-28 at 15:23 +0800, Krzysztof Kozlowski wrote: > 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@mediatek.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 Hello Krzysztof, The rest bits could be used in the future. I am not sure who will use them. I will list all 5*32 index in next version. BRs, Rex
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 + #endif /* _DT_BINDINGS_RESET_CONTROLLER_MT8192 */
To support reset of infra_ao, add the bit definition for thermal/PCIe/SVS. Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> --- include/dt-bindings/reset/mt8192-resets.h | 10 ++++++++++ 1 file changed, 10 insertions(+)