diff mbox series

[v5,2/5] dt-bindings: clock: Add AST2500/AST2600 HACE reset definition

Message ID 20220629032008.1579899-3-neal_liu@aspeedtech.com (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series Add Aspeed crypto driver for hardware acceleration | expand

Commit Message

Neal Liu June 29, 2022, 3:20 a.m. UTC 
Add HACE reset bit definition for AST2500/AST2600.

Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 include/dt-bindings/clock/aspeed-clock.h  | 3 ++-
 include/dt-bindings/clock/ast2600-clock.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Krzysztof Kozlowski June 29, 2022, 5:57 a.m. UTC | #1 
On 29/06/2022 05:20, Neal Liu wrote:
> Add HACE reset bit definition for AST2500/AST2600.
> 
> Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  include/dt-bindings/clock/aspeed-clock.h  | 3 ++-
>  include/dt-bindings/clock/ast2600-clock.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
> index 9ff4f6e4558c..6e040f7c3426 100644
> --- a/include/dt-bindings/clock/aspeed-clock.h
> +++ b/include/dt-bindings/clock/aspeed-clock.h
> @@ -46,11 +46,12 @@
>  #define ASPEED_RESET_MCTP		1
>  #define ASPEED_RESET_ADC		2
>  #define ASPEED_RESET_JTAG_MASTER	3
> -#define ASPEED_RESET_MIC		4
> +#define ASPEED_RESET_HACE		4

I did not ack such change. This is a significant change from previous
version, invalidating my previous ack.

This breaks the ABI, so NAK without proper explanation why ABI break is
accepted.

>  #define ASPEED_RESET_PWM		5
>  #define ASPEED_RESET_PECI		6
>  #define ASPEED_RESET_I2C		7
>  #define ASPEED_RESET_AHB		8
>  #define ASPEED_RESET_CRT1		9
> +#define ASPEED_RESET_MIC		18
>  
>  #endif
> diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
> index 62b9520a00fd..d8b0db2f7a7d 100644
> --- a/include/dt-bindings/clock/ast2600-clock.h
> +++ b/include/dt-bindings/clock/ast2600-clock.h
> @@ -111,6 +111,7 @@
>  #define ASPEED_RESET_PCIE_RC_O		19
>  #define ASPEED_RESET_PCIE_RC_OEN	18
>  #define ASPEED_RESET_PCI_DP		5
> +#define ASPEED_RESET_HACE		4
>  #define ASPEED_RESET_AHB		1
>  #define ASPEED_RESET_SDRAM		0
>  


Best regards,
Krzysztof
Neal Liu June 29, 2022, 7:59 a.m. UTC | #2 
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, June 29, 2022 1:58 PM
> To: Neal Liu <neal_liu@aspeedtech.com>; Corentin Labbe
> <clabbe.montjoie@gmail.com>; Christophe JAILLET
> <christophe.jaillet@wanadoo.fr>; Randy Dunlap <rdunlap@infradead.org>;
> Herbert Xu <herbert@gondor.apana.org.au>; David S . Miller
> <davem@davemloft.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>;
> Andrew Jeffery <andrew@aj.id.au>; Dhananjay Phadke
> <dhphadke@microsoft.com>; Johnny Huang
> <johnny_huang@aspeedtech.com>
> Cc: linux-aspeed@lists.ozlabs.org; linux-crypto@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
> Subject: Re: [PATCH v5 2/5] dt-bindings: clock: Add AST2500/AST2600 HACE
> reset definition
> 
> On 29/06/2022 05:20, Neal Liu wrote:
> > Add HACE reset bit definition for AST2500/AST2600.
> >
> > Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> > Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > ---
> >  include/dt-bindings/clock/aspeed-clock.h  | 3 ++-
> > include/dt-bindings/clock/ast2600-clock.h | 1 +
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/dt-bindings/clock/aspeed-clock.h
> > b/include/dt-bindings/clock/aspeed-clock.h
> > index 9ff4f6e4558c..6e040f7c3426 100644
> > --- a/include/dt-bindings/clock/aspeed-clock.h
> > +++ b/include/dt-bindings/clock/aspeed-clock.h
> > @@ -46,11 +46,12 @@
> >  #define ASPEED_RESET_MCTP		1
> >  #define ASPEED_RESET_ADC		2
> >  #define ASPEED_RESET_JTAG_MASTER	3
> > -#define ASPEED_RESET_MIC		4
> > +#define ASPEED_RESET_HACE		4
> 
> I did not ack such change. This is a significant change from previous version,
> invalidating my previous ack.
> 
> This breaks the ABI, so NAK without proper explanation why ABI break is
> accepted.

I changed the original define (MIC) into different value (see below diff), and add a new define for HACE.
How does that break the ABI? I'll be appreciated if you can explain it more details.
And sorry for not remove ack with new change.

> 
> >  #define ASPEED_RESET_PWM		5
> >  #define ASPEED_RESET_PECI		6
> >  #define ASPEED_RESET_I2C		7
> >  #define ASPEED_RESET_AHB		8
> >  #define ASPEED_RESET_CRT1		9
> > +#define ASPEED_RESET_MIC		18
> >
> >  #endif
> > diff --git a/include/dt-bindings/clock/ast2600-clock.h
> > b/include/dt-bindings/clock/ast2600-clock.h
> > index 62b9520a00fd..d8b0db2f7a7d 100644
> > --- a/include/dt-bindings/clock/ast2600-clock.h
> > +++ b/include/dt-bindings/clock/ast2600-clock.h
> > @@ -111,6 +111,7 @@
> >  #define ASPEED_RESET_PCIE_RC_O		19
> >  #define ASPEED_RESET_PCIE_RC_OEN	18
> >  #define ASPEED_RESET_PCI_DP		5
> > +#define ASPEED_RESET_HACE		4
> >  #define ASPEED_RESET_AHB		1
> >  #define ASPEED_RESET_SDRAM		0
> >
> 
> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski June 29, 2022, 8:03 a.m. UTC | #3 
On 29/06/2022 09:59, Neal Liu wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Wednesday, June 29, 2022 1:58 PM
>> To: Neal Liu <neal_liu@aspeedtech.com>; Corentin Labbe
>> <clabbe.montjoie@gmail.com>; Christophe JAILLET
>> <christophe.jaillet@wanadoo.fr>; Randy Dunlap <rdunlap@infradead.org>;
>> Herbert Xu <herbert@gondor.apana.org.au>; David S . Miller
>> <davem@davemloft.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof
>> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>;
>> Andrew Jeffery <andrew@aj.id.au>; Dhananjay Phadke
>> <dhphadke@microsoft.com>; Johnny Huang
>> <johnny_huang@aspeedtech.com>
>> Cc: linux-aspeed@lists.ozlabs.org; linux-crypto@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
>> Subject: Re: [PATCH v5 2/5] dt-bindings: clock: Add AST2500/AST2600 HACE
>> reset definition
>>
>> On 29/06/2022 05:20, Neal Liu wrote:
>>> Add HACE reset bit definition for AST2500/AST2600.
>>>
>>> Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
>>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> ---
>>>  include/dt-bindings/clock/aspeed-clock.h  | 3 ++-
>>> include/dt-bindings/clock/ast2600-clock.h | 1 +
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/dt-bindings/clock/aspeed-clock.h
>>> b/include/dt-bindings/clock/aspeed-clock.h
>>> index 9ff4f6e4558c..6e040f7c3426 100644
>>> --- a/include/dt-bindings/clock/aspeed-clock.h
>>> +++ b/include/dt-bindings/clock/aspeed-clock.h
>>> @@ -46,11 +46,12 @@
>>>  #define ASPEED_RESET_MCTP		1
>>>  #define ASPEED_RESET_ADC		2
>>>  #define ASPEED_RESET_JTAG_MASTER	3
>>> -#define ASPEED_RESET_MIC		4
>>> +#define ASPEED_RESET_HACE		4
>>
>> I did not ack such change. This is a significant change from previous version,
>> invalidating my previous ack.
>>
>> This breaks the ABI, so NAK without proper explanation why ABI break is
>> accepted.
> 
> I changed the original define (MIC) into different value (see below diff), and add a new define for HACE.
> How does that break the ABI? I'll be appreciated if you can explain it more details.
> And sorry for not remove ack with new change.

Yes, this breaks ABI. Previously the ASPEED_RESET_MIC define had value
of 4, now it has value of something else.

Best regards,
Krzysztof
Neal Liu June 29, 2022, 8:49 a.m. UTC | #4 
> On 29/06/2022 09:59, Neal Liu wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Wednesday, June 29, 2022 1:58 PM
> >> To: Neal Liu <neal_liu@aspeedtech.com>; Corentin Labbe
> >> <clabbe.montjoie@gmail.com>; Christophe JAILLET
> >> <christophe.jaillet@wanadoo.fr>; Randy Dunlap
> >> <rdunlap@infradead.org>; Herbert Xu <herbert@gondor.apana.org.au>;
> >> David S . Miller <davem@davemloft.net>; Rob Herring
> >> <robh+dt@kernel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>; Joel Stanley <joel@jms.id.au>;
> >> Andrew Jeffery <andrew@aj.id.au>; Dhananjay Phadke
> >> <dhphadke@microsoft.com>; Johnny Huang
> <johnny_huang@aspeedtech.com>
> >> Cc: linux-aspeed@lists.ozlabs.org; linux-crypto@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
> >> Subject: Re: [PATCH v5 2/5] dt-bindings: clock: Add AST2500/AST2600
> >> HACE reset definition
> >>
> >> On 29/06/2022 05:20, Neal Liu wrote:
> >>> Add HACE reset bit definition for AST2500/AST2600.
> >>>
> >>> Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> >>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
> >>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>> ---
> >>>  include/dt-bindings/clock/aspeed-clock.h  | 3 ++-
> >>> include/dt-bindings/clock/ast2600-clock.h | 1 +
> >>>  2 files changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/dt-bindings/clock/aspeed-clock.h
> >>> b/include/dt-bindings/clock/aspeed-clock.h
> >>> index 9ff4f6e4558c..6e040f7c3426 100644
> >>> --- a/include/dt-bindings/clock/aspeed-clock.h
> >>> +++ b/include/dt-bindings/clock/aspeed-clock.h
> >>> @@ -46,11 +46,12 @@
> >>>  #define ASPEED_RESET_MCTP		1
> >>>  #define ASPEED_RESET_ADC		2
> >>>  #define ASPEED_RESET_JTAG_MASTER	3
> >>> -#define ASPEED_RESET_MIC		4
> >>> +#define ASPEED_RESET_HACE		4
> >>
> >> I did not ack such change. This is a significant change from previous
> >> version, invalidating my previous ack.
> >>
> >> This breaks the ABI, so NAK without proper explanation why ABI break
> >> is accepted.
> >
> > I changed the original define (MIC) into different value (see below diff), and
> add a new define for HACE.
> > How does that break the ABI? I'll be appreciated if you can explain it more
> details.
> > And sorry for not remove ack with new change.
> 
> Yes, this breaks ABI. Previously the ASPEED_RESET_MIC define had value of 4,
> now it has value of something else.

Got your point. I'll re-define HACE without modifying ABI.
Thanks for your suggestion.
Dhananjay Phadke June 29, 2022, 11:39 a.m. UTC | #5 
On 6/29/2022 1:49 AM, Neal Liu wrote:
[...]
>>>>> diff --git a/include/dt-bindings/clock/aspeed-clock.h
>>>>> b/include/dt-bindings/clock/aspeed-clock.h
>>>>> index 9ff4f6e4558c..6e040f7c3426 100644
>>>>> --- a/include/dt-bindings/clock/aspeed-clock.h
>>>>> +++ b/include/dt-bindings/clock/aspeed-clock.h
>>>>> @@ -46,11 +46,12 @@
>>>>>   #define ASPEED_RESET_MCTP		1
>>>>>   #define ASPEED_RESET_ADC		2
>>>>>   #define ASPEED_RESET_JTAG_MASTER	3
>>>>> -#define ASPEED_RESET_MIC		4
>>>>> +#define ASPEED_RESET_HACE		4
>>>>
>>>> I did not ack such change. This is a significant change from previous
>>>> version, invalidating my previous ack.
>>>>
>>>> This breaks the ABI, so NAK without proper explanation why ABI break
>>>> is accepted.
>>>
>>> I changed the original define (MIC) into different value (see below diff), and
>> add a new define for HACE.
>>> How does that break the ABI? I'll be appreciated if you can explain it more
>> details.
>>> And sorry for not remove ack with new change.
>>
>> Yes, this breaks ABI. Previously the ASPEED_RESET_MIC define had value of 4,
>> now it has value of something else.
> 
> Got your point. I'll re-define HACE without modifying ABI.
> Thanks for your suggestion.
> 

As per HW manuals, new def is correct for AST2500, but not for AST2400.

AST2500:
SCU04[4] = HACE
SCU04[18] = MIC

AST2400:
SCU04[4] = MIC
SCU04[18] = HACE

The header file is shared between AST2400 and AST2500 (aspeed-g4.dtsi
and aspeed-g5.dtsi), which needs to be split into separate header files
given the collision.

Also, It will be better to split AST2500 and AST2600 changes in separate
patches.

Regards,
Dhananjay
diff mbox series

Patch

diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
index 9ff4f6e4558c..6e040f7c3426 100644
--- a/include/dt-bindings/clock/aspeed-clock.h
+++ b/include/dt-bindings/clock/aspeed-clock.h
@@ -46,11 +46,12 @@ 
 #define ASPEED_RESET_MCTP		1
 #define ASPEED_RESET_ADC		2
 #define ASPEED_RESET_JTAG_MASTER	3
-#define ASPEED_RESET_MIC		4
+#define ASPEED_RESET_HACE		4
 #define ASPEED_RESET_PWM		5
 #define ASPEED_RESET_PECI		6
 #define ASPEED_RESET_I2C		7
 #define ASPEED_RESET_AHB		8
 #define ASPEED_RESET_CRT1		9
+#define ASPEED_RESET_MIC		18
 
 #endif
diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
index 62b9520a00fd..d8b0db2f7a7d 100644
--- a/include/dt-bindings/clock/ast2600-clock.h
+++ b/include/dt-bindings/clock/ast2600-clock.h
@@ -111,6 +111,7 @@ 
 #define ASPEED_RESET_PCIE_RC_O		19
 #define ASPEED_RESET_PCIE_RC_OEN	18
 #define ASPEED_RESET_PCI_DP		5
+#define ASPEED_RESET_HACE		4
 #define ASPEED_RESET_AHB		1
 #define ASPEED_RESET_SDRAM		0