Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Jisheng Zhang]

On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> Add detailed information about thead,reset-sample driver, and improve
> usage documentation.
> 
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
>  docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
>  1 file changed, 67 insertions(+), 20 deletions(-)
> 
> diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> index 8bb9e91f1a9b..fe05fc5bb85a 100644
> --- a/docs/platform/thead-c9xx.md
> +++ b/docs/platform/thead-c9xx.md
> @@ -1,8 +1,8 @@
> -T-HEAD C9xx Series Processors
> -=============================
> +T-HEAD Processors
> +=================
>  
> -The **C9xx** series processors are high-performance RISC-V architecture
> -multi-core processors with AI vector acceleration engine.
> +T-HEAD provides high-performance RISC-V architecture multi-core
> +processors with AI vector acceleration engine.
>  
>  For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
>  
> @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
>  Platform Options
>  ----------------
>  
> -The *T-HEAD C9xx* does not have any platform-specific compile options
> +The *T-HEAD CPU* does not have any platform-specific compile options
>  because it uses generic platform.
>  
>  ```
>  CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
>  ```
>  
> -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
> +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
> +custom driver for the SMP system bootup; the single-core system doesn't need
> +it.
> +
> +T-HEAD Fdt Reset Driver Introduction
> +------------------------------------
> +
> +Every T-HEAD CPU provides a reset control signal and reset address signals.
> + - Reset address signal determines CPU where to start up.
> + - Reset control signal releases CPU from reset state and begins to execute
> +   at reset address.
> +
> +Many vendors would gather these signals into SoC control registers. These
> +register designs are similar but with different base addresses and bits
> +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> +users bring up their SMP system quickly with the below settings:

+DT maintainers and DT list.

I can submit a dt-binding for this if DT maintainers agree with below
properties. Could you please help review?

> +
> + - entry-reg:
> +	The base address to store reset address value
> +
> + - entry-cnt:
> +	The numbers of entry-reg, all of them set the same reset address
> +
> + - control-reg:
> +	The base address to reset the controller
> +
> + - control-val:
> +	Write which bits of control-reg for booting
> +
> + - csr-copy:
> +	This array determines which csrs to copy from primary hart to the
> +	secondary harts, which are set in sequence from left to right. The
> +	secondary harts should keep the same setting as the primary hart.
> +	These settings are also the first part of the bootup instructions
> +	for secondary harts.
> +
> + - using-csr-reset:
> +	A legacy reset controller for the SMP system, but abandoned in the
> +	latest T-HEAD processors.
> +
> +Here is the dts example:
> +```
> +	reset: reset-sample {
> +		compatible = "thead,reset-sample";
> +		entry-reg = <0xff 0xff019050>;
> +		entry-cnt = <4>;
> +		control-reg = <0xff 0xff015004>;
> +		control-val = <0x1c>;
> +		csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> +	};
> +```
> +
> +Legacy dts example (Abandoned):
> +```
> +	reset: reset-sample {
> +		compatible = "thead,reset-sample";
> +		using-csr-reset;
> +		csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> +	};
> +```
>  
>  DTS Example1: (Single core, eg: Allwinner D1 - c906)
>  ----------------------------------------------------
> @@ -181,16 +241,3 @@ DTS Example2: (Multi cores with soc reset-regs)
>  		};
>  	}
>  ```
> -
> -DTS Example2: (Multi cores with old reset csrs)
> ------------------------------------------------
> -```
> -	reset: reset-sample {
> -		compatible = "thead,reset-sample";
> -		using-csr-reset;
> -		csr-copy = <0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc
> -			    0x3b0 0x3b1 0x3b2 0x3b3
> -			    0x3b4 0x3b5 0x3b6 0x3b7
> -			    0x3a0>;
> -	};
> -```
> -- 
> 2.36.1
> 

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Guo Ren]

On Fri, Aug 11, 2023 at 12:45 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Add detailed information about thead,reset-sample driver, and improve
> > usage documentation.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> >  docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
> >  1 file changed, 67 insertions(+), 20 deletions(-)
> >
> > diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> > index 8bb9e91f1a9b..fe05fc5bb85a 100644
> > --- a/docs/platform/thead-c9xx.md
> > +++ b/docs/platform/thead-c9xx.md
> > @@ -1,8 +1,8 @@
> > -T-HEAD C9xx Series Processors
> > -=============================
> > +T-HEAD Processors
> > +=================
> >
> > -The **C9xx** series processors are high-performance RISC-V architecture
> > -multi-core processors with AI vector acceleration engine.
> > +T-HEAD provides high-performance RISC-V architecture multi-core
> > +processors with AI vector acceleration engine.
> >
> >  For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
> >
> > @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
> >  Platform Options
> >  ----------------
> >
> > -The *T-HEAD C9xx* does not have any platform-specific compile options
> > +The *T-HEAD CPU* does not have any platform-specific compile options
> >  because it uses generic platform.
> >
> >  ```
> >  CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
> >  ```
> >
> > -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> > -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> > +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
> > +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
> > +custom driver for the SMP system bootup; the single-core system doesn't need
> > +it.
> > +
> > +T-HEAD Fdt Reset Driver Introduction
> > +------------------------------------
> > +
> > +Every T-HEAD CPU provides a reset control signal and reset address signals.
> > + - Reset address signal determines CPU where to start up.
> > + - Reset control signal releases CPU from reset state and begins to execute
> > +   at reset address.
> > +
> > +Many vendors would gather these signals into SoC control registers. These
> > +register designs are similar but with different base addresses and bits
> > +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> > +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> > +users bring up their SMP system quickly with the below settings:
>
> +DT maintainers and DT list.
>
> I can submit a dt-binding for this if DT maintainers agree with below
> properties. Could you please help review?
Of cause, I would help review.

This documentation describes the fact of opensbi thead,reset-sample
driver's status. The node name and property names have existed for 2
years, changing them would cause confusion, so let's keep them
continuously.

>
> > +
> > + - entry-reg:
> > +     The base address to store reset address value
> > +
> > + - entry-cnt:
> > +     The numbers of entry-reg, all of them set the same reset address
> > +
> > + - control-reg:
> > +     The base address to reset the controller
> > +
> > + - control-val:
> > +     Write which bits of control-reg for booting
> > +
> > + - csr-copy:
> > +     This array determines which csrs to copy from primary hart to the
> > +     secondary harts, which are set in sequence from left to right. The
> > +     secondary harts should keep the same setting as the primary hart.
> > +     These settings are also the first part of the bootup instructions
> > +     for secondary harts.
> > +
> > + - using-csr-reset:
> > +     A legacy reset controller for the SMP system, but abandoned in the
> > +     latest T-HEAD processors.
> > +
> > +Here is the dts example:
> > +```
> > +     reset: reset-sample {
> > +             compatible = "thead,reset-sample";
> > +             entry-reg = <0xff 0xff019050>;
> > +             entry-cnt = <4>;
> > +             control-reg = <0xff 0xff015004>;
> > +             control-val = <0x1c>;
> > +             csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> > +     };
> > +```
> > +
> > +Legacy dts example (Abandoned):
> > +```
> > +     reset: reset-sample {
> > +             compatible = "thead,reset-sample";
> > +             using-csr-reset;
> > +             csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> > +     };
> > +```
> >
> >  DTS Example1: (Single core, eg: Allwinner D1 - c906)
> >  ----------------------------------------------------
> > @@ -181,16 +241,3 @@ DTS Example2: (Multi cores with soc reset-regs)
> >               };
> >       }
> >  ```
> > -
> > -DTS Example2: (Multi cores with old reset csrs)
> > ------------------------------------------------
> > -```
> > -     reset: reset-sample {
> > -             compatible = "thead,reset-sample";
> > -             using-csr-reset;
> > -             csr-copy = <0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc
> > -                         0x3b0 0x3b1 0x3b2 0x3b3
> > -                         0x3b4 0x3b5 0x3b6 0x3b7
> > -                         0x3a0>;
> > -     };
> > -```
> > --
> > 2.36.1
> >



