Date:   Wed, 25 Jan 2023 21:40:19 +0800
Message-ID: <87sffyhgvw.wl-me@linux.beauty>
From:   Li Chen <me@linux.beauty>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Li Chen <lchen@ambarella.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        "moderated list:ARM/Ambarella SoC support" 
        <linux-arm-kernel@lists.infradead.org>,
        "open list:COMMON CLK FRAMEWORK" <linux-clk@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings
In-Reply-To: <ec9fc589-2612-3315-3550-83b68bead926@linaro.org>
References: <20230123073305.149940-1-lchen@ambarella.com>
        <20230123073305.149940-8-lchen@ambarella.com>
        <0c19efb4-3bca-f500-ca24-14b9d24369ef@linaro.org>
        <87y1prgdyu.wl-me@linux.beauty>
        <b26a52ff-6b8a-8a64-7189-346cd2b0d705@linaro.org>
        <87tu0ehl88.wl-me@linux.beauty>
        <ec9fc589-2612-3315-3550-83b68bead926@linaro.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0
 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 25 Jan 2023 20:14:16 +0800,

Hi Krzysztof,

Krzysztof Kozlowski wrote:
>
> On 25/01/2023 13:06, Li Chen wrote:
> >>> Feel free to correct me if you think this
> >>> is not a good idea.
> >>
> >> This is bad idea. Compatibles should be specific. Devices should not use
> >> syscons to poke other registers, unless strictly necessary, but have
> >> strictly defined MMIO address space and use it.
> >
> > Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data.
> >
> > But I have three questions:
> >
> > 0. why syscon + offsets is a bad idea copared to specific compatibles?
>
> Specific compatibles are a requirement. They are needed to match device
> in exact way, not some generic and unspecific. The same with every other
> interface, it must be specific to allow only correct usage.
>
> It's of course different with generic fallbacks, but we do not talk
> about them here...
>
> > 1. when would it be a good idea to use syscon in device tree?
>
> When your device needs to poke one or few registers from some
> system-controller block.
>
> > 2. syscon VS reg, which is preferred in device tree?
>
> There is no such choice. Your DTS *must* describe the hardware. The
> hardware description is for example clock controller which has its own
> address space. If you now do not add clock controller's address space to
> the clock controller, it is not a proper hardware description. The same
> with every other property. If your device has interrupts, but you do not
> add them, it is not correct description.

Got it. But Ambarella hardware design is kind of strange. I want to add mroe
expalaination about why Ambarella's downstream kernel
use so much syscon in device trees:

For most SoCs from other vendors, they have seperate address space regions
for different peripherals, like
axi address space A: ENET
axi address space B: PCIe
axi address space B: USB
...

Ambarella is somewhat **different**, its SoCs have two system controllers regions:
RCT and scratchpad, take RCT for example:
"The S6LM system software
interacts with PLLs, PHYs and several other low-level hardware blocks using APB reset clock and test (RCT)
registers with a system-layer application programming interface (API).
This includes the setting of clock frequencies."

There are so many peripherals registers located inside RCT and scratchpad
(like usb/phy, gpio, sd, dac, enet, rng), and some peripherals even have no their
own modules for register definitions.

So most time(for a peripheral driver), the only differences between different
Ambarella SoCs are just the syscon(rct or scratchpad) offsets get changed.

I don't think such lazy hardware design is common in vendors other than ambarella.

If I switch to SoC-specific compatibles, and remove these syscon from device tree,
of_device_id->data may only contain system controller(rct or scratchpad) offset for many Ambarella drivers,
and ioremap/devm_ioremap carefully.

The question is: can upstream kernel accept such codes?

If yes, I will switch to SoC-specific compatibles and remove syscon without hesitation.

Regards,
Li