[RFC PATCH 2/2] lib: reset: thead: Correct the naming convention of dts

[Jessica Clarke]

On 13 Jun 2023, at 03:21, Guo Ren <guoren at kernel.org> wrote:
> 
> On Mon, Jun 12, 2023 at 9:05 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>> 
>> On 12 Jun 2023, at 01:57, Guo Ren <guoren at kernel.org> wrote:
>>> 
>>> Hi Conor,
>>> 
>>> Jisheng Zhang would update the Linux yaml patch, here is the final dts
>>> format of the reset controller:
>>> 
>>>              reset-controller at ffff019050 {
>>>                      compatible = "thead,cpu-reset";
>>>                      reg = <0xff 0xff019050 0x0 0x4>, <0xff
>>> 0xff015004 0x0 0x0>;
>>>                      reset-ctrl = <0x1c>;
>>>                      clone-csrs = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3
>> 
>> This is pretty horrible, there’s no indirect CSR read/write
>> instruction, so what are you expecting drivers to do with this? Have a
> We've done it in the opensbi:
> https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/reset/fdt_reset_thead.c
> 
> A lot of SoC customers would intergrate their own CSRs though our rtl
> code interface, these CSRs are initilized in their primary hart in
> early boot stage / jtag gdbinit script, the secondary harts just copy
> the CSRs' values from boot hart. That means we could provide a unified
> way to all customer.
> 
>> big switch statement for every possible CSR?
> We needn't big switch statement, boot hart store the csrs setting
> codes in the memory, and secondary harts excute them to set the csrs.
> Here is the code snippet:
> 
> struct custom_csr custom_csr[MAX_CUSTOM_CSR];
> 
> #define CSR_OPCODE 0x39073
> static void clone_csrs(int cnt)
> {
> unsigned long i;
> 
> for (i = 0; i < cnt; i++) {
> /* Write csr BIT[31 - 20] to stub */
> __reset_thead_csr_stub[3*i + 1] = CSR_OPCODE | (custom_csr[i].index << 20);
> 
> /* Mask csr BIT[31 - 20] */
> *(u32 *)&__fdt_reset_thead_csrr &= BIT(20) - 1;
> smp_mb();
> 
> /* Write csr BIT[31 - 20] to __fdt_reset_thead_csrr */
> *(u32 *)&__fdt_reset_thead_csrr |= custom_csr[i].index << 20;
> smp_mb();
> 
> RISCV_FENCE_I;
> 
> custom_csr[i].value = __fdt_reset_thead_csrr();
> }

So you want your highly-privileged firmware to be dynamically
generating code, with the ability to have a WX mapping in the
first place?

Jess

> }
> ==========
> 
> /* Prepare clone csrs */
> val_w = fdt_getprop(fdt, nodeoff, "csr-copy", &len);
> if (len > 0 && val_w) {
> int cnt;
> 
> cnt = len / sizeof(fdt32_t);
> if (cnt > MAX_CUSTOM_CSR)
> sbi_hart_hang();
> 
> for (i = 0; i < cnt; i++) {
> custom_csr[i].index = fdt32_to_cpu(val_w[i]);
> }
> 
> if (cnt)
> clone_csrs(cnt); // prepare the secondary harts' excution code
> }
> 
> 
> 
> 
> 
> 
>> 
>> Jess
>> 
>>> 0x7c5 0x7cc 0x7ce>;
>>>               };
>>> 
>>> The reset-ctrl is used to control different parts of soc, generally, a
>>> bit indicates a reset signal (a core/a interconnect/a subsystem).
>>> 
>>> On Thu, May 25, 2023 at 2:06 PM Guo Ren <guoren at kernel.org> wrote:
>>>> 
>>>> On Thu, May 25, 2023 at 1:33 PM Conor Dooley <conor at kernel.org> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 25 May 2023 04:15:36 IST, Guo Ren <guoren at kernel.org> wrote:
>>>>> 
>>>>>> So it could be "thead,cpu-reset", okay?
>>>>> 
>>>>> As a generic fallback compatible.
>>>>> 
>>>>>> Actually, our core could let SoC vendors define their own custom
>>>>>> CSRs/custom reset values of CSRs, so we don't know what would be added
>>>>>> in the future. Put a array in dts instead of hard-code table is much
>>>>>> more flexiblity.
>>>>> 
>>>>> If there's going to be 700 different variations depending on what people do with openc910, then allowing it to be passed sounds like a good idea.
>>>> Yes, that is what we want. th1520 is a example.
>>>> 
>>>> Thanks for the review and correction! Your help is greatly appreciated
>>>> in improving th1520 upstream.
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Conor.
>>>> 
>>>> 
>>>> 
>>>> --
>>>> Best Regards
>>>> Guo Ren
>>> 
>>> 
>>> 
>>> --
>>> Best Regards
>>> Guo Ren
>> 
> 
> 
> -- 
> Best Regards
> Guo Ren