| Message ID | 20250410-topic-sm8x50-upstream-iris-catalog-v5-0-44a431574c25@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: qcom: iris: re-organize catalog & add support for SM8650 | expand |
On 10/04/2025 17:29, Neil Armstrong wrote: > Re-organize the platform support core into a gen1 catalog C file > declaring common platform structure and include platform headers > containing platform specific entries and iris_platform_data > structure. > > The goal is to share most of the structure while having > clear and separate per-SoC catalog files. > > The organization is based on the curent drm/msm dpu1 catalog > entries. > > Add support for the IRIS accelerator for the SM8650 > platform, which uses the iris33 hardware. > > The vpu33 requires a different reset & poweroff sequence > in order to properly get out of runtime suspend. > > Follow-up of [1]: > https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > Changes in v4: > - Reorganized into catalog, rebased sm8650 support on top > - Link to v4: https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org > > Changes in v4: > - collected tags > - un-split power_off in vpu3x > - removed useless function defines > - added back vpu3x disappeared rename commit > - Link to v3: https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org > > Changes in v3: > - Collected review tags > - Removed bulky reset_controller ops > - Removed iris_vpu_power_off_controller split > - Link to v2: https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org > > Changes in v2: > - Collected bindings review > - Reworked rest handling by adding a secondary optional table to be used by controller poweroff > - Reworked power_off_controller to be reused and extended by vpu33 support > - Removed useless and unneeded vpu33 init > - Moved vpu33 into vpu3x files to reuse code from vpu3 > - Moved sm8650 data table into sm8550 > - Link to v1: https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org > > --- > Neil Armstrong (8): > media: qcom: iris: move sm8250 to gen1 catalog > media: qcom: iris: move sm8550 to gen2 catalog > dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator > media: platform: qcom/iris: add power_off_controller to vpu_ops > media: platform: qcom/iris: introduce optional controller_rst_tbl > media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x > media: platform: qcom/iris: add support for vpu33 > media: platform: qcom/iris: add sm8650 support > > .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- > drivers/media/platform/qcom/iris/Makefile | 6 +- > .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ > ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ > ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- > .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ > .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ > drivers/media/platform/qcom/iris/iris_core.h | 1 + > .../platform/qcom/iris/iris_platform_common.h | 3 + > drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- > drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + > drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- > drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 +++++++++++++++++++++ > drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- > drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + > 15 files changed, 598 insertions(+), 300 deletions(-) > --- > base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c > change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f > > Best regards, > -- > Neil Armstrong <neil.armstrong@linaro.org> > > Please fixup this 0007-media-platform-qcom-iris-add-support-for-vpu33.patch has no obvious style problems and is ready for submission. 0007-media-platform-qcom-iris-add-support-for-vpu33.patch:7: slighly ==> slightly also accounting for my comments in patches #1 and #2 you can add for the series Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On 11/04/2025 12:55, Bryan O'Donoghue wrote: > On 10/04/2025 17:29, Neil Armstrong wrote: >> Re-organize the platform support core into a gen1 catalog C file >> declaring common platform structure and include platform headers >> containing platform specific entries and iris_platform_data >> structure. >> >> The goal is to share most of the structure while having >> clear and separate per-SoC catalog files. >> >> The organization is based on the curent drm/msm dpu1 catalog >> entries. >> >> Add support for the IRIS accelerator for the SM8650 >> platform, which uses the iris33 hardware. >> >> The vpu33 requires a different reset & poweroff sequence >> in order to properly get out of runtime suspend. >> >> Follow-up of [1]: >> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> Changes in v4: >> - Reorganized into catalog, rebased sm8650 support on top >> - Link to v4: https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org >> >> Changes in v4: >> - collected tags >> - un-split power_off in vpu3x >> - removed useless function defines >> - added back vpu3x disappeared rename commit >> - Link to v3: https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org >> >> Changes in v3: >> - Collected review tags >> - Removed bulky reset_controller ops >> - Removed iris_vpu_power_off_controller split >> - Link to v2: https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org >> >> Changes in v2: >> - Collected bindings review >> - Reworked rest handling by adding a secondary optional table to be used by controller poweroff >> - Reworked power_off_controller to be reused and extended by vpu33 support >> - Removed useless and unneeded vpu33 init >> - Moved vpu33 into vpu3x files to reuse code from vpu3 >> - Moved sm8650 data table into sm8550 >> - Link to v1: https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org >> >> --- >> Neil Armstrong (8): >> media: qcom: iris: move sm8250 to gen1 catalog >> media: qcom: iris: move sm8550 to gen2 catalog >> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator >> media: platform: qcom/iris: add power_off_controller to vpu_ops >> media: platform: qcom/iris: introduce optional controller_rst_tbl >> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x >> media: platform: qcom/iris: add support for vpu33 >> media: platform: qcom/iris: add sm8650 support >> >> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- >> drivers/media/platform/qcom/iris/Makefile | 6 +- >> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ >> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ >> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- >> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ >> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ >> drivers/media/platform/qcom/iris/iris_core.h | 1 + >> .../platform/qcom/iris/iris_platform_common.h | 3 + >> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- >> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + >> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- >> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 +++++++++++++++++++++ >> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- >> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + >> 15 files changed, 598 insertions(+), 300 deletions(-) >> --- >> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c >> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f >> >> Best regards, >> -- >> Neil Armstrong <neil.armstrong@linaro.org> >> >> > > Please fixup this > > 0007-media-platform-qcom-iris-add-support-for-vpu33.patch has no obvious > style problems and is ready for submission. > 0007-media-platform-qcom-iris-add-support-for-vpu33.patch:7: slighly ==> > slightly > > also accounting for my comments in patches #1 and #2 you can add for the > series > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > There's an update to the yaml you need to account for now. https://gitlab.freedesktop.org/linux-media/users/bodonoghue/-/blob/next/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml?ref_type=heads#L25 --- bod
Hi Vikash, Dikshita, On 10/04/2025 18:29, Neil Armstrong wrote: > Re-organize the platform support core into a gen1 catalog C file > declaring common platform structure and include platform headers > containing platform specific entries and iris_platform_data > structure. > > The goal is to share most of the structure while having > clear and separate per-SoC catalog files. > > The organization is based on the curent drm/msm dpu1 catalog > entries. Any feedback on this patchset ? Thanks, Neil > > Add support for the IRIS accelerator for the SM8650 > platform, which uses the iris33 hardware. > > The vpu33 requires a different reset & poweroff sequence > in order to properly get out of runtime suspend. > > Follow-up of [1]: > https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > Changes in v4: > - Reorganized into catalog, rebased sm8650 support on top > - Link to v4: https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org > > Changes in v4: > - collected tags > - un-split power_off in vpu3x > - removed useless function defines > - added back vpu3x disappeared rename commit > - Link to v3: https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org > > Changes in v3: > - Collected review tags > - Removed bulky reset_controller ops > - Removed iris_vpu_power_off_controller split > - Link to v2: https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org > > Changes in v2: > - Collected bindings review > - Reworked rest handling by adding a secondary optional table to be used by controller poweroff > - Reworked power_off_controller to be reused and extended by vpu33 support > - Removed useless and unneeded vpu33 init > - Moved vpu33 into vpu3x files to reuse code from vpu3 > - Moved sm8650 data table into sm8550 > - Link to v1: https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org > > --- > Neil Armstrong (8): > media: qcom: iris: move sm8250 to gen1 catalog > media: qcom: iris: move sm8550 to gen2 catalog > dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator > media: platform: qcom/iris: add power_off_controller to vpu_ops > media: platform: qcom/iris: introduce optional controller_rst_tbl > media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x > media: platform: qcom/iris: add support for vpu33 > media: platform: qcom/iris: add sm8650 support > > .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- > drivers/media/platform/qcom/iris/Makefile | 6 +- > .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ > ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ > ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- > .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ > .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ > drivers/media/platform/qcom/iris/iris_core.h | 1 + > .../platform/qcom/iris/iris_platform_common.h | 3 + > drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- > drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + > drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- > drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 +++++++++++++++++++++ > drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- > drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + > 15 files changed, 598 insertions(+), 300 deletions(-) > --- > base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c > change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f > > Best regards,
Hi Neil, On 4/14/2025 1:05 PM, Neil Armstrong wrote: > Hi Vikash, Dikshita, > > On 10/04/2025 18:29, Neil Armstrong wrote: >> Re-organize the platform support core into a gen1 catalog C file >> declaring common platform structure and include platform headers >> containing platform specific entries and iris_platform_data >> structure. >> >> The goal is to share most of the structure while having >> clear and separate per-SoC catalog files. >> >> The organization is based on the curent drm/msm dpu1 catalog >> entries. > > Any feedback on this patchset ? Myself and Dikshita went through the approach you are bringing here, let me update some context here: - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the scaled down variant i.e have 2 PIPE vs others having 4. Similarly there are other irisv3 having 1 pipe as well. - With above variations, firmware and instance caps would change for the variant SOCs. - Above these, few(less) bindings/connections specific delta would be there, like there is reset delta in sm8550 and sm8650. Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific tables and SOC platform data, i.e sm8650_data (for sm8650). On top of this, individual SOC specific .c file can have any delta, from xxx_gen1/2.c) like reset table or preset register table, etc and export these delta structs in xxx_gen1.c or xxx_gen2.c. Going with above approach, sm8650.c would have only one reset table for now. Later if any delta is identified, the same can be added in it. All other common structs, can reside in xxx_gen2.c for now. Regards, Vikash > > Thanks, > Neil > >> >> Add support for the IRIS accelerator for the SM8650 >> platform, which uses the iris33 hardware. >> >> The vpu33 requires a different reset & poweroff sequence >> in order to properly get out of runtime suspend. >> >> Follow-up of [1]: >> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> Changes in v4: >> - Reorganized into catalog, rebased sm8650 support on top >> - Link to v4: >> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org >> >> Changes in v4: >> - collected tags >> - un-split power_off in vpu3x >> - removed useless function defines >> - added back vpu3x disappeared rename commit >> - Link to v3: >> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org >> >> Changes in v3: >> - Collected review tags >> - Removed bulky reset_controller ops >> - Removed iris_vpu_power_off_controller split >> - Link to v2: >> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org >> >> Changes in v2: >> - Collected bindings review >> - Reworked rest handling by adding a secondary optional table to be used by >> controller poweroff >> - Reworked power_off_controller to be reused and extended by vpu33 support >> - Removed useless and unneeded vpu33 init >> - Moved vpu33 into vpu3x files to reuse code from vpu3 >> - Moved sm8650 data table into sm8550 >> - Link to v1: >> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org >> >> --- >> Neil Armstrong (8): >> media: qcom: iris: move sm8250 to gen1 catalog >> media: qcom: iris: move sm8550 to gen2 catalog >> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator >> media: platform: qcom/iris: add power_off_controller to vpu_ops >> media: platform: qcom/iris: introduce optional controller_rst_tbl >> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x >> media: platform: qcom/iris: add support for vpu33 >> media: platform: qcom/iris: add sm8650 support >> >> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- >> drivers/media/platform/qcom/iris/Makefile | 6 +- >> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ >> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ >> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- >> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ >> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ >> drivers/media/platform/qcom/iris/iris_core.h | 1 + >> .../platform/qcom/iris/iris_platform_common.h | 3 + >> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- >> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + >> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- >> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 +++++++++++++++++++++ >> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- >> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + >> 15 files changed, 598 insertions(+), 300 deletions(-) >> --- >> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c >> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f >> >> Best regards, >
Hi, On 14/04/2025 12:54, Vikash Garodia wrote: > Hi Neil, > > On 4/14/2025 1:05 PM, Neil Armstrong wrote: >> Hi Vikash, Dikshita, >> >> On 10/04/2025 18:29, Neil Armstrong wrote: >>> Re-organize the platform support core into a gen1 catalog C file >>> declaring common platform structure and include platform headers >>> containing platform specific entries and iris_platform_data >>> structure. >>> >>> The goal is to share most of the structure while having >>> clear and separate per-SoC catalog files. >>> >>> The organization is based on the curent drm/msm dpu1 catalog >>> entries. >> >> Any feedback on this patchset ? > Myself and Dikshita went through the approach you are bringing here, let me > update some context here: > - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the scaled > down variant i.e have 2 PIPE vs others having 4. Similarly there are other > irisv3 having 1 pipe as well. > - With above variations, firmware and instance caps would change for the variant > SOCs. > - Above these, few(less) bindings/connections specific delta would be there, > like there is reset delta in sm8550 and sm8650. > > Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific tables and > SOC platform data, i.e sm8650_data (for sm8650). On top of this, individual SOC > specific .c file can have any delta, from xxx_gen1/2.c) like reset table or > preset register table, etc and export these delta structs in xxx_gen1.c or > xxx_gen2.c. > > Going with above approach, sm8650.c would have only one reset table for now. > Later if any delta is identified, the same can be added in it. All other common > structs, can reside in xxx_gen2.c for now. Thanks for reviewing, but... Sorry I don't understand what you and Dmitry are asking me... If I try really hard, you would like to have: iris_catalog_sm8550.c - iris_set_sm8550_preset_registers - sm8550_icc_table - sm8550_clk_reset_table - sm8550_bw_table_dec - sm8550_pmdomain_table - sm8550_opp_pd_table - sm8550_clk_table iris_catalog_sm8650.c - sm8650_clk_reset_table - sm8650_controller_reset_table iris_catalog_gen2.c - iris_hfi_gen2_command_ops_init - iris_hfi_gen2_response_ops_init ... - sm8550_dec_op_int_buf_tbl and: - struct iris_platform_data sm8550_data - struct iris_platform_data sm8650_data using data from iris_catalog_sm8550.c & iris_catalog_sm8550.c So this is basically what I _already_ propose except you move data in separate .c files for no reasons, please explain why you absolutely want distinct .c files per SoC. We are no more in the 1990's and we camn defintely have big .c files. And we still have a big issue, how to get the: - ARRAY_SIZE(sm8550_clk_reset_table) - ARRAY_SIZE(sm8550_bw_table_dec) - ARRAY_SIZE(sm8550_pmdomain_table) ... since they are declared in a separate .c file and you need a compile-time const value to fill all the _size attribute in iris_platform_data. So I recall my goal, I just want to add sm8650 support, and I'm not the owner of this driver, and I'm really happy to help, but giving me random ideas to solve your problem doesn't help us at all going forward. Neil > > Regards, > Vikash >> >> Thanks, >> Neil >> >>> >>> Add support for the IRIS accelerator for the SM8650 >>> platform, which uses the iris33 hardware. >>> >>> The vpu33 requires a different reset & poweroff sequence >>> in order to properly get out of runtime suspend. >>> >>> Follow-up of [1]: >>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ >>> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> Changes in v4: >>> - Reorganized into catalog, rebased sm8650 support on top >>> - Link to v4: >>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org >>> >>> Changes in v4: >>> - collected tags >>> - un-split power_off in vpu3x >>> - removed useless function defines >>> - added back vpu3x disappeared rename commit >>> - Link to v3: >>> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org >>> >>> Changes in v3: >>> - Collected review tags >>> - Removed bulky reset_controller ops >>> - Removed iris_vpu_power_off_controller split >>> - Link to v2: >>> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org >>> >>> Changes in v2: >>> - Collected bindings review >>> - Reworked rest handling by adding a secondary optional table to be used by >>> controller poweroff >>> - Reworked power_off_controller to be reused and extended by vpu33 support >>> - Removed useless and unneeded vpu33 init >>> - Moved vpu33 into vpu3x files to reuse code from vpu3 >>> - Moved sm8650 data table into sm8550 >>> - Link to v1: >>> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org >>> >>> --- >>> Neil Armstrong (8): >>> media: qcom: iris: move sm8250 to gen1 catalog >>> media: qcom: iris: move sm8550 to gen2 catalog >>> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator >>> media: platform: qcom/iris: add power_off_controller to vpu_ops >>> media: platform: qcom/iris: introduce optional controller_rst_tbl >>> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x >>> media: platform: qcom/iris: add support for vpu33 >>> media: platform: qcom/iris: add sm8650 support >>> >>> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- >>> drivers/media/platform/qcom/iris/Makefile | 6 +- >>> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ >>> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ >>> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- >>> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ >>> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ >>> drivers/media/platform/qcom/iris/iris_core.h | 1 + >>> .../platform/qcom/iris/iris_platform_common.h | 3 + >>> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- >>> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + >>> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- >>> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 +++++++++++++++++++++ >>> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- >>> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + >>> 15 files changed, 598 insertions(+), 300 deletions(-) >>> --- >>> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c >>> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f >>> >>> Best regards, >>
On Mon, Apr 14, 2025 at 04:24:40PM +0530, Vikash Garodia wrote: > Hi Neil, > > On 4/14/2025 1:05 PM, Neil Armstrong wrote: > > Hi Vikash, Dikshita, > > > > On 10/04/2025 18:29, Neil Armstrong wrote: > >> Re-organize the platform support core into a gen1 catalog C file > >> declaring common platform structure and include platform headers > >> containing platform specific entries and iris_platform_data > >> structure. > >> > >> The goal is to share most of the structure while having > >> clear and separate per-SoC catalog files. > >> > >> The organization is based on the curent drm/msm dpu1 catalog > >> entries. > > > > Any feedback on this patchset ? > Myself and Dikshita went through the approach you are bringing here, let me > update some context here: > - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the scaled > down variant i.e have 2 PIPE vs others having 4. Similarly there are other > irisv3 having 1 pipe as well. > - With above variations, firmware and instance caps would change for the variant > SOCs. > - Above these, few(less) bindings/connections specific delta would be there, > like there is reset delta in sm8550 and sm8650. > > Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific tables and > SOC platform data, i.e sm8650_data (for sm8650). On top of this, individual SOC > specific .c file can have any delta, from xxx_gen1/2.c) like reset table or > preset register table, etc and export these delta structs in xxx_gen1.c or > xxx_gen2.c. > > Going with above approach, sm8650.c would have only one reset table for now. > Later if any delta is identified, the same can be added in it. All other common > structs, can reside in xxx_gen2.c for now. SGTM. > > Regards, > Vikash > > > > Thanks, > > Neil > > > >> > >> Add support for the IRIS accelerator for the SM8650 > >> platform, which uses the iris33 hardware. > >> > >> The vpu33 requires a different reset & poweroff sequence > >> in order to properly get out of runtime suspend. > >> > >> Follow-up of [1]: > >> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ > >> > >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >> --- > >> Changes in v4: > >> - Reorganized into catalog, rebased sm8650 support on top > >> - Link to v4: > >> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org > >> > >> Changes in v4: > >> - collected tags > >> - un-split power_off in vpu3x > >> - removed useless function defines > >> - added back vpu3x disappeared rename commit > >> - Link to v3: > >> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org > >> > >> Changes in v3: > >> - Collected review tags > >> - Removed bulky reset_controller ops > >> - Removed iris_vpu_power_off_controller split > >> - Link to v2: > >> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org > >> > >> Changes in v2: > >> - Collected bindings review > >> - Reworked rest handling by adding a secondary optional table to be used by > >> controller poweroff > >> - Reworked power_off_controller to be reused and extended by vpu33 support > >> - Removed useless and unneeded vpu33 init > >> - Moved vpu33 into vpu3x files to reuse code from vpu3 > >> - Moved sm8650 data table into sm8550 > >> - Link to v1: > >> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org > >> > >> --- > >> Neil Armstrong (8): > >> media: qcom: iris: move sm8250 to gen1 catalog > >> media: qcom: iris: move sm8550 to gen2 catalog > >> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator > >> media: platform: qcom/iris: add power_off_controller to vpu_ops > >> media: platform: qcom/iris: introduce optional controller_rst_tbl > >> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x > >> media: platform: qcom/iris: add support for vpu33 > >> media: platform: qcom/iris: add sm8650 support > >> > >> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- > >> drivers/media/platform/qcom/iris/Makefile | 6 +- > >> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ > >> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ > >> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- > >> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ > >> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ > >> drivers/media/platform/qcom/iris/iris_core.h | 1 + > >> .../platform/qcom/iris/iris_platform_common.h | 3 + > >> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- > >> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + > >> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- > >> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 +++++++++++++++++++++ > >> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- > >> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + > >> 15 files changed, 598 insertions(+), 300 deletions(-) > >> --- > >> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c > >> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f > >> > >> Best regards, > >
On 4/14/2025 5:39 PM, neil.armstrong@linaro.org wrote: > Hi, > > On 14/04/2025 12:54, Vikash Garodia wrote: >> Hi Neil, >> >> On 4/14/2025 1:05 PM, Neil Armstrong wrote: >>> Hi Vikash, Dikshita, >>> >>> On 10/04/2025 18:29, Neil Armstrong wrote: >>>> Re-organize the platform support core into a gen1 catalog C file >>>> declaring common platform structure and include platform headers >>>> containing platform specific entries and iris_platform_data >>>> structure. >>>> >>>> The goal is to share most of the structure while having >>>> clear and separate per-SoC catalog files. >>>> >>>> The organization is based on the curent drm/msm dpu1 catalog >>>> entries. >>> >>> Any feedback on this patchset ? >> Myself and Dikshita went through the approach you are bringing here, let me >> update some context here: >> - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the scaled >> down variant i.e have 2 PIPE vs others having 4. Similarly there are other >> irisv3 having 1 pipe as well. >> - With above variations, firmware and instance caps would change for the variant >> SOCs. >> - Above these, few(less) bindings/connections specific delta would be there, >> like there is reset delta in sm8550 and sm8650. >> >> Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific tables and >> SOC platform data, i.e sm8650_data (for sm8650). On top of this, individual SOC >> specific .c file can have any delta, from xxx_gen1/2.c) like reset table or >> preset register table, etc and export these delta structs in xxx_gen1.c or >> xxx_gen2.c. >> >> Going with above approach, sm8650.c would have only one reset table for now. >> Later if any delta is identified, the same can be added in it. All other common >> structs, can reside in xxx_gen2.c for now. > > Thanks for reviewing, but... > Sorry I don't understand what you and Dmitry are asking me... > > If I try really hard, you would like to have: > > iris_catalog_sm8550.c > - iris_set_sm8550_preset_registers > - sm8550_icc_table > - sm8550_clk_reset_table > - sm8550_bw_table_dec > - sm8550_pmdomain_table > - sm8550_opp_pd_table > - sm8550_clk_table Move or rename existing 8550.c as xxx_gen2.c. This is with the existing assumption that everything under 8550.c is common for all gen2 to come in future. > > iris_catalog_sm8650.c > - sm8650_clk_reset_table > - sm8650_controller_reset_table yes, since reset is the only delta. > > iris_catalog_gen2.c > - iris_hfi_gen2_command_ops_init > - iris_hfi_gen2_response_ops_init > ... > - sm8550_dec_op_int_buf_tbl > > and: > - struct iris_platform_data sm8550_data > - struct iris_platform_data sm8650_data all this goes to xxx_gen2.c as well. > using data from iris_catalog_sm8550.c & iris_catalog_sm8550.c > > So this is basically what I _already_ propose except > you move data in separate .c files for no reasons, > please explain why you absolutely want distinct .c > files per SoC. We are no more in the 1990's and we camn > defintely have big .c files. Its not about the size of file alone, it is easy to understand later what would be the delta in the SOCs and what would common. For ex, just navigating through sm8650.c, anyone can comment that reset is the delta. > > And we still have a big issue, how to get the: > - ARRAY_SIZE(sm8550_clk_reset_table) > - ARRAY_SIZE(sm8550_bw_table_dec) > - ARRAY_SIZE(sm8550_pmdomain_table) > ... > > since they are declared in a separate .c file and you > need a compile-time const value to fill all the _size > attribute in iris_platform_data. I have not tries this, but isn't extern-ing the soc structs (in your case reset tables) into xxx_gen2.c enough here ? Also i think the tables you are pointing here, lies in the xxx_gen2.c only, so i am sure above ones would not be an issue at all. The only delta struct is reset table, lets see if extern helps. Regards, Vikash > > So I recall my goal, I just want to add sm8650 support, > and I'm not the owner of this driver, and I'm really happy > to help, but giving me random ideas to solve your problem > doesn't help us at all going forward. > > Neil > >> >> Regards, >> Vikash >>> >>> Thanks, >>> Neil >>> >>>> >>>> Add support for the IRIS accelerator for the SM8650 >>>> platform, which uses the iris33 hardware. >>>> >>>> The vpu33 requires a different reset & poweroff sequence >>>> in order to properly get out of runtime suspend. >>>> >>>> Follow-up of [1]: >>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ >>>> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> Changes in v4: >>>> - Reorganized into catalog, rebased sm8650 support on top >>>> - Link to v4: >>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org >>>> >>>> Changes in v4: >>>> - collected tags >>>> - un-split power_off in vpu3x >>>> - removed useless function defines >>>> - added back vpu3x disappeared rename commit >>>> - Link to v3: >>>> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org >>>> >>>> Changes in v3: >>>> - Collected review tags >>>> - Removed bulky reset_controller ops >>>> - Removed iris_vpu_power_off_controller split >>>> - Link to v2: >>>> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org >>>> >>>> Changes in v2: >>>> - Collected bindings review >>>> - Reworked rest handling by adding a secondary optional table to be used by >>>> controller poweroff >>>> - Reworked power_off_controller to be reused and extended by vpu33 support >>>> - Removed useless and unneeded vpu33 init >>>> - Moved vpu33 into vpu3x files to reuse code from vpu3 >>>> - Moved sm8650 data table into sm8550 >>>> - Link to v1: >>>> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org >>>> >>>> --- >>>> Neil Armstrong (8): >>>> media: qcom: iris: move sm8250 to gen1 catalog >>>> media: qcom: iris: move sm8550 to gen2 catalog >>>> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator >>>> media: platform: qcom/iris: add power_off_controller to vpu_ops >>>> media: platform: qcom/iris: introduce optional controller_rst_tbl >>>> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x >>>> media: platform: qcom/iris: add support for vpu33 >>>> media: platform: qcom/iris: add sm8650 support >>>> >>>> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- >>>> drivers/media/platform/qcom/iris/Makefile | 6 +- >>>> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ >>>> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ >>>> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- >>>> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ >>>> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ >>>> drivers/media/platform/qcom/iris/iris_core.h | 1 + >>>> .../platform/qcom/iris/iris_platform_common.h | 3 + >>>> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- >>>> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + >>>> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- >>>> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 >>>> +++++++++++++++++++++ >>>> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- >>>> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + >>>> 15 files changed, 598 insertions(+), 300 deletions(-) >>>> --- >>>> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c >>>> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f >>>> >>>> Best regards, >>> >
Hi, On 14/04/2025 21:48, Vikash Garodia wrote: > > On 4/14/2025 5:39 PM, neil.armstrong@linaro.org wrote: >> Hi, >> >> On 14/04/2025 12:54, Vikash Garodia wrote: >>> Hi Neil, >>> >>> On 4/14/2025 1:05 PM, Neil Armstrong wrote: >>>> Hi Vikash, Dikshita, >>>> >>>> On 10/04/2025 18:29, Neil Armstrong wrote: >>>>> Re-organize the platform support core into a gen1 catalog C file >>>>> declaring common platform structure and include platform headers >>>>> containing platform specific entries and iris_platform_data >>>>> structure. >>>>> >>>>> The goal is to share most of the structure while having >>>>> clear and separate per-SoC catalog files. >>>>> >>>>> The organization is based on the curent drm/msm dpu1 catalog >>>>> entries. >>>> >>>> Any feedback on this patchset ? >>> Myself and Dikshita went through the approach you are bringing here, let me >>> update some context here: >>> - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the scaled >>> down variant i.e have 2 PIPE vs others having 4. Similarly there are other >>> irisv3 having 1 pipe as well. >>> - With above variations, firmware and instance caps would change for the variant >>> SOCs. >>> - Above these, few(less) bindings/connections specific delta would be there, >>> like there is reset delta in sm8550 and sm8650. >>> >>> Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific tables and >>> SOC platform data, i.e sm8650_data (for sm8650). On top of this, individual SOC >>> specific .c file can have any delta, from xxx_gen1/2.c) like reset table or >>> preset register table, etc and export these delta structs in xxx_gen1.c or >>> xxx_gen2.c. >>> >>> Going with above approach, sm8650.c would have only one reset table for now. >>> Later if any delta is identified, the same can be added in it. All other common >>> structs, can reside in xxx_gen2.c for now. >> >> Thanks for reviewing, but... >> Sorry I don't understand what you and Dmitry are asking me... >> >> If I try really hard, you would like to have: >> >> iris_catalog_sm8550.c >> - iris_set_sm8550_preset_registers >> - sm8550_icc_table >> - sm8550_clk_reset_table >> - sm8550_bw_table_dec >> - sm8550_pmdomain_table >> - sm8550_opp_pd_table >> - sm8550_clk_table > Move or rename existing 8550.c as xxx_gen2.c. This is with the existing > assumption that everything under 8550.c is common for all gen2 to come in future. >> >> iris_catalog_sm8650.c >> - sm8650_clk_reset_table >> - sm8650_controller_reset_table > yes, since reset is the only delta. >> >> iris_catalog_gen2.c >> - iris_hfi_gen2_command_ops_init >> - iris_hfi_gen2_response_ops_init >> ... >> - sm8550_dec_op_int_buf_tbl >> >> and: >> - struct iris_platform_data sm8550_data >> - struct iris_platform_data sm8650_data > all this goes to xxx_gen2.c as well. Yeah so this is exactly my current approach, except it use .h files for each SoC for simplicity. > >> using data from iris_catalog_sm8550.c & iris_catalog_sm8550.c >> >> So this is basically what I _already_ propose except >> you move data in separate .c files for no reasons, >> please explain why you absolutely want distinct .c >> files per SoC. We are no more in the 1990's and we camn >> defintely have big .c files. > Its not about the size of file alone, it is easy to understand later what would > be the delta in the SOCs and what would common. For ex, just navigating through > sm8650.c, anyone can comment that reset is the delta. What's the problem with the current approach with .h file for each SoC ? >> >> And we still have a big issue, how to get the: >> - ARRAY_SIZE(sm8550_clk_reset_table) >> - ARRAY_SIZE(sm8550_bw_table_dec) >> - ARRAY_SIZE(sm8550_pmdomain_table) >> ... >> >> since they are declared in a separate .c file and you >> need a compile-time const value to fill all the _size >> attribute in iris_platform_data. > I have not tries this, but isn't extern-ing the soc structs (in your case reset > tables) into xxx_gen2.c enough here ? Also i think the tables you are pointing > here, lies in the xxx_gen2.c only, so i am sure above ones would not be an issue > at all. The only delta struct is reset table, lets see if extern helps. No it doesn't, because I wrote C for the last 25 years, and I tried it already, I also tried to export a const int with the table size, and it also doesn't work. The 3 only ways are: 1) add defines with sizes for each table 2) add a NULL entry at the end of each table, and update all code using those tables 3) declare in the same scope, which is my current proposal Neil > > Regards, > Vikash >> >> So I recall my goal, I just want to add sm8650 support, >> and I'm not the owner of this driver, and I'm really happy >> to help, but giving me random ideas to solve your problem >> doesn't help us at all going forward. >> >> Neil >> >>> >>> Regards, >>> Vikash >>>> >>>> Thanks, >>>> Neil >>>> >>>>> >>>>> Add support for the IRIS accelerator for the SM8650 >>>>> platform, which uses the iris33 hardware. >>>>> >>>>> The vpu33 requires a different reset & poweroff sequence >>>>> in order to properly get out of runtime suspend. >>>>> >>>>> Follow-up of [1]: >>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ >>>>> >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>> --- >>>>> Changes in v4: >>>>> - Reorganized into catalog, rebased sm8650 support on top >>>>> - Link to v4: >>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org >>>>> >>>>> Changes in v4: >>>>> - collected tags >>>>> - un-split power_off in vpu3x >>>>> - removed useless function defines >>>>> - added back vpu3x disappeared rename commit >>>>> - Link to v3: >>>>> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org >>>>> >>>>> Changes in v3: >>>>> - Collected review tags >>>>> - Removed bulky reset_controller ops >>>>> - Removed iris_vpu_power_off_controller split >>>>> - Link to v2: >>>>> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org >>>>> >>>>> Changes in v2: >>>>> - Collected bindings review >>>>> - Reworked rest handling by adding a secondary optional table to be used by >>>>> controller poweroff >>>>> - Reworked power_off_controller to be reused and extended by vpu33 support >>>>> - Removed useless and unneeded vpu33 init >>>>> - Moved vpu33 into vpu3x files to reuse code from vpu3 >>>>> - Moved sm8650 data table into sm8550 >>>>> - Link to v1: >>>>> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org >>>>> >>>>> --- >>>>> Neil Armstrong (8): >>>>> media: qcom: iris: move sm8250 to gen1 catalog >>>>> media: qcom: iris: move sm8550 to gen2 catalog >>>>> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator >>>>> media: platform: qcom/iris: add power_off_controller to vpu_ops >>>>> media: platform: qcom/iris: introduce optional controller_rst_tbl >>>>> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x >>>>> media: platform: qcom/iris: add support for vpu33 >>>>> media: platform: qcom/iris: add sm8650 support >>>>> >>>>> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- >>>>> drivers/media/platform/qcom/iris/Makefile | 6 +- >>>>> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ >>>>> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ >>>>> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- >>>>> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ >>>>> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ >>>>> drivers/media/platform/qcom/iris/iris_core.h | 1 + >>>>> .../platform/qcom/iris/iris_platform_common.h | 3 + >>>>> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- >>>>> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + >>>>> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- >>>>> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 >>>>> +++++++++++++++++++++ >>>>> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- >>>>> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + >>>>> 15 files changed, 598 insertions(+), 300 deletions(-) >>>>> --- >>>>> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c >>>>> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f >>>>> >>>>> Best regards, >>>> >>
On 4/15/2025 1:54 PM, neil.armstrong@linaro.org wrote: > Hi, > > On 14/04/2025 21:48, Vikash Garodia wrote: >> >> On 4/14/2025 5:39 PM, neil.armstrong@linaro.org wrote: >>> Hi, >>> >>> On 14/04/2025 12:54, Vikash Garodia wrote: >>>> Hi Neil, >>>> >>>> On 4/14/2025 1:05 PM, Neil Armstrong wrote: >>>>> Hi Vikash, Dikshita, >>>>> >>>>> On 10/04/2025 18:29, Neil Armstrong wrote: >>>>>> Re-organize the platform support core into a gen1 catalog C file >>>>>> declaring common platform structure and include platform headers >>>>>> containing platform specific entries and iris_platform_data >>>>>> structure. >>>>>> >>>>>> The goal is to share most of the structure while having >>>>>> clear and separate per-SoC catalog files. >>>>>> >>>>>> The organization is based on the curent drm/msm dpu1 catalog >>>>>> entries. >>>>> >>>>> Any feedback on this patchset ? >>>> Myself and Dikshita went through the approach you are bringing here, let me >>>> update some context here: >>>> - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the scaled >>>> down variant i.e have 2 PIPE vs others having 4. Similarly there are other >>>> irisv3 having 1 pipe as well. >>>> - With above variations, firmware and instance caps would change for the >>>> variant >>>> SOCs. >>>> - Above these, few(less) bindings/connections specific delta would be there, >>>> like there is reset delta in sm8550 and sm8650. >>>> >>>> Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific tables and >>>> SOC platform data, i.e sm8650_data (for sm8650). On top of this, individual SOC >>>> specific .c file can have any delta, from xxx_gen1/2.c) like reset table or >>>> preset register table, etc and export these delta structs in xxx_gen1.c or >>>> xxx_gen2.c. >>>> >>>> Going with above approach, sm8650.c would have only one reset table for now. >>>> Later if any delta is identified, the same can be added in it. All other common >>>> structs, can reside in xxx_gen2.c for now. >>> >>> Thanks for reviewing, but... >>> Sorry I don't understand what you and Dmitry are asking me... >>> >>> If I try really hard, you would like to have: >>> >>> iris_catalog_sm8550.c >>> - iris_set_sm8550_preset_registers >>> - sm8550_icc_table >>> - sm8550_clk_reset_table >>> - sm8550_bw_table_dec >>> - sm8550_pmdomain_table >>> - sm8550_opp_pd_table >>> - sm8550_clk_table >> Move or rename existing 8550.c as xxx_gen2.c. This is with the existing >> assumption that everything under 8550.c is common for all gen2 to come in future. >>> >>> iris_catalog_sm8650.c >>> - sm8650_clk_reset_table >>> - sm8650_controller_reset_table >> yes, since reset is the only delta. >>> >>> iris_catalog_gen2.c >>> - iris_hfi_gen2_command_ops_init >>> - iris_hfi_gen2_response_ops_init >>> ... >>> - sm8550_dec_op_int_buf_tbl >>> >>> and: >>> - struct iris_platform_data sm8550_data >>> - struct iris_platform_data sm8650_data >> all this goes to xxx_gen2.c as well. > > Yeah so this is exactly my current approach, except it use .h files > for each SoC for simplicity. > >> >>> using data from iris_catalog_sm8550.c & iris_catalog_sm8550.c >>> >>> So this is basically what I _already_ propose except >>> you move data in separate .c files for no reasons, >>> please explain why you absolutely want distinct .c >>> files per SoC. We are no more in the 1990's and we camn >>> defintely have big .c files. >> Its not about the size of file alone, it is easy to understand later what would >> be the delta in the SOCs and what would common. For ex, just navigating through >> sm8650.c, anyone can comment that reset is the delta. > > What's the problem with the current approach with .h file for each SoC ? > >>> >>> And we still have a big issue, how to get the: >>> - ARRAY_SIZE(sm8550_clk_reset_table) >>> - ARRAY_SIZE(sm8550_bw_table_dec) >>> - ARRAY_SIZE(sm8550_pmdomain_table) >>> ... >>> >>> since they are declared in a separate .c file and you >>> need a compile-time const value to fill all the _size >>> attribute in iris_platform_data. >> I have not tries this, but isn't extern-ing the soc structs (in your case reset >> tables) into xxx_gen2.c enough here ? Also i think the tables you are pointing >> here, lies in the xxx_gen2.c only, so i am sure above ones would not be an issue >> at all. The only delta struct is reset table, lets see if extern helps. > > No it doesn't, because I wrote C for the last 25 years, and I tried it already, > I also tried to export a const int with the table size, and it also doesn't work. Got it, i tried too, it didn't work. > > The 3 only ways are: > 1) add defines with sizes for each table This leaves manual update everytime. > 2) add a NULL entry at the end of each table, and update all code using those > tables Does not sound right to update the code, just to get the size. > 3) declare in the same scope, which is my current proposalThe proposal in the RFC is about moving the common structs to 8550.h, rather, it can be kept in xxx_gen2.c. 8550.h can have the delta part (i.e reset tables) and can be included in xxx_gen2.c. sm8650_data can reside in xxx_gen2.c, soc headers just brings the delta structs which can be overridden from common in xxx_gen2.c I am good with the header approach which contains the delta over and above xxx_gen2.c. Regards, Vikash > Neil > >> >> Regards, >> Vikash >>> >>> So I recall my goal, I just want to add sm8650 support, >>> and I'm not the owner of this driver, and I'm really happy >>> to help, but giving me random ideas to solve your problem >>> doesn't help us at all going forward. >>> >>> Neil >>> >>>> >>>> Regards, >>>> Vikash >>>>> >>>>> Thanks, >>>>> Neil >>>>> >>>>>> >>>>>> Add support for the IRIS accelerator for the SM8650 >>>>>> platform, which uses the iris33 hardware. >>>>>> >>>>>> The vpu33 requires a different reset & poweroff sequence >>>>>> in order to properly get out of runtime suspend. >>>>>> >>>>>> Follow-up of [1]: >>>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ >>>>>> >>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>> --- >>>>>> Changes in v4: >>>>>> - Reorganized into catalog, rebased sm8650 support on top >>>>>> - Link to v4: >>>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org >>>>>> >>>>>> Changes in v4: >>>>>> - collected tags >>>>>> - un-split power_off in vpu3x >>>>>> - removed useless function defines >>>>>> - added back vpu3x disappeared rename commit >>>>>> - Link to v3: >>>>>> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org >>>>>> >>>>>> Changes in v3: >>>>>> - Collected review tags >>>>>> - Removed bulky reset_controller ops >>>>>> - Removed iris_vpu_power_off_controller split >>>>>> - Link to v2: >>>>>> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org >>>>>> >>>>>> Changes in v2: >>>>>> - Collected bindings review >>>>>> - Reworked rest handling by adding a secondary optional table to be used by >>>>>> controller poweroff >>>>>> - Reworked power_off_controller to be reused and extended by vpu33 support >>>>>> - Removed useless and unneeded vpu33 init >>>>>> - Moved vpu33 into vpu3x files to reuse code from vpu3 >>>>>> - Moved sm8650 data table into sm8550 >>>>>> - Link to v1: >>>>>> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org >>>>>> >>>>>> --- >>>>>> Neil Armstrong (8): >>>>>> media: qcom: iris: move sm8250 to gen1 catalog >>>>>> media: qcom: iris: move sm8550 to gen2 catalog >>>>>> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS >>>>>> accelerator >>>>>> media: platform: qcom/iris: add power_off_controller to vpu_ops >>>>>> media: platform: qcom/iris: introduce optional controller_rst_tbl >>>>>> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x >>>>>> media: platform: qcom/iris: add support for vpu33 >>>>>> media: platform: qcom/iris: add sm8650 support >>>>>> >>>>>> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- >>>>>> drivers/media/platform/qcom/iris/Makefile | 6 +- >>>>>> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ >>>>>> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ >>>>>> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- >>>>>> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ >>>>>> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ >>>>>> drivers/media/platform/qcom/iris/iris_core.h | 1 + >>>>>> .../platform/qcom/iris/iris_platform_common.h | 3 + >>>>>> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- >>>>>> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + >>>>>> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- >>>>>> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 >>>>>> +++++++++++++++++++++ >>>>>> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- >>>>>> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + >>>>>> 15 files changed, 598 insertions(+), 300 deletions(-) >>>>>> --- >>>>>> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c >>>>>> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f >>>>>> >>>>>> Best regards, >>>>> >>> >
Hi, On 15/04/2025 13:26, Vikash Garodia wrote: > > On 4/15/2025 1:54 PM, neil.armstrong@linaro.org wrote: >> Hi, >> >> On 14/04/2025 21:48, Vikash Garodia wrote: >>> >>> On 4/14/2025 5:39 PM, neil.armstrong@linaro.org wrote: >>>> Hi, >>>> >>>> On 14/04/2025 12:54, Vikash Garodia wrote: >>>>> Hi Neil, >>>>> >>>>> On 4/14/2025 1:05 PM, Neil Armstrong wrote: >>>>>> Hi Vikash, Dikshita, >>>>>> >>>>>> On 10/04/2025 18:29, Neil Armstrong wrote: >>>>>>> Re-organize the platform support core into a gen1 catalog C file >>>>>>> declaring common platform structure and include platform headers >>>>>>> containing platform specific entries and iris_platform_data >>>>>>> structure. >>>>>>> >>>>>>> The goal is to share most of the structure while having >>>>>>> clear and separate per-SoC catalog files. >>>>>>> >>>>>>> The organization is based on the curent drm/msm dpu1 catalog >>>>>>> entries. >>>>>> >>>>>> Any feedback on this patchset ? >>>>> Myself and Dikshita went through the approach you are bringing here, let me >>>>> update some context here: >>>>> - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the scaled >>>>> down variant i.e have 2 PIPE vs others having 4. Similarly there are other >>>>> irisv3 having 1 pipe as well. >>>>> - With above variations, firmware and instance caps would change for the >>>>> variant >>>>> SOCs. >>>>> - Above these, few(less) bindings/connections specific delta would be there, >>>>> like there is reset delta in sm8550 and sm8650. >>>>> >>>>> Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific tables and >>>>> SOC platform data, i.e sm8650_data (for sm8650). On top of this, individual SOC >>>>> specific .c file can have any delta, from xxx_gen1/2.c) like reset table or >>>>> preset register table, etc and export these delta structs in xxx_gen1.c or >>>>> xxx_gen2.c. >>>>> >>>>> Going with above approach, sm8650.c would have only one reset table for now. >>>>> Later if any delta is identified, the same can be added in it. All other common >>>>> structs, can reside in xxx_gen2.c for now. >>>> >>>> Thanks for reviewing, but... >>>> Sorry I don't understand what you and Dmitry are asking me... >>>> >>>> If I try really hard, you would like to have: >>>> >>>> iris_catalog_sm8550.c >>>> - iris_set_sm8550_preset_registers >>>> - sm8550_icc_table >>>> - sm8550_clk_reset_table >>>> - sm8550_bw_table_dec >>>> - sm8550_pmdomain_table >>>> - sm8550_opp_pd_table >>>> - sm8550_clk_table >>> Move or rename existing 8550.c as xxx_gen2.c. This is with the existing >>> assumption that everything under 8550.c is common for all gen2 to come in future. >>>> >>>> iris_catalog_sm8650.c >>>> - sm8650_clk_reset_table >>>> - sm8650_controller_reset_table >>> yes, since reset is the only delta. >>>> >>>> iris_catalog_gen2.c >>>> - iris_hfi_gen2_command_ops_init >>>> - iris_hfi_gen2_response_ops_init >>>> ... >>>> - sm8550_dec_op_int_buf_tbl >>>> >>>> and: >>>> - struct iris_platform_data sm8550_data >>>> - struct iris_platform_data sm8650_data >>> all this goes to xxx_gen2.c as well. >> >> Yeah so this is exactly my current approach, except it use .h files >> for each SoC for simplicity. >> >>> >>>> using data from iris_catalog_sm8550.c & iris_catalog_sm8550.c >>>> >>>> So this is basically what I _already_ propose except >>>> you move data in separate .c files for no reasons, >>>> please explain why you absolutely want distinct .c >>>> files per SoC. We are no more in the 1990's and we camn >>>> defintely have big .c files. >>> Its not about the size of file alone, it is easy to understand later what would >>> be the delta in the SOCs and what would common. For ex, just navigating through >>> sm8650.c, anyone can comment that reset is the delta. >> >> What's the problem with the current approach with .h file for each SoC ? >> >>>> >>>> And we still have a big issue, how to get the: >>>> - ARRAY_SIZE(sm8550_clk_reset_table) >>>> - ARRAY_SIZE(sm8550_bw_table_dec) >>>> - ARRAY_SIZE(sm8550_pmdomain_table) >>>> ... >>>> >>>> since they are declared in a separate .c file and you >>>> need a compile-time const value to fill all the _size >>>> attribute in iris_platform_data. >>> I have not tries this, but isn't extern-ing the soc structs (in your case reset >>> tables) into xxx_gen2.c enough here ? Also i think the tables you are pointing >>> here, lies in the xxx_gen2.c only, so i am sure above ones would not be an issue >>> at all. The only delta struct is reset table, lets see if extern helps. >> >> No it doesn't, because I wrote C for the last 25 years, and I tried it already, >> I also tried to export a const int with the table size, and it also doesn't work. > Got it, i tried too, it didn't work. >> >> The 3 only ways are: >> 1) add defines with sizes for each table > This leaves manual update everytime. > >> 2) add a NULL entry at the end of each table, and update all code using those >> tables > Does not sound right to update the code, just to get the size. > >> 3) declare in the same scope, which is my current proposalThe proposal in the RFC is about moving the common structs to 8550.h, rather, it > can be kept in xxx_gen2.c. > 8550.h can have the delta part (i.e reset tables) and can be included in > xxx_gen2.c. sm8650_data can reside in xxx_gen2.c, soc headers just brings the > delta structs which can be overridden from common in xxx_gen2.c > I am good with the header approach which contains the delta over and above > xxx_gen2.c. I'll try to do that, but now I don't see the point of the SoC header files if they only contain the reset tables. Neil > > Regards, > Vikash >> Neil >> >>> >>> Regards, >>> Vikash >>>> >>>> So I recall my goal, I just want to add sm8650 support, >>>> and I'm not the owner of this driver, and I'm really happy >>>> to help, but giving me random ideas to solve your problem >>>> doesn't help us at all going forward. >>>> >>>> Neil >>>> >>>>> >>>>> Regards, >>>>> Vikash >>>>>> >>>>>> Thanks, >>>>>> Neil >>>>>> >>>>>>> >>>>>>> Add support for the IRIS accelerator for the SM8650 >>>>>>> platform, which uses the iris33 hardware. >>>>>>> >>>>>>> The vpu33 requires a different reset & poweroff sequence >>>>>>> in order to properly get out of runtime suspend. >>>>>>> >>>>>>> Follow-up of [1]: >>>>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ >>>>>>> >>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>> --- >>>>>>> Changes in v4: >>>>>>> - Reorganized into catalog, rebased sm8650 support on top >>>>>>> - Link to v4: >>>>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org >>>>>>> >>>>>>> Changes in v4: >>>>>>> - collected tags >>>>>>> - un-split power_off in vpu3x >>>>>>> - removed useless function defines >>>>>>> - added back vpu3x disappeared rename commit >>>>>>> - Link to v3: >>>>>>> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org >>>>>>> >>>>>>> Changes in v3: >>>>>>> - Collected review tags >>>>>>> - Removed bulky reset_controller ops >>>>>>> - Removed iris_vpu_power_off_controller split >>>>>>> - Link to v2: >>>>>>> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org >>>>>>> >>>>>>> Changes in v2: >>>>>>> - Collected bindings review >>>>>>> - Reworked rest handling by adding a secondary optional table to be used by >>>>>>> controller poweroff >>>>>>> - Reworked power_off_controller to be reused and extended by vpu33 support >>>>>>> - Removed useless and unneeded vpu33 init >>>>>>> - Moved vpu33 into vpu3x files to reuse code from vpu3 >>>>>>> - Moved sm8650 data table into sm8550 >>>>>>> - Link to v1: >>>>>>> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org >>>>>>> >>>>>>> --- >>>>>>> Neil Armstrong (8): >>>>>>> media: qcom: iris: move sm8250 to gen1 catalog >>>>>>> media: qcom: iris: move sm8550 to gen2 catalog >>>>>>> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS >>>>>>> accelerator >>>>>>> media: platform: qcom/iris: add power_off_controller to vpu_ops >>>>>>> media: platform: qcom/iris: introduce optional controller_rst_tbl >>>>>>> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x >>>>>>> media: platform: qcom/iris: add support for vpu33 >>>>>>> media: platform: qcom/iris: add sm8650 support >>>>>>> >>>>>>> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- >>>>>>> drivers/media/platform/qcom/iris/Makefile | 6 +- >>>>>>> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ >>>>>>> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ >>>>>>> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- >>>>>>> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ >>>>>>> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ >>>>>>> drivers/media/platform/qcom/iris/iris_core.h | 1 + >>>>>>> .../platform/qcom/iris/iris_platform_common.h | 3 + >>>>>>> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- >>>>>>> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + >>>>>>> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- >>>>>>> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 >>>>>>> +++++++++++++++++++++ >>>>>>> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- >>>>>>> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + >>>>>>> 15 files changed, 598 insertions(+), 300 deletions(-) >>>>>>> --- >>>>>>> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c >>>>>>> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f >>>>>>> >>>>>>> Best regards, >>>>>> >>>> >>
On 4/15/2025 5:25 PM, neil.armstrong@linaro.org wrote: > Hi, > > On 15/04/2025 13:26, Vikash Garodia wrote: >> >> On 4/15/2025 1:54 PM, neil.armstrong@linaro.org wrote: >>> Hi, >>> >>> On 14/04/2025 21:48, Vikash Garodia wrote: >>>> >>>> On 4/14/2025 5:39 PM, neil.armstrong@linaro.org wrote: >>>>> Hi, >>>>> >>>>> On 14/04/2025 12:54, Vikash Garodia wrote: >>>>>> Hi Neil, >>>>>> >>>>>> On 4/14/2025 1:05 PM, Neil Armstrong wrote: >>>>>>> Hi Vikash, Dikshita, >>>>>>> >>>>>>> On 10/04/2025 18:29, Neil Armstrong wrote: >>>>>>>> Re-organize the platform support core into a gen1 catalog C file >>>>>>>> declaring common platform structure and include platform headers >>>>>>>> containing platform specific entries and iris_platform_data >>>>>>>> structure. >>>>>>>> >>>>>>>> The goal is to share most of the structure while having >>>>>>>> clear and separate per-SoC catalog files. >>>>>>>> >>>>>>>> The organization is based on the curent drm/msm dpu1 catalog >>>>>>>> entries. >>>>>>> >>>>>>> Any feedback on this patchset ? >>>>>> Myself and Dikshita went through the approach you are bringing here, let me >>>>>> update some context here: >>>>>> - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the >>>>>> scaled >>>>>> down variant i.e have 2 PIPE vs others having 4. Similarly there are other >>>>>> irisv3 having 1 pipe as well. >>>>>> - With above variations, firmware and instance caps would change for the >>>>>> variant >>>>>> SOCs. >>>>>> - Above these, few(less) bindings/connections specific delta would be there, >>>>>> like there is reset delta in sm8550 and sm8650. >>>>>> >>>>>> Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific >>>>>> tables and >>>>>> SOC platform data, i.e sm8650_data (for sm8650). On top of this, >>>>>> individual SOC >>>>>> specific .c file can have any delta, from xxx_gen1/2.c) like reset table or >>>>>> preset register table, etc and export these delta structs in xxx_gen1.c or >>>>>> xxx_gen2.c. >>>>>> >>>>>> Going with above approach, sm8650.c would have only one reset table for now. >>>>>> Later if any delta is identified, the same can be added in it. All other >>>>>> common >>>>>> structs, can reside in xxx_gen2.c for now. >>>>> >>>>> Thanks for reviewing, but... >>>>> Sorry I don't understand what you and Dmitry are asking me... >>>>> >>>>> If I try really hard, you would like to have: >>>>> >>>>> iris_catalog_sm8550.c >>>>> - iris_set_sm8550_preset_registers >>>>> - sm8550_icc_table >>>>> - sm8550_clk_reset_table >>>>> - sm8550_bw_table_dec >>>>> - sm8550_pmdomain_table >>>>> - sm8550_opp_pd_table >>>>> - sm8550_clk_table >>>> Move or rename existing 8550.c as xxx_gen2.c. This is with the existing >>>> assumption that everything under 8550.c is common for all gen2 to come in >>>> future. >>>>> >>>>> iris_catalog_sm8650.c >>>>> - sm8650_clk_reset_table >>>>> - sm8650_controller_reset_table >>>> yes, since reset is the only delta. >>>>> >>>>> iris_catalog_gen2.c >>>>> - iris_hfi_gen2_command_ops_init >>>>> - iris_hfi_gen2_response_ops_init >>>>> ... >>>>> - sm8550_dec_op_int_buf_tbl >>>>> >>>>> and: >>>>> - struct iris_platform_data sm8550_data >>>>> - struct iris_platform_data sm8650_data >>>> all this goes to xxx_gen2.c as well. >>> >>> Yeah so this is exactly my current approach, except it use .h files >>> for each SoC for simplicity. >>> >>>> >>>>> using data from iris_catalog_sm8550.c & iris_catalog_sm8550.c >>>>> >>>>> So this is basically what I _already_ propose except >>>>> you move data in separate .c files for no reasons, >>>>> please explain why you absolutely want distinct .c >>>>> files per SoC. We are no more in the 1990's and we camn >>>>> defintely have big .c files. >>>> Its not about the size of file alone, it is easy to understand later what would >>>> be the delta in the SOCs and what would common. For ex, just navigating through >>>> sm8650.c, anyone can comment that reset is the delta. >>> >>> What's the problem with the current approach with .h file for each SoC ? >>> >>>>> >>>>> And we still have a big issue, how to get the: >>>>> - ARRAY_SIZE(sm8550_clk_reset_table) >>>>> - ARRAY_SIZE(sm8550_bw_table_dec) >>>>> - ARRAY_SIZE(sm8550_pmdomain_table) >>>>> ... >>>>> >>>>> since they are declared in a separate .c file and you >>>>> need a compile-time const value to fill all the _size >>>>> attribute in iris_platform_data. >>>> I have not tries this, but isn't extern-ing the soc structs (in your case reset >>>> tables) into xxx_gen2.c enough here ? Also i think the tables you are pointing >>>> here, lies in the xxx_gen2.c only, so i am sure above ones would not be an >>>> issue >>>> at all. The only delta struct is reset table, lets see if extern helps. >>> >>> No it doesn't, because I wrote C for the last 25 years, and I tried it already, >>> I also tried to export a const int with the table size, and it also doesn't >>> work. >> Got it, i tried too, it didn't work. >>> >>> The 3 only ways are: >>> 1) add defines with sizes for each table >> This leaves manual update everytime. >> >>> 2) add a NULL entry at the end of each table, and update all code using those >>> tables >> Does not sound right to update the code, just to get the size. >> >>> 3) declare in the same scope, which is my current proposalThe proposal in the >>> RFC is about moving the common structs to 8550.h, rather, it >> can be kept in xxx_gen2.c. >> 8550.h can have the delta part (i.e reset tables) and can be included in >> xxx_gen2.c. sm8650_data can reside in xxx_gen2.c, soc headers just brings the >> delta structs which can be overridden from common in xxx_gen2.c >> I am good with the header approach which contains the delta over and above >> xxx_gen2.c. > > I'll try to do that, but now I don't see the point of the SoC header files if > they only contain the reset tables. We already know that preset registers are also coming in (it is still a mystery though on why h264 dec works without it, i have not got chance to explore it more), so it would be useful when you extend it later. Regards, Vikash > > Neil > >> >> Regards, >> Vikash >>> Neil >>> >>>> >>>> Regards, >>>> Vikash >>>>> >>>>> So I recall my goal, I just want to add sm8650 support, >>>>> and I'm not the owner of this driver, and I'm really happy >>>>> to help, but giving me random ideas to solve your problem >>>>> doesn't help us at all going forward. >>>>> >>>>> Neil >>>>> >>>>>> >>>>>> Regards, >>>>>> Vikash >>>>>>> >>>>>>> Thanks, >>>>>>> Neil >>>>>>> >>>>>>>> >>>>>>>> Add support for the IRIS accelerator for the SM8650 >>>>>>>> platform, which uses the iris33 hardware. >>>>>>>> >>>>>>>> The vpu33 requires a different reset & poweroff sequence >>>>>>>> in order to properly get out of runtime suspend. >>>>>>>> >>>>>>>> Follow-up of [1]: >>>>>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ >>>>>>>> >>>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>>> --- >>>>>>>> Changes in v4: >>>>>>>> - Reorganized into catalog, rebased sm8650 support on top >>>>>>>> - Link to v4: >>>>>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org >>>>>>>> >>>>>>>> Changes in v4: >>>>>>>> - collected tags >>>>>>>> - un-split power_off in vpu3x >>>>>>>> - removed useless function defines >>>>>>>> - added back vpu3x disappeared rename commit >>>>>>>> - Link to v3: >>>>>>>> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org >>>>>>>> >>>>>>>> Changes in v3: >>>>>>>> - Collected review tags >>>>>>>> - Removed bulky reset_controller ops >>>>>>>> - Removed iris_vpu_power_off_controller split >>>>>>>> - Link to v2: >>>>>>>> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - Collected bindings review >>>>>>>> - Reworked rest handling by adding a secondary optional table to be used by >>>>>>>> controller poweroff >>>>>>>> - Reworked power_off_controller to be reused and extended by vpu33 support >>>>>>>> - Removed useless and unneeded vpu33 init >>>>>>>> - Moved vpu33 into vpu3x files to reuse code from vpu3 >>>>>>>> - Moved sm8650 data table into sm8550 >>>>>>>> - Link to v1: >>>>>>>> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org >>>>>>>> >>>>>>>> --- >>>>>>>> Neil Armstrong (8): >>>>>>>> media: qcom: iris: move sm8250 to gen1 catalog >>>>>>>> media: qcom: iris: move sm8550 to gen2 catalog >>>>>>>> dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS >>>>>>>> accelerator >>>>>>>> media: platform: qcom/iris: add power_off_controller to vpu_ops >>>>>>>> media: platform: qcom/iris: introduce optional controller_rst_tbl >>>>>>>> media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x >>>>>>>> media: platform: qcom/iris: add support for vpu33 >>>>>>>> media: platform: qcom/iris: add sm8650 support >>>>>>>> >>>>>>>> .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- >>>>>>>> drivers/media/platform/qcom/iris/Makefile | 6 +- >>>>>>>> .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ >>>>>>>> ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ >>>>>>>> ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- >>>>>>>> .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ >>>>>>>> .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ >>>>>>>> drivers/media/platform/qcom/iris/iris_core.h | 1 + >>>>>>>> .../platform/qcom/iris/iris_platform_common.h | 3 + >>>>>>>> drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- >>>>>>>> drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + >>>>>>>> drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- >>>>>>>> drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 >>>>>>>> +++++++++++++++++++++ >>>>>>>> drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- >>>>>>>> drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + >>>>>>>> 15 files changed, 598 insertions(+), 300 deletions(-) >>>>>>>> --- >>>>>>>> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c >>>>>>>> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f >>>>>>>> >>>>>>>> Best regards, >>>>>>> >>>>> >>> >
Re-organize the platform support core into a gen1 catalog C file declaring common platform structure and include platform headers containing platform specific entries and iris_platform_data structure. The goal is to share most of the structure while having clear and separate per-SoC catalog files. The organization is based on the curent drm/msm dpu1 catalog entries. Add support for the IRIS accelerator for the SM8650 platform, which uses the iris33 hardware. The vpu33 requires a different reset & poweroff sequence in order to properly get out of runtime suspend. Follow-up of [1]: https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/ Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- Changes in v4: - Reorganized into catalog, rebased sm8650 support on top - Link to v4: https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org Changes in v4: - collected tags - un-split power_off in vpu3x - removed useless function defines - added back vpu3x disappeared rename commit - Link to v3: https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org Changes in v3: - Collected review tags - Removed bulky reset_controller ops - Removed iris_vpu_power_off_controller split - Link to v2: https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org Changes in v2: - Collected bindings review - Reworked rest handling by adding a secondary optional table to be used by controller poweroff - Reworked power_off_controller to be reused and extended by vpu33 support - Removed useless and unneeded vpu33 init - Moved vpu33 into vpu3x files to reuse code from vpu3 - Moved sm8650 data table into sm8550 - Link to v1: https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org --- Neil Armstrong (8): media: qcom: iris: move sm8250 to gen1 catalog media: qcom: iris: move sm8550 to gen2 catalog dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator media: platform: qcom/iris: add power_off_controller to vpu_ops media: platform: qcom/iris: introduce optional controller_rst_tbl media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x media: platform: qcom/iris: add support for vpu33 media: platform: qcom/iris: add sm8650 support .../bindings/media/qcom,sm8550-iris.yaml | 33 ++- drivers/media/platform/qcom/iris/Makefile | 6 +- .../media/platform/qcom/iris/iris_catalog_gen1.c | 83 +++++++ ...{iris_platform_sm8550.c => iris_catalog_gen2.c} | 85 +------ ...ris_platform_sm8250.c => iris_catalog_sm8250.h} | 80 +----- .../media/platform/qcom/iris/iris_catalog_sm8550.h | 91 +++++++ .../media/platform/qcom/iris/iris_catalog_sm8650.h | 68 +++++ drivers/media/platform/qcom/iris/iris_core.h | 1 + .../platform/qcom/iris/iris_platform_common.h | 3 + drivers/media/platform/qcom/iris/iris_probe.c | 43 +++- drivers/media/platform/qcom/iris/iris_vpu2.c | 1 + drivers/media/platform/qcom/iris/iris_vpu3.c | 122 --------- drivers/media/platform/qcom/iris/iris_vpu3x.c | 275 +++++++++++++++++++++ drivers/media/platform/qcom/iris/iris_vpu_common.c | 4 +- drivers/media/platform/qcom/iris/iris_vpu_common.h | 3 + 15 files changed, 598 insertions(+), 300 deletions(-) --- base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f Best regards,