linux-kernel - RE: [PATCH v6 2/5] dt-bindings: clock: Add AST2500/AST2600 HACE reset definition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <HK0PR06MB3202A31CD5731DE84210904A80BA9@HK0PR06MB3202.apcprd06.prod.outlook.com>
Date:   Thu, 30 Jun 2022 07:12:45 +0000
From:   Neal Liu <neal_liu@...eedtech.com>
To:     Dhananjay Phadke <dphadke@...ux.microsoft.com>,
        Corentin Labbe <clabbe.montjoie@...il.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Randy Dunlap <rdunlap@...radead.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S . Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Joel Stanley <joel@....id.au>,
        Andrew Jeffery <andrew@...id.au>,
        Dhananjay Phadke <dhphadke@...rosoft.com>,
        Johnny Huang <johnny_huang@...eedtech.com>
CC:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        BMC-SW <BMC-SW@...eedtech.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v6 2/5] dt-bindings: clock: Add AST2500/AST2600 HACE reset
 definition

> 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 <neal_liu@...eedtech.com>
> >>> Signed-off-by: Johnny Huang <johnny_huang@...eedtech.com>
> >>> ---
> >>>    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.