[v6,5/6] arm64: dts: qcom: sm8450: Add opp table support to PCIe

PCIe needs to choose the appropriate performance state of RPMH power
domain and interconnect bandwidth based up on the PCIe gen speed.

Add the OPP table support to specify RPMH performance states and
interconnect peak bandwidth.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 74 ++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

On 29-01-24, 21:34, Manivannan Sadhasivam wrote:
> On Fri, Jan 12, 2024 at 07:52:04PM +0530, Krishna chaitanya chundru wrote:
> > PCIe needs to choose the appropriate performance state of RPMH power
> > domain and interconnect bandwidth based up on the PCIe gen speed.
> > 
> > Add the OPP table support to specify RPMH performance states and
> > interconnect peak bandwidth.
> > 
> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8450.dtsi | 74 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > index 6b1d2e0d9d14..eab85ecaeff0 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> > @@ -1827,7 +1827,32 @@ pcie0: pcie@1c00000 {
> >  			pinctrl-names = "default";
> >  			pinctrl-0 = <&pcie0_default_state>;
> >  
> > +			operating-points-v2 = <&pcie0_opp_table>;
> > +
> >  			status = "disabled";
> > +
> > +			pcie0_opp_table: opp-table {
> > +				compatible = "operating-points-v2";
> > +
> > +				opp-2500000 {
> > +					opp-hz = /bits/ 64 <2500000>;
> > +					required-opps = <&rpmhpd_opp_low_svs>;
> > +					opp-peak-kBps = <250000 250000>;
> 
> This is a question for Viresh: We already have macros in the driver to derive
> the bandwidth based on link speed. So if OPP core exposes a callback to allow
> the consumers to set the bw on its own, we can get rid of this entry.
> 
> Similar to config_clks()/config_regulators(). Is that feasible?

I don't have any issues with a new callback for bw. But, AFAIU, the DT
is required to represent the hardware irrespective of what any OS
would do with it. So DT should ideally have these values here, right ?

Also, the driver has already moved away from using those macros now
and depend on the OPP core to do the right thing. It only uses the
macro for the cases where the DT OPP table isn't available. And as
said by few others as well already, the driver really should try to
add OPPs dynamically in that case to avoid multiple code paths and
stick to a single OPP based solution.

On 30-01-24, 12:44, Manivannan Sadhasivam wrote:
> On Tue, Jan 30, 2024 at 11:41:11AM +0530, Viresh Kumar wrote:
> > I don't have any issues with a new callback for bw. But, AFAIU, the DT
> > is required to represent the hardware irrespective of what any OS
> > would do with it. So DT should ideally have these values here, right ?
> > 
> 
> Not necessarily. Because, right now the bandwidth values of the all peripherals
> are encoded within the drivers. Only OPP has the requirement to define the
> values in DT.

I have a bit different argument here. I am saying that it doesn't
matter if we have OPP framework or something else using these values.
The hardware must be represented properly by the DT, so Linux or any
other firmware/OS can program the device. So DT should have bandwidth
values anyway. And that's the way we have designed things in Linux
now.

> > Also, the driver has already moved away from using those macros now
> > and depend on the OPP core to do the right thing. It only uses the
> > macro for the cases where the DT OPP table isn't available. And as
> > said by few others as well already, the driver really should try to
> > add OPPs dynamically in that case to avoid multiple code paths and
> > stick to a single OPP based solution.
> > 
> 
> Still I prefer to use OPP for bandwidth control because both the voltage and
> bandwidth values need to be updated at the same time. My only point here is, if
> OPP exposes a callback for bw, then we can keep the DT behavior consistent.

Feels like we are going a bit backward on this. The current view, as
per me, is that driver shouldn't need to micromanage all these
configurations and the OPP core should be able to handle them. That's
why we want to handle all configurations from there.

This also means that the DT needs to contain all this information and
drivers shouldn't use special math functions to calculate these
values. Drivers need to move away from them, instead of getting more
of those.

I don't see how a callback would be helpful here, if the driver relies
on DT values only. Or am I confusing things here ??

On 30-01-24, 15:18, Manivannan Sadhasivam wrote:
> So you are saying that the ICC core itself should get the bw values from DT
> instead of hardcoding in the driver? If so, I'd like to get the opinion from
> Georgi/Bjorn.

Not really. The drivers or the ICC core doesn't need to do anything I
guess. Since the values are coming via the OPP, we must just use it to
hide all these details.

Why is the ICC core required to get into this here ? ICC core should
be ready to get the information from DT (may or may not via the OPP
core), or from driver.

On Tue, Jan 30, 2024 at 03:25:08PM +0530, Viresh Kumar wrote:
> On 30-01-24, 15:18, Manivannan Sadhasivam wrote:
> > So you are saying that the ICC core itself should get the bw values from DT
> > instead of hardcoding in the driver? If so, I'd like to get the opinion from
> > Georgi/Bjorn.
> 
> Not really. The drivers or the ICC core doesn't need to do anything I
> guess. Since the values are coming via the OPP, we must just use it to
> hide all these details.
> 
> Why is the ICC core required to get into this here ? ICC core should
> be ready to get the information from DT (may or may not via the OPP
> core), or from driver.
> 

Agree. But what I'm saying is, right now there is no DT property in the
interconnect consumer nodes to specificy the bw requirements. This is all
hardcoded in the respective ICC consumer drivers.

But when we use OPP to control bw, the bw requirements come from DT. This is
what I see as a difference. Because, only nodes making use of OPP will specify
bw in DT and other nodes making use of just ICC will not.

Maybe I'm worrying too much about these details... But it looks like
inconsistency to me.

- Mani