| Message ID | 20250409-topic-sm8x50-iris-v10-v4-6-40e411594285@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: qcom: iris: add support for SM8650 | expand |
Hi Neil, On 4/9/2025 8:08 PM, Neil Armstrong wrote: > Add support for the SM8650 platform by re-using the SM8550 > definitions and using the vpu33 ops. > > The SM8650/vpu33 requires more reset lines, but the H.264 > decoder capabilities are identical. > > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > .../platform/qcom/iris/iris_platform_common.h | 1 + > .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ > drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ > 3 files changed, 69 insertions(+) > > diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h > index fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 > --- a/drivers/media/platform/qcom/iris/iris_platform_common.h > +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h > @@ -35,6 +35,7 @@ enum pipe_type { > > extern struct iris_platform_data sm8250_data; > extern struct iris_platform_data sm8550_data; > +extern struct iris_platform_data sm8650_data; > > enum platform_clk_type { > IRIS_AXI_CLK, > diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c > index 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 > --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c > +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c > @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { > > static const char * const sm8550_clk_reset_table[] = { "bus" }; > > +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; > + > +static const char * const sm8650_controller_reset_table[] = { "xo" }; > + > static const struct bw_info sm8550_bw_table_dec[] = { > { ((4096 * 2160) / 256) * 60, 1608000 }, > { ((4096 * 2160) / 256) * 30, 826000 }, > @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { > .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, > .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), > }; > + > +/* > + * Shares most of SM8550 data except: > + * - vpu_ops to iris_vpu33_ops > + * - clk_rst_tbl to sm8650_clk_reset_table > + * - controller_rst_tbl to sm8650_controller_reset_table > + * - fwname to "qcom/vpu/vpu33_p4.mbn" > + */ > +struct iris_platform_data sm8650_data = { > + .get_instance = iris_hfi_gen2_get_instance, > + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, > + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, > + .vpu_ops = &iris_vpu33_ops, > + .set_preset_registers = iris_set_sm8550_preset_registers, > + .icc_tbl = sm8550_icc_table, > + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), > + .clk_rst_tbl = sm8650_clk_reset_table, > + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), > + .controller_rst_tbl = sm8650_controller_reset_table, > + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), > + .bw_tbl_dec = sm8550_bw_table_dec, > + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), > + .pmdomain_tbl = sm8550_pmdomain_table, > + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), > + .opp_pd_tbl = sm8550_opp_pd_table, > + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), > + .clk_tbl = sm8550_clk_table, > + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), > + /* Upper bound of DMA address range */ > + .dma_mask = 0xe0000000 - 1, > + .fwname = "qcom/vpu/vpu33_p4.mbn", > + .pas_id = IRIS_PAS_ID, > + .inst_caps = &platform_inst_cap_sm8550, > + .inst_fw_caps = inst_fw_cap_sm8550, > + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), > + .tz_cp_config_data = &tz_cp_config_sm8550, > + .core_arch = VIDEO_ARCH_LX, > + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, > + .ubwc_config = &ubwc_config_sm8550, > + .num_vpp_pipe = 4, > + .max_session_count = 16, > + .max_core_mbpf = ((8192 * 4352) / 256) * 2, > + .input_config_params = > + sm8550_vdec_input_config_params, > + .input_config_params_size = > + ARRAY_SIZE(sm8550_vdec_input_config_params), > + .output_config_params = > + sm8550_vdec_output_config_params, > + .output_config_params_size = > + ARRAY_SIZE(sm8550_vdec_output_config_params), > + .dec_input_prop = sm8550_vdec_subscribe_input_properties, > + .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), > + .dec_output_prop = sm8550_vdec_subscribe_output_properties, > + .dec_output_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), > + > + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, > + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), > + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, > + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), > +}; While i was extending the data for QCS8300 (one another iris-v3 variant), i realize that this file iris_platform_sm8550.c is getting dumped with all SOC platform data. It would be a good idea at this point to split it into something like this 1. Introduce SOC specific c file and move the respective SOC platform data to it, for ex, in this case sm8650_data 2. Move the common structs from iris_platform_sm8550.c to iris_platform_common.h. This way more SOCs getting added in future, can include the common header to reuse them, otherwise it would end up using 8550.c for all future SOC. Share your comments if you have any better approach to manage/re-use these platform data considering more SOCs getting added. Regards, Vikash > diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c > index 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 > --- a/drivers/media/platform/qcom/iris/iris_probe.c > +++ b/drivers/media/platform/qcom/iris/iris_probe.c > @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { > .data = &sm8250_data, > }, > #endif > + { > + .compatible = "qcom,sm8650-iris", > + .data = &sm8650_data, > + }, > { }, > }; > MODULE_DEVICE_TABLE(of, iris_dt_match); >
On 09/04/2025 18:57, Vikash Garodia wrote: > Hi Neil, > > On 4/9/2025 8:08 PM, Neil Armstrong wrote: >> Add support for the SM8650 platform by re-using the SM8550 >> definitions and using the vpu33 ops. >> >> The SM8650/vpu33 requires more reset lines, but the H.264 >> decoder capabilities are identical. >> >> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> .../platform/qcom/iris/iris_platform_common.h | 1 + >> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >> 3 files changed, 69 insertions(+) >> >> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h >> index fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >> @@ -35,6 +35,7 @@ enum pipe_type { >> >> extern struct iris_platform_data sm8250_data; >> extern struct iris_platform_data sm8550_data; >> +extern struct iris_platform_data sm8650_data; >> >> enum platform_clk_type { >> IRIS_AXI_CLK, >> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >> index 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >> >> static const char * const sm8550_clk_reset_table[] = { "bus" }; >> >> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >> + >> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >> + >> static const struct bw_info sm8550_bw_table_dec[] = { >> { ((4096 * 2160) / 256) * 60, 1608000 }, >> { ((4096 * 2160) / 256) * 30, 826000 }, >> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >> }; >> + >> +/* >> + * Shares most of SM8550 data except: >> + * - vpu_ops to iris_vpu33_ops >> + * - clk_rst_tbl to sm8650_clk_reset_table >> + * - controller_rst_tbl to sm8650_controller_reset_table >> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >> + */ >> +struct iris_platform_data sm8650_data = { >> + .get_instance = iris_hfi_gen2_get_instance, >> + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, >> + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, >> + .vpu_ops = &iris_vpu33_ops, >> + .set_preset_registers = iris_set_sm8550_preset_registers, >> + .icc_tbl = sm8550_icc_table, >> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >> + .clk_rst_tbl = sm8650_clk_reset_table, >> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >> + .controller_rst_tbl = sm8650_controller_reset_table, >> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >> + .bw_tbl_dec = sm8550_bw_table_dec, >> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >> + .pmdomain_tbl = sm8550_pmdomain_table, >> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >> + .opp_pd_tbl = sm8550_opp_pd_table, >> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >> + .clk_tbl = sm8550_clk_table, >> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >> + /* Upper bound of DMA address range */ >> + .dma_mask = 0xe0000000 - 1, >> + .fwname = "qcom/vpu/vpu33_p4.mbn", >> + .pas_id = IRIS_PAS_ID, >> + .inst_caps = &platform_inst_cap_sm8550, >> + .inst_fw_caps = inst_fw_cap_sm8550, >> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >> + .tz_cp_config_data = &tz_cp_config_sm8550, >> + .core_arch = VIDEO_ARCH_LX, >> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >> + .ubwc_config = &ubwc_config_sm8550, >> + .num_vpp_pipe = 4, >> + .max_session_count = 16, >> + .max_core_mbpf = ((8192 * 4352) / 256) * 2, >> + .input_config_params = >> + sm8550_vdec_input_config_params, >> + .input_config_params_size = >> + ARRAY_SIZE(sm8550_vdec_input_config_params), >> + .output_config_params = >> + sm8550_vdec_output_config_params, >> + .output_config_params_size = >> + ARRAY_SIZE(sm8550_vdec_output_config_params), >> + .dec_input_prop = sm8550_vdec_subscribe_input_properties, >> + .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), >> + .dec_output_prop = sm8550_vdec_subscribe_output_properties, >> + .dec_output_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), >> + >> + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, >> + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), >> + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >> +}; > While i was extending the data for QCS8300 (one another iris-v3 variant), i > realize that this file iris_platform_sm8550.c is getting dumped with all SOC > platform data. It would be a good idea at this point to split it into something > like this > 1. Introduce SOC specific c file and move the respective SOC platform data to > it, for ex, in this case sm8650_data > 2. Move the common structs from iris_platform_sm8550.c to > iris_platform_common.h. This way more SOCs getting added in future, can include > the common header to reuse them, otherwise it would end up using 8550.c for all > future SOC. > > Share your comments if you have any better approach to manage/re-use these > platform data considering more SOCs getting added. Right, yes the architecture is fine, but I don't feel iris_platform_common is the right place, perhaps we could introduce a platform_catalog.c where we could place all the common platform data and reuse them from the platform_<soc>.c files ? I can design prototype on top of this patchset as an RFC. Neil > > Regards, > Vikash > >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c >> index 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >> --- a/drivers/media/platform/qcom/iris/iris_probe.c >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >> .data = &sm8250_data, >> }, >> #endif >> + { >> + .compatible = "qcom,sm8650-iris", >> + .data = &sm8650_data, >> + }, >> { }, >> }; >> MODULE_DEVICE_TABLE(of, iris_dt_match); >>
On 4/10/2025 2:31 PM, Neil Armstrong wrote: > On 09/04/2025 18:57, Vikash Garodia wrote: >> Hi Neil, >> >> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>> Add support for the SM8650 platform by re-using the SM8550 >>> definitions and using the vpu33 ops. >>> >>> The SM8650/vpu33 requires more reset lines, but the H.264 >>> decoder capabilities are identical. >>> >>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>> 3 files changed, 69 insertions(+) >>> >>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>> index >>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>> @@ -35,6 +35,7 @@ enum pipe_type { >>> extern struct iris_platform_data sm8250_data; >>> extern struct iris_platform_data sm8550_data; >>> +extern struct iris_platform_data sm8650_data; >>> enum platform_clk_type { >>> IRIS_AXI_CLK, >>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>> index >>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>> + >>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>> + >>> static const struct bw_info sm8550_bw_table_dec[] = { >>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>> { ((4096 * 2160) / 256) * 30, 826000 }, >>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>> }; >>> + >>> +/* >>> + * Shares most of SM8550 data except: >>> + * - vpu_ops to iris_vpu33_ops >>> + * - clk_rst_tbl to sm8650_clk_reset_table >>> + * - controller_rst_tbl to sm8650_controller_reset_table >>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>> + */ >>> +struct iris_platform_data sm8650_data = { >>> + .get_instance = iris_hfi_gen2_get_instance, >>> + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, >>> + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, >>> + .vpu_ops = &iris_vpu33_ops, >>> + .set_preset_registers = iris_set_sm8550_preset_registers, >>> + .icc_tbl = sm8550_icc_table, >>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>> + .clk_rst_tbl = sm8650_clk_reset_table, >>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>> + .controller_rst_tbl = sm8650_controller_reset_table, >>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>> + .bw_tbl_dec = sm8550_bw_table_dec, >>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>> + .pmdomain_tbl = sm8550_pmdomain_table, >>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>> + .opp_pd_tbl = sm8550_opp_pd_table, >>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>> + .clk_tbl = sm8550_clk_table, >>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>> + /* Upper bound of DMA address range */ >>> + .dma_mask = 0xe0000000 - 1, >>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>> + .pas_id = IRIS_PAS_ID, >>> + .inst_caps = &platform_inst_cap_sm8550, >>> + .inst_fw_caps = inst_fw_cap_sm8550, >>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>> + .core_arch = VIDEO_ARCH_LX, >>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>> + .ubwc_config = &ubwc_config_sm8550, >>> + .num_vpp_pipe = 4, >>> + .max_session_count = 16, >>> + .max_core_mbpf = ((8192 * 4352) / 256) * 2, >>> + .input_config_params = >>> + sm8550_vdec_input_config_params, >>> + .input_config_params_size = >>> + ARRAY_SIZE(sm8550_vdec_input_config_params), >>> + .output_config_params = >>> + sm8550_vdec_output_config_params, >>> + .output_config_params_size = >>> + ARRAY_SIZE(sm8550_vdec_output_config_params), >>> + .dec_input_prop = sm8550_vdec_subscribe_input_properties, >>> + .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), >>> + .dec_output_prop = sm8550_vdec_subscribe_output_properties, >>> + .dec_output_prop_size = >>> ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), >>> + >>> + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, >>> + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), >>> + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>> +}; >> While i was extending the data for QCS8300 (one another iris-v3 variant), i >> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >> platform data. It would be a good idea at this point to split it into something >> like this >> 1. Introduce SOC specific c file and move the respective SOC platform data to >> it, for ex, in this case sm8650_data >> 2. Move the common structs from iris_platform_sm8550.c to >> iris_platform_common.h. This way more SOCs getting added in future, can include >> the common header to reuse them, otherwise it would end up using 8550.c for all >> future SOC. >> >> Share your comments if you have any better approach to manage/re-use these >> platform data considering more SOCs getting added. > > Right, yes the architecture is fine, but I don't feel iris_platform_common is > the right > place, perhaps we could introduce a platform_catalog.c where we could place all > the common > platform data and reuse them from the platform_<soc>.c files ? Common structs would certainly need to be part of a header which can be included. Where do you plan to keep common struct to be used across SOC specific file in your approach ? > > I can design prototype on top of this patchset as an RFC. I was thinking that the changes are not that big, and can be done in existing series though. Thanks, Vikash > > Neil > >> >> Regards, >> Vikash >> >>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>> b/drivers/media/platform/qcom/iris/iris_probe.c >>> index >>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>> .data = &sm8250_data, >>> }, >>> #endif >>> + { >>> + .compatible = "qcom,sm8650-iris", >>> + .data = &sm8650_data, >>> + }, >>> { }, >>> }; >>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>> >
On 10/04/2025 11:13, Vikash Garodia wrote: > > On 4/10/2025 2:31 PM, Neil Armstrong wrote: >> On 09/04/2025 18:57, Vikash Garodia wrote: >>> Hi Neil, >>> >>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>> Add support for the SM8650 platform by re-using the SM8550 >>>> definitions and using the vpu33 ops. >>>> >>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>> decoder capabilities are identical. >>>> >>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>> 3 files changed, 69 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> index >>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>> extern struct iris_platform_data sm8250_data; >>>> extern struct iris_platform_data sm8550_data; >>>> +extern struct iris_platform_data sm8650_data; >>>> enum platform_clk_type { >>>> IRIS_AXI_CLK, >>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> index >>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>>> + >>>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>>> + >>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>> }; >>>> + >>>> +/* >>>> + * Shares most of SM8550 data except: >>>> + * - vpu_ops to iris_vpu33_ops >>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>> + */ >>>> +struct iris_platform_data sm8650_data = { >>>> + .get_instance = iris_hfi_gen2_get_instance, >>>> + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, >>>> + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, >>>> + .vpu_ops = &iris_vpu33_ops, >>>> + .set_preset_registers = iris_set_sm8550_preset_registers, >>>> + .icc_tbl = sm8550_icc_table, >>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>> + .clk_tbl = sm8550_clk_table, >>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>> + /* Upper bound of DMA address range */ >>>> + .dma_mask = 0xe0000000 - 1, >>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>> + .pas_id = IRIS_PAS_ID, >>>> + .inst_caps = &platform_inst_cap_sm8550, >>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>> + .core_arch = VIDEO_ARCH_LX, >>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>> + .ubwc_config = &ubwc_config_sm8550, >>>> + .num_vpp_pipe = 4, >>>> + .max_session_count = 16, >>>> + .max_core_mbpf = ((8192 * 4352) / 256) * 2, >>>> + .input_config_params = >>>> + sm8550_vdec_input_config_params, >>>> + .input_config_params_size = >>>> + ARRAY_SIZE(sm8550_vdec_input_config_params), >>>> + .output_config_params = >>>> + sm8550_vdec_output_config_params, >>>> + .output_config_params_size = >>>> + ARRAY_SIZE(sm8550_vdec_output_config_params), >>>> + .dec_input_prop = sm8550_vdec_subscribe_input_properties, >>>> + .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), >>>> + .dec_output_prop = sm8550_vdec_subscribe_output_properties, >>>> + .dec_output_prop_size = >>>> ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), >>>> + >>>> + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, >>>> + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), >>>> + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>> +}; >>> While i was extending the data for QCS8300 (one another iris-v3 variant), i >>> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >>> platform data. It would be a good idea at this point to split it into something >>> like this >>> 1. Introduce SOC specific c file and move the respective SOC platform data to >>> it, for ex, in this case sm8650_data >>> 2. Move the common structs from iris_platform_sm8550.c to >>> iris_platform_common.h. This way more SOCs getting added in future, can include >>> the common header to reuse them, otherwise it would end up using 8550.c for all >>> future SOC. >>> >>> Share your comments if you have any better approach to manage/re-use these >>> platform data considering more SOCs getting added. >> >> Right, yes the architecture is fine, but I don't feel iris_platform_common is >> the right >> place, perhaps we could introduce a platform_catalog.c where we could place all >> the common >> platform data and reuse them from the platform_<soc>.c files ? > Common structs would certainly need to be part of a header which can be > included. Where do you plan to keep common struct to be used across SOC specific > file in your approach ? Common struct in header would mean they would be duplicated for each platform when compiled, which would sub-optimal. I tried to look at the drm/msm/dpu1 catalog, but: 1) in our case 99% is common, only a few stuff differs 2) we have a single struct which requires the array sizes This makes using the drm/msm/dpu1 catalog structure impossible. >> >> I can design prototype on top of this patchset as an RFC. > I was thinking that the changes are not that big, and can be done in existing > series though. I disagree, this is a much larger subject than sm8650 support itself. Neil > > Thanks, > Vikash >> >> Neil >> >>> >>> Regards, >>> Vikash >>> >>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>> index >>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>>> .data = &sm8250_data, >>>> }, >>>> #endif >>>> + { >>>> + .compatible = "qcom,sm8650-iris", >>>> + .data = &sm8650_data, >>>> + }, >>>> { }, >>>> }; >>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>> >>
On 4/10/2025 2:43 PM, Vikash Garodia wrote: > > On 4/10/2025 2:31 PM, Neil Armstrong wrote: >> On 09/04/2025 18:57, Vikash Garodia wrote: >>> Hi Neil, >>> >>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>> Add support for the SM8650 platform by re-using the SM8550 >>>> definitions and using the vpu33 ops. >>>> >>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>> decoder capabilities are identical. >>>> >>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>> 3 files changed, 69 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> index >>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>> extern struct iris_platform_data sm8250_data; >>>> extern struct iris_platform_data sm8550_data; >>>> +extern struct iris_platform_data sm8650_data; >>>> enum platform_clk_type { >>>> IRIS_AXI_CLK, >>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> index >>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>>> + >>>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>>> + >>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>> }; >>>> + >>>> +/* >>>> + * Shares most of SM8550 data except: >>>> + * - vpu_ops to iris_vpu33_ops >>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>> + */ >>>> +struct iris_platform_data sm8650_data = { >>>> + .get_instance = iris_hfi_gen2_get_instance, >>>> + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, >>>> + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, >>>> + .vpu_ops = &iris_vpu33_ops, >>>> + .set_preset_registers = iris_set_sm8550_preset_registers, >>>> + .icc_tbl = sm8550_icc_table, >>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>> + .clk_tbl = sm8550_clk_table, >>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>> + /* Upper bound of DMA address range */ >>>> + .dma_mask = 0xe0000000 - 1, >>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>> + .pas_id = IRIS_PAS_ID, >>>> + .inst_caps = &platform_inst_cap_sm8550, >>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>> + .core_arch = VIDEO_ARCH_LX, >>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>> + .ubwc_config = &ubwc_config_sm8550, >>>> + .num_vpp_pipe = 4, >>>> + .max_session_count = 16, >>>> + .max_core_mbpf = ((8192 * 4352) / 256) * 2, >>>> + .input_config_params = >>>> + sm8550_vdec_input_config_params, >>>> + .input_config_params_size = >>>> + ARRAY_SIZE(sm8550_vdec_input_config_params), >>>> + .output_config_params = >>>> + sm8550_vdec_output_config_params, >>>> + .output_config_params_size = >>>> + ARRAY_SIZE(sm8550_vdec_output_config_params), >>>> + .dec_input_prop = sm8550_vdec_subscribe_input_properties, >>>> + .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), >>>> + .dec_output_prop = sm8550_vdec_subscribe_output_properties, >>>> + .dec_output_prop_size = >>>> ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), >>>> + >>>> + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, >>>> + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), >>>> + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>> +}; >>> While i was extending the data for QCS8300 (one another iris-v3 variant), i >>> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >>> platform data. It would be a good idea at this point to split it into something >>> like this >>> 1. Introduce SOC specific c file and move the respective SOC platform data to >>> it, for ex, in this case sm8650_data >>> 2. Move the common structs from iris_platform_sm8550.c to >>> iris_platform_common.h. This way more SOCs getting added in future, can include >>> the common header to reuse them, otherwise it would end up using 8550.c for all >>> future SOC. >>> >>> Share your comments if you have any better approach to manage/re-use these >>> platform data considering more SOCs getting added. >> >> Right, yes the architecture is fine, but I don't feel iris_platform_common is >> the right >> place, perhaps we could introduce a platform_catalog.c where we could place all >> the common >> platform data and reuse them from the platform_<soc>.c files ? > Common structs would certainly need to be part of a header which can be > included. Where do you plan to keep common struct to be used across SOC specific > file in your approach ? >> >> I can design prototype on top of this patchset as an RFC. > I was thinking that the changes are not that big, and can be done in existing > series though. > For now, I think you can introduce a platform_sm8650.c as part of this series and use the common structure from platform_sm8550.c. Shouldn't be a big change. Later you can post a separate patch series to add platform_catalog.c and have common struct placed there which can be used across different SOC platform files. or other way is, Post a patch series to introduce platform_catalog.c with common struct and then rebase your 8650 series on top of it. Thanks, Dikshita > Thanks, > Vikash >> >> Neil >> >>> >>> Regards, >>> Vikash >>> >>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>> index >>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>>> .data = &sm8250_data, >>>> }, >>>> #endif >>>> + { >>>> + .compatible = "qcom,sm8650-iris", >>>> + .data = &sm8650_data, >>>> + }, >>>> { }, >>>> }; >>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>> >>
On 10/04/2025 15:07, Dikshita Agarwal wrote: > > > On 4/10/2025 2:43 PM, Vikash Garodia wrote: >> >> On 4/10/2025 2:31 PM, Neil Armstrong wrote: >>> On 09/04/2025 18:57, Vikash Garodia wrote: >>>> Hi Neil, >>>> >>>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>>> Add support for the SM8650 platform by re-using the SM8550 >>>>> definitions and using the vpu33 ops. >>>>> >>>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>>> decoder capabilities are identical. >>>>> >>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>> --- >>>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >>>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>>> 3 files changed, 69 insertions(+) >>>>> >>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>> index >>>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>>> extern struct iris_platform_data sm8250_data; >>>>> extern struct iris_platform_data sm8550_data; >>>>> +extern struct iris_platform_data sm8650_data; >>>>> enum platform_clk_type { >>>>> IRIS_AXI_CLK, >>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>> index >>>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>>>> + >>>>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>>>> + >>>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>> }; >>>>> + >>>>> +/* >>>>> + * Shares most of SM8550 data except: >>>>> + * - vpu_ops to iris_vpu33_ops >>>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>>> + */ >>>>> +struct iris_platform_data sm8650_data = { >>>>> + .get_instance = iris_hfi_gen2_get_instance, >>>>> + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, >>>>> + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, >>>>> + .vpu_ops = &iris_vpu33_ops, >>>>> + .set_preset_registers = iris_set_sm8550_preset_registers, >>>>> + .icc_tbl = sm8550_icc_table, >>>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>>> + .clk_tbl = sm8550_clk_table, >>>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>>> + /* Upper bound of DMA address range */ >>>>> + .dma_mask = 0xe0000000 - 1, >>>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>>> + .pas_id = IRIS_PAS_ID, >>>>> + .inst_caps = &platform_inst_cap_sm8550, >>>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>>> + .core_arch = VIDEO_ARCH_LX, >>>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>>> + .ubwc_config = &ubwc_config_sm8550, >>>>> + .num_vpp_pipe = 4, >>>>> + .max_session_count = 16, >>>>> + .max_core_mbpf = ((8192 * 4352) / 256) * 2, >>>>> + .input_config_params = >>>>> + sm8550_vdec_input_config_params, >>>>> + .input_config_params_size = >>>>> + ARRAY_SIZE(sm8550_vdec_input_config_params), >>>>> + .output_config_params = >>>>> + sm8550_vdec_output_config_params, >>>>> + .output_config_params_size = >>>>> + ARRAY_SIZE(sm8550_vdec_output_config_params), >>>>> + .dec_input_prop = sm8550_vdec_subscribe_input_properties, >>>>> + .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), >>>>> + .dec_output_prop = sm8550_vdec_subscribe_output_properties, >>>>> + .dec_output_prop_size = >>>>> ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), >>>>> + >>>>> + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, >>>>> + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), >>>>> + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>> +}; >>>> While i was extending the data for QCS8300 (one another iris-v3 variant), i >>>> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >>>> platform data. It would be a good idea at this point to split it into something >>>> like this >>>> 1. Introduce SOC specific c file and move the respective SOC platform data to >>>> it, for ex, in this case sm8650_data >>>> 2. Move the common structs from iris_platform_sm8550.c to >>>> iris_platform_common.h. This way more SOCs getting added in future, can include >>>> the common header to reuse them, otherwise it would end up using 8550.c for all >>>> future SOC. >>>> >>>> Share your comments if you have any better approach to manage/re-use these >>>> platform data considering more SOCs getting added. >>> >>> Right, yes the architecture is fine, but I don't feel iris_platform_common is >>> the right >>> place, perhaps we could introduce a platform_catalog.c where we could place all >>> the common >>> platform data and reuse them from the platform_<soc>.c files ? >> Common structs would certainly need to be part of a header which can be >> included. Where do you plan to keep common struct to be used across SOC specific >> file in your approach ? >>> >>> I can design prototype on top of this patchset as an RFC. >> I was thinking that the changes are not that big, and can be done in existing >> series though. >> > For now, I think you can introduce a platform_sm8650.c as part of this > series and use the common structure from platform_sm8550.c. > Shouldn't be a big change. I tried but I don't how to solve this, you need a build-time ARRAY_SIZE for the arrays, so they need to be in the same .c file to allow a build-time evaluation of ARRAY_SIZE. If he common tables are moved into a header they will be duplicated into both platform_sm8650 & platform_sm8550 objects which is not what we want. The solution would be to write all the platform tables & iris_platform_data into headers, with common headers, then include those into a platform_catalog.c like is done for the drm/msm/dpu1 catalog. Neil > > Later you can post a separate patch series to add platform_catalog.c and > have common struct placed there which can be used across different SOC > platform files. > > or other way is, > Post a patch series to introduce platform_catalog.c with common struct and > then rebase your 8650 series on top of it. > > Thanks, > Dikshita >> Thanks, >> Vikash >>> >>> Neil >>> >>>> >>>> Regards, >>>> Vikash >>>> >>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>>> index >>>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>>>> .data = &sm8250_data, >>>>> }, >>>>> #endif >>>>> + { >>>>> + .compatible = "qcom,sm8650-iris", >>>>> + .data = &sm8650_data, >>>>> + }, >>>>> { }, >>>>> }; >>>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>>> >>>
On 10/04/2025 16:40, neil.armstrong@linaro.org wrote: > On 10/04/2025 15:07, Dikshita Agarwal wrote: >> >> >> On 4/10/2025 2:43 PM, Vikash Garodia wrote: >>> >>> On 4/10/2025 2:31 PM, Neil Armstrong wrote: >>>> On 09/04/2025 18:57, Vikash Garodia wrote: >>>>> Hi Neil, >>>>> >>>>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>>>> Add support for the SM8650 platform by re-using the SM8550 >>>>>> definitions and using the vpu33 ops. >>>>>> >>>>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>>>> decoder capabilities are identical. >>>>>> >>>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>> --- >>>>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 +++++++ >>>>>> +++++++++++++++ >>>>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>>>> 3 files changed, 69 insertions(+) >>>>>> >>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>> index >>>>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>>>> extern struct iris_platform_data sm8250_data; >>>>>> extern struct iris_platform_data sm8550_data; >>>>>> +extern struct iris_platform_data sm8650_data; >>>>>> enum platform_clk_type { >>>>>> IRIS_AXI_CLK, >>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>> index >>>>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>> @@ -144,6 +144,10 @@ static const struct icc_info >>>>>> sm8550_icc_table[] = { >>>>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>>>> +static const char * const sm8650_clk_reset_table[] = { "bus", >>>>>> "core" }; >>>>>> + >>>>>> +static const char * const sm8650_controller_reset_table[] = >>>>>> { "xo" }; >>>>>> + >>>>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>>> .dec_op_int_buf_tbl_size = >>>>>> ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>>> }; >>>>>> + >>>>>> +/* >>>>>> + * Shares most of SM8550 data except: >>>>>> + * - vpu_ops to iris_vpu33_ops >>>>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>>>> + */ >>>>>> +struct iris_platform_data sm8650_data = { >>>>>> + .get_instance = iris_hfi_gen2_get_instance, >>>>>> + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, >>>>>> + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, >>>>>> + .vpu_ops = &iris_vpu33_ops, >>>>>> + .set_preset_registers = iris_set_sm8550_preset_registers, >>>>>> + .icc_tbl = sm8550_icc_table, >>>>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>>>> + .controller_rst_tbl_size = >>>>>> ARRAY_SIZE(sm8650_controller_reset_table), >>>>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>>>> + .clk_tbl = sm8550_clk_table, >>>>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>>>> + /* Upper bound of DMA address range */ >>>>>> + .dma_mask = 0xe0000000 - 1, >>>>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>>>> + .pas_id = IRIS_PAS_ID, >>>>>> + .inst_caps = &platform_inst_cap_sm8550, >>>>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>>>> + .core_arch = VIDEO_ARCH_LX, >>>>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>>>> + .ubwc_config = &ubwc_config_sm8550, >>>>>> + .num_vpp_pipe = 4, >>>>>> + .max_session_count = 16, >>>>>> + .max_core_mbpf = ((8192 * 4352) / 256) * 2, >>>>>> + .input_config_params = >>>>>> + sm8550_vdec_input_config_params, >>>>>> + .input_config_params_size = >>>>>> + ARRAY_SIZE(sm8550_vdec_input_config_params), >>>>>> + .output_config_params = >>>>>> + sm8550_vdec_output_config_params, >>>>>> + .output_config_params_size = >>>>>> + ARRAY_SIZE(sm8550_vdec_output_config_params), >>>>>> + .dec_input_prop = sm8550_vdec_subscribe_input_properties, >>>>>> + .dec_input_prop_size = >>>>>> ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), >>>>>> + .dec_output_prop = sm8550_vdec_subscribe_output_properties, >>>>>> + .dec_output_prop_size = >>>>>> ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), >>>>>> + >>>>>> + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, >>>>>> + .dec_ip_int_buf_tbl_size = >>>>>> ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), >>>>>> + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>>> + .dec_op_int_buf_tbl_size = >>>>>> ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>>> +}; >>>>> While i was extending the data for QCS8300 (one another iris-v3 >>>>> variant), i >>>>> realize that this file iris_platform_sm8550.c is getting dumped >>>>> with all SOC >>>>> platform data. It would be a good idea at this point to split it >>>>> into something >>>>> like this >>>>> 1. Introduce SOC specific c file and move the respective SOC >>>>> platform data to >>>>> it, for ex, in this case sm8650_data >>>>> 2. Move the common structs from iris_platform_sm8550.c to >>>>> iris_platform_common.h. This way more SOCs getting added in future, >>>>> can include >>>>> the common header to reuse them, otherwise it would end up using >>>>> 8550.c for all >>>>> future SOC. >>>>> >>>>> Share your comments if you have any better approach to manage/re- >>>>> use these >>>>> platform data considering more SOCs getting added. >>>> >>>> Right, yes the architecture is fine, but I don't feel >>>> iris_platform_common is >>>> the right >>>> place, perhaps we could introduce a platform_catalog.c where we >>>> could place all >>>> the common >>>> platform data and reuse them from the platform_<soc>.c files ? >>> Common structs would certainly need to be part of a header which can be >>> included. Where do you plan to keep common struct to be used across >>> SOC specific >>> file in your approach ? >>>> >>>> I can design prototype on top of this patchset as an RFC. >>> I was thinking that the changes are not that big, and can be done in >>> existing >>> series though. >>> >> For now, I think you can introduce a platform_sm8650.c as part of this >> series and use the common structure from platform_sm8550.c. >> Shouldn't be a big change. > > I tried but I don't how to solve this, you need a build-time ARRAY_SIZE for > the arrays, so they need to be in the same .c file to allow a build-time > evaluation of ARRAY_SIZE. If he common tables are moved into a header > they will be duplicated into both platform_sm8650 & platform_sm8550 objects > which is not what we want. > > The solution would be to write all the platform tables & iris_platform_data > into headers, with common headers, then include those into a > platform_catalog.c > like is done for the drm/msm/dpu1 catalog. > > Neil > >> >> Later you can post a separate patch series to add platform_catalog.c and >> have common struct placed there which can be used across different SOC >> platform files. >> >> or other way is, >> Post a patch series to introduce platform_catalog.c with common struct >> and >> then rebase your 8650 series on top of it. >> >> Thanks, >> Dikshita >>> Thanks, >>> Vikash >>>> >>>> Neil >>>> >>>>> >>>>> Regards, >>>>> Vikash >>>>> >>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>>>> index >>>>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>>>> @@ -345,6 +345,10 @@ static const struct of_device_id >>>>>> iris_dt_match[] = { >>>>>> .data = &sm8250_data, >>>>>> }, >>>>>> #endif >>>>>> + { >>>>>> + .compatible = "qcom,sm8650-iris", >>>>>> + .data = &sm8650_data, >>>>>> + }, >>>>>> { }, >>>>>> }; >>>>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>>>> >>>> > Eh as I read the suggestion about putting the structs - instantiating the structs in the header, I wondered if that would link but anyway. One way to solve this without going the dpu1 route right now is to rename the platform files iris_platform_sm8250.c -> iris_platform_common_hfi_gen1.c iris_platform_sm8550.c -> iris_platform_common_hfi_gen2.c The differentiation around HFI into "generations" instead of incrementing the already existing HFIXXX version is unfortunate. At least this way we have a repository of common HFI code located in each file, in expectation of HFI GEN3 whenever. --- bod
On 10/04/2025 18:06, Bryan O'Donoghue wrote: > On 10/04/2025 16:40, neil.armstrong@linaro.org wrote: >> On 10/04/2025 15:07, Dikshita Agarwal wrote: >>> >>> >>> On 4/10/2025 2:43 PM, Vikash Garodia wrote: >>>> >>>> On 4/10/2025 2:31 PM, Neil Armstrong wrote: >>>>> On 09/04/2025 18:57, Vikash Garodia wrote: >>>>>> Hi Neil, >>>>>> >>>>>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>>>>> Add support for the SM8650 platform by re-using the SM8550 >>>>>>> definitions and using the vpu33 ops. >>>>>>> >>>>>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>>>>> decoder capabilities are identical. >>>>>>> >>>>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>> --- >>>>>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>>>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 +++++++ +++++++++++++++ >>>>>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>>>>> 3 files changed, 69 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>>> index >>>>>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>>>>> extern struct iris_platform_data sm8250_data; >>>>>>> extern struct iris_platform_data sm8550_data; >>>>>>> +extern struct iris_platform_data sm8650_data; >>>>>>> enum platform_clk_type { >>>>>>> IRIS_AXI_CLK, >>>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>>> index >>>>>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>>>>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>>>>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>>>>>> + >>>>>>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>>>>>> + >>>>>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>>>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>>>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>>>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>>>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>>>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>>>> }; >>>>>>> + >>>>>>> +/* >>>>>>> + * Shares most of SM8550 data except: >>>>>>> + * - vpu_ops to iris_vpu33_ops >>>>>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>>>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>>>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>>>>> + */ >>>>>>> +struct iris_platform_data sm8650_data = { >>>>>>> + .get_instance = iris_hfi_gen2_get_instance, >>>>>>> + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, >>>>>>> + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, >>>>>>> + .vpu_ops = &iris_vpu33_ops, >>>>>>> + .set_preset_registers = iris_set_sm8550_preset_registers, >>>>>>> + .icc_tbl = sm8550_icc_table, >>>>>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>>>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>>>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>>>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>>>>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>>>>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>>>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>>>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>>>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>>>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>>>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>>>>> + .clk_tbl = sm8550_clk_table, >>>>>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>>>>> + /* Upper bound of DMA address range */ >>>>>>> + .dma_mask = 0xe0000000 - 1, >>>>>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>>>>> + .pas_id = IRIS_PAS_ID, >>>>>>> + .inst_caps = &platform_inst_cap_sm8550, >>>>>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>>>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>>>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>>>>> + .core_arch = VIDEO_ARCH_LX, >>>>>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>>>>> + .ubwc_config = &ubwc_config_sm8550, >>>>>>> + .num_vpp_pipe = 4, >>>>>>> + .max_session_count = 16, >>>>>>> + .max_core_mbpf = ((8192 * 4352) / 256) * 2, >>>>>>> + .input_config_params = >>>>>>> + sm8550_vdec_input_config_params, >>>>>>> + .input_config_params_size = >>>>>>> + ARRAY_SIZE(sm8550_vdec_input_config_params), >>>>>>> + .output_config_params = >>>>>>> + sm8550_vdec_output_config_params, >>>>>>> + .output_config_params_size = >>>>>>> + ARRAY_SIZE(sm8550_vdec_output_config_params), >>>>>>> + .dec_input_prop = sm8550_vdec_subscribe_input_properties, >>>>>>> + .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), >>>>>>> + .dec_output_prop = sm8550_vdec_subscribe_output_properties, >>>>>>> + .dec_output_prop_size = >>>>>>> ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), >>>>>>> + >>>>>>> + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, >>>>>>> + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), >>>>>>> + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>>>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>>>> +}; >>>>>> While i was extending the data for QCS8300 (one another iris-v3 variant), i >>>>>> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >>>>>> platform data. It would be a good idea at this point to split it into something >>>>>> like this >>>>>> 1. Introduce SOC specific c file and move the respective SOC platform data to >>>>>> it, for ex, in this case sm8650_data >>>>>> 2. Move the common structs from iris_platform_sm8550.c to >>>>>> iris_platform_common.h. This way more SOCs getting added in future, can include >>>>>> the common header to reuse them, otherwise it would end up using 8550.c for all >>>>>> future SOC. >>>>>> >>>>>> Share your comments if you have any better approach to manage/re- use these >>>>>> platform data considering more SOCs getting added. >>>>> >>>>> Right, yes the architecture is fine, but I don't feel iris_platform_common is >>>>> the right >>>>> place, perhaps we could introduce a platform_catalog.c where we could place all >>>>> the common >>>>> platform data and reuse them from the platform_<soc>.c files ? >>>> Common structs would certainly need to be part of a header which can be >>>> included. Where do you plan to keep common struct to be used across SOC specific >>>> file in your approach ? >>>>> >>>>> I can design prototype on top of this patchset as an RFC. >>>> I was thinking that the changes are not that big, and can be done in existing >>>> series though. >>>> >>> For now, I think you can introduce a platform_sm8650.c as part of this >>> series and use the common structure from platform_sm8550.c. >>> Shouldn't be a big change. >> >> I tried but I don't how to solve this, you need a build-time ARRAY_SIZE for >> the arrays, so they need to be in the same .c file to allow a build-time >> evaluation of ARRAY_SIZE. If he common tables are moved into a header >> they will be duplicated into both platform_sm8650 & platform_sm8550 objects >> which is not what we want. >> >> The solution would be to write all the platform tables & iris_platform_data >> into headers, with common headers, then include those into a platform_catalog.c >> like is done for the drm/msm/dpu1 catalog. >> >> Neil >> >>> >>> Later you can post a separate patch series to add platform_catalog.c and >>> have common struct placed there which can be used across different SOC >>> platform files. >>> >>> or other way is, >>> Post a patch series to introduce platform_catalog.c with common struct and >>> then rebase your 8650 series on top of it. >>> >>> Thanks, >>> Dikshita >>>> Thanks, >>>> Vikash >>>>> >>>>> Neil >>>>> >>>>>> >>>>>> Regards, >>>>>> Vikash >>>>>> >>>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>>>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>>>>> index >>>>>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>>>>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>>>>>> .data = &sm8250_data, >>>>>>> }, >>>>>>> #endif >>>>>>> + { >>>>>>> + .compatible = "qcom,sm8650-iris", >>>>>>> + .data = &sm8650_data, >>>>>>> + }, >>>>>>> { }, >>>>>>> }; >>>>>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>>>>> >>>>> >> > > Eh as I read the suggestion about putting the structs - instantiating the structs in the header, I wondered if that would link but anyway. > > One way to solve this without going the dpu1 route right now is to rename the platform files > > iris_platform_sm8250.c -> iris_platform_common_hfi_gen1.c > iris_platform_sm8550.c -> iris_platform_common_hfi_gen2.c > > The differentiation around HFI into "generations" instead of incrementing the already existing HFIXXX version is unfortunate. > > At least this way we have a repository of common HFI code located in each file, in expectation of HFI GEN3 whenever. Yeah I somehow did something like that, please review: https://lore.kernel.org/all/20250410-topic-sm8x50-upstream-iris-catalog-v5-0-44a431574c25@linaro.org/ Neil > > --- > bod
diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h index fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 --- a/drivers/media/platform/qcom/iris/iris_platform_common.h +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h @@ -35,6 +35,7 @@ enum pipe_type { extern struct iris_platform_data sm8250_data; extern struct iris_platform_data sm8550_data; +extern struct iris_platform_data sm8650_data; enum platform_clk_type { IRIS_AXI_CLK, diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c index 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { static const char * const sm8550_clk_reset_table[] = { "bus" }; +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; + +static const char * const sm8650_controller_reset_table[] = { "xo" }; + static const struct bw_info sm8550_bw_table_dec[] = { { ((4096 * 2160) / 256) * 60, 1608000 }, { ((4096 * 2160) / 256) * 30, 826000 }, @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), }; + +/* + * Shares most of SM8550 data except: + * - vpu_ops to iris_vpu33_ops + * - clk_rst_tbl to sm8650_clk_reset_table + * - controller_rst_tbl to sm8650_controller_reset_table + * - fwname to "qcom/vpu/vpu33_p4.mbn" + */ +struct iris_platform_data sm8650_data = { + .get_instance = iris_hfi_gen2_get_instance, + .init_hfi_command_ops = iris_hfi_gen2_command_ops_init, + .init_hfi_response_ops = iris_hfi_gen2_response_ops_init, + .vpu_ops = &iris_vpu33_ops, + .set_preset_registers = iris_set_sm8550_preset_registers, + .icc_tbl = sm8550_icc_table, + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), + .clk_rst_tbl = sm8650_clk_reset_table, + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), + .controller_rst_tbl = sm8650_controller_reset_table, + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), + .bw_tbl_dec = sm8550_bw_table_dec, + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), + .pmdomain_tbl = sm8550_pmdomain_table, + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), + .opp_pd_tbl = sm8550_opp_pd_table, + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), + .clk_tbl = sm8550_clk_table, + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), + /* Upper bound of DMA address range */ + .dma_mask = 0xe0000000 - 1, + .fwname = "qcom/vpu/vpu33_p4.mbn", + .pas_id = IRIS_PAS_ID, + .inst_caps = &platform_inst_cap_sm8550, + .inst_fw_caps = inst_fw_cap_sm8550, + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), + .tz_cp_config_data = &tz_cp_config_sm8550, + .core_arch = VIDEO_ARCH_LX, + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, + .ubwc_config = &ubwc_config_sm8550, + .num_vpp_pipe = 4, + .max_session_count = 16, + .max_core_mbpf = ((8192 * 4352) / 256) * 2, + .input_config_params = + sm8550_vdec_input_config_params, + .input_config_params_size = + ARRAY_SIZE(sm8550_vdec_input_config_params), + .output_config_params = + sm8550_vdec_output_config_params, + .output_config_params_size = + ARRAY_SIZE(sm8550_vdec_output_config_params), + .dec_input_prop = sm8550_vdec_subscribe_input_properties, + .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties), + .dec_output_prop = sm8550_vdec_subscribe_output_properties, + .dec_output_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_output_properties), + + .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl, + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl), + .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), +}; diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c index 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 --- a/drivers/media/platform/qcom/iris/iris_probe.c +++ b/drivers/media/platform/qcom/iris/iris_probe.c @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { .data = &sm8250_data, }, #endif + { + .compatible = "qcom,sm8650-iris", + .data = &sm8650_data, + }, { }, }; MODULE_DEVICE_TABLE(of, iris_dt_match);