| Message ID | 20231026155042.551731-1-bryan.odonoghue@linaro.org |
|---|---|
| Headers | show |
| Series | media: qcom: camss: Introduce support for named power-domains | expand |
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.
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
V2: - Incorporates Konrad's suggestion re: removing 'id' - Adds RB - Konrad - Adds in a flag to indicate if a VFE has a power domain. As I rebased this series I realised we had some magic indexing for VFE v VFE Lite, which isn't the root cause of my bug bear in this series but is the same sin - inferring functionality from indexing. Once we transition fully to named pds we won't need a 'has_pd' to flag which VFEs need power-domain attachment and which don't. That transition will require populating all upstream dtsi with pd-names and then deprecating the old way. has_pd is a far better choice than inferring from indexes so, I've added. Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/commits/aa45a2b58aa1e187a2698a65164d694251f08fa1 V1: At the moment the Qcom CAMSS driver relies on the declaration order of power-domains within the dtsi to determine which power-domain relates to a VFE and which power-domain relates to the top-level (top) CAMSS power-domain. VFE power-domains must be declared prior to the top power-domain. The top power-domain must be declared last. Early SoCs have just one top power-domain with later SoCs introducing VFE specific power-domains. Differentiating between the number of power-domains results in lots of code which is brittle and which we can mostly get rid of with named power-domains. The reliance on declaration ordering is in-effect magic number indexing. This series introduces named power-domains for CAMSS and refactors some of the code in CAMSS to support the new named power-domains. We continue to support the legacy indexing model with an intention to remove after a reasonable transition period. New SoC additions should use named power-domains from now on. Tested on x13s, rb5, db410c Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-10-23-camss-named-power-domains Bryan O'Donoghue (5): media: qcom: camss: Flag which VFEs require a power-domain media: qcom: camss: Convert to per-VFE pointer for power-domain linkages media: qcom: camss: Use common VFE pm_domain_on/pm_domain_off where applicable media: qcom: camss: Move VFE power-domain specifics into vfe.c media: qcom: camss: Add support for named power-domains .../media/platform/qcom/camss/camss-vfe-170.c | 36 -------- .../media/platform/qcom/camss/camss-vfe-4-1.c | 8 +- .../media/platform/qcom/camss/camss-vfe-4-7.c | 36 -------- .../media/platform/qcom/camss/camss-vfe-4-8.c | 31 ------- .../media/platform/qcom/camss/camss-vfe-480.c | 36 -------- drivers/media/platform/qcom/camss/camss-vfe.c | 77 ++++++++++++++++ drivers/media/platform/qcom/camss/camss-vfe.h | 16 ++++ drivers/media/platform/qcom/camss/camss.c | 87 ++++++++++++------- drivers/media/platform/qcom/camss/camss.h | 7 +- 9 files changed, 156 insertions(+), 178 deletions(-)