[v2,5/5] media: qcom: camss: Add support for named power-domains
Commit Message
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(-)
Comments
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
@@ -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);
@@ -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;
}
@@ -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;