| Message ID | 20231026155042.551731-6-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: qcom: camss: Introduce support for named power-domains | expand |
On 26.10.2023 17:50, Bryan O'Donoghue wrote: > Right now we use fixed indexes to assign power-domains, with a > requirement for the TOP GDSC to come last in the list. > > Adding support for named power-domains means the declaration in the dtsi > can come in any order. > > After this change we continue to support the old indexing - if a SoC > resource declration or the in-use dtb doesn't declare power-domain names > we fall back to the default legacy indexing. > > From this point on though new SoC additions should contain named > power-domains, eventually we will drop support for legacy indexing. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/media/platform/qcom/camss/camss-vfe.c | 24 ++++++++++++++++- > drivers/media/platform/qcom/camss/camss.c | 26 +++++++++++++++---- > drivers/media/platform/qcom/camss/camss.h | 2 ++ > 3 files changed, 46 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c > index ebd5da6ad3f2f..cb48723efd8a0 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe.c > @@ -1381,7 +1381,29 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, > if (!res->line_num) > return -EINVAL; > > - if (res->has_pd) { > + /* Power domain */ Unnecessary, I think > + > + if (res->pd_name) { No need to nullcheck, dev_pm_domain_attach_by_name seems to return NULL when the name is NULL [...] > - if (IS_ERR(camss->genpd)) { > + if (camss->res->pd_name) { ditto > + camss->genpd = dev_pm_domain_attach_by_name(camss->dev, > + camss->res->pd_name); > + if (IS_ERR(camss->genpd)) { > + ret = PTR_ERR(camss->genpd); > + goto fail_pm; > + } > + } > + Looks good otherwise, I think Konrad
On 31/10/2023 10:53, Konrad Dybcio wrote: > On 26.10.2023 17:50, Bryan O'Donoghue wrote: >> Right now we use fixed indexes to assign power-domains, with a >> requirement for the TOP GDSC to come last in the list. >> >> Adding support for named power-domains means the declaration in the dtsi >> can come in any order. >> >> After this change we continue to support the old indexing - if a SoC >> resource declration or the in-use dtb doesn't declare power-domain names >> we fall back to the default legacy indexing. >> >> From this point on though new SoC additions should contain named >> power-domains, eventually we will drop support for legacy indexing. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> drivers/media/platform/qcom/camss/camss-vfe.c | 24 ++++++++++++++++- >> drivers/media/platform/qcom/camss/camss.c | 26 +++++++++++++++---- >> drivers/media/platform/qcom/camss/camss.h | 2 ++ >> 3 files changed, 46 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c >> index ebd5da6ad3f2f..cb48723efd8a0 100644 >> --- a/drivers/media/platform/qcom/camss/camss-vfe.c >> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c >> @@ -1381,7 +1381,29 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, >> if (!res->line_num) >> return -EINVAL; >> >> - if (res->has_pd) { >> + /* Power domain */ > Unnecessary, I think > Consistent with existing commentary in this function -> /* Memory */ /* Interrupts */ >> + >> + if (res->pd_name) { > No need to nullcheck, dev_pm_domain_attach_by_name seems to return > NULL when the name is NULL It looks so. Then again I'm sure checking here saves a few instructions and stack operations.. --- bod
On 31/10/2023 10:53, Konrad Dybcio wrote: >> + >> + if (res->pd_name) { > No need to nullcheck, dev_pm_domain_attach_by_name seems to return > NULL when the name is NULL So I tried removing the NULL check and of_property_match_string chokes [ 9.303798] msm_vfe_subdev_init/1386 camss->dev 000000004c790a88 res->pd_name ife0 [ 9.317650] msm_vfe_subdev_init/1386 camss->dev 000000004c790a88 res->pd_name ife1 [ 9.328085] msm_vfe_subdev_init/1386 camss->dev 000000004c790a88 res->pd_name (null) [ 9.330602] lt9611uxc 5-002b: LT9611 revision: 0x17.04.93 [ 9.336128] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 [ 9.350861] Mem abort info: [ 9.353751] ESR = 0x0000000096000004 [ 9.357617] EC = 0x25: DABT (current EL), IL = 32 bits [ 9.363083] SET = 0, FnV = 0 [ 9.366231] EA = 0, S1PTW = 0 [ 9.368917] remoteproc remoteproc1: powering up 17300000.remoteproc [ 9.369463] FSC = 0x04: level 0 translation fault [ 9.380922] Data abort info: [ 9.383919] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 [ 9.389579] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 9.394775] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 9.395986] remoteproc remoteproc1: Booting fw image qcom/sm8250/adsp.mbn, size 15515796 [ 9.400187] ax88179_178a 2-1.1:1.0 eth0: register 'ax88179_178a' at usb-xhci-hcd.0.auto-1.1, ASIX AX88179 USB 3.0 Gigabit Ethernet, 00:0e:c6:81:79:01 [ 9.400237] user pgtable: 4k pages, 48-bit VAs, pgdp=00000001067b2000 [ 9.400239] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000 [ 9.400242] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP [ 9.400243] Modules linked in: venus_enc venus_dec videobuf2_dma_contig qcom_camss(+) fastrpc qrtr_smd venus_core imx412 videobuf2_dma_sg mcp251xfd msm v4l2_fwnode v4l2_mem2mem videc [ 9.409624] lt9611uxc 5-002b: LT9611 version: 0x43 [ 9.422292] snd_soc_sm8250 qcom_spmi_adc_tm5 qcom_spmi_adc5 videobuf2_common xhci_plat_hcd drm_dp_aux_bus snd_soc_qcom_sdw xhci_hcd crct10dif_ce qrtr rtc_pm8xxx qcom_vadc_common qcs [ 9.492865] lt9611uxc 5-002b: failed to find dsi host [ 9.529472] CPU: 7 PID: 205 Comm: (udev-worker) Not tainted 6.6.0-rc3-00380-g7b823ffc4ec0-dirty #1 [ 9.529474] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) [ 9.529475] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 9.529477] pc : __pi_strcmp+0x24/0x140 [ 9.529482] lr : of_property_match_string+0x6c/0x130 [ 9.536672] msm_dsi ae94000.dsi: supply refgen not found, using dummy regulator [ 9.543865] sp : ffff800080d8b6d0 [ 9.543866] x29: ffff800080d8b6d0 x28: ffffd06ec3419ef8 x27: ffff12a54b8660a0 [ 9.543868] x26: 0000000000000000 x25: ffffd06f3cca22b0 x24: ffffd06f3d8e2798 [ 9.543870] x23: 0000000000000000 x22: 0000000000000000 x21: fffffbfffde0e590 [ 9.599837] x20: fffffbfffde0e59e x19: fffffbfffde0e595 x18: ffffffffffffffff [ 9.607171] x17: 6d616e5f64703e2d x16: ffffd06f3bc66bc4 x15: 3937633430303030 [ 9.614503] x14: ffffffffffffffff x13: 0000000000000020 x12: 0101010101010101 [ 9.621839] x11: 7f7f7f7f7f7f7f7f x10: fffffbfffde0e590 x9 : 7f7f7f7f7f7f7f7f [ 9.629174] x8 : 0101010101010101 x7 : 0000000080000000 x6 : 0000000000000000 [ 9.636507] x5 : 6f63710000000000 x4 : 000000706f740031 x3 : 6566760030656676 [ 9.643840] x2 : fffffbfffde0e5a0 x1 : fffffbfffde0e590 x0 : 0000000000000000 [ 9.651174] Call trace: [ 9.653698] __pi_strcmp+0x24/0x140 [ 9.657285] genpd_dev_pm_attach_by_name+0x2c/0x64 [ 9.662217] dev_pm_domain_attach_by_name+0x20/0x2c [ 9.667231] msm_vfe_subdev_init+0x78/0x50c [qcom_camss] [ 9.672704] camss_probe+0x288/0xc8c [qcom_camss] [ 9.677542] platform_probe+0x68/0xc0 [ 9.681311] really_probe+0x184/0x3c8 [ 9.685081] __driver_probe_device+0x7c/0x16c [ 9.689562] driver_probe_device+0x3c/0x110 [ 9.693862] __driver_attach+0xf4/0x1fc [ 9.697811] bus_for_each_dev+0x74/0xd4 [ 9.701762] driver_attach+0x24/0x30 [ 9.705446] bus_add_driver+0x110/0x214 [ 9.709397] driver_register+0x60/0x128 [ 9.713348] __platform_driver_register+0x28/0x34 [ 9.718180] qcom_camss_driver_init+0x20/0x1000 [qcom_camss] [ 9.723998] do_one_initcall+0x6c/0x1b0 [ 9.727950] do_init_module+0x58/0x1e4 [ 9.731804] load_module+0x1df4/0x1ee0 [ 9.735656] init_module_from_file+0x84/0xc4 [ 9.740041] __arm64_sys_finit_module+0x1f4/0x300 [ 9.744871] invoke_syscall+0x48/0x114 [ 9.748724] el0_svc_common.constprop.0+0xc0/0xe0 [ 9.753555] do_el0_svc+0x1c/0x28 [ 9.756962] el0_svc+0x40/0xe8 [ 9.760102] el0t_64_sync_handler+0x100/0x12c [ 9.764583] el0t_64_sync+0x190/0x194 [ 9.768352] Code: 54000401 b50002c6 d503201f f86a6803 (f8408402) [ 9.774609] ---[ end trace 0000000000000000 ]--- --- bod
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c index ebd5da6ad3f2f..cb48723efd8a0 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe.c +++ b/drivers/media/platform/qcom/camss/camss-vfe.c @@ -1381,7 +1381,29 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe, if (!res->line_num) return -EINVAL; - if (res->has_pd) { + /* Power domain */ + + if (res->pd_name) { + vfe->genpd = dev_pm_domain_attach_by_name(camss->dev, + res->pd_name); + if (IS_ERR(vfe->genpd)) { + ret = PTR_ERR(vfe->genpd); + return ret; + } + } + + if (!vfe->genpd && res->has_pd) { + /* + * Legacy magic index. + * Requires + * power-domain = <VFE_X>, + * <VFE_Y>, + * <TITAN_TOP> + * id must correspondng to the index of the VFE which must + * come before the TOP GDSC. VFE Lite has no individually + * collapasible domain which is why id < vfe_num is a valid + * check. + */ vfe->genpd = dev_pm_domain_attach_by_id(camss->dev, id); if (IS_ERR(vfe->genpd)) { ret = PTR_ERR(vfe->genpd); diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c index 03e955c7a7e4c..837bab28d40e2 100644 --- a/drivers/media/platform/qcom/camss/camss.c +++ b/drivers/media/platform/qcom/camss/camss.c @@ -1514,12 +1514,28 @@ static int camss_configure_pd(struct camss *camss) return 0; /* - * VFE power domains are in the beginning of the list, and while all - * power domains should be attached, only if TITAN_TOP power domain is - * found in the list, it should be linked over here. + * If a power-domain name is defined try to use it. + * It is possible we are running a new kernel with an old dtb so + * fallback to indexes even if a pd_name is defined but not found. */ - camss->genpd = dev_pm_domain_attach_by_id(camss->dev, camss->genpd_num - 1); - if (IS_ERR(camss->genpd)) { + if (camss->res->pd_name) { + camss->genpd = dev_pm_domain_attach_by_name(camss->dev, + camss->res->pd_name); + if (IS_ERR(camss->genpd)) { + ret = PTR_ERR(camss->genpd); + goto fail_pm; + } + } + + if (!camss->genpd) { + /* + * Legacy magic index. TITAN_TOP GDSC must be the last + * item in the power-domain list. + */ + camss->genpd = dev_pm_domain_attach_by_id(camss->dev, + camss->genpd_num - 1); + } + if (IS_ERR_OR_NULL(camss->genpd)) { ret = PTR_ERR(camss->genpd); goto fail_pm; } diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h index 1ba824a2cb76c..cd8186fe1797b 100644 --- a/drivers/media/platform/qcom/camss/camss.h +++ b/drivers/media/platform/qcom/camss/camss.h @@ -48,6 +48,7 @@ struct camss_subdev_resources { u32 clock_rate[CAMSS_RES_MAX][CAMSS_RES_MAX]; char *reg[CAMSS_RES_MAX]; char *interrupt[CAMSS_RES_MAX]; + char *pd_name; u8 line_num; bool has_pd; const void *ops; @@ -84,6 +85,7 @@ enum icc_count { struct camss_resources { enum camss_version version; + const char *pd_name; const struct camss_subdev_resources *csiphy_res; const struct camss_subdev_resources *csid_res; const struct camss_subdev_resources *ispif_res;
Right now we use fixed indexes to assign power-domains, with a requirement for the TOP GDSC to come last in the list. Adding support for named power-domains means the declaration in the dtsi can come in any order. After this change we continue to support the old indexing - if a SoC resource declration or the in-use dtb doesn't declare power-domain names we fall back to the default legacy indexing. From this point on though new SoC additions should contain named power-domains, eventually we will drop support for legacy indexing. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/media/platform/qcom/camss/camss-vfe.c | 24 ++++++++++++++++- drivers/media/platform/qcom/camss/camss.c | 26 +++++++++++++++---- drivers/media/platform/qcom/camss/camss.h | 2 ++ 3 files changed, 46 insertions(+), 6 deletions(-)