Message-ID: <fd4cb74c-3a8e-c9c0-f9ac-312fb916275c@gmail.com>
Date:   Thu, 16 Mar 2023 10:40:10 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Álvaro Fernández Rojas <noltari@...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()

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

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?

> [    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.

> 
>>
>>>
>>> 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

Powered by blists - more mailing lists