| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4bd79f53cab95db9286067836722dd4b@codeaurora.org>
Date: Fri, 31 Jan 2020 17:46:23 +0530
From: smasetty@...eaurora.org
To: Doug Anderson <dianders@...omium.org>
Cc: freedreno <freedreno@...ts.freedesktop.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>, dri-devel@...edesktop.org,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Jordan Crouse <jcrouse@...eaurora.org>,
Matthias Kaehlcke <mka@...omium.org>,
Rob Clark <robdclark@...omium.org>,
linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH v2] arm64: dts: qcom: sc7180: Add A618 gpu dt blob
On 2020-01-28 03:59, Doug Anderson wrote:
> Hi,
>
> On Mon, Jan 27, 2020 at 1:30 AM Sharat Masetty
> <smasetty@...eaurora.org> wrote:
>>
>> This patch adds the required dt nodes and properties
>> to enabled A618 GPU.
>>
>> Signed-off-by: Sharat Masetty <smasetty@...eaurora.org>
>> ---
>> arch/arm64/boot/dts/qcom/sc7180.dtsi | 103
>> +++++++++++++++++++++++++++++++++++
>> 1 file changed, 103 insertions(+)
>
> Note that +Matthias Kaehlcke commented on v1 your patch:
>
> https://lore.kernel.org/r/20191204220033.GH228856@google.com/
>
> ...so he should have been CCed on v2. I would also note that some of
> the comments below are echos of what Matthias already said in the
> previous version but just weren't addressed.
>
>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index b859431..277d84d 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -7,6 +7,7 @@
>>
>> #include <dt-bindings/clock/qcom,gcc-sc7180.h>
>> #include <dt-bindings/clock/qcom,rpmh.h>
>> +#include <dt-bindings/clock/qcom,gpucc-sc7180.h>
>
> Header files should be sorted alphabetically. ...or, even better,
> base your patch atop mine:
>
> https://lore.kernel.org/r/20200124144154.v2.10.I1a4b93fb005791e29a9dcf288fc8bd459a555a59@changeid/
>
> ...which adds the gpucc header file so you don't have to. ...and when
> you do so, email out a Reviewed-by and/or Tested-by for my patch. ;-)
>
>
>> #include <dt-bindings/interrupt-controller/arm-gic.h>
>> #include <dt-bindings/interconnect/qcom,sc7180.h>
>> #include <dt-bindings/phy/phy-qcom-qusb2.h>
>> @@ -1619,6 +1620,108 @@
>> #interconnect-cells = <1>;
>> qcom,bcm-voters = <&apps_bcm_voter>;
>> };
>> +
>> + gpu: gpu@...0000 {
>> + compatible = "qcom,adreno-618.0",
>> "qcom,adreno";
>
> Though it's not controversial, please send a patch to:
>
> Documentation/devicetree/bindings/display/msm/gmu.txt
>
> ...to add 'qcom,adreno-618.0', like:
>
> for example:
> "qcom,adreno-gmu-618.0", "qcom,adreno-gmu"
> "qcom,adreno-gmu-630.2", "qcom,adreno-gmu"
>
> Probably as part of this you will be asked to convert this file to
> yaml. IMO we don't need to block landing this patch on the effort to
> convert it to yaml, but you should still work on it. ...or maybe
> Jordan wants to work on it?
>
>
>> + #stream-id-cells = <16>;
>> + reg = <0 0x05000000 0 0x40000>, <0 0x0509e000
>> 0 0x1000>,
>> + <0 0x05061000 0 0x800>;
>> + reg-names = "kgsl_3d0_reg_memory", "cx_mem",
>> "cx_dbgc";
>> + interrupts = <GIC_SPI 300
>> IRQ_TYPE_LEVEL_HIGH>;
>> + iommus = <&adreno_smmu 0>;
>> + operating-points-v2 = <&gpu_opp_table>;
>> + interconnects = <&gem_noc MASTER_GFX3D
>> &mc_virt SLAVE_EBI1>;
>
> Running:
> $ git fetch
> git://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
> for-next
> $ git grep gem_noc FETCH_HEAD
> $ git fetch
> git://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
> arm64-for-5.7-to-be-rebased
> $ git grep gem_noc FETCH_HEAD
>
> ...shows no hits. That's because the interconnect patches haven't
> landed in the tree that you're targeting. In the very least you
> should mention somewhere in your email that your patch depends on the
> interconnect patches landing, perhaps pointing at:
>
> https://lore.kernel.org/r/1577782737-32068-4-git-send-email-okukatla@codeaurora.org
>
> ...but even better would be to split your patch into two parts. The
> first patch would be exactly like your patch except without the
> "interconnects" line. The 2nd patch would add the interconnects line.
> This would allow Bjorn/Andy to land the first patch now and then land
> the second patch when the interconnect series is ready. I can confirm
> that you can still get basic GPU functionality even without the
> interconnects bit so it would be worth landing earlier.
>
>
> I will also note that by basing on a tree that has private patches to
> the same file you're touching you make it very hard for a maintainer
> to apply. When I try this:
>
> $ curl https://patchwork.kernel.org/patch/11352261/mbox/ | git am -3
>
> I get:
>
> error: sha1 information is lacking or useless
> (arch/arm64/boot/dts/qcom/sc7180.dtsi).
> error: could not build fake ancestor
> Patch failed at 0001 arm64: dts: qcom: sc7180: Add A618 gpu dt blob
>
> ...yes, I can apply it with 'git am --show-current-patch | patch -p1'
> but it's ugly (and it ends up making things sort in the wrong order).
>
>
>> + adreno_smmu: iommu@...0000 {
>> + compatible = "qcom,sc7180-smmu-v2",
>> "qcom,smmu-v2";
>
> Please send a patch to:
>
> Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>
> ...adding 'qcom,sc7180-smmu-v2'. If you do this it will point out
> that you've added a new clock: "mem_iface_clk". Is this truly a new
> clock in sc7180 compared to previous IOMMUs? ...or is it not really
> needed?
>
>
>> + reg = <0 0x05040000 0 0x10000>;
>> + #iommu-cells = <1>;
>> + #global-interrupts = <2>;
>> + interrupts = <GIC_SPI 229
>> IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 231
>> IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 364
>> IRQ_TYPE_EDGE_RISING>,
>> + <GIC_SPI 365
>> IRQ_TYPE_EDGE_RISING>,
>> + <GIC_SPI 366
>> IRQ_TYPE_EDGE_RISING>,
>> + <GIC_SPI 367
>> IRQ_TYPE_EDGE_RISING>,
>> + <GIC_SPI 368
>> IRQ_TYPE_EDGE_RISING>,
>> + <GIC_SPI 369
>> IRQ_TYPE_EDGE_RISING>,
>> + <GIC_SPI 370
>> IRQ_TYPE_EDGE_RISING>,
>> + <GIC_SPI 371
>> IRQ_TYPE_EDGE_RISING>;
>> + clocks = <&gcc GCC_GPU_MEMNOC_GFX_CLK>,
>> + <&gcc GCC_GPU_CFG_AHB_CLK>,
>> + <&gcc GCC_DDRSS_GPU_AXI_CLK>;
>> +
>> + clock-names = "bus", "iface", "mem_iface_clk";
>
> nit: keep clocks and clock-names next to each other (no blank line).
> If you really feel like it needs more space add it between the
> clock-names and power-domains.
>
>> + power-domains = <&gpucc CX_GDSC>;
>
> Similar to interconnects, gpucc hasn't landed yet. Somewhere you
> should point out this fact and ideally point to:
>
> https://lore.kernel.org/r/20200124144154.v2.10.I1a4b93fb005791e29a9dcf288fc8bd459a555a59@changeid/
>
> ...unlike interconnects, your patch can't land without gpucc, so you
> should point this out as a hard dependency.
>
>
>> + };
>> +
>> + gmu: gmu@...a000 {
>> + compatible="qcom,adreno-gmu-618",
>> "qcom,adreno-gmu";
>
> As per the bindings, "qcom,adreno-gmu-618" should be
> "qcom,adreno-gmu-618.0", right?
>
> ...and I bet you'd never have guessed that I'll request that you add
> "qcom,adreno-gmu-618" to:
>
> Documentation/devicetree/bindings/display/msm/gmu.txt
>
> ...and that you'll probably be asked to convert to yaml. Again, maybe
> Jordan wants to attempt this?
>
>
>> + reg = <0 0x0506a000 0 0x31000>, <0 0x0b290000
>> 0 0x10000>,
>> + <0 0x0b490000 0 0x10000>;
>> + reg-names = "gmu", "gmu_pdc", "gmu_pdc_seq";
>> + interrupts = <GIC_SPI 304
>> IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "hfi", "gmu";
>> + clocks = <&gpucc GPU_CC_CX_GMU_CLK>,
>> + <&gpucc GPU_CC_CXO_CLK>,
>> + <&gcc GCC_DDRSS_GPU_AXI_CLK>,
>> + <&gcc GCC_GPU_MEMNOC_GFX_CLK>;
>> + clock-names = "gmu", "cxo", "axi", "memnoc";
>> + power-domains = <&gpucc CX_GDSC>;
>
> Bindings claim that you need both CX and GC. Is sc7180 somehow
> different? Bindings also claim that you should be providing
> power-domain-names.
No this is still needed, We need the GX power domain for GPU recovery
use cases where the shutdown was not successful. I am working the Taniya
to get the dependencies sorted out to bring this change in. This should
be
okay for the time being.
>
>
>
>> + iommus = <&adreno_smmu 5>;
>> + operating-points-v2 = <&gmu_opp_table>;
>> +
>> + gmu_opp_table: opp-table {
>> + compatible = "operating-points-v2";
>> +
>> + opp-200000000 {
>> + opp-hz = /bits/ 64
>> <200000000>;
>> + opp-level =
>> <RPMH_REGULATOR_LEVEL_MIN_SVS>;
>> + };
>> + };
>> + };
>> };
>>
>> thermal-zones {
>
> Using the "thermal-zones" as context, it looks as if you're asserting
> that your new nodes belong at the very end of the pile of nodes with
> addresses. This is not true. Looking at the branch
> 'arm64-for-5.7-to-be-rebased' on the Qualcomm tree, I see:
>
> cpufreq_hw: cpufreq@...23000
>
> ...which has a larger address than your 0x0506a000. Please sort your
> nodes numerically.
>
>
> -Doug
Powered by blists - more mailing lists