-- 
Best Regards
 Guo Ren

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Jessica Clarke]

On 10 Aug 2023, at 17:33, Jisheng Zhang <jszhang@kernel.org> wrote:
> 
> On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
>> From: Guo Ren <guoren@linux.alibaba.com>
>> 
>> Add detailed information about thead,reset-sample driver, and improve
>> usage documentation.
>> 
>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
>> Signed-off-by: Guo Ren <guoren@kernel.org>
>> ---
>> docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
>> 1 file changed, 67 insertions(+), 20 deletions(-)
>> 
>> diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
>> index 8bb9e91f1a9b..fe05fc5bb85a 100644
>> --- a/docs/platform/thead-c9xx.md
>> +++ b/docs/platform/thead-c9xx.md
>> @@ -1,8 +1,8 @@
>> -T-HEAD C9xx Series Processors
>> -=============================
>> +T-HEAD Processors
>> +=================
>> 
>> -The **C9xx** series processors are high-performance RISC-V architecture
>> -multi-core processors with AI vector acceleration engine.
>> +T-HEAD provides high-performance RISC-V architecture multi-core
>> +processors with AI vector acceleration engine.
>> 
>> For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
>> 
>> @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
>> Platform Options
>> ----------------
>> 
>> -The *T-HEAD C9xx* does not have any platform-specific compile options
>> +The *T-HEAD CPU* does not have any platform-specific compile options
>> because it uses generic platform.
>> 
>> ```
>> CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
>> ```
>> 
>> -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
>> -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
>> +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
>> +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
>> +custom driver for the SMP system bootup; the single-core system doesn't need
>> +it.
>> +
>> +T-HEAD Fdt Reset Driver Introduction
>> +------------------------------------
>> +
>> +Every T-HEAD CPU provides a reset control signal and reset address signals.
>> + - Reset address signal determines CPU where to start up.
>> + - Reset control signal releases CPU from reset state and begins to execute
>> +   at reset address.
>> +
>> +Many vendors would gather these signals into SoC control registers. These
>> +register designs are similar but with different base addresses and bits
>> +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
>> +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
>> +users bring up their SMP system quickly with the below settings:
> 
> +DT maintainers and DT list.
> 
> I can submit a dt-binding for this if DT maintainers agree with below
> properties. Could you please help review?

I thought this was already discussed on the OpenSBI list 2 months ago,
and received pretty negative feedback.

Jess

