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

[Jessica Clarke]

On 14 Jun 2023, at 02:07, Guo Ren <guoren at kernel.org> wrote:
> 
> On Tue, Jun 13, 2023 at 10:43 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>> 
>> 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?
> We don't expect WX mapping, but the current fdt_reset_init is after
> the pmp config. I need to move it into early_init. Thx for pointing it
> out.

So you didn’t test your code?

Besides, my point was not about correctness, but about the security
concerns that come with generating code at run time. If you don’t need
to do it, don’t.

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
>> 
>> 
> 
> 
> --
> Best Regards
> Guo Ren