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

[Rex-BC Chen <rex-bc.chen@mediatek.com>]

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