>> +
>> + - entry-reg:
>> + The base address to store reset address value
>> +
>> + - entry-cnt:
>> + The numbers of entry-reg, all of them set the same reset address
>> +
>> + - control-reg:
>> + The base address to reset the controller
>> +
>> + - control-val:
>> + Write which bits of control-reg for booting
>> +
>> + - csr-copy:
>> + This array determines which csrs to copy from primary hart to the
>> + secondary harts, which are set in sequence from left to right. The
>> + secondary harts should keep the same setting as the primary hart.
>> + These settings are also the first part of the bootup instructions
>> + for secondary harts.
>> +
>> + - using-csr-reset:
>> + A legacy reset controller for the SMP system, but abandoned in the
>> + latest T-HEAD processors.
>> +
>> +Here is the dts example:
>> +```
>> + reset: reset-sample {
>> + compatible = "thead,reset-sample";
>> + entry-reg = <0xff 0xff019050>;
>> + entry-cnt = <4>;
>> + control-reg = <0xff 0xff015004>;
>> + control-val = <0x1c>;
>> + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
>> + };
>> +```
>> +
>> +Legacy dts example (Abandoned):
>> +```
>> + reset: reset-sample {
>> + compatible = "thead,reset-sample";
>> + using-csr-reset;
>> + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
>> + };
>> +```
>> 
>> DTS Example1: (Single core, eg: Allwinner D1 - c906)
>> ----------------------------------------------------
>> @@ -181,16 +241,3 @@ DTS Example2: (Multi cores with soc reset-regs)
>> };
>> }
>> ```
>> -
>> -DTS Example2: (Multi cores with old reset csrs)
>> ------------------------------------------------
>> -```
>> - reset: reset-sample {
>> - compatible = "thead,reset-sample";
>> - using-csr-reset;
>> - csr-copy = <0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc
>> -     0x3b0 0x3b1 0x3b2 0x3b3
>> -     0x3b4 0x3b5 0x3b6 0x3b7
>> -     0x3a0>;
>> - };
>> -```
>> -- 
>> 2.36.1

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Guo Ren]

On Fri, Aug 11, 2023 at 8:42 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 10 Aug 2023, at 17:33, Jisheng Zhang <jszhang@kernel.org> wrote:
> >
> > On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> >> From: Guo Ren <guoren@linux.alibaba.com>
> >>
> >> Add detailed information about thead,reset-sample driver, and improve
> >> usage documentation.
> >>
> >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> >> Signed-off-by: Guo Ren <guoren@kernel.org>
> >> ---
> >> docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
> >> 1 file changed, 67 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> >> index 8bb9e91f1a9b..fe05fc5bb85a 100644
> >> --- a/docs/platform/thead-c9xx.md
> >> +++ b/docs/platform/thead-c9xx.md
> >> @@ -1,8 +1,8 @@
> >> -T-HEAD C9xx Series Processors
> >> -=============================
> >> +T-HEAD Processors
> >> +=================
> >>
> >> -The **C9xx** series processors are high-performance RISC-V architecture
> >> -multi-core processors with AI vector acceleration engine.
> >> +T-HEAD provides high-performance RISC-V architecture multi-core
> >> +processors with AI vector acceleration engine.
> >>
> >> For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
> >>
> >> @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
> >> Platform Options
> >> ----------------
> >>
> >> -The *T-HEAD C9xx* does not have any platform-specific compile options
> >> +The *T-HEAD CPU* does not have any platform-specific compile options
> >> because it uses generic platform.
> >>
> >> ```
> >> CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
> >> ```
> >>
> >> -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> >> -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> >> +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
> >> +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
> >> +custom driver for the SMP system bootup; the single-core system doesn't need
> >> +it.
> >> +
> >> +T-HEAD Fdt Reset Driver Introduction
> >> +------------------------------------
> >> +
> >> +Every T-HEAD CPU provides a reset control signal and reset address signals.
> >> + - Reset address signal determines CPU where to start up.
> >> + - Reset control signal releases CPU from reset state and begins to execute
> >> +   at reset address.
> >> +
> >> +Many vendors would gather these signals into SoC control registers. These
> >> +register designs are similar but with different base addresses and bits
> >> +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> >> +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> >> +users bring up their SMP system quickly with the below settings:
> >
> > +DT maintainers and DT list.
> >
> > I can submit a dt-binding for this if DT maintainers agree with below
> > properties. Could you please help review?
>
> I thought this was already discussed on the OpenSBI list 2 months ago,
> and received pretty negative feedback.
Yes, we want to correct all DT grammar & compile problems, and make it
legal first and try again. I thought every vendor has their own choice
of how to deliver their hardware support. The motivation of this
driver is to ease the delivery of T-HEAD CPU cores on different
platforms; people only need three things: jtag_init_script & opensbi &
linux_Image, then they could boot on their own FPGA prototype
platform, and they needn't prepare any software stuff, all the generic
binaries could be directly used. The th1520 could be a good example
for them. That's why we consistently push this thing.

>
> Jess
>
> >> +
> >> + - entry-reg:
> >> + The base address to store reset address value
> >> +
> >> + - entry-cnt:
> >> + The numbers of entry-reg, all of them set the same reset address
> >> +
> >> + - control-reg:
> >> + The base address to reset the controller
> >> +
> >> + - control-val:
> >> + Write which bits of control-reg for booting
> >> +
> >> + - csr-copy:
> >> + This array determines which csrs to copy from primary hart to the
> >> + secondary harts, which are set in sequence from left to right. The
> >> + secondary harts should keep the same setting as the primary hart.
> >> + These settings are also the first part of the bootup instructions
> >> + for secondary harts.
> >> +
> >> + - using-csr-reset:
> >> + A legacy reset controller for the SMP system, but abandoned in the
> >> + latest T-HEAD processors.
> >> +
> >> +Here is the dts example:
> >> +```
> >> + reset: reset-sample {
> >> + compatible = "thead,reset-sample";
> >> + entry-reg = <0xff 0xff019050>;
> >> + entry-cnt = <4>;
> >> + control-reg = <0xff 0xff015004>;
> >> + control-val = <0x1c>;
> >> + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> >> + };
> >> +```
> >> +
> >> +Legacy dts example (Abandoned):
> >> +```
> >> + reset: reset-sample {
> >> + compatible = "thead,reset-sample";
> >> + using-csr-reset;
> >> + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> >> + };
> >> +```
> >>
> >> DTS Example1: (Single core, eg: Allwinner D1 - c906)
> >> ----------------------------------------------------
> >> @@ -181,16 +241,3 @@ DTS Example2: (Multi cores with soc reset-regs)
> >> };
> >> }
> >> ```
> >> -
> >> -DTS Example2: (Multi cores with old reset csrs)
> >> ------------------------------------------------
> >> -```
> >> - reset: reset-sample {
> >> - compatible = "thead,reset-sample";
> >> - using-csr-reset;
> >> - csr-copy = <0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc
> >> -     0x3b0 0x3b1 0x3b2 0x3b3
> >> -     0x3b4 0x3b5 0x3b6 0x3b7
> >> -     0x3a0>;
> >> - };
> >> -```
> >> --
> >> 2.36.1
>
>


-- 
Best Regards
 Guo Ren

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Jessica Clarke]

On 11 Aug 2023, at 03:01, Guo Ren <guoren@kernel.org> wrote:
> 
> On Fri, Aug 11, 2023 at 8:42 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>> 
>> On 10 Aug 2023, at 17:33, Jisheng Zhang <jszhang@kernel.org> wrote:
>>> 
>>> On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
>>>> From: Guo Ren <guoren@linux.alibaba.com>
>>>> 
>>>> Add detailed information about thead,reset-sample driver, and improve
>>>> usage documentation.
>>>> 
>>>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>>>> ---
>>>> docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
>>>> 1 file changed, 67 insertions(+), 20 deletions(-)
>>>> 
>>>> diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
>>>> index 8bb9e91f1a9b..fe05fc5bb85a 100644
>>>> --- a/docs/platform/thead-c9xx.md
>>>> +++ b/docs/platform/thead-c9xx.md
>>>> @@ -1,8 +1,8 @@
>>>> -T-HEAD C9xx Series Processors
>>>> -=============================
>>>> +T-HEAD Processors
>>>> +=================
>>>> 
>>>> -The **C9xx** series processors are high-performance RISC-V architecture
>>>> -multi-core processors with AI vector acceleration engine.
>>>> +T-HEAD provides high-performance RISC-V architecture multi-core
>>>> +processors with AI vector acceleration engine.
>>>> 
>>>> For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
>>>> 
>>>> @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
>>>> Platform Options
>>>> ----------------
>>>> 
>>>> -The *T-HEAD C9xx* does not have any platform-specific compile options
>>>> +The *T-HEAD CPU* does not have any platform-specific compile options
>>>> because it uses generic platform.
>>>> 
>>>> ```
>>>> CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
>>>> ```
>>>> 
>>>> -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
>>>> -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
>>>> +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
>>>> +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
>>>> +custom driver for the SMP system bootup; the single-core system doesn't need
>>>> +it.
>>>> +
>>>> +T-HEAD Fdt Reset Driver Introduction
>>>> +------------------------------------
>>>> +
>>>> +Every T-HEAD CPU provides a reset control signal and reset address signals.
>>>> + - Reset address signal determines CPU where to start up.
>>>> + - Reset control signal releases CPU from reset state and begins to execute
>>>> +   at reset address.
>>>> +
>>>> +Many vendors would gather these signals into SoC control registers. These
>>>> +register designs are similar but with different base addresses and bits
>>>> +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
>>>> +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
>>>> +users bring up their SMP system quickly with the below settings:
>>> 
>>> +DT maintainers and DT list.
>>> 
>>> I can submit a dt-binding for this if DT maintainers agree with below
>>> properties. Could you please help review?
>> 
>> I thought this was already discussed on the OpenSBI list 2 months ago,
>> and received pretty negative feedback.
> Yes, we want to correct all DT grammar & compile problems, and make it
> legal first and try again. I thought every vendor has their own choice
> of how to deliver their hardware support. The motivation of this
> driver is to ease the delivery of T-HEAD CPU cores on different
> platforms; people only need three things: jtag_init_script & opensbi &
> linux_Image, then they could boot on their own FPGA prototype
> platform, and they needn't prepare any software stuff, all the generic
> binaries could be directly used. The th1520 could be a good example
> for them. That's why we consistently push this thing.

