Re: [PATCH v2 2/4] dt-bindings: reset: Add MT6735 reset bindings

From: AngeloGioacchino Del Regno  <angelogioacchino.delregno@collabora.com>
To: Yassine Oudjana <yassine.oudjana@gmail.com>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Yassine Oudjana <y.oudjana@protonmail.com>,
	Tinghan Shen <tinghan.shen@mediatek.com>,
	Chun-Jie Chen <chun-jie.chen@mediatek.com>,
	Weiyi Lu <weiyi.lu@mediatek.com>, Ikjoon Jang <ikjn@chromium.org>,
	Miles Chen <miles.chen@mediatek.com>,
	Sam Shih <sam.shih@mediatek.com>,
	Chen-Yu Tsai <wenst@chromium.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	devicetree@vger.kernel.org, linux-mediatek@lists.infradead.org,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	~postmarketos/upstreaming@lists.sr.ht,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v2 2/4] dt-bindings: reset: Add MT6735 reset bindings
Date: Mon, 23 May 2022 14:15:49 +0200	[thread overview]
Message-ID: <9639208e-1afc-30ef-d0ea-5eaaa6316af3@collabora.com> (raw)
In-Reply-To: <T9C6CR.ZDXJ22Q377EZ1@gmail.com>

Il 20/05/22 11:13, Yassine Oudjana ha scritto:
> 
> On Fri, May 20 2022 at 10:55:24 +0200, AngeloGioacchino Del Regno 
> <angelogioacchino.delregno@collabora.com> wrote:
>> Il 19/05/22 16:22, Yassine Oudjana ha scritto:
>>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>>>
>>> Add reset definitions for Mediatek MT6735 resets provided by
>>> infracfg and pericfg.
>>>
>>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> ---
>>>   MAINTAINERS                                   |  2 ++
>>>   .../reset/mediatek,mt6735-infracfg.h          | 31 +++++++++++++++++++
>>>   .../reset/mediatek,mt6735-pericfg.h           | 31 +++++++++++++++++++
>>>   3 files changed, 64 insertions(+)
>>>   create mode 100644 include/dt-bindings/reset/mediatek,mt6735-infracfg.h
>>>   create mode 100644 include/dt-bindings/reset/mediatek,mt6735-pericfg.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index a59069263cfb..1c0af554a7b6 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -12503,6 +12503,8 @@ F:    
>>> include/dt-bindings/clock/mediatek,mt6735-apmixedsys.h
>>>   F:    include/dt-bindings/clock/mediatek,mt6735-infracfg.h
>>>   F:    include/dt-bindings/clock/mediatek,mt6735-pericfg.h
>>>   F:    include/dt-bindings/clock/mediatek,mt6735-topckgen.h
>>> +F:    include/dt-bindings/reset/mediatek,mt6735-infracfg.h
>>> +F:    include/dt-bindings/reset/mediatek,mt6735-pericfg.h
>>>   \x7f  MEDIATEK MT76 WIRELESS LAN DRIVER
>>>   M:    Felix Fietkau <nbd@nbd.name>
>>
>> ..snip..
>>
>>> diff --git a/include/dt-bindings/reset/mediatek,mt6735-pericfg.h 
>>> b/include/dt-bindings/reset/mediatek,mt6735-pericfg.h
>>> new file mode 100644
>>> index 000000000000..6cdfaa7ddadf
>>> --- /dev/null
>>> +++ b/include/dt-bindings/reset/mediatek,mt6735-pericfg.h
>>> @@ -0,0 +1,31 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>> +
>>> +#ifndef _DT_BINDINGS_RST_MT6735_PERICFG_H
>>> +#define _DT_BINDINGS_RST_MT6735_PERICFG_H
>>> +
>>> +#define UART0_SW_RST            0
>>> +#define UART1_SW_RST            1
>>> +#define UART2_SW_RST            2
>>> +#define UART3_SW_RST            3
>>> +#define UART4_SW_RST            4
>>
>> where's number 5?
>>
>>> +#define BTIF_SW_RST            6
>>> +#define DISP_PWM_SW_RST            7
>>> +#define PWM_SW_RST            8
>>
>> ...and where's 9?
>>
>>> +#define AUXADC_SW_RST            10
>>> +#define DMA_SW_RST            11
>>> +#define IRDA_SW_RST            12
>>> +#define IRTX_SW_RST            13
>>
>> and 14, 15?
>>
>>> +#define THERM_SW_RST            16
>>> +#define MSDC2_SW_RST            17
>>> +#define MSDC3_SW_RST            17
>>
>> MSDC 2 and 3 are both 17?! :-)
>>
>>> +#define MSDC0_SW_RST            19
>>> +#define MSDC1_SW_RST            20
>>
>> 21?
>>
>>> +#define I2C0_SW_RST            22
>>> +#define I2C1_SW_RST            23
>>> +#define I2C2_SW_RST            24
>>> +#define I2C3_SW_RST            25
>>> +#define USB_SW_RST            28
>>> +
>>
>> and 29-32?
>>
>>> +#define SPI0_SW_RST            33
>>> +
>>> +#endif
>>
>> I have a hunch that you've misunderstood the changes in the resets...
>>
>> What Rex-BC has done in his reset cleanup is exactly to stop directly
>> mapping these to the actual bits that we're using... so the definitions
>> in there will simply be sequential, and the actual mapping is done in
>> your clk-mt6735-pericfg.c driver.
> 
> I did notice that, but reading the documentation in reset.h:
> 
> * @rst_idx_map:Pointer to an array containing ids if input argument is index.
> *        This array is not necessary if our input argument does not mean index.
> 
> I thought that it wasn't necessary to use it. Thinking
> about it now however, I guess that was to maintain compatibility
> with old device trees. I'll change it next time.
> Maybe a note should be put there to avoid confusion
> in the future.

I agree this comment should be clarified.

Thanks,
Angelo