Message-ID: <60f5c2e98e3a2048f86a79c3aa1ed945dc0cb4aa.camel@gmail.com>
Subject: Re: [PATCH 6/6] arm64: dts: exynos: Add initial device tree support
 for Exynos7885 SoC
From:   David Virag <virag.david003@gmail.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Sam Protsenko <semen.protsenko@linaro.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Sylwester Nawrocki <s.nawrocki@samsung.com>,
        Tomasz Figa <tomasz.figa@gmail.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org
Date:   Sun, 05 Dec 2021 19:14:23 +0100
In-Reply-To: <de24b968-f359-25bf-76d7-69328401f83d@canonical.com>
References: <20211205153302.76418-1-virag.david003@gmail.com>
         <20211205153302.76418-7-virag.david003@gmail.com>
         <de24b968-f359-25bf-76d7-69328401f83d@canonical.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.2 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Sun, 2021-12-05 at 18:31 +0100, Krzysztof Kozlowski wrote:

[...]

> > +       fimc_is_mclk0_in: fimc_is_mclk0_in {
> > +               samsung,pins = "gpc0-0";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> > +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > +               samsung,pin-drv = <2>;
> > +       };
> > +
> > +       fimc_is_mclk0_out: fimc_is_mclk0_out {
> > +               samsung,pins = "gpc0-0";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_OUTPUT>;
> > +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > +               samsung,pin-drv = <2>;
> > +       };
> > +
> > +       fimc_is_mclk0_fn: fimc_is_mclk0_fn {
> 
> I cannot get the point of these pin configurations - three groups
> with
> only function difference. How this would be used by the driver? Maybe
> just keep the one really used. Same for others below.
> 

They seem to be changed in some cases by the FIMC-IS and/or Camera
module drivers in the downstream kernel. I'm not exactly sure about why
and how are they needed, as the code for FIMC-IS is quite large and
it's not my priority to work on it right now. I can remove these
configurations for now if that's okay, maybe I, or someone else will
re-add it later if it's needed.

> 
> > +               samsung,pins = "gpc0-0";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> > +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > +               samsung,pin-drv = <2>;
> > +       };
> > +

[...]

> > +
> > +       arm-pmu {
> > +               compatible = "arm,armv8-pmuv3";
> 
> Wrong compatible. Please use specific, although I don't know which
> one
> you have - 53 or 73... since you have two clusters, I would expect
> two
> PMUs, hmm....

I was thinking the same, but there's a problem: As I'm also guessing we
have two PMUs for the a53 and a73 cores, we'd need to seperate it but I
have no access to the documentation that would let me know which
interrupts we would need for both of these PMUs. The downstream dts
doesn't tell us anything specific in this case, and I have no idea how
else am I supposed to know which interrupts are right without a TRM.

I'd be guessing either the 82, 83 or the 218, 219 interrupts would be
the right one for the a73 cores, and I suspect that it should be 82 and
83, but I can't really confirm this.

Do you have any idea how to proceed in this case? Maybe there is a way
to test which ones would be right?

> 
> > +               interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>,
> > +                            <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>,
> > +                            <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>,
> > +                            <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>,
> > +                            <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>,
> > +                            <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>,
> > +                            <GIC_SPI 218 IRQ_TYPE_LEVEL_HIGH>,
> > +                            <GIC_SPI 219 IRQ_TYPE_LEVEL_HIGH>;
> > +               interrupt-affinity = <&cpu6>,
> > +                                    <&cpu7>,
> > +                                    <&cpu0>,
> > +                                    <&cpu1>,
> > +                                    <&cpu2>,
> > +                                    <&cpu3>,
> > +                                    <&cpu4>,
> > +                                    <&cpu5>;
> > +       };
> > +

[...]

> Best regards,
> Krzysztof

The rest of the things you mentioned will be fixed in v2

Best Regards,
David