What’s changed to make people say yes rather than no this time?

I for one will not give positive feedback for self-modifying code in my
firmware (outside of the necessary self-relocation at startup before
the PMP is enabled).

Jess

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Guo Ren]

On Fri, Aug 11, 2023 at 10:05 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 11 Aug 2023, at 03:01, Guo Ren <guoren@kernel.org> wrote:
> >
> > On Fri, Aug 11, 2023 at 8:42 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>
> >> On 10 Aug 2023, at 17:33, Jisheng Zhang <jszhang@kernel.org> wrote:
> >>>
> >>> On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> >>>> From: Guo Ren <guoren@linux.alibaba.com>
> >>>>
> >>>> Add detailed information about thead,reset-sample driver, and improve
> >>>> usage documentation.
> >>>>
> >>>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> >>>> Signed-off-by: Guo Ren <guoren@kernel.org>
> >>>> ---
> >>>> docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
> >>>> 1 file changed, 67 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> >>>> index 8bb9e91f1a9b..fe05fc5bb85a 100644
> >>>> --- a/docs/platform/thead-c9xx.md
> >>>> +++ b/docs/platform/thead-c9xx.md
> >>>> @@ -1,8 +1,8 @@
> >>>> -T-HEAD C9xx Series Processors
> >>>> -=============================
> >>>> +T-HEAD Processors
> >>>> +=================
> >>>>
> >>>> -The **C9xx** series processors are high-performance RISC-V architecture
> >>>> -multi-core processors with AI vector acceleration engine.
> >>>> +T-HEAD provides high-performance RISC-V architecture multi-core
> >>>> +processors with AI vector acceleration engine.
> >>>>
> >>>> For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
> >>>>
> >>>> @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
> >>>> Platform Options
> >>>> ----------------
> >>>>
> >>>> -The *T-HEAD C9xx* does not have any platform-specific compile options
> >>>> +The *T-HEAD CPU* does not have any platform-specific compile options
> >>>> because it uses generic platform.
> >>>>
> >>>> ```
> >>>> CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
> >>>> ```
> >>>>
> >>>> -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> >>>> -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> >>>> +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
> >>>> +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
> >>>> +custom driver for the SMP system bootup; the single-core system doesn't need
> >>>> +it.
> >>>> +
> >>>> +T-HEAD Fdt Reset Driver Introduction
> >>>> +------------------------------------
> >>>> +
> >>>> +Every T-HEAD CPU provides a reset control signal and reset address signals.
> >>>> + - Reset address signal determines CPU where to start up.
> >>>> + - Reset control signal releases CPU from reset state and begins to execute
> >>>> +   at reset address.
> >>>> +
> >>>> +Many vendors would gather these signals into SoC control registers. These
> >>>> +register designs are similar but with different base addresses and bits
> >>>> +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> >>>> +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> >>>> +users bring up their SMP system quickly with the below settings:
> >>>
> >>> +DT maintainers and DT list.
> >>>
> >>> I can submit a dt-binding for this if DT maintainers agree with below
> >>> properties. Could you please help review?
> >>
> >> I thought this was already discussed on the OpenSBI list 2 months ago,
> >> and received pretty negative feedback.
> > Yes, we want to correct all DT grammar & compile problems, and make it
> > legal first and try again. I thought every vendor has their own choice
> > of how to deliver their hardware support. The motivation of this
> > driver is to ease the delivery of T-HEAD CPU cores on different
> > platforms; people only need three things: jtag_init_script & opensbi &
> > linux_Image, then they could boot on their own FPGA prototype
> > platform, and they needn't prepare any software stuff, all the generic
> > binaries could be directly used. The th1520 could be a good example
> > for them. That's why we consistently push this thing.
>
> What’s changed to make people say yes rather than no this time?
>
> I for one will not give positive feedback for self-modifying code in my
> firmware (outside of the necessary self-relocation at startup before
> the PMP is enabled).
I appreciate you pointing out that problem and that has been solved.
So I don't see any more security problems. You agree with
self-relocation but not self-modifying code. Is this a little
conflict? They are all self-modifying text areas before PMP lock.

Do you against alternative mechanisms in opensbi, which is also a
self-modifying code? We found that ticket_lock performance is lower
than qspinlock when opensbi tlb_flush and icahe_flush on the 128 cores
platform. We also want to introduce qspinlock into opensbi. Do you
think it's proper? I also want Anup's opinion before our work, thx.

>
> Jess
>


-- 
Best Regards
 Guo Ren

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Jessica Clarke]

