pmdomain: mediatek: Fix power domain count
PATCH 1/5 pmdomain: mediatek: Fix power domain count
The wrong value of the number of domains is wrong which leads to failures when trying to enumerate nested power domains.
PM: genpd_xlate_onecell: invalid domain index 0
PM: genpd_xlate_onecell: invalid domain index 1
PM: genpd_xlate_onecell: invalid domain index 3
PM: genpd_xlate_onecell: invalid domain index 4
PM: genpd_xlate_onecell: invalid domain index 5
PM: genpd_xlate_onecell: invalid domain index 13
PM: genpd_xlate_onecell: invalid domain index 14
Attempts to use these power domains fail, so fix this by using the correct value of calculated power domains.
--- drivers/pmdomain/mediatek/mtk-pm-domains.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pmdomain/mediatek/mtk-pm-domains.c +1 -1
@@ -1228,7 +1228,7 @@ static int scpsys_probe(struct platform_device *pdev) scpsys->soc_data = soc; scpsys->pd_data.domains = scpsys->domains; - scpsys->pd_data.num_domains = soc->num_domains; + scpsys->pd_data.num_domains = num_domains; parent = dev->parent;if (!parent) {
Signature · 1 line
2.51.0
PATCH 2/5 clk: mediatek: Fix MT8196 topckgen2 orphan clocks
There are a few clocks with names spelled incorrecly, so when a child tries to associate itself to a parent, there isn't a parent. Fixing the names restores the proper parent-child relations of these clocks and eliminates the orphaned clocks.
--- drivers/clk/mediatek/clk-mt8196-topckgen2.c | 92 ++++++++++----------- 1 file changed, 46 insertions(+), 46 deletions(-)
drivers/clk/mediatek/clk-mt8196-topckgen2.c +46 -46
@@ -147,13 +147,13 @@ static const struct mtk_fixed_factor top_divs[] = {static const char *const seninf_parents[] = {"clk26m" , -"ck_osc_d10" , -"ck_osc_d8" , -"ck_osc_d5" , -"ck_osc_d4" , +"osc_d10" , +"osc_d8" , +"osc_d5" , +"osc_d4" ,"univpll2_d6_d2" ,"mainpll2_d9" , -"ck_osc_d2" , +"osc_d2" ,"mainpll2_d4_d2" ,"univpll2_d4_d2" ,"mmpll2_d4_d2" ,
@@ -166,10 +166,10 @@ static const char * const seninf_parents[] = {static const char *const img1_parents[] = {"clk26m" , -"ck_osc_d4" , -"ck_osc_d3" , +"osc_d4" , +"osc_d3" ,"mmpll2_d6_d2" , -"ck_osc_d2" , +"osc_d2" ,"imgpll_d5_d2" ,"mmpll2_d5_d2" ,"univpll2_d4_d2" ,
@@ -185,24 +185,24 @@ static const char * const img1_parents[] = {static const char *const ipe_parents[] = {"clk26m" , -"ck_osc_d4" , -"ck_osc_d3" , -"ck_osc_d2" , +"osc_d4" , +"osc_d3" , +"osc_d2" ,"univpll2_d6" ,"mmpll2_d6" ,"univpll2_d5" ,"imgpll_d5" , -"ck_mainpll_d4" , +"mainpll_d4" ,"mmpll2_d5" ,"imgpll_d4" };static const char *const cam_parents[] = {"clk26m" , -"ck_osc_d10" , -"ck_osc_d4" , -"ck_osc_d3" , -"ck_osc_d2" , +"osc_d10" , +"osc_d4" , +"osc_d3" , +"osc_d2" ,"mmpll2_d5_d2" ,"univpll2_d4_d2" ,"univpll2_d7" ,
@@ -219,8 +219,8 @@ static const char * const cam_parents[] = {static const char *const camtm_parents[] = {"clk26m" ,"univpll2_d6_d4" , -"ck_osc_d4" , -"ck_osc_d3" , +"osc_d4" , +"osc_d3" ,"univpll2_d6_d2" };
@@ -239,7 +239,7 @@ static const char * const dpe_parents[] = {static const char *const vdec_parents[] = {"clk26m" , -"ck_mainpll_d5_d2" , +"mainpll_d5_d2" ,"mainpll2_d4_d4" ,"mainpll2_d7_d2" ,"mainpll2_d6_d2" ,
@@ -256,9 +256,9 @@ static const char * const vdec_parents[] = {static const char *const ccusys_parents[] = {"clk26m" , -"ck_osc_d4" , -"ck_osc_d3" , -"ck_osc_d2" , +"osc_d4" , +"osc_d3" , +"osc_d2" ,"mmpll2_d5_d2" ,"univpll2_d4_d2" ,"mmpll2_d7" ,
@@ -273,8 +273,8 @@ static const char * const ccusys_parents[] = {static const char *const ccutm_parents[] = {"clk26m" ,"univpll2_d6_d4" , -"ck_osc_d4" , -"ck_osc_d3" , +"osc_d4" , +"osc_d3" ,"univpll2_d6_d2" };
@@ -309,14 +309,14 @@ static const char * const dp0_parents[] = {"tvdpll1_d16" ,"tvdpll1_d8" ,"tvdpll1_d4" , -"ck_tvdpll1_d2" +"tvdpll1_d2" };static const char *const disp_parents[] = {"clk26m" , -"ck_mainpll_d5_d2" , -"ck_mainpll_d4_d2" , -"ck_mainpll_d6" , +"mainpll_d5_d2" , +"mainpll_d4_d2" , +"mainpll_d6" ,"mainpll2_d5" ,"mmpll2_d6" , "mainpll2_d4",
@@ -326,7 +326,7 @@ static const char * const disp_parents[] = {static const char *const mdp_parents[] = {"clk26m" , -"ck_mainpll_d5_d2" , +"mainpll_d5_d2" ,"mainpll2_d5_d2" ,"mmpll2_d6_d2" ,"mainpll2_d9" ,
@@ -342,13 +342,13 @@ static const char * const mdp_parents[] = {static const char *const mminfra_parents[] = {"clk26m" , -"ck_osc_d4" , -"ck_mainpll_d7_d2" , -"ck_mainpll_d5_d2" , -"ck_mainpll_d9" , +"osc_d4" , +"mainpll_d7_d2" , +"mainpll_d5_d2" , +"mainpll_d9" ,"mmpll2_d6_d2" ,"mainpll2_d4_d2" , -"ck_mainpll_d6" , +"mainpll_d6" ,"univpll2_d6" ,"mainpll2_d5" ,"mmpll2_d6" ,
@@ -361,14 +361,14 @@ static const char * const mminfra_parents[] = {static const char *const mminfra_snoc_parents[] = {"clk26m" , -"ck_osc_d4" , -"ck_mainpll_d7_d2" , -"ck_mainpll_d9" , -"ck_mainpll_d7" , -"ck_mainpll_d6" , +"osc_d4" , +"mainpll_d7_d2" , +"mainpll_d9" , +"mainpll_d7" , +"mainpll_d6" ,"mmpll2_d4_d2" , -"ck_mainpll_d5" , -"ck_mainpll_d4" , +"mainpll_d5" , +"mainpll_d4" ,"univpll2_d4" ,"mmpll2_d4" ,"mainpll2_d3" ,
@@ -381,17 +381,17 @@ static const char * const mmup_parents[] = {"clk26m" ,"mainpll2_d6" ,"mainpll2_d5" , -"ck_osc_d2" , -"ck_osc" , -"ck_mainpll_d4" , +"osc_d2" , +"ulposc" , +"mainpll_d4" ,"univpll2_d4" ,"mainpll2_d3" };static const char *const mminfra_ao_parents[] = {"clk26m" , -"ck_osc_d4" , -"ck_mainpll_d3" +"osc_d4" , +"mainpll_d3" }; static const char * const dvo_parents[] = {
Signature · 1 line
2.51.0
Re: [PATCH 2/5] clk: mediatek: Fix MT8196 topckgen2 orphan clocks
Hi,
Quoted 7 lines · Adam Ford
There are a few clocks with names spelled incorrecly, so when a child tries to associate itself to a parent, there isn't a parent. Fixing the names restores the proper parent-child relations of these clocks and eliminates the orphaned clocks.
Fixes b093e0f17099 ("clk: mediatek: Add MT8196 topckgen2 clock support")Signed-off-by Adam Ford <[email protected]>
I intended to drop the ck_ prefix from the clock names in the original driver submission, but it appears I missed some of them.
Thanks!
Laura
PATCH 3/5 clk: mediatek: Fix MT8196 topckgen orphan clock
In reply to Adam FordThere is a clock with its name spelled incorrecly, so when a child tries to associate itself to a parent, there isn't a parent. Fixing the name restores the proper parent-child relations of this clocks and eliminates the orphaned clock.
--- drivers/clk/mediatek/clk-mt8196-topckgen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/clk/mediatek/clk-mt8196-topckgen.c +1 -1
@@ -324,7 +324,7 @@ static const char * const emi_parents[] = {"mainpll_d5_d8" ,"mainpll_d5_d4" ,"mainpll_d4_d4" , -"emipll1_ck" +"emipll" }; static const char * const ap2conn_host_parents[] = {
Signature · 1 line
2.51.0
Re: [PATCH 3/5] clk: mediatek: Fix MT8196 topckgen orphan clock
Quoted 16 lines · Adam Ford
There is a clock with its name spelled incorrecly, so when a child tries to associate itself to a parent, there isn't a parent. Fixing the name restores the proper parent-child relations of this clocks and eliminates the orphaned clock.
Fixes 895ab0134d64 ("clk: mediatek: Add MT8196 topckgen clock support")Signed-off-by Adam Ford <[email protected]>--- drivers/clk/mediatek/clk-mt8196-topckgen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/clk/mediatek/clk-mt8196-topckgen.c +0
-0
drivers/clk/mediatek/clk-mt8196-topckgen.c static const char * const emi_parents[] = { · line 330
Quoted 8 lines
drivers/clk/mediatek/clk-mt8196-topckgen.c +1
-1@@ -324,7 +324,7 @@ static const char * const emi_parents[] = {"mainpll_d5_d8" ,"mainpll_d5_d4" ,"mainpll_d4_d4" , -"emipll1_ck" +"emipll" }; static const char * const ap2conn_host_parents[] = {
PATCH 4/5 regulator: mt6363: Fix interrmittent timeout
In reply to Adam FordSometimes, the mt6363 regulator would fail to initialize and return with a TIMEOUT error, so add an extra instruction to wake up the bus before issuing the commands.
--- drivers/regulator/mt6363-regulator.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/regulator/mt6363-regulator.c +8 -1
@@ -861,7 +861,7 @@ static int mt6363_regulator_probe(struct platform_device *pdev)struct irq_domain *domain;struct irq_fwspec fwspec;struct spmi_device *sdev; -int i, ret; +int i, ret, val; config.regmap =mt6363_spmi_register_regmap (dev);if (IS_ERR (config.regmap))
@@ -870,6 +870,13 @@ static int mt6363_regulator_probe(struct platform_device *pdev) config.dev = dev; sdev =to_spmi_device (dev->parent); +/* +* The first read may fail if the bootloader sets sleep mode: wake up +* this PMIC with W/R on the SPMI bus and ignore the first result. +* This matches the MT6373 driver behavior. +*/ +regmap_read (config.regmap,MT6363_TOP_TRAP , &val); + interrupt_parent =of_irq_find_parent (dev->of_node);if (!interrupt_parent)return dev_err_probe (dev, -EINVAL ,"Cannot find IRQ parent\n" );
Signature · 1 line
2.51.0
Mark Brown
maintainerRe: [PATCH 4/5] regulator: mt6363: Fix interrmittent timeout
Quoted 3 lines · Adam Ford
Sometimes, the mt6363 regulator would fail to initialize and return with a TIMEOUT error, so add an extra instruction to wake up the bus before issuing the commands.
Please don't mix patches for multiple subsystems in a single series if there's no dependencies, it just makes it look like there's dependencies and makes it more complicated to get things applied. You should also supply a cover letter.
PATCH 5/5 drivers: clk: mediatek: Fix error finding regmap
In reply to Adam FordThe clock driver for clk-mt8196-vdisp-ao doesn't use the same common clk functions that other clocks use. As such, this clock returns an error:
Cannot find regmap for /soc: -ENOMEM
clk-mt8196-vdisp-ao 3e800000.syscon: probe with driver clk-mt8196-vdisp-ao failed with error -12
Fix this by using the common clock calls. With this patch, the following new clocks properly enumerate:
mm_v_disp_vdisp_ao_config
mm_v_disp_dpc
mm_v_smi_sub_somm0
--- drivers/clk/mediatek/clk-mt8196-vdisp_ao.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clk/mediatek/clk-mt8196-vdisp_ao.c +2 -2
@@ -67,8 +67,8 @@ static const struct of_device_id of_match_clk_mt8196_vdisp_ao[] = {MODULE_DEVICE_TABLE (of, of_match_clk_mt8196_vdisp_ao);static struct platform_driver clk_mt8196_vdisp_ao_drv = { - .probe = mtk_clk_pdev_probe, - .remove = mtk_clk_pdev_remove, + .probe = mtk_clk_simple_probe, + .remove = mtk_clk_simple_remove, .driver = { .name ="clk-mt8196-vdisp-ao" , .of_match_table = of_match_clk_mt8196_vdisp_ao,
Signature · 1 line
2.51.0
Re: [PATCH 5/5] drivers: clk: mediatek: Fix error finding regmap
Hi,
Quoted 24 lines · Adam Ford
The clock driver for clk-mt8196-vdisp-ao doesn't use the same common clk functions that other clocks use. As such, this clock returns an error:
Cannot find regmap for /soc:
-ENOMEM
clk-mt8196-vdisp-ao 3e800000.syscon: probe with driver clk-mt8196-vdisp-ao failed with error -12Fix this by using the common clock calls. With this patch, the following new clocks properly enumerate:
mm_v_disp_vdisp_ao_config
mm_v_disp_dpc
mm_v_smi_sub_somm0Fixes d4fb7e15a520 ("clk: mediatek: Add MT8196 disp-ao clock support")Signed-off-by Adam Ford <[email protected]>--- drivers/clk/mediatek/clk-mt8196-vdisp_ao.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/clk/mediatek/clk-mt8196-vdisp_ao.c +0
-0
drivers/clk/mediatek/clk-mt8196-vdisp_ao.c static const struct of_device_id of_match_clk_mt8196_vdisp_ao[] = { · line 74
Quoted 10 lines
drivers/clk/mediatek/clk-mt8196-vdisp_ao.c +2
-2@@ -67,8 +67,8 @@ static const struct of_device_id of_match_clk_mt8196_vdisp_ao[] = {MODULE_DEVICE_TABLE (of, of_match_clk_mt8196_vdisp_ao);static struct platform_driver clk_mt8196_vdisp_ao_drv = { - .probe = mtk_clk_pdev_probe, - .remove = mtk_clk_pdev_remove, + .probe = mtk_clk_simple_probe, + .remove = mtk_clk_simple_remove, .driver = { .name ="clk-mt8196-vdisp-ao" , .of_match_table = of_match_clk_mt8196_vdisp_ao,
This driver is actually expected to be registered by the mtk-mmsys driver via platform_device_register_data() (related changes are in a pending series [1]), which is why it is probed via mtk_clk_pdev_probe.
[1] https://lore.kernel.org/all/[email protected]/
Best,
Laura
Mark Brown
maintainerRe: (subset) [PATCH 1/5] pmdomain: mediatek: Fix power domain count
In reply to Adam FordQuoted 11 lines · Adam Ford
The wrong value of the number of domains is wrong which leads to failures when trying to enumerate nested power domains.
PM:
genpd_xlate_onecell: invalid domain index 0
PM:genpd_xlate_onecell: invalid domain index 1
PM:genpd_xlate_onecell: invalid domain index 3
PM:genpd_xlate_onecell: invalid domain index 4
PM:genpd_xlate_onecell: invalid domain index 5
PM:genpd_xlate_onecell: invalid domain index 13
PM:genpd_xlate_onecell: invalid domain index 14
Quoted 1 line
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[4/5] regulator: mt6363: Fix interrmittent timeout
commit: 1a4b0c999101b2532723f9bd9818b70ffa7580f4
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks,
Mark
Quoted 12 lines · Adam Ford
The wrong value of the number of domains is wrong which leads to failures when trying to enumerate nested power domains.
PM:
genpd_xlate_onecell: invalid domain index 0
PM:genpd_xlate_onecell: invalid domain index 1
PM:genpd_xlate_onecell: invalid domain index 3
PM:genpd_xlate_onecell: invalid domain index 4
PM:genpd_xlate_onecell: invalid domain index 5
PM:genpd_xlate_onecell: invalid domain index 13
PM:genpd_xlate_onecell: invalid domain index 14
Quoted 4 lines
Attempts to use these power domains fail, so fix this by using the correct value of calculated power domains.
Signed-off-by Adam Ford <[email protected]>
We should have a fixes tag for this too I think:
Voter power domains")
Quoted 9 lines
--- drivers/pmdomain/mediatek/mtk-pm-domains.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pmdomain/mediatek/mtk-pm-domains.c +0
-0
drivers/pmdomain/mediatek/mtk-pm-domains.c static int scpsys_probe(struct platform_device *pdev) · line 1231
Quoted 5 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c +1
-1@@ -1228,7 +1228,7 @@ static int scpsys_probe(struct platform_device *pdev) scpsys->soc_data = soc; scpsys->pd_data.domains = scpsys->domains; - scpsys->pd_data.num_domains = soc->num_domains; + scpsys->pd_data.num_domains = num_domains;
Not sure this is the complete fix, as scpsys_add_one_domain() seems to be using the wrong value of "num_domains" too, no?
Quoted 6 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c +0
-1parent = dev->parent;if (!parent) { --2.51.0
Kind regards
Uffe
Il 12/02/26 12:34, Ulf Hansson ha scritto:
Quoted 35 lines · Adam Ford
Quoted 12 lines
The wrong value of the number of domains is wrong which leads to failures when trying to enumerate nested power domains.
PM:
genpd_xlate_onecell: invalid domain index 0
PM:genpd_xlate_onecell: invalid domain index 1
PM:genpd_xlate_onecell: invalid domain index 3
PM:genpd_xlate_onecell: invalid domain index 4
PM:genpd_xlate_onecell: invalid domain index 5
PM:genpd_xlate_onecell: invalid domain index 13
PM:genpd_xlate_onecell: invalid domain index 14Quoted 4 lines
Attempts to use these power domains fail, so fix this by using the correct value of calculated power domains.
Signed-off-by Adam Ford <[email protected]>We should have a fixes tag for this too I think:
Fixes 88914db077b6 ("pmdomain: mediatek: Add support for HardwareVoter power domains")
Quoted 11 lines
--- drivers/pmdomain/mediatek/mtk-pm-domains.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pmdomain/mediatek/mtk-pm-domains.c +0
-0@@ -1228,7 +1228,7 @@ static int scpsys_probe(struct platform_device *pdev) scpsys->soc_data = soc;
Quoted 7 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c static int scpsys_probe(struct platform_device *pdev) · line 1231
Quoted 3 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c +1
-1scpsys->pd_data.domains = scpsys->domains; - scpsys->pd_data.num_domains = soc->num_domains; + scpsys->pd_data.num_domains = num_domains;Not sure this is the complete fix, as
scpsys_add_one_domain()seems to be using the wrong value of "num_domains" too, no?
No, scpsys_add_one_domain() uses num_domains from the soc_data for DIRECT_CTL, which is expected. Maybe that can be improved, but it's how it is supposed to work.
So yeah, this patch is resolving an issue, and it is a complete fix.
Quoted 9 lines
Quoted 6 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c +0
-1parent = dev->parent;if (!parent) { --2.51.0
Kind regards
Uffe
Quoted 45 lines · AngeloGioacchino Del Regno
Il 12/02/26 12:34, Ulf Hansson ha scritto:
Quoted 35 lines · Adam Ford
Quoted 12 lines
The wrong value of the number of domains is wrong which leads to failures when trying to enumerate nested power domains.
PM:
genpd_xlate_onecell: invalid domain index 0
PM:genpd_xlate_onecell: invalid domain index 1
PM:genpd_xlate_onecell: invalid domain index 3
PM:genpd_xlate_onecell: invalid domain index 4
PM:genpd_xlate_onecell: invalid domain index 5
PM:genpd_xlate_onecell: invalid domain index 13
PM:genpd_xlate_onecell: invalid domain index 14Quoted 4 lines
Attempts to use these power domains fail, so fix this by using the correct value of calculated power domains.
Signed-off-by Adam Ford <[email protected]>We should have a fixes tag for this too I think:
Fixes 88914db077b6 ("pmdomain: mediatek: Add support for HardwareVoter power domains")
Quoted 11 lines
--- drivers/pmdomain/mediatek/mtk-pm-domains.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pmdomain/mediatek/mtk-pm-domains.c +0
-0@@ -1228,7 +1228,7 @@ static int scpsys_probe(struct platform_device *pdev) scpsys->soc_data = soc;Quoted 7 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c static int scpsys_probe(struct platform_device *pdev) · line 1231
Quoted 3 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c +1
-1scpsys->pd_data.domains = scpsys->domains; - scpsys->pd_data.num_domains = soc->num_domains; + scpsys->pd_data.num_domains = num_domains;Not sure this is the complete fix, as
scpsys_add_one_domain()seems to be using the wrong value of "num_domains" too, no?
Quoted 4 lines
No,
scpsys_add_one_domain()usesnum_domainsfrom thesoc_dataforDIRECT_CTL, which is expected. Maybe that can be improved, but it's how it is supposed to work.So yeah, this patch is resolving an issue, and it is a complete fix.
I had a closer look and unfortunately, it still looks weird to me.
More precisely, we are allocating and registering a number of genpds, that corresponds to the sum of "soc->num_domains + soc->num_hwv_domains". The index each genpd gets in the array (struct genpd_onecell_data) must correspond to the index specified in DT, for both a consumer-node and a child-domain-node, right?
It seems like adding them dynamically changes the index. In other words, the index specified using the "reg" property in a child-domain-node, may not match what a consumer-node should specify in its "power-domains" property. Isn't that a problem? Perhaps not, because we never have both "scpsys_domain_data" and "scpsys_hwv_domain_data", but then why are we adding them in ->probe() with "soc->num_domains + soc->num_hwv_domains"?
[...]
Kind regards
Uffe
AngeloGioacchino Del Regno
maintainerIl 11/03/26 18:29, Ulf Hansson ha scritto:
Quoted 60 lines
Quoted 45 lines · AngeloGioacchino Del Regno
Il 12/02/26 12:34, Ulf Hansson ha scritto:
Quoted 35 lines · Adam Ford
Quoted 12 lines
The wrong value of the number of domains is wrong which leads to failures when trying to enumerate nested power domains.
PM:
genpd_xlate_onecell: invalid domain index 0
PM:genpd_xlate_onecell: invalid domain index 1
PM:genpd_xlate_onecell: invalid domain index 3
PM:genpd_xlate_onecell: invalid domain index 4
PM:genpd_xlate_onecell: invalid domain index 5
PM:genpd_xlate_onecell: invalid domain index 13
PM:genpd_xlate_onecell: invalid domain index 14Quoted 4 lines
Attempts to use these power domains fail, so fix this by using the correct value of calculated power domains.
Signed-off-by Adam Ford <[email protected]>We should have a fixes tag for this too I think:
Fixes 88914db077b6 ("pmdomain: mediatek: Add support for HardwareVoter power domains")
Quoted 11 lines
--- drivers/pmdomain/mediatek/mtk-pm-domains.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pmdomain/mediatek/mtk-pm-domains.c +0
-0@@ -1228,7 +1228,7 @@ static int scpsys_probe(struct platform_device *pdev) scpsys->soc_data = soc;Quoted 7 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c static int scpsys_probe(struct platform_device *pdev) · line 1231
Quoted 3 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c +1
-1scpsys->pd_data.domains = scpsys->domains; - scpsys->pd_data.num_domains = soc->num_domains; + scpsys->pd_data.num_domains = num_domains;Not sure this is the complete fix, as
scpsys_add_one_domain()seems to be using the wrong value of "num_domains" too, no?Quoted 4 lines
No,
scpsys_add_one_domain()usesnum_domainsfrom thesoc_dataforDIRECT_CTL, which is expected. Maybe that can be improved, but it's how it is supposed to work.So yeah, this patch is resolving an issue, and it is a complete fix.
I had a closer look and unfortunately, it still looks weird to me.
More precisely, we are allocating and registering a number of genpds, that corresponds to the sum of "
soc->num_domains+soc->num_hwv_domains". The index each genpd gets in the array (structgenpd_onecell_data) must correspond to the index specified in DT, for both a consumer-node and a child-domain-node, right?
Quoted 8 lines
It seems like adding them dynamically changes the index. In other words, the index specified using the "reg" property in a child-domain-node, may not match what a consumer-node should specify in its "power-domains" property. Isn't that a problem? Perhaps not, because we never have both "
scpsys_domain_data" and "scpsys_hwv_domain_data", but then why are we adding them in ->probe() with "soc->num_domains+soc->num_hwv_domains"?
As you understood, having both is not really supported right now, and the intention there was to have the flexibility to do so without restructuring the entire thing in the future.
Though, from what I understand, the confusion comes from a single line (and reading it again, I do agree, because I got confused myself for a moment as well, and I'm the one who wrote this, so that's bad bad bad):
num_domains = soc->num_domains + soc->num_hwv_domains;
where, to avoid confusion, this should've been:
num_domains = soc->num_domains ? soc->num_domains : soc->num_hwv_domains;
(perhaps with a check `if (num_domains && num_hwv_domains) dev_err(only one
supported)` but being this taken from hardcoded data, I'm not really for it)
...and still, like the proposed fix does:
scpsys->pd_data.num_domains = num_domains;
While I agree, again, in that the code could be improved (and it shall be), how the logic behaves, right now, with the proposed fix, still looks good to me.
I can eventually just send a patch that clarifies the num_domains if you prefer.
Cheers,
Angelo
Quoted 4 lines
[...]
Kind regards
Uffe
Quoted 71 lines · AngeloGioacchino Del Regno
Il 11/03/26 18:29, Ulf Hansson ha scritto:
Quoted 60 lines
Quoted 45 lines · AngeloGioacchino Del Regno
Il 12/02/26 12:34, Ulf Hansson ha scritto:
Quoted 35 lines · Adam Ford
Quoted 12 lines
The wrong value of the number of domains is wrong which leads to failures when trying to enumerate nested power domains.
PM:
genpd_xlate_onecell: invalid domain index 0
PM:genpd_xlate_onecell: invalid domain index 1
PM:genpd_xlate_onecell: invalid domain index 3
PM:genpd_xlate_onecell: invalid domain index 4
PM:genpd_xlate_onecell: invalid domain index 5
PM:genpd_xlate_onecell: invalid domain index 13
PM:genpd_xlate_onecell: invalid domain index 14Quoted 4 lines
Attempts to use these power domains fail, so fix this by using the correct value of calculated power domains.
Signed-off-by Adam Ford <[email protected]>We should have a fixes tag for this too I think:
Fixes 88914db077b6 ("pmdomain: mediatek: Add support for HardwareVoter power domains")
Quoted 11 lines
--- drivers/pmdomain/mediatek/mtk-pm-domains.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pmdomain/mediatek/mtk-pm-domains.c +0
-0@@ -1228,7 +1228,7 @@ static int scpsys_probe(struct platform_device *pdev) scpsys->soc_data = soc;Quoted 7 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c static int scpsys_probe(struct platform_device *pdev) · line 1231
Quoted 3 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c +1
-1scpsys->pd_data.domains = scpsys->domains; - scpsys->pd_data.num_domains = soc->num_domains; + scpsys->pd_data.num_domains = num_domains;Not sure this is the complete fix, as
scpsys_add_one_domain()seems to be using the wrong value of "num_domains" too, no?Quoted 4 lines
No,
scpsys_add_one_domain()usesnum_domainsfrom thesoc_dataforDIRECT_CTL, which is expected. Maybe that can be improved, but it's how it is supposed to work.So yeah, this patch is resolving an issue, and it is a complete fix.
I had a closer look and unfortunately, it still looks weird to me.
More precisely, we are allocating and registering a number of genpds, that corresponds to the sum of "
soc->num_domains+soc->num_hwv_domains". The index each genpd gets in the array (structgenpd_onecell_data) must correspond to the index specified in DT, for both a consumer-node and a child-domain-node, right?Quoted 8 lines
It seems like adding them dynamically changes the index. In other words, the index specified using the "reg" property in a child-domain-node, may not match what a consumer-node should specify in its "power-domains" property. Isn't that a problem? Perhaps not, because we never have both "
scpsys_domain_data" and "scpsys_hwv_domain_data", but then why are we adding them in ->probe() with "soc->num_domains+soc->num_hwv_domains"?
Quoted 3 lines
As you understood, having both is not really supported right now, and the intention there was to have the flexibility to do so without restructuring the entire thing in the future.
Thanks for clarifying!
Although, having both seems problematic for the reasons I pointed out above. At least, it looks like some more re-work will be needed to support that case.
Quoted 11 lines
Though, from what I understand, the confusion comes from a single line (and reading it again, I do agree, because I got confused myself for a moment as well, and I'm the one who wrote this, so that's bad bad bad):
num_domains = soc->num_domains + soc->num_hwv_domains;where, to avoid confusion, this should've been:
num_domains = soc->num_domains ? soc->num_domains : soc->num_hwv_domains;
Agreed.
Quoted 3 lines
(perhaps with a check `if (
num_domains&&num_hwv_domains) dev_err(only one
supported)` but being this taken from hardcoded data, I'm not really for it)
Agreed, a print would be superfluous.
Quoted 8 lines
...and still, like the proposed fix does:
scpsys->pd_data.num_domains = num_domains;While I agree, again, in that the code could be improved (and it shall be), how the logic behaves, right now, with the proposed fix, still looks good to me.
Okay, I'm queuing the $subject patch as a fix and by adding a stable/fixes-tag.
Quoted 2 lines
I can eventually just send a patch that clarifies the
num_domainsif you prefer.
Well, actually, I would suggest splitting the driver into two drivers (each handling its own type of match data) and adding some shared helper functions, that both drivers can use.
It's more work, but we should end up with more maintainable code, I think.
What do you think?
[...]
Kind regards
Uffe
AngeloGioacchino Del Regno
maintainerIl 12/03/26 15:42, Ulf Hansson ha scritto:
Quoted 73 lines
Quoted 71 lines · AngeloGioacchino Del Regno
Il 11/03/26 18:29, Ulf Hansson ha scritto:
Quoted 68 lines
Quoted 45 lines · AngeloGioacchino Del Regno
Il 12/02/26 12:34, Ulf Hansson ha scritto:
Quoted 35 lines · Adam Ford
Quoted 12 lines
The wrong value of the number of domains is wrong which leads to failures when trying to enumerate nested power domains.
PM:
genpd_xlate_onecell: invalid domain index 0
PM:genpd_xlate_onecell: invalid domain index 1
PM:genpd_xlate_onecell: invalid domain index 3
PM:genpd_xlate_onecell: invalid domain index 4
PM:genpd_xlate_onecell: invalid domain index 5
PM:genpd_xlate_onecell: invalid domain index 13
PM:genpd_xlate_onecell: invalid domain index 14Quoted 4 lines
Attempts to use these power domains fail, so fix this by using the correct value of calculated power domains.
Signed-off-by Adam Ford <[email protected]>We should have a fixes tag for this too I think:
Fixes 88914db077b6 ("pmdomain: mediatek: Add support for HardwareVoter power domains")
Quoted 11 lines
--- drivers/pmdomain/mediatek/mtk-pm-domains.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pmdomain/mediatek/mtk-pm-domains.c +0
-0@@ -1228,7 +1228,7 @@ static int scpsys_probe(struct platform_device *pdev) scpsys->soc_data = soc;Quoted 7 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c static int scpsys_probe(struct platform_device *pdev) · line 1231
Quoted 3 lines
drivers/pmdomain/mediatek/mtk-pm-domains.c +1
-1scpsys->pd_data.domains = scpsys->domains; - scpsys->pd_data.num_domains = soc->num_domains; + scpsys->pd_data.num_domains = num_domains;Not sure this is the complete fix, as
scpsys_add_one_domain()seems to be using the wrong value of "num_domains" too, no?Quoted 4 lines
No,
scpsys_add_one_domain()usesnum_domainsfrom thesoc_dataforDIRECT_CTL, which is expected. Maybe that can be improved, but it's how it is supposed to work.So yeah, this patch is resolving an issue, and it is a complete fix.
I had a closer look and unfortunately, it still looks weird to me.
More precisely, we are allocating and registering a number of genpds, that corresponds to the sum of "
soc->num_domains+soc->num_hwv_domains". The index each genpd gets in the array (structgenpd_onecell_data) must correspond to the index specified in DT, for both a consumer-node and a child-domain-node, right?It seems like adding them dynamically changes the index. In other words, the index specified using the "reg" property in a child-domain-node, may not match what a consumer-node should specify in its "power-domains" property. Isn't that a problem? Perhaps not, because we never have both "
scpsys_domain_data" and "scpsys_hwv_domain_data", but then why are we adding them in ->probe() with "soc->num_domains+soc->num_hwv_domains"?
Quoted 6 lines
Quoted 3 lines
As you understood, having both is not really supported right now, and the intention there was to have the flexibility to do so without restructuring the entire thing in the future.
Thanks for clarifying!
Oh you're welcome, of course.
Quoted 3 lines
Although, having both seems problematic for the reasons I pointed out above. At least, it looks like some more re-work will be needed to support that case.
Yeah, that's right.
It's still a lot of work, but hopefully I made it "less than if this wasn't there".
Not sure if I did, but there's that, anyway...
Quoted 13 lines
Quoted 6 lines
Though, from what I understand, the confusion comes from a single line (and reading it again, I do agree, because I got confused myself for a moment as well, and I'm the one who wrote this, so that's bad bad bad):
Quoted 6 lines
num_domains = soc->num_domains + soc->num_hwv_domains;where, to avoid confusion, this should've been:
num_domains = soc->num_domains ? soc->num_domains : soc->num_hwv_domains;
Quoted 8 lines
Agreed.
Quoted 3 lines
(perhaps with a check `if (
num_domains&&num_hwv_domains) dev_err(only one
supported)` but being this taken from hardcoded data, I'm not really for it)Agreed, a print would be superfluous.
Eh I forgot to add a "return -EINVAL" in the mix, which was the whole point of the "parenthesis"... ugh. :-)
In any case I can understand that we're on the same page about superfluous stuff, which is enough for the context of that.
Quoted 14 lines
Quoted 8 lines
...and still, like the proposed fix does:
scpsys->pd_data.num_domains = num_domains;While I agree, again, in that the code could be improved (and it shall be), how the logic behaves, right now, with the proposed fix, still looks good to me.
Okay, I'm queuing the $subject patch as a fix and by adding a stable/fixes-tag.
Quoted 2 lines
I can eventually just send a patch that clarifies the
num_domainsif you prefer.
Quoted 8 lines
Well, actually, I would suggest splitting the driver into two drivers (each handling its own type of match data) and adding some shared helper functions, that both drivers can use.
It's more work, but we should end up with more maintainable code, I think.
What do you think?
I had the same idea initially, but with how fundamentally broken is the "Hardware Voter" mechanism in the current MediaTek SoCs (Dimensity 9400 MT6991, 9500 MT6993 and derivatives like Kompanio Ultra MT8196 and Genio Pro) I didn't want to really do that.
I suspect that the HWV will change in the future, but I don't really know how, nor I have any knowledge that would make me able to safely predict.
So, well, while in theory having two drivers, with some common code in between, could make this more maintainable, I'm afraid that if we do this *right now* in the current state of things, we will end up with three drivers instead...
Small clarification about the brokenness of the HWV: all of the HWV Domains and clocks aren't really linked together internally... as much as HWV Domains and some external regulators, so, basically, as of now, the mechanism is practically useless if not for MCU-vs-AP *only partial* voting of resources (as dependencies are not handled, and some of them are not even HWV dependencies.... eh.. anyway, a big thread happened when initially upstreaming this entire thing, where a bit more is explained...)
Ewww... I'm losing myself in blurb again..!
In any case - my plan of action is to keep things as they are (structurally, but of course with the necessary quality/readability enhancements) until something new, with a proper resource voting system implementation in hardware, comes out.
If everything goes according to the plan (again... if), once a SoC with a proper implementation (HW ... but really it's firmware that can't be changed) we'll be sure that any decision that we'll take on this will most probably result in a flexible and maintainable implementation.
And that's why I'm basically saying "I would agree, but not now" :-)
Cheers,
Angelo
Quoted 4 lines
[...]
Kind regards
Uffe