Re: [PATCH 08/11] MIPS: mobileye: Add EyeQ5 dtsi

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

Hello Rob,

> On Wed, Oct 4, 2023 at 11:11 AM Gregory CLEMENT
> <gregory.clement@bootlin.com> wrote:
>>
>> Add a device tree include file for the Mobileye EyeQ5 SoC.
>>
>> Based on the work of Slava Samsonov <stanislav.samsonov@intel.com>
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> ---
>>  arch/mips/boot/dts/Makefile                   |   1 +
>>  arch/mips/boot/dts/mobileye/Makefile          |   4 +
>>  .../boot/dts/mobileye/eyeq5-fixed-clocks.dtsi | 315 ++++++++++++++++++
>>  arch/mips/boot/dts/mobileye/eyeq5.dtsi        | 138 ++++++++
>>  4 files changed, 458 insertions(+)
>>  create mode 100644 arch/mips/boot/dts/mobileye/Makefile
>>  create mode 100644 arch/mips/boot/dts/mobileye/eyeq5-fixed-clocks.dtsi
>>  create mode 100644 arch/mips/boot/dts/mobileye/eyeq5.dtsi
>>
>> diff --git a/arch/mips/boot/dts/Makefile b/arch/mips/boot/dts/Makefile
>> index 928f38a79dff..edb8e8dee758 100644
>> --- a/arch/mips/boot/dts/Makefile
>> +++ b/arch/mips/boot/dts/Makefile
>> @@ -8,6 +8,7 @@ subdir-$(CONFIG_LANTIQ)                 += lantiq
>>  subdir-$(CONFIG_MACH_LOONGSON64)       += loongson
>>  subdir-$(CONFIG_SOC_VCOREIII)          += mscc
>>  subdir-$(CONFIG_MIPS_MALTA)            += mti
>> +subdir-$(CONFIG_SOC_EYEQ5)             += mobileye
>>  subdir-$(CONFIG_LEGACY_BOARD_SEAD3)    += mti
>>  subdir-$(CONFIG_FIT_IMAGE_FDT_NI169445)        += ni
>>  subdir-$(CONFIG_MACH_PIC32)            += pic32
>> diff --git a/arch/mips/boot/dts/mobileye/Makefile b/arch/mips/boot/dts/mobileye/Makefile
>> new file mode 100644
>> index 000000000000..99c4124fd4c0
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/mobileye/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +# Copyright 2023 Mobileye Vision Technologies Ltd.
>> +
>> +obj-$(CONFIG_BUILTIN_DTB)      += $(addsuffix .o, $(dtb-y))
>
> You didn't add anything to 'dtb-y'. Did you test this?

Initially yes, and finally we switch on the FIT image generation, so we
don't use it anymore

>
> Also, CONFIG_BUILTIN_DTB is supposed to be for legacy bootloaders
> which don't understand DT. For a new SoC, fix the bootloader.

I can remove it

>
>> diff --git a/arch/mips/boot/dts/mobileye/eyeq5-fixed-clocks.dtsi b/arch/mips/boot/dts/mobileye/eyeq5-fixed-clocks.dtsi
>> new file mode 100644
>> index 000000000000..a0066465ac8b
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/mobileye/eyeq5-fixed-clocks.dtsi
>> @@ -0,0 +1,315 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +/*
>> + * Copyright 2023 Mobileye Vision Technologies Ltd.
>> + */
>
> I assume these aren't all really fixed, but just 'I don't have a clock
> driver yet'. That creates an ABI issue when you add the clock
> driver(s). Just FYI.

Indeed they aren't all fixed. The plan is to replace the relevant ones by a
real clock driver when ready.

In this case some part of the dts file will be modified. But is it a
real issue ?

Booting with a new kernel with an old dtb will still continue to work in
the same way. it's only new tdb with old kernel that won't work, but we
are not supposed to support this case.


>
>> +
>> +/ {
>> +       /* Fixed clock */
>> +       pll_cpu: pll_cpu {
>
> Don't use _ in node names.

OK
[...]

>> +/* PLL_CPU derivatives */
>> +       occ_cpu: occ_cpu {
>> +               compatible = "fixed-factor-clock";
>> +               clocks = <&pll_cpu>;
>> +               #clock-cells = <0>;
>> +               clock-div = <1>;
>> +               clock-mult = <1>;
>> +               clock-output-names = "occ_cpu";
>
> Isn't the default name the node name? Drop these unless you really
> have a need and they aren't redundant.

indeed it's not used, I remove them too.
[...]

>> --- /dev/null
>> +++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
>> @@ -0,0 +1,138 @@
>> +// SPDX-License-Identifier: GPL-2.0
>
> Doesn't match eyeq5-fixed-clocks.dtsi

OK

>
>> +/*
>> + * Copyright 2023 Mobileye Vision Technologies Ltd.
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/mips-gic.h>
>> +#include <dt-bindings/soc/mobileye,eyeq5.h>
>> +
>> +/memreserve/ 0x40000000 0xc0000000; /* DDR32 */
>> +/memreserve/ 0x08000000 0x08000000; /* DDR_LOW */
>> +
>> +#include "eyeq5-fixed-clocks.dtsi"
>> +
>> +/* almost all GIC IRQs has the same characteristics. provide short form */
>
> Maybe so, but I prefer not having 2 levels of lookup to figure out values.
>
>> +#define GIC_IRQ(x) GIC_SHARED (x) IRQ_TYPE_LEVEL_HIGH

OK I remove it.

>> +
>> +/ {
>> +       #address-cells = <2>;
>> +       #size-cells = <2>;
>> +       cpus {
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               cpu@0 {
>> +                       device_type = "cpu";
>> +                       compatible = "mti,i6500";
>> +                       reg = <0>;
>> +                       clocks = <&core0_clk>;
>> +               };
>> +       };
>> +
>> +       reserved-memory {
>> +               #address-cells = <2>;
>> +               #size-cells = <2>;
>> +               ranges;
>> +
>> +/* These reserved memory regions are also defined in bootmanager
>> + * for configuring inbound translation for BARS, don't change
>> + * these without syncing with bootmanager
>> + */
>
> Indent with the rest of the node.

OK

Thanks,

Gregory

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com