Re: [PATCH V3 12/17] dt-binding: mt8192: Add infra_ao reset bit

[Rex-BC Chen]

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.

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

BRs,
Rex