Re: [PATCH v2] arm64: dts: qcom: sc7180: Add A618 gpu dt blob

[Jordan Crouse]

On Mon, Jan 27, 2020 at 02:29:53PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jan 27, 2020 at 1:30 AM Sharat Masetty <smasetty@xxxxxxxxxxxxxx> wrote:
> >
> > This patch adds the required dt nodes and properties
> > to enabled A618 GPU.
> >
> > Signed-off-by: Sharat Masetty <smasetty@xxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx/
> 
> ...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:
> 
> 20200124144154.v2.10.I1a4b93fb005791e29a9dcf288fc8bd459a555a59@changeid/">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@5000000 {
> > +                       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?

I'll toss it on my to-do list. There always seems to be something more urgent
but I should just bite the bullet and get it done.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project