On 11 Aug 2023, at 03:27, Guo Ren <guoren@kernel.org> wrote:
> 
> On Fri, Aug 11, 2023 at 10:05 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>> 
>> On 11 Aug 2023, at 03:01, Guo Ren <guoren@kernel.org> wrote:
>>> 
>>> On Fri, Aug 11, 2023 at 8:42 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>>>> 
>>>> On 10 Aug 2023, at 17:33, Jisheng Zhang <jszhang@kernel.org> wrote:
>>>>> 
>>>>> On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
>>>>>> From: Guo Ren <guoren@linux.alibaba.com>
>>>>>> 
>>>>>> Add detailed information about thead,reset-sample driver, and improve
>>>>>> usage documentation.
>>>>>> 
>>>>>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
>>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>>>>>> ---
>>>>>> docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
>>>>>> 1 file changed, 67 insertions(+), 20 deletions(-)
>>>>>> 
>>>>>> diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
>>>>>> index 8bb9e91f1a9b..fe05fc5bb85a 100644
>>>>>> --- a/docs/platform/thead-c9xx.md
>>>>>> +++ b/docs/platform/thead-c9xx.md
>>>>>> @@ -1,8 +1,8 @@
>>>>>> -T-HEAD C9xx Series Processors
>>>>>> -=============================
>>>>>> +T-HEAD Processors
>>>>>> +=================
>>>>>> 
>>>>>> -The **C9xx** series processors are high-performance RISC-V architecture
>>>>>> -multi-core processors with AI vector acceleration engine.
>>>>>> +T-HEAD provides high-performance RISC-V architecture multi-core
>>>>>> +processors with AI vector acceleration engine.
>>>>>> 
>>>>>> For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
>>>>>> 
>>>>>> @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
>>>>>> Platform Options
>>>>>> ----------------
>>>>>> 
>>>>>> -The *T-HEAD C9xx* does not have any platform-specific compile options
>>>>>> +The *T-HEAD CPU* does not have any platform-specific compile options
>>>>>> because it uses generic platform.
>>>>>> 
>>>>>> ```
>>>>>> CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
>>>>>> ```
>>>>>> 
>>>>>> -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
>>>>>> -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
>>>>>> +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
>>>>>> +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
>>>>>> +custom driver for the SMP system bootup; the single-core system doesn't need
>>>>>> +it.
>>>>>> +
>>>>>> +T-HEAD Fdt Reset Driver Introduction
>>>>>> +------------------------------------
>>>>>> +
>>>>>> +Every T-HEAD CPU provides a reset control signal and reset address signals.
>>>>>> + - Reset address signal determines CPU where to start up.
>>>>>> + - Reset control signal releases CPU from reset state and begins to execute
>>>>>> +   at reset address.
>>>>>> +
>>>>>> +Many vendors would gather these signals into SoC control registers. These
>>>>>> +register designs are similar but with different base addresses and bits
>>>>>> +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
>>>>>> +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
>>>>>> +users bring up their SMP system quickly with the below settings:
>>>>> 
>>>>> +DT maintainers and DT list.
>>>>> 
>>>>> I can submit a dt-binding for this if DT maintainers agree with below
>>>>> properties. Could you please help review?
>>>> 
>>>> I thought this was already discussed on the OpenSBI list 2 months ago,
>>>> and received pretty negative feedback.
>>> Yes, we want to correct all DT grammar & compile problems, and make it
>>> legal first and try again. I thought every vendor has their own choice
>>> of how to deliver their hardware support. The motivation of this
>>> driver is to ease the delivery of T-HEAD CPU cores on different
>>> platforms; people only need three things: jtag_init_script & opensbi &
>>> linux_Image, then they could boot on their own FPGA prototype
>>> platform, and they needn't prepare any software stuff, all the generic
>>> binaries could be directly used. The th1520 could be a good example
>>> for them. That's why we consistently push this thing.
>> 
>> What’s changed to make people say yes rather than no this time?
>> 
>> I for one will not give positive feedback for self-modifying code in my
>> firmware (outside of the necessary self-relocation at startup before
>> the PMP is enabled).
> I appreciate you pointing out that problem and that has been solved.
> So I don't see any more security problems. You agree with
> self-relocation but not self-modifying code. Is this a little
> conflict? They are all self-modifying text areas before PMP lock.

No. Self-relocation modifies data (in the absence of text relocations,
which aren’t supported in OpenSBI), something which happens during
normal execution. The instruction bytes themselves are never changing.
Self-modifying code modifies code, something which should not be done
unless you absolutely have to.

> Do you against alternative mechanisms in opensbi, which is also a
> self-modifying code?

Yes. I hate that Linux does it, but I understand why it is that way.

> We found that ticket_lock performance is lower
> than qspinlock when opensbi tlb_flush and icahe_flush on the 128 cores
> platform. We also want to introduce qspinlock into opensbi. Do you
> think it's proper?

No. If you have operations that are that performance-sensitive in
firmware then they should not be in firmware. The cost of an SBI call
will far outweigh the cost of slightly slower locking.

Jess

> I also want Anup's opinion before our work, thx.
> 
>> 
>> Jess
>> 
> 
> 
> -- 
> Best Regards
> Guo Ren

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Guo Ren]

