Message-ID: <e9a07956-86d4-ae96-a641-020b3d85c51c@canonical.com>
Date:   Mon, 6 Dec 2021 09:26:36 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH 6/6] arm64: dts: exynos: Add initial device tree support
 for Exynos7885 SoC
Content-Language: en-US
To:     David Virag <virag.david003@gmail.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
References: <20211205153302.76418-1-virag.david003@gmail.com>
 <20211205153302.76418-7-virag.david003@gmail.com>
 <de24b968-f359-25bf-76d7-69328401f83d@canonical.com>
 <60f5c2e98e3a2048f86a79c3aa1ed945dc0cb4aa.camel@gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
In-Reply-To: <60f5c2e98e3a2048f86a79c3aa1ed945dc0cb4aa.camel@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 05/12/2021 19:14, David Virag wrote:
> 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>,

These are for cpu6 and cpu7, because they match the index in
"interrupt-affinity". cpu6 and cpu7 are a73 cores in your DTSI.

>>> +                            <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>;

These are for a53, judging by affinity.

>>> +               interrupt-affinity = <&cpu6>,
>>> +                                    <&cpu7>,
>>> +                                    <&cpu0>,
>>> +                                    <&cpu1>,
>>> +                                    <&cpu2>,
>>> +                                    <&cpu3>,
>>> +                                    <&cpu4>,
>>> +                                    <&cpu5>;
>>> +       };
>>> +
> 
> [...]

Best regards,
Krzysztof