Add HACE reset bit definition for AST2500/AST2600.
Signed-off-by: Neal Liu <[email protected]>
Signed-off-by: Johnny Huang <[email protected]>
---
include/dt-bindings/clock/aspeed-clock.h | 1 +
include/dt-bindings/clock/ast2600-clock.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
index 9ff4f6e4558c..06d568382c77 100644
--- a/include/dt-bindings/clock/aspeed-clock.h
+++ b/include/dt-bindings/clock/aspeed-clock.h
@@ -52,5 +52,6 @@
#define ASPEED_RESET_I2C 7
#define ASPEED_RESET_AHB 8
#define ASPEED_RESET_CRT1 9
+#define ASPEED_RESET_HACE 10
#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
--
2.25.1
On 29/06/2022 11:44, Neal Liu wrote:
> Add HACE reset bit definition for AST2500/AST2600.
>
> Signed-off-by: Neal Liu <[email protected]>
> Signed-off-by: Johnny Huang <[email protected]>
> ---
> include/dt-bindings/clock/aspeed-clock.h | 1 +
> include/dt-bindings/clock/ast2600-clock.h | 1 +
> 2 files changed, 2 insertions(+)
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
> -----Original Message-----
> From: Dhananjay Phadke <[email protected]>
> Sent: Wednesday, June 29, 2022 7:50 PM
> To: Neal Liu <[email protected]>; Corentin Labbe
> <[email protected]>; Christophe JAILLET
> <[email protected]>; Randy Dunlap <[email protected]>;
> Herbert Xu <[email protected]>; David S . Miller
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof
> Kozlowski <[email protected]>; Joel Stanley <[email protected]>;
> Andrew Jeffery <[email protected]>; Dhananjay Phadke
> <[email protected]>; Johnny Huang
> <[email protected]>
> Cc: [email protected]; [email protected]; BMC-SW
> <[email protected]>; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v6 2/5] dt-bindings: clock: Add AST2500/AST2600 HACE
> reset definition
>
> On 6/29/2022 2:44 AM, Neal Liu wrote:
> > Add HACE reset bit definition for AST2500/AST2600.
> >
> > Signed-off-by: Neal Liu <[email protected]>
> > Signed-off-by: Johnny Huang <[email protected]>
> > ---
> > include/dt-bindings/clock/aspeed-clock.h | 1 +
> > include/dt-bindings/clock/ast2600-clock.h | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/include/dt-bindings/clock/aspeed-clock.h
> > b/include/dt-bindings/clock/aspeed-clock.h
> > index 9ff4f6e4558c..06d568382c77 100644
> > --- a/include/dt-bindings/clock/aspeed-clock.h
> > +++ b/include/dt-bindings/clock/aspeed-clock.h
> > @@ -52,5 +52,6 @@
> > #define ASPEED_RESET_I2C 7
> > #define ASPEED_RESET_AHB 8
> > #define ASPEED_RESET_CRT1 9
> > +#define ASPEED_RESET_HACE 10
>
> NAK.
>
> I replied to older v5 of this patch, but this v6 also looks incorrect as per HW
> manual.
>
> https://lore.kernel.org/linux-arm-kernel/20220629032008.1579899-1-neal_liu
> @aspeedtech.com/T/#m000bd3388b3e41117aa0eef10bf6f8a6a3a85cce
>
> For both AST2400 and AST2500:
> SCU04[10] = PECI.
>
> It will be best to refactor/split aspeed-clock.h into separate files.
Hi, based on @Krzysztof mentioned, change these define is not allowed due to breaking ABI.
So another way is to define a new value(interface), and we can change driver's implementation.
I know this is not intuitive to hardware register's value, it also confused me at the first time.
On 6/29/2022 8:17 PM, Neal Liu wrote:
[...]
>> On 6/29/2022 2:44 AM, Neal Liu wrote:
>>> Add HACE reset bit definition for AST2500/AST2600.
>>>
>>> Signed-off-by: Neal Liu <[email protected]>
>>> Signed-off-by: Johnny Huang <[email protected]>
>>> ---
>>> include/dt-bindings/clock/aspeed-clock.h | 1 +
>>> include/dt-bindings/clock/ast2600-clock.h | 1 +
>>> 2 files changed, 2 insertions(+)
>>>
>>> diff --git a/include/dt-bindings/clock/aspeed-clock.h
>>> b/include/dt-bindings/clock/aspeed-clock.h
>>> index 9ff4f6e4558c..06d568382c77 100644
>>> --- a/include/dt-bindings/clock/aspeed-clock.h
>>> +++ b/include/dt-bindings/clock/aspeed-clock.h
>>> @@ -52,5 +52,6 @@
>>> #define ASPEED_RESET_I2C 7
>>> #define ASPEED_RESET_AHB 8
>>> #define ASPEED_RESET_CRT1 9
>>> +#define ASPEED_RESET_HACE 10
>>
>> NAK.
>>
>> I replied to older v5 of this patch, but this v6 also looks incorrect as per HW
>> manual.
>>
>> https://lore.kernel.org/linux-arm-kernel/20220629032008.1579899-1-neal_liu
>> @aspeedtech.com/T/#m000bd3388b3e41117aa0eef10bf6f8a6a3a85cce
>>
>> For both AST2400 and AST2500:
>> SCU04[10] = PECI.
>>
>> It will be best to refactor/split aspeed-clock.h into separate files.
>
> Hi, based on @Krzysztof mentioned, change these define is not allowed due to breaking ABI.
> So another way is to define a new value(interface), and we can change driver's implementation.
> I know this is not intuitive to hardware register's value, it also confused me at the first time.
>
>
This is not SW ABI issue. Each controller in the device-tree needs
correct clock and reset paths. aspeed-clock.h is shared between g4 and
g5 dtsi. Not sure how you picked bit 10 for HACE, it's for resetting
PECI controller.
See drivers/clk/clk-aspeed.c, which BTW is duplicating same stuff.
[ASPEED_RESET_MIC] = 18,
[ASPEED_RESET_PWM] = 9,
[ASPEED_RESET_PECI] = 10,
FWIW, the reset bit for HACE and MIC are interchanged for AST2400 and
AST2500 (at least as per HW datasheet).
So this is really fixing what's apparently already broken.
Regards,
Dhananjay
> On 6/29/2022 8:17 PM, Neal Liu wrote:
> [...]
> >> On 6/29/2022 2:44 AM, Neal Liu wrote:
> >>> Add HACE reset bit definition for AST2500/AST2600.
> >>>
> >>> Signed-off-by: Neal Liu <[email protected]>
> >>> Signed-off-by: Johnny Huang <[email protected]>
> >>> ---
> >>> include/dt-bindings/clock/aspeed-clock.h | 1 +
> >>> include/dt-bindings/clock/ast2600-clock.h | 1 +
> >>> 2 files changed, 2 insertions(+)
> >>>
> >>> diff --git a/include/dt-bindings/clock/aspeed-clock.h
> >>> b/include/dt-bindings/clock/aspeed-clock.h
> >>> index 9ff4f6e4558c..06d568382c77 100644
> >>> --- a/include/dt-bindings/clock/aspeed-clock.h
> >>> +++ b/include/dt-bindings/clock/aspeed-clock.h
> >>> @@ -52,5 +52,6 @@
> >>> #define ASPEED_RESET_I2C 7
> >>> #define ASPEED_RESET_AHB 8
> >>> #define ASPEED_RESET_CRT1 9
> >>> +#define ASPEED_RESET_HACE 10
> >>
> >> NAK.
> >>
> >> I replied to older v5 of this patch, but this v6 also looks incorrect
> >> as per HW manual.
> >>
> >> https://lore.kernel.org/linux-arm-kernel/20220629032008.1579899-1-nea
> >> l_liu
> @aspeedtech.com/T/#m000bd3388b3e41117aa0eef10bf6f8a6a3a85cce
> >>
> >> For both AST2400 and AST2500:
> >> SCU04[10] = PECI.
> >>
> >> It will be best to refactor/split aspeed-clock.h into separate files.
> >
> > Hi, based on @Krzysztof mentioned, change these define is not allowed due
> to breaking ABI.
> > So another way is to define a new value(interface), and we can change
> driver's implementation.
> > I know this is not intuitive to hardware register's value, it also confused me at
> the first time.
> >
> >
>
> This is not SW ABI issue. Each controller in the device-tree needs correct clock
> and reset paths. aspeed-clock.h is shared between g4 and
> g5 dtsi. Not sure how you picked bit 10 for HACE, it's for resetting PECI
> controller.
>
> See drivers/clk/clk-aspeed.c, which BTW is duplicating same stuff.
> [ASPEED_RESET_MIC] = 18,
> [ASPEED_RESET_PWM] = 9,
> [ASPEED_RESET_PECI] = 10,
>
> FWIW, the reset bit for HACE and MIC are interchanged for AST2400 and
> AST2500 (at least as per HW datasheet).
>
> So this is really fixing what's apparently already broken.
Actually, "ASPEED_RESET_MIC" is just an index, the reset bit in SCU is bit 18 for both ast2400 and ast2500.
So we have to add the real reset bit in drivers/clk/clk-aspeed.c, which would be:
[ASPEED_RESET_HACE] = 4,
I would send another patch for this change. I think it can be independent with these patch series.
So no need to combine them here.