On Fri, Aug 11, 2023 at 10:46 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 11 Aug 2023, at 03:27, Guo Ren <guoren@kernel.org> wrote:
> >
> > On Fri, Aug 11, 2023 at 10:05 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>
> >> On 11 Aug 2023, at 03:01, Guo Ren <guoren@kernel.org> wrote:
> >>>
> >>> On Fri, Aug 11, 2023 at 8:42 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >>>>
> >>>> On 10 Aug 2023, at 17:33, Jisheng Zhang <jszhang@kernel.org> wrote:
> >>>>>
> >>>>> On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> >>>>>> From: Guo Ren <guoren@linux.alibaba.com>
> >>>>>>
> >>>>>> Add detailed information about thead,reset-sample driver, and improve
> >>>>>> usage documentation.
> >>>>>>
> >>>>>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> >>>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
> >>>>>> ---
> >>>>>> docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
> >>>>>> 1 file changed, 67 insertions(+), 20 deletions(-)
> >>>>>>
> >>>>>> diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> >>>>>> index 8bb9e91f1a9b..fe05fc5bb85a 100644
> >>>>>> --- a/docs/platform/thead-c9xx.md
> >>>>>> +++ b/docs/platform/thead-c9xx.md
> >>>>>> @@ -1,8 +1,8 @@
> >>>>>> -T-HEAD C9xx Series Processors
> >>>>>> -=============================
> >>>>>> +T-HEAD Processors
> >>>>>> +=================
> >>>>>>
> >>>>>> -The **C9xx** series processors are high-performance RISC-V architecture
> >>>>>> -multi-core processors with AI vector acceleration engine.
> >>>>>> +T-HEAD provides high-performance RISC-V architecture multi-core
> >>>>>> +processors with AI vector acceleration engine.
> >>>>>>
> >>>>>> For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
> >>>>>>
> >>>>>> @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
> >>>>>> Platform Options
> >>>>>> ----------------
> >>>>>>
> >>>>>> -The *T-HEAD C9xx* does not have any platform-specific compile options
> >>>>>> +The *T-HEAD CPU* does not have any platform-specific compile options
> >>>>>> because it uses generic platform.
> >>>>>>
> >>>>>> ```
> >>>>>> CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
> >>>>>> ```
> >>>>>>
> >>>>>> -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> >>>>>> -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> >>>>>> +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
> >>>>>> +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
> >>>>>> +custom driver for the SMP system bootup; the single-core system doesn't need
> >>>>>> +it.
> >>>>>> +
> >>>>>> +T-HEAD Fdt Reset Driver Introduction
> >>>>>> +------------------------------------
> >>>>>> +
> >>>>>> +Every T-HEAD CPU provides a reset control signal and reset address signals.
> >>>>>> + - Reset address signal determines CPU where to start up.
> >>>>>> + - Reset control signal releases CPU from reset state and begins to execute
> >>>>>> +   at reset address.
> >>>>>> +
> >>>>>> +Many vendors would gather these signals into SoC control registers. These
> >>>>>> +register designs are similar but with different base addresses and bits
> >>>>>> +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> >>>>>> +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> >>>>>> +users bring up their SMP system quickly with the below settings:
> >>>>>
> >>>>> +DT maintainers and DT list.
> >>>>>
> >>>>> I can submit a dt-binding for this if DT maintainers agree with below
> >>>>> properties. Could you please help review?
> >>>>
> >>>> I thought this was already discussed on the OpenSBI list 2 months ago,
> >>>> and received pretty negative feedback.
> >>> Yes, we want to correct all DT grammar & compile problems, and make it
> >>> legal first and try again. I thought every vendor has their own choice
> >>> of how to deliver their hardware support. The motivation of this
> >>> driver is to ease the delivery of T-HEAD CPU cores on different
> >>> platforms; people only need three things: jtag_init_script & opensbi &
> >>> linux_Image, then they could boot on their own FPGA prototype
> >>> platform, and they needn't prepare any software stuff, all the generic
> >>> binaries could be directly used. The th1520 could be a good example
> >>> for them. That's why we consistently push this thing.
> >>
> >> What’s changed to make people say yes rather than no this time?
> >>
> >> I for one will not give positive feedback for self-modifying code in my
> >> firmware (outside of the necessary self-relocation at startup before
> >> the PMP is enabled).
> > I appreciate you pointing out that problem and that has been solved.
> > So I don't see any more security problems. You agree with
> > self-relocation but not self-modifying code. Is this a little
> > conflict? They are all self-modifying text areas before PMP lock.
>
> No. Self-relocation modifies data (in the absence of text relocations,
> which aren’t supported in OpenSBI), something which happens during
> normal execution. The instruction bytes themselves are never changing.
> Self-modifying code modifies code, something which should not be done
> unless you absolutely have to.
>
> > Do you against alternative mechanisms in opensbi, which is also a
> > self-modifying code?
>
> Yes. I hate that Linux does it, but I understand why it is that way.
>
> > We found that ticket_lock performance is lower
> > than qspinlock when opensbi tlb_flush and icahe_flush on the 128 cores
> > platform. We also want to introduce qspinlock into opensbi. Do you
> > think it's proper?
>
> No. If you have operations that are that performance-sensitive in
> firmware then they should not be in firmware. The cost of an SBI call
> will far outweigh the cost of slightly slower locking.
This is a good opinion for me to reference; thx. My intuition is the
same. We should keep tlb_flush, icache_flush in Linux and decrease
sbi_call for ipi operations.

>
> Jess
>
> > I also want Anup's opinion before our work, thx.
> >
> >>
> >> Jess
> >>
> >
> >
> > --
> > Best Regards
> > Guo Ren
>
>


-- 
Best Regards
 Guo Ren

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

On Fri, Aug 11, 2023 at 12:33:32AM +0800, Jisheng Zhang wrote:
> On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>

> > +T-HEAD Fdt Reset Driver Introduction
> > +------------------------------------
> > +
> > +Every T-HEAD CPU provides a reset control signal and reset address signals.
> > + - Reset address signal determines CPU where to start up.
> > + - Reset control signal releases CPU from reset state and begins to execute
> > +   at reset address.
> > +
> > +Many vendors would gather these signals into SoC control registers. These
> > +register designs are similar but with different base addresses and bits
> > +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> > +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> > +users bring up their SMP system quickly with the below settings:
> 
> +DT maintainers and DT list.
> 
> I can submit a dt-binding for this if DT maintainers agree with below
> properties. Could you please help review?

I already reviewed this once & nothing has improved.
In fact, things have gotten worse IMO with this "using-csr-reset" that I
don't think existed in the original iteration that I saw.
I did see things getting better with the use of standard stuff like
"reg" in our earlier discussion which I don't see here.
What is the point in carrying out any further review if things will be
flat out ignored?

Thanks,
Conor.

> > +
> > + - entry-reg:
> > +	The base address to store reset address value
> > +
> > + - entry-cnt:
> > +	The numbers of entry-reg, all of them set the same reset address
> > +
> > + - control-reg:
> > +	The base address to reset the controller
> > +
> > + - control-val:
> > +	Write which bits of control-reg for booting
> > +
> > + - csr-copy:
> > +	This array determines which csrs to copy from primary hart to the
> > +	secondary harts, which are set in sequence from left to right. The
> > +	secondary harts should keep the same setting as the primary hart.
> > +	These settings are also the first part of the bootup instructions
> > +	for secondary harts.
> > +
> > + - using-csr-reset:
> > +	A legacy reset controller for the SMP system, but abandoned in the
> > +	latest T-HEAD processors.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Guo Ren]

On Sat, Aug 12, 2023 at 1:57 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Aug 11, 2023 at 12:33:32AM +0800, Jisheng Zhang wrote:
> > On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
>
> > > +T-HEAD Fdt Reset Driver Introduction
> > > +------------------------------------
> > > +
> > > +Every T-HEAD CPU provides a reset control signal and reset address signals.
> > > + - Reset address signal determines CPU where to start up.
> > > + - Reset control signal releases CPU from reset state and begins to execute
> > > +   at reset address.
> > > +
> > > +Many vendors would gather these signals into SoC control registers. These
> > > +register designs are similar but with different base addresses and bits
> > > +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> > > +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> > > +users bring up their SMP system quickly with the below settings:
> >
> > +DT maintainers and DT list.
> >
> > I can submit a dt-binding for this if DT maintainers agree with below
> > properties. Could you please help review?
>
> I already reviewed this once & nothing has improved.
> In fact, things have gotten worse IMO with this "using-csr-reset" that I
> don't think existed in the original iteration that I saw.
> I did see things getting better with the use of standard stuff like
> "reg" in our earlier discussion which I don't see here.
It's not a patch to improve the implementation and we don't make a
deal in that discussion. This patch improves the document because the
previous doc didn't describe the whole thing about the
thead,reset-sample driver.

