Message-ID: <CAPLW+4=qDpwU0Qkc9Q8bewnQb283RonAUHwwM-H86qkbPrU9gw@mail.gmail.com>
Date:   Mon, 6 Sep 2021 18:16:51 +0300
From:   Sam Protsenko <semen.protsenko@...aro.org>
To:     Paweł Chmiel <pawel.mikolaj.chmiel@...il.com>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Sylwester Nawrocki <s.nawrocki@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Tomasz Figa <tomasz.figa@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>,
        Ryu Euiyoul <ryu.real@...sung.com>,
        Tom Gall <tom.gall@...aro.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        John Stultz <john.stultz@...aro.org>,
        Amit Pundir <amit.pundir@...aro.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Samsung SOC <linux-samsung-soc@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>
Subject: Re: [PATCH 12/12] arm64: dts: exynos: Add Exynos850 SoC support

On Fri, 6 Aug 2021 at 23:32, Paweł Chmiel
<pawel.mikolaj.chmiel@...il.com> wrote:
>
> W dniu 06.08.2021 o 14:32, Krzysztof Kozlowski pisze:
> > On 06/08/2021 14:07, Sam Protsenko wrote:
> >> On Fri, 6 Aug 2021 at 10:49, Krzysztof Kozlowski
> >> <krzysztof.kozlowski@...onical.com> wrote:
> >>>
> >>> On 06/08/2021 01:06, Sam Protsenko wrote:
> >>>> On Sat, 31 Jul 2021 at 12:03, Krzysztof Kozlowski
> >>>> <krzysztof.kozlowski@...onical.com> wrote:
> >>>>
> >>>>>>
> >>>>>> This patch adds minimal SoC support. Particular board device tree files
> >>>>>> can include exynos850.dtsi file to get SoC related nodes, and then
> >>>>>> reference those nodes further as needed.
> >>>>>>
> >>>>>> Signed-off-by: Sam Protsenko <semen.protsenko@...aro.org>
> >>>>>> ---
> >>>>>>   .../boot/dts/exynos/exynos850-pinctrl.dtsi    | 782 ++++++++++++++++++
> >>>>>>   arch/arm64/boot/dts/exynos/exynos850-usi.dtsi |  30 +
> >>>>>>   arch/arm64/boot/dts/exynos/exynos850.dtsi     | 245 ++++++
> >>>>>
> >>>>> Not buildable. Missing Makefile, missing DTS. Please submit with initial
> >>>>> DTS, otherwise no one is able to verify it even compiles.
> >>>>>
> >>>>
> >>>> This device is not available for purchase yet. I'll send the patch for
> >>>> board dts once it's announced. I can do all the testing for now, if
> >>>> you have any specific requests. Would it be possible for us to review
> >>>> and apply only SoC support for now? Will send v2 soon...
> >>>
> >>> What you propose is equal to adding a driver (C source code) without
> >>> ability to compile it. What's the point of having it in the kernel? It's
> >>> unverifiable, unbuildable and unusable.
> >>>
> >>
> >> Yes, I understand. That's adding code with no users, and it's not a
> >> good practice.
> >>
> >>> We can review the DTSI however merging has to be with a DTS. Usually the
> >>> SoC vendor adds first an evalkit (e.g. SMDK board). Maybe you have one
> >>> for Exynos850? Otherwise if you cannot disclose the actual board, the
> >>> DTSI will have to wait. You can submit drivers, though.
> >>>
> >>
> >> Sure, let's go this way. I'll send v2 soon. Improving patches and
> >> having Reviewed-by tag for those would good enough for me at this
> >> point. I'll continue to prepare another Exynos850 related patches
> >> until the actual board is announced, like proper clock driver, reset,
> >> MMC, etc. Is it ok if I send those for a review too (so I can fix all
> >> issues ahead)?
> >
> > Sure, prepare all necessary drivers earlier. I suspect clocks will be a
> > real pain because of significant changes modeled in vendor kernel. I
> > remember Paweł Chmiel (+Cc) was doing something for these:
> > https://github.com/PabloPL/linux/tree/exynos7420
> >
> > I mentioned before - you should also modify the chipid driver. Check
> > also other drivers in drivers/soc/samsung, although some are needed only
> > for suspend&resume.
> >
> > BTW, Paweł,
> > How is your Exynos7420 progress? :)
> Hi
>
> Sadly i had to postpone it for a while. Maybe will have more time now to
> get back to it.
>
> About clock driver. In vendor sources there is clk driver with something
> called virtual clocks (different than real ones). That driver calls
> another driver called pwrcal, responsible for real manipulation of
> clocks in hardware. This one has info about real clocks and also
> additional info about for example rate for some of them, which is read
> from binary from memory, by another driver called ect_parser in case of
> devices at which i did looked.
>
> In my case i was able to find some more info about real clocks there -
> for example register names and offsets
> https://github.com/krzk/linux-vendor-backup/blob/mokee/android-3.18-samsung-galaxy-s7-sm-g930f-exynos8890/drivers/soc/samsung/pwrcal/S5E8890/S5E8890-cmusfr.h
> and some clocks hierarchy info inside
> https://github.com/krzk/linux-vendor-backup/blob/mokee/android-3.18-samsung-galaxy-s7-sm-g930f-exynos8890/drivers/soc/samsung/pwrcal/S5E8890/S5E8890-cmu.c
> but there was still many info missing.
>
> Finding a way (which could be applied to other Exynos SOC) to "convert"
> or use that vendor code and turn it into mainline driver, especially
> without TRM which is not available for all/most of them, would be great.
>
> I'm wondering if Exynos850 device has the same issue as on 7420 (and
> probably 8890/7578 and maybe also other 64 bit Exynos devices) - broken
> firmware. For example i had to specify in dts timer clock frequency, on
> few devices there is also a problem with timer registers not properly
> configured by FW, which probably won't be fixed by vendor and patches
> with workaround for it in kernel were rejected :/.

