| Message ID | 20220128161002.2308563-3-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | interconnect: qcom: msm8939: Coalesce snoc and snoc_mm | expand |
On Fri, 28 Jan 2022 at 19:10, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > The current msm8939 snoc and snoc_mm definitions are represented as > separate entities based on downstream definition which declares two > identical and therefore overlapping mmio regions. > > Downstream: > reg = <0x580000 0x14080>, > <0x580000 0x14080>; > reg-names = "snoc-base", "snoc-mm-base"; > > Upstream: > snoc: interconnect@580000 { > compatible = "qcom,msm8939-snoc"; > #interconnect-cells = <1>; > reg = <0x580000 0x14080>; > clock-names = "bus", "bus_a"; > clocks = <&rpmcc RPM_SMD_SNOC_CLK>, > <&rpmcc RPM_SMD_SNOC_A_CLK>; > status = "okay"; > }; > > snoc: interconnect@580000 { > compatible = "qcom,msm8939-snoc_mm"; > #interconnect-cells = <1>; > reg = <0x580000 0x14080>; > clock-names = "bus", "bus_a", > clocks = <&rpmcc RPM_SMD_SYSMMNOC_CLK>, > <&rpmcc RPM_SMD_SYSMMNOC_A_CLK>; > status = "okay"; > }; > > This overlapping declaration leads to the following failure on boot. > > [ 1.212340] qnoc-msm8939 580000.interconnect_mm: can't request region for resource [mem 0x00580000-0x0059407f] > [ 1.212391] qnoc-msm8939 580000.interconnect_mm: Cannot ioremap interconnect bus resource > [ 1.221524] qnoc-msm8939: probe of 580000.interconnect_mm failed with error -16 > > snoc_mm is a complete misnomer though, as there is no distinct register > space, simply an additional clock to drive higher frequences on snoc for > new multi-media 'mm' devices tacked on to the old msm8916 snoc. > > The difference can be captured with > > - A new clock > - Performance points/clock settings in the relevant multi-media devices. > > Fix the above bug by representing snoc_mm as two new clocks to the existing > snoc, not a separate interconnect bus. This would lead to higher frequencies being set on both 'normal' and mm snoc clocks, thus (possibly) increasing power consumption. The proper fix should be implemented following patches 4 and 5 from https://lore.kernel.org/all/20211215002324.1727-1-shawn.guo@linaro.org/ > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/interconnect/qcom/msm8939.c | 30 +++++------------------------ > 1 file changed, 5 insertions(+), 25 deletions(-) > > diff --git a/drivers/interconnect/qcom/msm8939.c b/drivers/interconnect/qcom/msm8939.c > index d188f3636e4c3..7030911e25adc 100644 > --- a/drivers/interconnect/qcom/msm8939.c > +++ b/drivers/interconnect/qcom/msm8939.c > @@ -1271,25 +1271,6 @@ static struct qcom_icc_node *msm8939_snoc_nodes[] = { > [SNOC_INT_BIMC] = &snoc_int_bimc, > [SNOC_PCNOC_MAS] = &snoc_pcnoc_mas, > [SNOC_QDSS_INT] = &qdss_int, > -}; > - > -static const struct regmap_config msm8939_snoc_regmap_config = { > - .reg_bits = 32, > - .reg_stride = 4, > - .val_bits = 32, > - .max_register = 0x14080, > - .fast_io = true, > -}; > - > -static struct qcom_icc_desc msm8939_snoc = { > - .type = QCOM_ICC_NOC, > - .nodes = msm8939_snoc_nodes, > - .num_nodes = ARRAY_SIZE(msm8939_snoc_nodes), > - .regmap_cfg = &msm8939_snoc_regmap_config, > - .qos_offset = 0x7000, > -}; > - > -static struct qcom_icc_node *msm8939_snoc_mm_nodes[] = { > [MASTER_VIDEO_P0] = &mas_video, > [MASTER_JPEG] = &mas_jpeg, > [MASTER_VFE] = &mas_vfe, > @@ -1301,7 +1282,7 @@ static struct qcom_icc_node *msm8939_snoc_mm_nodes[] = { > [SNOC_MM_INT_2] = &mm_int_2, > }; > > -static const struct regmap_config msm8939_snoc_mm_regmap_config = { > +static const struct regmap_config msm8939_snoc_regmap_config = { > .reg_bits = 32, > .reg_stride = 4, > .val_bits = 32, > @@ -1309,11 +1290,11 @@ static const struct regmap_config msm8939_snoc_mm_regmap_config = { > .fast_io = true, > }; > > -static struct qcom_icc_desc msm8939_snoc_mm = { > +static struct qcom_icc_desc msm8939_snoc = { > .type = QCOM_ICC_NOC, > - .nodes = msm8939_snoc_mm_nodes, > - .num_nodes = ARRAY_SIZE(msm8939_snoc_mm_nodes), > - .regmap_cfg = &msm8939_snoc_mm_regmap_config, > + .nodes = msm8939_snoc_nodes, > + .num_nodes = ARRAY_SIZE(msm8939_snoc_nodes), > + .regmap_cfg = &msm8939_snoc_regmap_config, > .qos_offset = 0x7000, > }; > > @@ -1420,7 +1401,6 @@ static const struct of_device_id msm8939_noc_of_match[] = { > { .compatible = "qcom,msm8939-bimc", .data = &msm8939_bimc }, > { .compatible = "qcom,msm8939-pcnoc", .data = &msm8939_pcnoc }, > { .compatible = "qcom,msm8939-snoc", .data = &msm8939_snoc }, > - { .compatible = "qcom,msm8939-snoc-mm", .data = &msm8939_snoc_mm }, > { } > }; > MODULE_DEVICE_TABLE(of, msm8939_noc_of_match); > -- > 2.33.0 >
On 28/01/2022 22:24, Dmitry Baryshkov wrote: > This would lead to higher frequencies being set on both 'normal' and > mm snoc clocks, thus (possibly) increasing power consumption. > How so ? There are four clocks bus bus_a bus_mm bus_a_mm The last two clocks SNOC performance points are 0 | 19.2 | XO 1 | 50 | GPLL0 2 | 100 | GPLL0 3 | 133.3 | GPLL0 4 | 160 | GPLL0 5 | 200 | GPLL0 6 | 266.6 | GPLL0 SNOC_MM performance points are 0 | 19.2 | XO 1 | 50 | GPLL0 2 | 100 | GPLL0 3 | 133.3 | GPLL0 4 | 160 | GPLL0 5 | 200 | GPLL0 6 | 266.6 | GPLL0 7 | 320 | GPLL0 8 | 400 | GPLL0 Its GPLL0 being set, the snoc_mm clocks really just map back to GPLL0
On 28/01/2022 23:23, Bryan O'Donoghue wrote: > On 28/01/2022 22:24, Dmitry Baryshkov wrote: >> This would lead to higher frequencies being set on both 'normal' and >> mm snoc clocks, thus (possibly) increasing power consumption. >> > How so ? > > There are four clocks > > bus > bus_a > bus_mm > bus_a_mm > > The last two clocks > > SNOC performance points are > 0 | 19.2 | XO > 1 | 50 | GPLL0 > 2 | 100 | GPLL0 > 3 | 133.3 | GPLL0 > 4 | 160 | GPLL0 > 5 | 200 | GPLL0 > 6 | 266.6 | GPLL0 > > SNOC_MM performance points are > 0 | 19.2 | XO > 1 | 50 | GPLL0 > 2 | 100 | GPLL0 > 3 | 133.3 | GPLL0 > 4 | 160 | GPLL0 > 5 | 200 | GPLL0 > 6 | 266.6 | GPLL0 > 7 | 320 | GPLL0 > 8 | 400 | GPLL0 > > Its GPLL0 being set, the snoc_mm clocks really just map back to GPLL0 So taking an example If venus interconnects = <&snoc_mm MASTER_VIDEO_P0 &bimc SLAVE_EBI_CH0>; or mdp interconnects = <&snoc_mm MASTER_MDP_PORT0 &bimc SLAVE_EBI_CH0>, <&snoc_mm MASTER_MDP_PORT1 &bimc SLAVE_EBI_CH0>; wants performance point #6 GPLL0 runs at 266.6 but for performance point #7 it runs at 320 MHz Since the points all map back to a GPLL0 frequency, how does aggregating snoc and snoc_mm - result in higher power consumption ?
On 28/01/2022 23:30, Bryan O'Donoghue wrote: > On 28/01/2022 23:23, Bryan O'Donoghue wrote: >> On 28/01/2022 22:24, Dmitry Baryshkov wrote: >>> This would lead to higher frequencies being set on both 'normal' and >>> mm snoc clocks, thus (possibly) increasing power consumption. >>> >> How so ? >> >> There are four clocks >> >> bus >> bus_a >> bus_mm >> bus_a_mm >> >> The last two clocks >> >> SNOC performance points are >> 0 | 19.2 | XO >> 1 | 50 | GPLL0 >> 2 | 100 | GPLL0 >> 3 | 133.3 | GPLL0 >> 4 | 160 | GPLL0 >> 5 | 200 | GPLL0 >> 6 | 266.6 | GPLL0 >> >> SNOC_MM performance points are >> 0 | 19.2 | XO >> 1 | 50 | GPLL0 >> 2 | 100 | GPLL0 >> 3 | 133.3 | GPLL0 >> 4 | 160 | GPLL0 >> 5 | 200 | GPLL0 >> 6 | 266.6 | GPLL0 >> 7 | 320 | GPLL0 >> 8 | 400 | GPLL0 >> >> Its GPLL0 being set, the snoc_mm clocks really just map back to GPLL0 > > So taking an example > > If venus > interconnects = <&snoc_mm MASTER_VIDEO_P0 &bimc SLAVE_EBI_CH0>; > > or mdp > interconnects = <&snoc_mm MASTER_MDP_PORT0 &bimc SLAVE_EBI_CH0>, > <&snoc_mm MASTER_MDP_PORT1 &bimc SLAVE_EBI_CH0>; > > wants performance point #6 GPLL0 runs at 266.6 but for performance point > #7 it runs at 320 MHz > > Since the points all map back to a GPLL0 frequency, how does aggregating > snoc and snoc_mm - result in higher power consumption ? Anyway - thanks for the pointer to the virt clock. I don't mind re-implementing the fix this way given there's an established upstream canon. --- bod
On Sat, 29 Jan 2022 at 02:23, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 28/01/2022 22:24, Dmitry Baryshkov wrote: > > This would lead to higher frequencies being set on both 'normal' and > > mm snoc clocks, thus (possibly) increasing power consumption. > > > How so ? If I remember correctly, bus clocks are set to max(sum(avg_bw), max(peak_bw)) calculated over all bandwidth paths (nodes). If you merge snoc and snoc_mm, the resulting sum(avg_bw) would be a sum of (older) snoc's and snoc_mm's sums. Thus the bus clocks (both bus and bus_mm) would be set to higher frequencies. > > There are four clocks > > bus > bus_a > bus_mm > bus_a_mm > > The last two clocks > > SNOC performance points are > 0 | 19.2 | XO > 1 | 50 | GPLL0 > 2 | 100 | GPLL0 > 3 | 133.3 | GPLL0 > 4 | 160 | GPLL0 > 5 | 200 | GPLL0 > 6 | 266.6 | GPLL0 > > SNOC_MM performance points are > 0 | 19.2 | XO > 1 | 50 | GPLL0 > 2 | 100 | GPLL0 > 3 | 133.3 | GPLL0 > 4 | 160 | GPLL0 > 5 | 200 | GPLL0 > 6 | 266.6 | GPLL0 > 7 | 320 | GPLL0 > 8 | 400 | GPLL0 > > Its GPLL0 being set, the snoc_mm clocks really just map back to GPLL0
diff --git a/drivers/interconnect/qcom/msm8939.c b/drivers/interconnect/qcom/msm8939.c index d188f3636e4c3..7030911e25adc 100644 --- a/drivers/interconnect/qcom/msm8939.c +++ b/drivers/interconnect/qcom/msm8939.c @@ -1271,25 +1271,6 @@ static struct qcom_icc_node *msm8939_snoc_nodes[] = { [SNOC_INT_BIMC] = &snoc_int_bimc, [SNOC_PCNOC_MAS] = &snoc_pcnoc_mas, [SNOC_QDSS_INT] = &qdss_int, -}; - -static const struct regmap_config msm8939_snoc_regmap_config = { - .reg_bits = 32, - .reg_stride = 4, - .val_bits = 32, - .max_register = 0x14080, - .fast_io = true, -}; - -static struct qcom_icc_desc msm8939_snoc = { - .type = QCOM_ICC_NOC, - .nodes = msm8939_snoc_nodes, - .num_nodes = ARRAY_SIZE(msm8939_snoc_nodes), - .regmap_cfg = &msm8939_snoc_regmap_config, - .qos_offset = 0x7000, -}; - -static struct qcom_icc_node *msm8939_snoc_mm_nodes[] = { [MASTER_VIDEO_P0] = &mas_video, [MASTER_JPEG] = &mas_jpeg, [MASTER_VFE] = &mas_vfe, @@ -1301,7 +1282,7 @@ static struct qcom_icc_node *msm8939_snoc_mm_nodes[] = { [SNOC_MM_INT_2] = &mm_int_2, }; -static const struct regmap_config msm8939_snoc_mm_regmap_config = { +static const struct regmap_config msm8939_snoc_regmap_config = { .reg_bits = 32, .reg_stride = 4, .val_bits = 32, @@ -1309,11 +1290,11 @@ static const struct regmap_config msm8939_snoc_mm_regmap_config = { .fast_io = true, }; -static struct qcom_icc_desc msm8939_snoc_mm = { +static struct qcom_icc_desc msm8939_snoc = { .type = QCOM_ICC_NOC, - .nodes = msm8939_snoc_mm_nodes, - .num_nodes = ARRAY_SIZE(msm8939_snoc_mm_nodes), - .regmap_cfg = &msm8939_snoc_mm_regmap_config, + .nodes = msm8939_snoc_nodes, + .num_nodes = ARRAY_SIZE(msm8939_snoc_nodes), + .regmap_cfg = &msm8939_snoc_regmap_config, .qos_offset = 0x7000, }; @@ -1420,7 +1401,6 @@ static const struct of_device_id msm8939_noc_of_match[] = { { .compatible = "qcom,msm8939-bimc", .data = &msm8939_bimc }, { .compatible = "qcom,msm8939-pcnoc", .data = &msm8939_pcnoc }, { .compatible = "qcom,msm8939-snoc", .data = &msm8939_snoc }, - { .compatible = "qcom,msm8939-snoc-mm", .data = &msm8939_snoc_mm }, { } }; MODULE_DEVICE_TABLE(of, msm8939_noc_of_match);
The current msm8939 snoc and snoc_mm definitions are represented as separate entities based on downstream definition which declares two identical and therefore overlapping mmio regions. Downstream: reg = <0x580000 0x14080>, <0x580000 0x14080>; reg-names = "snoc-base", "snoc-mm-base"; Upstream: snoc: interconnect@580000 { compatible = "qcom,msm8939-snoc"; #interconnect-cells = <1>; reg = <0x580000 0x14080>; clock-names = "bus", "bus_a"; clocks = <&rpmcc RPM_SMD_SNOC_CLK>, <&rpmcc RPM_SMD_SNOC_A_CLK>; status = "okay"; }; snoc: interconnect@580000 { compatible = "qcom,msm8939-snoc_mm"; #interconnect-cells = <1>; reg = <0x580000 0x14080>; clock-names = "bus", "bus_a", clocks = <&rpmcc RPM_SMD_SYSMMNOC_CLK>, <&rpmcc RPM_SMD_SYSMMNOC_A_CLK>; status = "okay"; }; This overlapping declaration leads to the following failure on boot. [ 1.212340] qnoc-msm8939 580000.interconnect_mm: can't request region for resource [mem 0x00580000-0x0059407f] [ 1.212391] qnoc-msm8939 580000.interconnect_mm: Cannot ioremap interconnect bus resource [ 1.221524] qnoc-msm8939: probe of 580000.interconnect_mm failed with error -16 snoc_mm is a complete misnomer though, as there is no distinct register space, simply an additional clock to drive higher frequences on snoc for new multi-media 'mm' devices tacked on to the old msm8916 snoc. The difference can be captured with - A new clock - Performance points/clock settings in the relevant multi-media devices. Fix the above bug by representing snoc_mm as two new clocks to the existing snoc, not a separate interconnect bus. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/interconnect/qcom/msm8939.c | 30 +++++------------------------ 1 file changed, 5 insertions(+), 25 deletions(-)