Message-ID: <CAMhs-H_M_weq5gKG5cNTeNT5NakovrXUm-mn3gjN8kR=aiQwSw@mail.gmail.com>
Date:   Fri, 1 Oct 2021 07:04:12 +0200
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Strontium <strntydog@...il.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Ilya Lipnitskiy <ilya.lipnitskiy@...il.com>,
        Greg Ungerer <gerg@...nel.org>,
        "open list:MIPS" <linux-mips@...r.kernel.org>,
        Wei Li <liwei391@...wei.com>,
        "Maciej W. Rozycki" <macro@...am.me.uk>,
        Felix Fietkau <nbd@....name>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>
Subject: Re: [PATCH v3] MIPS: add support for buggy MT7621S core detection

Hi Steven,

On Fri, Oct 1, 2021 at 4:02 AM Strontium <strntydog@...il.com> wrote:
>
> On 30/9/21 23:41, Ilya Lipnitskiy wrote:
> > Hi Sergio, Greg, Steven,
> >
> > On Thu, Sep 30, 2021 at 6:35 AM Sergio Paracuellos
> > <sergio.paracuellos@...il.com> wrote:
> >> Hi Greg,
> >>
> >> On Thu, Sep 30, 2021 at 3:13 PM Greg Ungerer <gerg@...nel.org> wrote:
> >>> Hi Steven,
> >>>
> >>> On 16/9/21 6:54 pm, Strontium wrote:
> >>>> Hi Greg,
> >>>>
> >>>> I had trouble with this as well.  This line from the patch:
> >>>>
> >>>>> if (!(launch->flags & LAUNCH_FREADY))
> >>>> is checking ram which I believe is supposed to be set by the bootloader.
> >>>> On my platform it looked like the preloaded uboot wasn't setting that as
> >>>> expected.
> >>>> If you have control over your bootloader you can force this ram address
> >>>> to be what the kernel wants, or you can do what i did, because i didn't
> >>>> have that targets uboot src, and wedge before the kernel starts to force
> >>>> the ram to the required state, like so:
> >>> Well, my solution was to revert that patch locally :-)
> >>>
> >>> But many people will not have control of or the desire to change
> >>> their u-boot loader. I would have figured this ends up being a
> >>> real regression for many (most?) users of this SoC.
> >> Agree.
> >>
> >>>
> >>>> #define CORE0_INITIAL_CPU_STATE (0xf00)
> >>>> #define CORE_FL_OFFSET (0x1C)
> >>>> #define FLAG_LAUNCH_FREADY (1)
> >>>>
> >>>> #define WRITEREG(r, v) *(volatile uint32_t *)(r) = v
> >>>> #define KSEG1ADDR(_x) (((_x)&0x1fffffff) | 0xa0000000)
> >>>>
> >>>> void set_core(uint32_t core)
> >>>> {
> >>>>      uint32_t start = CORE0_INITIAL_CPU_STATE + (0x40 * core);
> >>>>      WRITEREG(KSEG1ADDR(start + CORE_FL_OFFSET), FLAG_LAUNCH_FREADY);
> >>>> }
> >>>>
> >>>> void fix_cores(void) {
> >>>>      // Fixes the flags for each core, just before running the kernel.
> >>>>      // Means we don't have to patch the kernel check for valid CPU's.
> >>>>      for (int i = 0; i < 4; i++) {
> >>>>          set_core(i);
> >>>>      }
> >>>> }
> >>>>
> >>>> It seems that memory section is supposed to set all the cores registers
> >>>> before the kernel runs, but i never found it did anything except that 1
> >>>> flag.
> >>>>
> >>>> Obviously a better way would be to properly detect the number of cores
> >>>> and not rely on the boot loader to set a flag in ram, I don't know if
> >>>> that's even possible.
> >>> I can't help but think this commit is not a proper fix for this problem.
> >> I also do think this commit should be reverted. Ilya, do you have a
> >> strong opinion to maintain it instead?
> > Not a strong opinion - I think we need a better fix that would work on
> > the platform I tested with as well as Greg's. I'm okay with reverting
> > it while trying to come up with said fix. Downstream projects, such as
> > OpenWrt can keep this patch or apply it only when building for MT7621S
> > targets until the detection logic is made more robust.
> >
> > Greg - if and when a proper fix is made, a test against your platform
> > would help so we don't regress again in the future.
> >
> > Ilya
>
> Hi Ilya, Sergio and Greg,
>
> Could we, instead of checking data passed from the bootloader check
> something set in the device tree?
>
> For example currently `linux/drivers/staging/mt7621-dts/mt7621.dtsi`
> defines:
>     cpus {
>         cpu@0 {
>             compatible = "mips,mips1004Kc";
>         };
>
>         cpu@1 {
>             compatible = "mips,mips1004Kc";
>         };
>     };
>
>
> But that's not true for an mt7621s.  For this device, it should be defined:
>
>     cpus {
>         cpu@0 {
>             compatible = "mips,mips1004Kc";
>         };
>     };
>
>
> And if it was, the code that detects the cpu cores could check this and
> enable either the number of cores it probes, or the number of cpu's
> defined by the device tree, whichever is the lesser.
>
> Then Downstream just needs to properly set up the cpu in the device tree
> for the effected targets and it should work.
>
> If something like this is acceptable, I would be happy to propose a
> patch along these lines for testing.

I guess this would become a new mt7621s.dts that only include the
other original mt7621.dtsi and just overlay cpus. But I think we can
check the register related with chip name and so on [0] and see if
something is different in order to set 'soc' related code and
attributes to checkable values. Check [1] and [2]. Ilya, maybe you can
check whaever value is in there to see if the "S" stuff is different
with other normal mt7621 chips?

Best regards,
     Sergio Paracuellos

[0]: https://elixir.bootlin.com/linux/latest/source/arch/mips/include/asm/mach-ralink/mt7621.h#L15
[1]: https://elixir.bootlin.com/linux/latest/source/arch/mips/ralink/mt7621.c#L86
[2]: https://elixir.bootlin.com/linux/latest/source/arch/mips/ralink/mt7621.c#L59

>
> Steven

Powered by blists - more mailing lists