| Message ID | 20231220-sa8295p-gpu-v1-0-d8cdf2257f97@quicinc.com |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sa8295p: Enable GPU | expand |
Quoting Bjorn Andersson (2023-12-20 19:50:36) > The GX GDSC is modelled to aid the GMU in powering down the GPU in the > event that the GPU crashes, so that it can be restarted again. But in > the event that the power-domain is supplied through a dedicated > regulator (in contrast to being a subdomin of another power-domain), > something needs to turn that regulator on, both to make sure things are > powered and to match the operation in gdsc_disable(). > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/clk/qcom/gdsc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index 5358e28122ab..d1139c895503 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc) > */ > int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain) > { > + struct gdsc *sc = domain_to_gdsc(domain); > + int ret = 0; > + > /* Do nothing but give genpd the impression that we were successful */ Update this comment. > - return 0; > + if (sc->rsupply) > + ret = regulator_enable(sc->rsupply); > + > + return ret; > }
On Thu, 21 Dec 2023 at 05:52, Bjorn Andersson <quic_bjorande@quicinc.com> wrote: > > From: Bjorn Andersson <andersson@kernel.org> > > The SA8295P ADP has a MAX20411 LDO regulator on I2C 12, supplying the > VDD_GFX pads. Enable the bus and add the maxim,max20411 device on the > bus. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 40 ++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > index fd253942e5e5..e16406c9c19d 100644 > --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > @@ -266,6 +266,26 @@ &dispcc1 { > status = "okay"; > }; > > +&i2c12 { > + pinctrl-names = "default"; > + pinctrl-0 = <&qup1_i2c4_state>; > + > + status = "okay"; > + > + vdd_gfx: regulator@39 { > + compatible = "maxim,max20411"; > + reg = <0x39>; > + > + regulator-min-microvolt = <800000>; > + regulator-max-microvolt = <968750>; > + > + enable-gpios = <&pmm8540a_gpios 2 GPIO_ACTIVE_HIGH>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&max20411_en>; > + }; > +}; > + > &mdss0 { > status = "okay"; > }; > @@ -476,6 +496,10 @@ &pcie4_phy { > status = "okay"; > }; > > +&qup1 { > + status = "okay"; > +}; > + > &qup2 { > status = "okay"; > }; > @@ -728,4 +752,20 @@ wake-n-pins { > bias-pull-up; > }; > }; > + > + qup1_i2c4_state: qup1-i2c4-state { > + pins = "gpio0", "gpio1"; > + function = "qup12"; > + > + drive-strength = <2>; > + bias-pull-up; > + }; > +}; > + > +&pmm8540a_gpios { I think pmm8540a_gpios comes before tlmm in the dictionary order. Other than that LGTM > + max20411_en: max20411-en-state { > + pins = "gpio2"; > + function = "normal"; > + output-enable; > + }; > };
On 21.12.2023 04:50, Bjorn Andersson wrote: > From: Bjorn Andersson <andersson@kernel.org> > > The SA8295P ADP has a MAX20411 LDO regulator on I2C 12, supplying the > VDD_GFX pads. Enable the bus and add the maxim,max20411 device on the > bus. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 40 ++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > index fd253942e5e5..e16406c9c19d 100644 > --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > @@ -266,6 +266,26 @@ &dispcc1 { > status = "okay"; > }; > > +&i2c12 { > + pinctrl-names = "default"; > + pinctrl-0 = <&qup1_i2c4_state>; property-n property-names (same below) > + > + status = "okay"; > + > + vdd_gfx: regulator@39 { > + compatible = "maxim,max20411"; > + reg = <0x39>; > + > + regulator-min-microvolt = <800000>; > + regulator-max-microvolt = <968750>; Is this ever going to be scaled? I suppose you could add some OPP code to drm/msm and use opp-microvolts.. Or lock this down at min=max Konrad
On 21.12.2023 04:50, Bjorn Andersson wrote: > The GX GDSC is modelled to aid the GMU in powering down the GPU in the > event that the GPU crashes, so that it can be restarted again. But in > the event that the power-domain is supplied through a dedicated > regulator (in contrast to being a subdomin of another power-domain), > something needs to turn that regulator on, both to make sure things are > powered and to match the operation in gdsc_disable(). > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/clk/qcom/gdsc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index 5358e28122ab..d1139c895503 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc) > */ > int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain) I suppose the name is confusing now.. But at the same time I can't come up with anything that's less than like 6 words.. Konrad
On 21.12.2023 14:04, Dmitry Baryshkov wrote: > On Thu, 21 Dec 2023 at 15:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> On 21.12.2023 04:50, Bjorn Andersson wrote: >>> The GX GDSC is modelled to aid the GMU in powering down the GPU in the >>> event that the GPU crashes, so that it can be restarted again. But in >>> the event that the power-domain is supplied through a dedicated >>> regulator (in contrast to being a subdomin of another power-domain), >>> something needs to turn that regulator on, both to make sure things are >>> powered and to match the operation in gdsc_disable(). >>> >>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> >>> --- >>> drivers/clk/qcom/gdsc.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >>> index 5358e28122ab..d1139c895503 100644 >>> --- a/drivers/clk/qcom/gdsc.c >>> +++ b/drivers/clk/qcom/gdsc.c >>> @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc) >>> */ >>> int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain) >> I suppose the name is confusing now.. >> >> But at the same time I can't come up with anything that's less than >> like 6 words.. > > gdsc_gx_enable() ;-) except not really only gx and not really enable :( gdsc_shared_enable would probably be closer to our current nomenclature.. Konrad
On Thu, 21 Dec 2023 at 15:16, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 21.12.2023 14:04, Dmitry Baryshkov wrote: > > On Thu, 21 Dec 2023 at 15:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > >> > >> On 21.12.2023 04:50, Bjorn Andersson wrote: > >>> The GX GDSC is modelled to aid the GMU in powering down the GPU in the > >>> event that the GPU crashes, so that it can be restarted again. But in > >>> the event that the power-domain is supplied through a dedicated > >>> regulator (in contrast to being a subdomin of another power-domain), > >>> something needs to turn that regulator on, both to make sure things are > >>> powered and to match the operation in gdsc_disable(). > >>> > >>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > >>> --- > >>> drivers/clk/qcom/gdsc.c | 8 +++++++- > >>> 1 file changed, 7 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > >>> index 5358e28122ab..d1139c895503 100644 > >>> --- a/drivers/clk/qcom/gdsc.c > >>> +++ b/drivers/clk/qcom/gdsc.c > >>> @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc) > >>> */ > >>> int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain) > >> I suppose the name is confusing now.. > >> > >> But at the same time I can't come up with anything that's less than > >> like 6 words.. > > > > gdsc_gx_enable() ;-) > except not really only gx and not really enable :( > > gdsc_shared_enable would probably be closer to our current > nomenclature.. gdsc_dummy_gx_enable*( Or gdsc_dummy_gmu_gx_enable(). Still less than 6 words. I'm trying my best!
Due to the different PMIC configuration found in the SA8295P platform, compared to SC8280XP, the VDD_GFX pads are supplied by an dedicated MAX20411 LDO. Support for expressing the regulator supply is added to the binding, the support for enabling the parent supply for GX is added, the missing gfx.lvl power-domain is dropped, and the DeviceTree is wired up to enable the GPU in this configuration. Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> --- Bjorn Andersson (8): dt-bindings: clock: qcom: Allow VDD_GFX supply to GX clk: qcom: gdsc: Enable supply reglator in GPU GX handler clk: qcom: gpucc-sc8280xp: Add external supply for GX gdsc soc: qcom: rpmhpd: Drop SA8540P gfx.lvl arm64: dts: qcom: sa8540p: Drop gfx.lvl as power-domain for gpucc arm64: dts: qcom: sa8295p-adp: add max20411 arm64: dts: qcom: sa8295p-adp: Enable GPU arm64: defconfig: Enable MAX20411 regulator driver .../devicetree/bindings/clock/qcom,gpucc.yaml | 3 + arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 69 ++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sa8540p.dtsi | 2 + arch/arm64/configs/defconfig | 1 + drivers/clk/qcom/gdsc.c | 8 ++- drivers/clk/qcom/gpucc-sc8280xp.c | 1 + drivers/pmdomain/qcom/rpmhpd.c | 1 - 7 files changed, 83 insertions(+), 2 deletions(-) --- base-commit: 20d857259d7d10cd0d5e8b60608455986167cfad change-id: 20231220-sa8295p-gpu-51c5f343e3ec Best regards,