> What is the point in carrying out any further review if things will be
> flat out ignored?

>
> Thanks,
> Conor.
>
> > > +
> > > + - entry-reg:
> > > +   The base address to store reset address value
> > > +
> > > + - entry-cnt:
> > > +   The numbers of entry-reg, all of them set the same reset address
> > > +
> > > + - control-reg:
> > > +   The base address to reset the controller
> > > +
> > > + - control-val:
> > > +   Write which bits of control-reg for booting
> > > +
> > > + - csr-copy:
> > > +   This array determines which csrs to copy from primary hart to the
> > > +   secondary harts, which are set in sequence from left to right. The
> > > +   secondary harts should keep the same setting as the primary hart.
> > > +   These settings are also the first part of the bootup instructions
> > > +   for secondary harts.
> > > +
> > > + - using-csr-reset:
> > > +   A legacy reset controller for the SMP system, but abandoned in the
> > > +   latest T-HEAD processors.
>


-- 
Best Regards
 Guo Ren

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Guo Ren]

On Sat, Aug 12, 2023 at 8:07 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Sat, Aug 12, 2023 at 1:57 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Fri, Aug 11, 2023 at 12:33:32AM +0800, Jisheng Zhang wrote:
> > > On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > > > +T-HEAD Fdt Reset Driver Introduction
> > > > +------------------------------------
> > > > +
> > > > +Every T-HEAD CPU provides a reset control signal and reset address signals.
> > > > + - Reset address signal determines CPU where to start up.
> > > > + - Reset control signal releases CPU from reset state and begins to execute
> > > > +   at reset address.
> > > > +
> > > > +Many vendors would gather these signals into SoC control registers. These
> > > > +register designs are similar but with different base addresses and bits
> > > > +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> > > > +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> > > > +users bring up their SMP system quickly with the below settings:
> > >
> > > +DT maintainers and DT list.
> > >
> > > I can submit a dt-binding for this if DT maintainers agree with below
> > > properties. Could you please help review?
> >
> > I already reviewed this once & nothing has improved.
> > In fact, things have gotten worse IMO with this "using-csr-reset" that I
> > don't think existed in the original iteration that I saw.
> > I did see things getting better with the use of standard stuff like
> > "reg" in our earlier discussion which I don't see here.
> It's not a patch to improve the implementation and we don't make a
> deal in that discussion. This patch improves the document because the
> previous doc didn't describe the whole thing about the
> thead,reset-sample driver.
>
> > What is the point in carrying out any further review if things will be
> > flat out ignored?
I think the critical problem is JC objects to the whole of this
existing driver,  so any improvement is meaningless for the current.
Your naming advice is okay for me, but it does not relate to this
patch.

>
> >
> > Thanks,
> > Conor.
> >
> > > > +
> > > > + - entry-reg:
> > > > +   The base address to store reset address value
> > > > +
> > > > + - entry-cnt:
> > > > +   The numbers of entry-reg, all of them set the same reset address
> > > > +
> > > > + - control-reg:
> > > > +   The base address to reset the controller
> > > > +
> > > > + - control-val:
> > > > +   Write which bits of control-reg for booting
> > > > +
> > > > + - csr-copy:
> > > > +   This array determines which csrs to copy from primary hart to the
> > > > +   secondary harts, which are set in sequence from left to right. The
> > > > +   secondary harts should keep the same setting as the primary hart.
> > > > +   These settings are also the first part of the bootup instructions
> > > > +   for secondary harts.
> > > > +
> > > > + - using-csr-reset:
> > > > +   A legacy reset controller for the SMP system, but abandoned in the
> > > > +   latest T-HEAD processors.
> >
>
>
> --
> Best Regards
>  Guo Ren



-- 
Best Regards
 Guo Ren

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]

On Sat, Aug 12, 2023 at 08:07:50AM +0800, Guo Ren wrote:
> On Sat, Aug 12, 2023 at 1:57 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Fri, Aug 11, 2023 at 12:33:32AM +0800, Jisheng Zhang wrote:
> > > On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > > > +T-HEAD Fdt Reset Driver Introduction
> > > > +------------------------------------
> > > > +
> > > > +Every T-HEAD CPU provides a reset control signal and reset address signals.
> > > > + - Reset address signal determines CPU where to start up.
> > > > + - Reset control signal releases CPU from reset state and begins to execute
> > > > +   at reset address.
> > > > +
> > > > +Many vendors would gather these signals into SoC control registers. These
> > > > +register designs are similar but with different base addresses and bits
> > > > +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> > > > +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> > > > +users bring up their SMP system quickly with the below settings:
> > >
> > > +DT maintainers and DT list.
> > >
> > > I can submit a dt-binding for this if DT maintainers agree with below
> > > properties. Could you please help review?
> >
> > I already reviewed this once & nothing has improved.
> > In fact, things have gotten worse IMO with this "using-csr-reset" that I
> > don't think existed in the original iteration that I saw.
> > I did see things getting better with the use of standard stuff like
> > "reg" in our earlier discussion which I don't see here.

> It's not a patch to improve the implementation and we don't make a
> deal in that discussion. This patch improves the document because the
> previous doc didn't describe the whole thing about the
> thead,reset-sample driver.

That's all well and good, but there's still no point in Rob, Krzysztof
or I reviewing it, which is what was being requested in the email I was
replying to.

> > What is the point in carrying out any further review if things will be
> > flat out ignored?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Guo Ren]

Hi Jisheng,

Could you give a review on this patch? This only corrects the
documentation for the current thead, reset-sample driver of opensbi.

