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