| Message ID | 20230217-topic-icc-fixes-v5-v5-8-c9a550f9fdb9@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | The great interconnecification fixation | expand |
On 17.02.2023 11:46, Konrad Dybcio wrote: > A0NoC only does bus scaling through RPM votes and does not have any > ICC clocks. Describe this. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- This is bad, as devm_clk_get_bulk{"", _optional} doesn't check if num_clocks makes sense and passes "-1" down the devres alloc chain.. I'll rework this for the next revision by simply assigning the common "bus", "bus_a" set everywhere instead of relying on it being there by default.. Konrad > drivers/interconnect/qcom/msm8996.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c > index 1a5e0ad36cc4..45eb8675fb11 100644 > --- a/drivers/interconnect/qcom/msm8996.c > +++ b/drivers/interconnect/qcom/msm8996.c > @@ -1817,6 +1817,7 @@ static const struct qcom_icc_desc msm8996_a0noc = { > .type = QCOM_ICC_NOC, > .nodes = a0noc_nodes, > .num_nodes = ARRAY_SIZE(a0noc_nodes), > + .num_bus_clocks = -1, /* No bus clock scaling */ > .intf_clocks = a0noc_intf_clocks, > .num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks), > .has_bus_pd = true, >
On 17.02.2023 20:27, Konrad Dybcio wrote: > > > On 17.02.2023 11:46, Konrad Dybcio wrote: >> A0NoC only does bus scaling through RPM votes and does not have any >> ICC clocks. Describe this. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- > This is bad, as devm_clk_get_bulk{"", _optional} doesn't > check if num_clocks makes sense and passes "-1" down the > devres alloc chain.. > > I'll rework this for the next revision by simply assigning > the common "bus", "bus_a" set everywhere instead of relying > on it being there by default.. Or maybe I shouldn't, as that will require redefining the array over and over again.. Perhaps just passing <&xo_board>, <&xo_board> to a0noc's "bus", "bus_a", similar to what's been done on SDM630's GNoC would be less messy? Konrad > > Konrad >> drivers/interconnect/qcom/msm8996.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c >> index 1a5e0ad36cc4..45eb8675fb11 100644 >> --- a/drivers/interconnect/qcom/msm8996.c >> +++ b/drivers/interconnect/qcom/msm8996.c >> @@ -1817,6 +1817,7 @@ static const struct qcom_icc_desc msm8996_a0noc = { >> .type = QCOM_ICC_NOC, >> .nodes = a0noc_nodes, >> .num_nodes = ARRAY_SIZE(a0noc_nodes), >> + .num_bus_clocks = -1, /* No bus clock scaling */ >> .intf_clocks = a0noc_intf_clocks, >> .num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks), >> .has_bus_pd = true, >>
On Fri, 17 Feb 2023 at 21:53, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 17.02.2023 20:27, Konrad Dybcio wrote: > > > > > > On 17.02.2023 11:46, Konrad Dybcio wrote: > >> A0NoC only does bus scaling through RPM votes and does not have any > >> ICC clocks. Describe this. > >> > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >> --- > > This is bad, as devm_clk_get_bulk{"", _optional} doesn't > > check if num_clocks makes sense and passes "-1" down the > > devres alloc chain.. > > > > I'll rework this for the next revision by simply assigning > > the common "bus", "bus_a" set everywhere instead of relying > > on it being there by default.. > Or maybe I shouldn't, as that will require redefining the array > over and over again.. Perhaps just passing <&xo_board>, <&xo_board> > to a0noc's "bus", "bus_a", similar to what's been done on SDM630's > GNoC would be less messy? What about simply skipping a call to devm_clk_get if num_bus_clocks is negative? > > Konrad > > > > Konrad > >> drivers/interconnect/qcom/msm8996.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c > >> index 1a5e0ad36cc4..45eb8675fb11 100644 > >> --- a/drivers/interconnect/qcom/msm8996.c > >> +++ b/drivers/interconnect/qcom/msm8996.c > >> @@ -1817,6 +1817,7 @@ static const struct qcom_icc_desc msm8996_a0noc = { > >> .type = QCOM_ICC_NOC, > >> .nodes = a0noc_nodes, > >> .num_nodes = ARRAY_SIZE(a0noc_nodes), > >> + .num_bus_clocks = -1, /* No bus clock scaling */ > >> .intf_clocks = a0noc_intf_clocks, > >> .num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks), > >> .has_bus_pd = true, > >>
On 17.02.2023 21:26, Dmitry Baryshkov wrote: > On Fri, 17 Feb 2023 at 21:53, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> >> >> On 17.02.2023 20:27, Konrad Dybcio wrote: >>> >>> >>> On 17.02.2023 11:46, Konrad Dybcio wrote: >>>> A0NoC only does bus scaling through RPM votes and does not have any >>>> ICC clocks. Describe this. >>>> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- >>> This is bad, as devm_clk_get_bulk{"", _optional} doesn't >>> check if num_clocks makes sense and passes "-1" down the >>> devres alloc chain.. >>> >>> I'll rework this for the next revision by simply assigning >>> the common "bus", "bus_a" set everywhere instead of relying >>> on it being there by default.. >> Or maybe I shouldn't, as that will require redefining the array >> over and over again.. Perhaps just passing <&xo_board>, <&xo_board> >> to a0noc's "bus", "bus_a", similar to what's been done on SDM630's >> GNoC would be less messy? > > What about simply skipping a call to devm_clk_get if num_bus_clocks is negative? I tested that locally before reporting the mistake on my side and while it works, I just consider it.. ugly, because: num_clocks = >0 => use the externally specified num_/clocks (logical) =0 => use the default 2 <0 => consider there's zero ..but maybe that's just me.. if you don't find it ugly, I may just go with that. Konrad > >> >> Konrad >>> >>> Konrad >>>> drivers/interconnect/qcom/msm8996.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c >>>> index 1a5e0ad36cc4..45eb8675fb11 100644 >>>> --- a/drivers/interconnect/qcom/msm8996.c >>>> +++ b/drivers/interconnect/qcom/msm8996.c >>>> @@ -1817,6 +1817,7 @@ static const struct qcom_icc_desc msm8996_a0noc = { >>>> .type = QCOM_ICC_NOC, >>>> .nodes = a0noc_nodes, >>>> .num_nodes = ARRAY_SIZE(a0noc_nodes), >>>> + .num_bus_clocks = -1, /* No bus clock scaling */ >>>> .intf_clocks = a0noc_intf_clocks, >>>> .num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks), >>>> .has_bus_pd = true, >>>> > > >
On 17/02/2023 22:28, Konrad Dybcio wrote: > > > On 17.02.2023 21:26, Dmitry Baryshkov wrote: >> On Fri, 17 Feb 2023 at 21:53, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >>> >>> >>> >>> On 17.02.2023 20:27, Konrad Dybcio wrote: >>>> >>>> >>>> On 17.02.2023 11:46, Konrad Dybcio wrote: >>>>> A0NoC only does bus scaling through RPM votes and does not have any >>>>> ICC clocks. Describe this. >>>>> >>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>> --- >>>> This is bad, as devm_clk_get_bulk{"", _optional} doesn't >>>> check if num_clocks makes sense and passes "-1" down the >>>> devres alloc chain.. >>>> >>>> I'll rework this for the next revision by simply assigning >>>> the common "bus", "bus_a" set everywhere instead of relying >>>> on it being there by default.. >>> Or maybe I shouldn't, as that will require redefining the array >>> over and over again.. Perhaps just passing <&xo_board>, <&xo_board> >>> to a0noc's "bus", "bus_a", similar to what's been done on SDM630's >>> GNoC would be less messy? >> >> What about simply skipping a call to devm_clk_get if num_bus_clocks is negative? > I tested that locally before reporting the mistake on my side and > while it works, I just consider it.. ugly, because: > > num_clocks = >> 0 => use the externally specified num_/clocks (logical) > =0 => use the default 2 > <0 => consider there's zero > > ..but maybe that's just me.. if you don't find it ugly, I may just > go with that. Would 'lesser ugliness' count? Maybe add a define 'ICC_RPM_NO_CLOCKS = -1'? I think that spawning default bus & bus_a everywhere would be worse. When this driver is rewritten in Rust, we'll have a clear distinction between None (meaning default two clocks) and Some(0) (meaning no clocks). Wait, does that sound even more ugly?
On Fri, Feb 17, 2023 at 09:28:54PM +0100, Konrad Dybcio wrote: > > > On 17.02.2023 21:26, Dmitry Baryshkov wrote: > > On Fri, 17 Feb 2023 at 21:53, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > >> > >> > >> > >> On 17.02.2023 20:27, Konrad Dybcio wrote: > >>> > >>> > >>> On 17.02.2023 11:46, Konrad Dybcio wrote: > >>>> A0NoC only does bus scaling through RPM votes and does not have any > >>>> ICC clocks. Describe this. > >>>> > >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >>>> --- > >>> This is bad, as devm_clk_get_bulk{"", _optional} doesn't > >>> check if num_clocks makes sense and passes "-1" down the > >>> devres alloc chain.. > >>> > >>> I'll rework this for the next revision by simply assigning > >>> the common "bus", "bus_a" set everywhere instead of relying > >>> on it being there by default.. > >> Or maybe I shouldn't, as that will require redefining the array > >> over and over again.. Perhaps just passing <&xo_board>, <&xo_board> > >> to a0noc's "bus", "bus_a", similar to what's been done on SDM630's > >> GNoC would be less messy? > > > > What about simply skipping a call to devm_clk_get if num_bus_clocks is negative? > I tested that locally before reporting the mistake on my side and > while it works, I just consider it.. ugly, because: > > num_clocks = > >0 => use the externally specified num_/clocks (logical) > =0 => use the default 2 Why not let go of this "convenience" and have num_clocks actually mean number of clocks? Regards, Bjorn > <0 => consider there's zero > > ..but maybe that's just me.. if you don't find it ugly, I may just > go with that. > > Konrad > > > >> > >> Konrad > >>> > >>> Konrad > >>>> drivers/interconnect/qcom/msm8996.c | 1 + > >>>> 1 file changed, 1 insertion(+) > >>>> > >>>> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c > >>>> index 1a5e0ad36cc4..45eb8675fb11 100644 > >>>> --- a/drivers/interconnect/qcom/msm8996.c > >>>> +++ b/drivers/interconnect/qcom/msm8996.c > >>>> @@ -1817,6 +1817,7 @@ static const struct qcom_icc_desc msm8996_a0noc = { > >>>> .type = QCOM_ICC_NOC, > >>>> .nodes = a0noc_nodes, > >>>> .num_nodes = ARRAY_SIZE(a0noc_nodes), > >>>> + .num_bus_clocks = -1, /* No bus clock scaling */ > >>>> .intf_clocks = a0noc_intf_clocks, > >>>> .num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks), > >>>> .has_bus_pd = true, > >>>> > > > > > >
On 23.02.2023 15:07, Bjorn Andersson wrote: > On Fri, Feb 17, 2023 at 09:28:54PM +0100, Konrad Dybcio wrote: >> >> >> On 17.02.2023 21:26, Dmitry Baryshkov wrote: >>> On Fri, 17 Feb 2023 at 21:53, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >>>> >>>> >>>> >>>> On 17.02.2023 20:27, Konrad Dybcio wrote: >>>>> >>>>> >>>>> On 17.02.2023 11:46, Konrad Dybcio wrote: >>>>>> A0NoC only does bus scaling through RPM votes and does not have any >>>>>> ICC clocks. Describe this. >>>>>> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>>> --- >>>>> This is bad, as devm_clk_get_bulk{"", _optional} doesn't >>>>> check if num_clocks makes sense and passes "-1" down the >>>>> devres alloc chain.. >>>>> >>>>> I'll rework this for the next revision by simply assigning >>>>> the common "bus", "bus_a" set everywhere instead of relying >>>>> on it being there by default.. >>>> Or maybe I shouldn't, as that will require redefining the array >>>> over and over again.. Perhaps just passing <&xo_board>, <&xo_board> >>>> to a0noc's "bus", "bus_a", similar to what's been done on SDM630's >>>> GNoC would be less messy? >>> >>> What about simply skipping a call to devm_clk_get if num_bus_clocks is negative? >> I tested that locally before reporting the mistake on my side and >> while it works, I just consider it.. ugly, because: >> >> num_clocks = >>> 0 => use the externally specified num_/clocks (logical) >> =0 => use the default 2 > > Why not let go of this "convenience" and have num_clocks actually mean > number of clocks? Sounds good.. Should I keep the default "bus", "bus_a" on absence of .bus_clocks still, to avoid duplicating objects? Konrad > > Regards, > Bjorn > >> <0 => consider there's zero >> >> ..but maybe that's just me.. if you don't find it ugly, I may just >> go with that. >> >> Konrad >>> >>>> >>>> Konrad >>>>> >>>>> Konrad >>>>>> drivers/interconnect/qcom/msm8996.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c >>>>>> index 1a5e0ad36cc4..45eb8675fb11 100644 >>>>>> --- a/drivers/interconnect/qcom/msm8996.c >>>>>> +++ b/drivers/interconnect/qcom/msm8996.c >>>>>> @@ -1817,6 +1817,7 @@ static const struct qcom_icc_desc msm8996_a0noc = { >>>>>> .type = QCOM_ICC_NOC, >>>>>> .nodes = a0noc_nodes, >>>>>> .num_nodes = ARRAY_SIZE(a0noc_nodes), >>>>>> + .num_bus_clocks = -1, /* No bus clock scaling */ >>>>>> .intf_clocks = a0noc_intf_clocks, >>>>>> .num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks), >>>>>> .has_bus_pd = true, >>>>>> >>> >>> >>>
diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c index 1a5e0ad36cc4..45eb8675fb11 100644 --- a/drivers/interconnect/qcom/msm8996.c +++ b/drivers/interconnect/qcom/msm8996.c @@ -1817,6 +1817,7 @@ static const struct qcom_icc_desc msm8996_a0noc = { .type = QCOM_ICC_NOC, .nodes = a0noc_nodes, .num_nodes = ARRAY_SIZE(a0noc_nodes), + .num_bus_clocks = -1, /* No bus clock scaling */ .intf_clocks = a0noc_intf_clocks, .num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks), .has_bus_pd = true,
A0NoC only does bus scaling through RPM votes and does not have any ICC clocks. Describe this. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/interconnect/qcom/msm8996.c | 1 + 1 file changed, 1 insertion(+)