Message-ID: <CAKR-sGdc3zLyNdzfZ383nu05q=S5We9Q257w4dm7wWLHQ+XtpQ@mail.gmail.com>
Date:   Thu, 16 Mar 2023 19:03:43 +0100
From:   Álvaro Fernández Rojas <noltari@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     William Zhang <william.zhang@...adcom.com>,
        Jonas Gorski <jonas.gorski@...il.com>,
        bcm-kernel-feedback-list@...adcom.com, tsbogend@...ha.franken.de,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mips: bmips: BCM6358: disable arch_sync_dma_for_cpu_all()

El jue, 16 mar 2023 a las 18:40, Florian Fainelli
(<f.fainelli@...il.com>) escribió:
>
> On 3/14/23 11:14, Álvaro Fernández Rojas wrote:
> > Hi Florian,
> >
> > El lun, 13 mar 2023 a las 22:46, Florian Fainelli
> > (<f.fainelli@...il.com>) escribió:
> >>
> >> (please don't top post)
> >>
> >> On 3/13/23 14:39, Álvaro Fernández Rojas wrote:
> >>> Hi Florian,
> >>>
> >>> I did another test changing from TP1 to TP0 and this is the result:
> >>> [ 0.000000] Linux version 5.15.98 (noltari@...antis)
> >>> (mips-openwrt-linux-musl-gcc (OpenWrt GCC 12.2.0 r22187+1-19817fa3f5)
> >>> 12.2.0, GNU ld (GNU Binutils) 2.40.0) #0 SMP Sun Mar 12 18:23:28 2023
> >>> [ 0.000000] bmips_cpu_setup: read_c0_brcm_config_0() = 0xe30e1006
> >>> [ 0.000000] bmips_cpu_setup: BMIPS_RAC_CONFIG = 0x2a00015
> >>> [ 0.000000] CPU0 revision is: 0002a010 (Broadcom BMIPS4350)
> >>>
> >>> And there were no exceptions with EHCI/OHCI as opposed to TP1.
> >>> So the issue is only happening when booting from TP1.
> >>
> >> Ah, that explains it then, I was just about to ask you which TP was the
> >> kernel booted on.
> >>
> >>> Maybe it's due to the fact that BCM6358 has a shared TLB?
> >>
> >> I think it has to do with the fact that the BMIPS_RAC_CONFIG_1 is likely
> >> not enabling the RAC since that register pertains to TP1, could you dump
> >> its contents, and if they do not set bit 0 and/or 1, please set them and
> >> try again and see whether it works any better? The RAC provides
> >> substantial performance improvements, it would be a change to keep it
> >> disabled.
> >
> > This is the code that I added to bmips_cpu_setup():
> >      case CPU_BMIPS4350:
> >          cfg = read_c0_brcm_cmt_local();
> >          pr_info("bmips_cpu_setup: read_c0_brcm_cmt_local() = 0x%x\n", cfg);
> >
> >          cfg = read_c0_brcm_config_0();
> >          pr_info("bmips_cpu_setup: read_c0_brcm_config_0() = 0x%x\n", cfg);
> >
> >          cfg = __raw_readl(cbr + BMIPS_RAC_ADDRESS_RANGE);
> >          pr_info("bmips_cpu_setup: BMIPS_RAC_ADDRESS_RANGE = 0x%x\n", cfg);
> >
> >          cfg = __raw_readl(cbr + BMIPS_L2_CONFIG);
> >          pr_info("bmips_cpu_setup: BMIPS_L2_CONFIG = 0x%x\n", cfg);
> >
> >          cfg = __raw_readl(cbr + BMIPS_LMB_CONTROL);
> >          pr_info("bmips_cpu_setup: BMIPS_LMB_CONTROL = 0x%x\n", cfg);
> >
> >          cfg = __raw_readl(cbr + BMIPS_RAC_CONFIG);
> >          pr_info("bmips_cpu_setup: BMIPS_RAC_CONFIG = 0x%x\n", cfg);
> >          __raw_writel(cfg | BIT(0) | BIT(1), cbr + BMIPS_RAC_CONFIG);
> >          pr_info("bmips_cpu_setup: BMIPS_RAC_CONFIG = 0x%x\n", cfg);
> >          __raw_readl(cbr + BMIPS_RAC_CONFIG);
> >
> >          cfg = __raw_readl(cbr + BMIPS_RAC_CONFIG_1);
> >          pr_info("bmips_cpu_setup: BMIPS_RAC_CONFIG_1 = 0x%x\n", cfg);
> >          __raw_writel(cfg | BIT(0) | BIT(1), cbr + BMIPS_RAC_CONFIG_1);
> >          pr_info("bmips_cpu_setup: BMIPS_RAC_CONFIG_1 = 0x%x\n", cfg);
> >          __raw_readl(cbr + BMIPS_RAC_CONFIG_1);
> >          break;
> >
> > And this is the result:
> > [    0.000000] bmips_cpu_setup: read_c0_brcm_cmt_local() = 0x80000000
>
> OK, so we are executing from TP1, but we knew that already.
>
> > [    0.000000] bmips_cpu_setup: read_c0_brcm_config_0() = 0xe30e1006
>
> bit 31: instruction cache enabled
> bit 30: data cache enabled
> bit 29: RAC present
> bit 25: DSU power enabled
> bit 24: data cache power enabled
> bit 19: low-latency memory bus (LMB) present
> bit 18: concurrent multi threading (CMT) present
> bit 17: reserved
> bit 12: split instruction cache
> bit 2: number of Hi/Lo special registers - 1
> bit 1: eDSP present

I'm curious, is there any bit that indicates a split data cache?

>
> This seems to match the recommended and default values
>
> > [    0.000000] bmips_cpu_setup: BMIPS_RAC_ADDRESS_RANGE = 0x277bdab0
>
> That does not look intended, the reset value is supposed to be 0. Can
> you apply the same values as the ones programmed in the CPU_BMIPS_3300 case?

I've just tried this but it still panics...

>
> > [    0.000000] bmips_cpu_setup: BMIPS_L2_CONFIG = 0x241a0008
>
>
> > [    0.000000] bmips_cpu_setup: BMIPS_LMB_CONTROL = 0x0
>
> LMB not enabled, that's OK.
>
> > [    0.000000] bmips_cpu_setup: BMIPS_RAC_CONFIG = 0x3c1b8041
> > [    0.000000] bmips_cpu_setup: BMIPS_RAC_CONFIG = 0x3c1b8041
> > [    0.000000] bmips_cpu_setup: BMIPS_RAC_CONFIG_1 = 0x3600008
> > [    0.000000] bmips_cpu_setup: BMIPS_RAC_CONFIG_1 = 0x3600008
> >
> > As you can see the bit's aren't set and all the registers appear to
> > have strange values and not the usual ones when initialized by the
> > bootloader...
>
> Yes, so maybe the best way to go about is indeed to go with your change
> such that if we are running on TP1, it is safe to assume that CFE has
> not done any sensible initialization, and we have a non functional RAC.

Yes, I will send v2 checking if we're running on TP1.

>
> >
> >>
> >>>
> >>> Maybe the correct way of solving the issue would be by adding the
> >>> following code at bcm6358_quirks():
> >>> if (read_c0_brcm_cmt_local() & (1 << 31))
> >>>       bmips_dma_sync_disabled = 1;
> >>>
> >>> BTW, if I understood it correctly, you want me to reverse the logic,
> >>> so bmips_dma_sync_disabled instead of bmips_dma_sync_enabled.
> >>> Is this correct?
> >>
> >> Yes, I want the logic such that we need to set a variable to 1/true
> >> rather setting one to 0, less change to get it wrong IMHO.
> >>
> >>>
> >>> Best regards,
> >>> Álvaro.
> >>>
> >>>
> >>> El lun, 13 mar 2023 a las 18:37, Florian Fainelli
> >>> (<f.fainelli@...il.com>) escribió:
> >>>>
> >>>> On 3/12/23 11:50, Álvaro Fernández Rojas wrote:
> >>>>> Hi Florian,
> >>>>>
> >>>>> I tried what you suggested but it stil panics on EHCI:
> >>>>>
> >>>>> [    0.000000] Linux version 5.15.98 (noltari@...antis)
> >>>>> (mips-openwrt-linux-musl-gcc (OpenWrt GCC 12.2.0 r22187+1-19817fa3f5)
> >>>>> 12.2.0, GNU ld (GNU Binutils) 2.40.0) #0 SMP Sun Mar 12 18:23:28 2023
> >>>>> [    0.000000] bmips_cpu_setup: read_c0_brcm_config_0() = 0xe30e1006
> >>>>> [    0.000000] bmips_cpu_setup: cbr + BMIPS_RAC_CONFIG = 0x3c1b8041
> >>>>> [    0.000000] CPU0 revision is: 0002a010 (Broadcom BMIPS4350)
> >>>>>
> >>>>> It looks like bit 29 is set so RAC should be present.
> >>>>> And RAC_I seems to be set, but not RAC_D...
> >>>>>
> >>>>> BTW, this is what I added to bmips_cpu_setup:
> >>>>>
> >>>>> case CPU_BMIPS4350:
> >>>>> cfg = read_c0_brcm_config_0();
> >>>>> pr_info("bmips_cpu_setup: read_c0_brcm_config_0() = 0x%x\n", cfg);
> >>>>>
> >>>>> cfg = __raw_readl(cbr + BMIPS_RAC_CONFIG);
> >>>>> pr_info("bmips_cpu_setup: cbr + BMIPS_RAC_CONFIG = 0x%x\n", cfg);
> >>>>> __raw_writel(cfg | BIT(0) | BIT(1), cbr + BMIPS_RAC_CONFIG);
> >>>>> __raw_readl(cbr + BMIPS_RAC_CONFIG);
> >>>>> break;
> >>>>
> >>>> Thanks for running those experiments, I cannot explain what you are
> >>>> seeing, so there must be some sort of erratum applicable to the
> >>>> BMIPS4380 revision used on the 6358 somehow...
> >>>>
> >>>> If you can make the suggested change to use negative logic in order to
> >>>> disable the RAC flushing, that would work for me, also maybe add a
> >>>> Fixes: tag so it gets backported to stable trees?
> >>>>
> >>>> Thanks!
> >>>> --
> >>>> Florian
> >>>>
> >>
> >> --
> >> Florian
> >>
> >
> > Álvaro
>
> --
> Florian
>

Álvaro

Powered by blists - more mailing lists