Re: [PATCH v2 2/5] dt-bindings: mips: mips-cm: Add a new compatible string for EyeQ6

[Gregory CLEMENT <gregory.clement@bootlin.com>]

Hello Rob,

> On Thu, Jan 16, 2025 at 11:59:20AM +0100, Gregory CLEMENT wrote:
>> The CM3.5 used on EyeQ6 reports that Hardware Cache Initialization is
>> complete, but in reality it's not the case. It also incorrectly
>> indicates that Hardware Cache Initialization is supported. This new
>> compatible string allows warning about this broken feature that cannot
>> be detected at runtime.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> ---
>>  .../devicetree/bindings/mips/mti,mips-cm.yaml      | 24 ++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml b/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
>> index 9f500804737d23e19f50a9326168686c05d3a54e..4713673f0cfc7785bb183917ee382a815ebfe9e1 100644
>> --- a/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
>> +++ b/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
>> @@ -14,7 +14,12 @@ maintainers:
>>  
>>  properties:
>>    compatible:
>> -    const: mti,mips-cm
>> +    oneOf:
>> +      - const: mti,mips-cm
>> +      - const: mti,eyeq6-cm
>
> Being a mobileye device, the vendor prefix should be mobileye.

I chose mti because actually this block is part of the I6500 and
provided as is by MIPS.

>
>> +        description:
>> +          On EyeQ6 the HCI (Hardware Cache Initialization) information for
>> +          the L2 cache in multi-cluster configuration is broken.
>>  
>>    reg:
>>      description:
>> @@ -25,14 +30,29 @@ properties:
>>  
>>  required:
>>    - compatible
>> -  - reg
>>  
>>  additionalProperties: false
>>  
>> +if:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: mti,eyeq6-cm
>> +then:
>> +  properties:
>> +    reg: false
>> +else:
>> +  required:
>> +    - reg
>
> How does one access this block with no registers? Is this some subset of 
> a larger block? If so, need to define that block first.

CM stands for Coherence Manager. This component is mandatory when you
want to do SMP across MIPS core. This is part of the MIPS architecture,
and the address of the CM is provided by the Coprocessor 0.

"CP0 is incorporated on the CPU chip and supports the virtual memory
system and exception handling. CP0 is also referred to as the System
Control Coprocessor."

So to summarize, in a functional system, this information doesn't have
to be exposed through the device tree, as it is available at runtime
from any MIPS CPU.

>
> These 2 blocks don't look related and the only property shared is 
> 'compatible'. This should be a separate doc.

As mentioned in the cover letter, I reused the work from Jiaxun, who
needed to deal with bogus CM but in a different way. In his use case,
the issue with the CM was that the address in CP0 was wrong. In my case,
this address is correct; it is only one piece of information reported by
the CM that is wrong. I don't mind creating a separate doc if you
still think it is the right thing to do.

Gregory

>
> Rob

-- 
Grégory CLEMENT, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com