On Fri, Aug 11, 2023 at 12:45 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Add detailed information about thead,reset-sample driver, and improve
> > usage documentation.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> >  docs/platform/thead-c9xx.md | 87 ++++++++++++++++++++++++++++---------
> >  1 file changed, 67 insertions(+), 20 deletions(-)
> >
> > diff --git a/docs/platform/thead-c9xx.md b/docs/platform/thead-c9xx.md
> > index 8bb9e91f1a9b..fe05fc5bb85a 100644
> > --- a/docs/platform/thead-c9xx.md
> > +++ b/docs/platform/thead-c9xx.md
> > @@ -1,8 +1,8 @@
> > -T-HEAD C9xx Series Processors
> > -=============================
> > +T-HEAD Processors
> > +=================
> >
> > -The **C9xx** series processors are high-performance RISC-V architecture
> > -multi-core processors with AI vector acceleration engine.
> > +T-HEAD provides high-performance RISC-V architecture multi-core
> > +processors with AI vector acceleration engine.
> >
> >  For more details, refer [T-HEAD.CN](https://www.t-head.cn/)
> >
> > @@ -12,15 +12,75 @@ To build the platform-specific library and firmware images, provide the
> >  Platform Options
> >  ----------------
> >
> > -The *T-HEAD C9xx* does not have any platform-specific compile options
> > +The *T-HEAD CPU* does not have any platform-specific compile options
> >  because it uses generic platform.
> >
> >  ```
> >  CROSS_COMPILE=riscv64-linux-gnu- PLATFORM=generic /usr/bin/make
> >  ```
> >
> > -The *T-HEAD C9xx* DTB provided to OpenSBI generic firmwares will usually have
> > -"riscv,clint0", "riscv,plic0", "thead,reset-sample" compatible strings.
> > +The *T-HEAD CPU* DTB provided to OpenSBI generic firmwares will usually have
> > +"thead,reset-sample" compatible strings. The "thead,reset-sample" is a T-HEAD
> > +custom driver for the SMP system bootup; the single-core system doesn't need
> > +it.
> > +
> > +T-HEAD Fdt Reset Driver Introduction
> > +------------------------------------
> > +
> > +Every T-HEAD CPU provides a reset control signal and reset address signals.
> > + - Reset address signal determines CPU where to start up.
> > + - Reset control signal releases CPU from reset state and begins to execute
> > +   at reset address.
> > +
> > +Many vendors would gather these signals into SoC control registers. These
> > +register designs are similar but with different base addresses and bits
> > +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> > +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> > +users bring up their SMP system quickly with the below settings:
>
> +DT maintainers and DT list.
>
> I can submit a dt-binding for this if DT maintainers agree with below
> properties. Could you please help review?
>
> > +
> > + - entry-reg:
> > +     The base address to store reset address value
> > +
> > + - entry-cnt:
> > +     The numbers of entry-reg, all of them set the same reset address
> > +
> > + - control-reg:
> > +     The base address to reset the controller
> > +
> > + - control-val:
> > +     Write which bits of control-reg for booting
> > +
> > + - csr-copy:
> > +     This array determines which csrs to copy from primary hart to the
> > +     secondary harts, which are set in sequence from left to right. The
> > +     secondary harts should keep the same setting as the primary hart.
> > +     These settings are also the first part of the bootup instructions
> > +     for secondary harts.
> > +
> > + - using-csr-reset:
> > +     A legacy reset controller for the SMP system, but abandoned in the
> > +     latest T-HEAD processors.
> > +
> > +Here is the dts example:
> > +```
> > +     reset: reset-sample {
> > +             compatible = "thead,reset-sample";
> > +             entry-reg = <0xff 0xff019050>;
> > +             entry-cnt = <4>;
> > +             control-reg = <0xff 0xff015004>;
> > +             control-val = <0x1c>;
> > +             csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> > +     };
> > +```
> > +
> > +Legacy dts example (Abandoned):
> > +```
> > +     reset: reset-sample {
> > +             compatible = "thead,reset-sample";
> > +             using-csr-reset;
> > +             csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>;
> > +     };
> > +```
> >
> >  DTS Example1: (Single core, eg: Allwinner D1 - c906)
> >  ----------------------------------------------------
> > @@ -181,16 +241,3 @@ DTS Example2: (Multi cores with soc reset-regs)
> >               };
> >       }
> >  ```
> > -
> > -DTS Example2: (Multi cores with old reset csrs)
> > ------------------------------------------------
> > -```
> > -     reset: reset-sample {
> > -             compatible = "thead,reset-sample";
> > -             using-csr-reset;
> > -             csr-copy = <0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc
> > -                         0x3b0 0x3b1 0x3b2 0x3b3
> > -                         0x3b4 0x3b5 0x3b6 0x3b7
> > -                         0x3a0>;
> > -     };
> > -```
> > --
> > 2.36.1
> >



-- 
Best Regards
 Guo Ren

Re: [PATCH] docs/platform: thead-c9xx: Improve the documentation

[Guo Ren]

On Sat, Aug 12, 2023 at 8:16 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Sat, Aug 12, 2023 at 08:07:50AM +0800, Guo Ren wrote:
> > On Sat, Aug 12, 2023 at 1:57 AM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Fri, Aug 11, 2023 at 12:33:32AM +0800, Jisheng Zhang wrote:
> > > > On Tue, Aug 08, 2023 at 09:29:58AM -0400, guoren@kernel.org wrote:
> > > > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > > > +T-HEAD Fdt Reset Driver Introduction
> > > > > +------------------------------------
> > > > > +
> > > > > +Every T-HEAD CPU provides a reset control signal and reset address signals.
> > > > > + - Reset address signal determines CPU where to start up.
> > > > > + - Reset control signal releases CPU from reset state and begins to execute
> > > > > +   at reset address.
> > > > > +
> > > > > +Many vendors would gather these signals into SoC control registers. These
> > > > > +register designs are similar but with different base addresses and bits
> > > > > +definitions. We only provide standard opensbi, Linux binaries, and jtag gdbinit
> > > > > +script to simplify Linux booting at the FPGA stage. The fdt reset driver helps
> > > > > +users bring up their SMP system quickly with the below settings:
> > > >
> > > > +DT maintainers and DT list.
> > > >
> > > > I can submit a dt-binding for this if DT maintainers agree with below
> > > > properties. Could you please help review?
> > >
> > > I already reviewed this once & nothing has improved.
> > > In fact, things have gotten worse IMO with this "using-csr-reset" that I
> > > don't think existed in the original iteration that I saw.
> > > I did see things getting better with the use of standard stuff like
> > > "reg" in our earlier discussion which I don't see here.
>
> > It's not a patch to improve the implementation and we don't make a
> > deal in that discussion. This patch improves the document because the
> > previous doc didn't describe the whole thing about the
> > thead,reset-sample driver.
>
> That's all well and good, but there's still no point in Rob, Krzysztof
> or I reviewing it, which is what was being requested in the email I was
> replying to.
We aim not to deliver any custom software binaries to our CPU
customers. Only generic opensbi & Linux images are all enough at the
prototype stage.

>
> > > What is the point in carrying out any further review if things will be
> > > flat out ignored?



-- 
Best Regards
 Guo Ren