Hi Paweł,

Sorry for the late reply. Thanks for your input! I just started
implementing the clock driver, and maybe I can share some useful stuff
in exchange.

ECT parser: in downstream kernel there is an option to dump ECT tables
via some DebugFS file. I did that, and it seems to me it would be
easier to just hard code necessary table in corresponding drivers
code. E.g., PLL tables can be hard-coded in the clock driver (which is
how it seems to be implemented for other Exynos SoC upstream anyway).
So I don't think there is a huge need to upstream ECT parser itself.
But dumping the tables can be useful to implement corresponding
drivers (clocks, DVFS, APM, etc).

Investigating downstream clock driver for Exynos850 and its
dependencies (like VCLK, RA, CMUCAL), I figured it's much easier to
implement the clock driver completely from scratch. Looking into
clk-exynos7.c and clk-exynos5433.c implementation, this is probably
how upstream design should look like. And it has nothing to do with
downstream implementation. E.g., downstream driver has one single CMU
node in dts (for the whole clock subsystem), but for upstream drivers
we want to have separate nodes for each particular CMU. Also
downstream implementation is over-complicated; that might have some
sense for the vendor, if they have a bunch of similar SoCs or sharing
driver code between different OS kernels, but for upstream kernel it's
just unneeded complexity (several layers of abstraction that should be
just removed, and useful stuff should be integrated in already
existing upstream infrastructure). So I ended up using TRM and trying
to make something similar to mentioned upstream drivers. Of course,
there are some questions on whether we should use manual clocks
control, or rely on automatic clock gating and Q-channel
communication. But I'm having some progress already, and hopefully
will submit the minimal driver in a week or so.

As for downstream design and how to make a sense of it, for converting
that to something more upstreamable, here is what I figured.

1. Clock driver (clk-exynosXXXX.c) registers some vclk clocks
("Virtual Clocks"). Those are clocks which will be actually used. But
those are lacking any hardware specific data yet (like register
offsets, etc).

2. ".calid" field from those vclk's is used to find more
hardware-related info (later in run-time) for those registered clocks,
from cmucal-node.c file. That file contains actual parent information
and some info on HW stuff, but not real addresses/offset, but only
indexes.

3. Those indexes are resolved further (in run-time), obtaining actual
offsets from cmucal-sfr.c file, in ra_init() function.

4. Instead of using existing CCF clocks, custom vclk clock ops are
used. Those are defined in composite.c (has nothing to do with CCF
composite clocks). So the whole VCLK type looks like vendor
re-implementation of composite clocks. For example, one VCLK clock can
have the whole list of clocks which will be controlled via that single
VCLK clock, and all operations will be called for each clock from the
list.

5. Further those operations are actually calling the code from ra.c,
different functions are used for different clock types (PLL_TYPE,
MUX_TYPE, etc). So if you're curious about actual ops implementation
(like PLL, muxes, etc), just look there.

That should be enough to convert downstream driver to upstream one.
Basically one can use downstream source code to figure out info about
registers and offsets (from cmucal-sfr.c) and info about internal
clocks structure from cmucal-node.c and cmucal-vclk.c, and implement
everything from scratch using clk-exynos7.c/clk-exynos5433.c as a
template. I'd say it's easier to just use TRM for that, though :)

As for the broken firmware and clock freq registers: yes, CNTFRQ_EL0
registers wasn't set in EL3 firmware too, so I'm setting the
"clock-frequency" property for arch timer in dts as a workaround right
now, in my local tree. But I already let vendor guys know about that
problem, and they are trying to fix that right now (at least for
Exynos850 SoC).

Thanks!

> >
> >> And should I maybe add RFC tag for those?
> >
> > No need. Drivers can be merged before DTS users.
> >
> > Best regards,
> > Krzysztof
> >
>

Powered by blists - more mailing lists