| Message ID | 20240827-iris_v3-v3-4-c5fdbbe65e70@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Qualcomm iris video decoder driver | expand |
On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote: > From: Dikshita Agarwal <quic_dikshita@quicinc.com> > > Add support for initializing Iris "resources", which are clocks, > interconnects, power domains, reset clocks, and clock frequencies > used for iris hardware. > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > --- ... > +struct iris_platform_data sm8550_data = { > + .icc_tbl = sm8550_icc_table, > + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), > + .clk_rst_tbl = sm8550_clk_reset_table, > + .clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table), > + .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), > +}; > diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c > index 0a54fdaa1ab5..2616a31224f9 100644 > --- a/drivers/media/platform/qcom/iris/iris_probe.c > +++ b/drivers/media/platform/qcom/iris/iris_probe.c > @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev) > if (core->irq < 0) > return core->irq; > > + core->iris_platform_data = of_device_get_match_data(core->dev); > + if (!core->iris_platform_data) { > + ret = -ENODEV; > + dev_err_probe(core->dev, ret, "init platform failed\n"); That's not even possible. I would suggest dropping entire if. But if yoi insist, then without this weird redundant code. return -EINVAL. > + return ret; > + } > + > + ret = iris_init_resources(core); > + if (ret) { > + dev_err_probe(core->dev, ret, "init resource failed\n"); > + return ret; How many same errors are you printing? Not mentioning that syntax of dev_errp_rpboe is different... > + } > + > ret = v4l2_device_register(dev, &core->v4l2_dev); > if (ret) > return ret; > @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev) > } > > static const struct of_device_id iris_dt_match[] = { > - { .compatible = "qcom,sm8550-iris", }, > - { .compatible = "qcom,sm8250-venus", }, > + { > + .compatible = "qcom,sm8550-iris", > + .data = &sm8550_data, > + }, > + { > + .compatible = "qcom,sm8250-venus", > + .data = &sm8250_data, You just added this. No, please do not add code which is immediatly incorrect. > + }, > { }, > }; > MODULE_DEVICE_TABLE(of, iris_dt_match); > diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c > new file mode 100644 > index 000000000000..57c6f9f3449b > --- /dev/null > +++ b/drivers/media/platform/qcom/iris/iris_resources.c > @@ -0,0 +1,171 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/clk.h> > +#include <linux/interconnect.h> > +#include <linux/pm_domain.h> > +#include <linux/pm_opp.h> > +#include <linux/reset.h> > + > +#include "iris_core.h" > +#include "iris_resources.h" > + > +static int iris_init_icc(struct iris_core *core) > +{ > + const struct icc_info *icc_tbl; > + u32 ret, i = 0; > + > + icc_tbl = core->iris_platform_data->icc_tbl; > + > + core->icc_count = core->iris_platform_data->icc_tbl_size; > + core->icc_tbl = devm_kzalloc(core->dev, > + sizeof(struct icc_bulk_data) * core->icc_count, > + GFP_KERNEL); > + if (!core->icc_tbl) > + return -ENOMEM; > + > + for (i = 0; i < core->icc_count; i++) { > + core->icc_tbl[i].name = icc_tbl[i].name; > + core->icc_tbl[i].avg_bw = icc_tbl[i].bw_min_kbps; > + core->icc_tbl[i].peak_bw = 0; > + } > + > + ret = devm_of_icc_bulk_get(core->dev, core->icc_count, core->icc_tbl); > + if (ret) > + dev_err(core->dev, "failed to get interconnect paths, NoC will stay unconfigured!\n"); > + > + return ret; > +} > + > +static int iris_pd_get(struct iris_core *core) > +{ > + int ret; > + > + struct dev_pm_domain_attach_data iris_pd_data = { > + .pd_names = core->iris_platform_data->pmdomain_tbl, > + .num_pd_names = core->iris_platform_data->pmdomain_tbl_size, > + .pd_flags = PD_FLAG_NO_DEV_LINK, > + }; > + > + ret = devm_pm_domain_attach_list(core->dev, &iris_pd_data, &core->pmdomain_tbl); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int iris_opp_pd_get(struct iris_core *core) > +{ > + int ret; > + > + struct dev_pm_domain_attach_data iris_opp_pd_data = { > + .pd_names = core->iris_platform_data->opp_pd_tbl, > + .num_pd_names = core->iris_platform_data->opp_pd_tbl_size, > + .pd_flags = PD_FLAG_DEV_LINK_ON, > + }; > + > + ret = devm_pm_domain_attach_list(core->dev, &iris_opp_pd_data, &core->opp_pmdomain_tbl); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int iris_init_power_domains(struct iris_core *core) > +{ > + const struct platform_clk_data *clk_tbl; > + u32 clk_cnt, i; > + int ret; > + > + ret = iris_pd_get(core); > + if (ret) > + return ret; > + > + ret = iris_opp_pd_get(core); > + if (ret) > + return ret; > + > + clk_tbl = core->iris_platform_data->clk_tbl; > + clk_cnt = core->iris_platform_data->clk_tbl_size; > + > + for (i = 0; i < clk_cnt; i++) { > + if (clk_tbl[i].clk_type == IRIS_HW_CLK) { > + ret = devm_pm_opp_set_clkname(core->dev, clk_tbl[i].clk_name); > + if (ret) > + return ret; > + } > + } > + > + ret = devm_pm_opp_of_add_table(core->dev); > + if (ret) { > + dev_err(core->dev, "failed to add opp table\n"); > + return ret; > + } > + > + return ret; > +} > + > +static int iris_init_clocks(struct iris_core *core) > +{ > + int ret; > + > + ret = devm_clk_bulk_get_all(core->dev, &core->clock_tbl); > + if (ret < 0) { > + dev_err(core->dev, "failed to get bulk clock\n"); Syntax is: return dev_err_probe(). If this is probe path. Is it? > + return ret; > + } > + > + core->clk_count = ret; > + > + return 0; > +} > + > +static int iris_init_resets(struct iris_core *core) > +{ > + const char * const *rst_tbl; > + u32 rst_tbl_size; > + u32 i = 0, ret; > + > + rst_tbl = core->iris_platform_data->clk_rst_tbl; > + rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size; > + > + core->resets = devm_kzalloc(core->dev, > + sizeof(*core->resets) * rst_tbl_size, > + GFP_KERNEL); > + if (rst_tbl_size && !core->resets) > + return -ENOMEM; > + > + for (i = 0; i < rst_tbl_size; i++) > + core->resets[i].id = rst_tbl[i]; > + > + ret = devm_reset_control_bulk_get_exclusive(core->dev, rst_tbl_size, core->resets); > + if (ret) { > + dev_err(core->dev, "failed to get resets\n"); Syntax is: return dev_err_probe(). If this is probe path. Is it? > + return ret; > + } > + > + return 0; > +} > + > +int iris_init_resources(struct iris_core *core) > +{ > + int ret; > + > + ret = iris_init_icc(core); > + if (ret) > + return ret; > + > + ret = iris_init_power_domains(core); > + if (ret) > + return ret; > + > + ret = iris_init_clocks(core); > + if (ret) > + return ret; > + > + ret = iris_init_resets(core); > + > + return ret; > +} This should be just part of of main unit file, next to probe. It is unusual to see probe parts not next to probe. Sorry, that's wrong. Best regards, Krzysztof
On 8/27/2024 4:21 PM, Krzysztof Kozlowski wrote: > On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote: >> From: Dikshita Agarwal <quic_dikshita@quicinc.com> >> >> Add support for initializing Iris "resources", which are clocks, >> interconnects, power domains, reset clocks, and clock frequencies >> used for iris hardware. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> --- > > ... > >> +struct iris_platform_data sm8550_data = { >> + .icc_tbl = sm8550_icc_table, >> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >> + .clk_rst_tbl = sm8550_clk_reset_table, >> + .clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table), >> + .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), >> +}; >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c >> index 0a54fdaa1ab5..2616a31224f9 100644 >> --- a/drivers/media/platform/qcom/iris/iris_probe.c >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev) >> if (core->irq < 0) >> return core->irq; >> >> + core->iris_platform_data = of_device_get_match_data(core->dev); >> + if (!core->iris_platform_data) { >> + ret = -ENODEV; >> + dev_err_probe(core->dev, ret, "init platform failed\n"); > > That's not even possible. I would suggest dropping entire if. But if yoi > insist, then without this weird redundant code. return -EINVAL. > Its possible if platform data is not initialized and this is only place we check it, there is no further NULL check for the same. >> + return ret; >> + } >> + >> + ret = iris_init_resources(core); >> + if (ret) { >> + dev_err_probe(core->dev, ret, "init resource failed\n"); >> + return ret; > > How many same errors are you printing? Not mentioning that syntax of > dev_errp_rpboe is different... We have these errors at multiple points to know at what point the probe failed which is useful while debugging probe failures. But Sure we will revisit this code and fix the syntax of dev_err_probe. > > >> + } >> + >> ret = v4l2_device_register(dev, &core->v4l2_dev); >> if (ret) >> return ret; >> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev) >> } >> >> static const struct of_device_id iris_dt_match[] = { >> - { .compatible = "qcom,sm8550-iris", }, >> - { .compatible = "qcom,sm8250-venus", }, >> + { >> + .compatible = "qcom,sm8550-iris", >> + .data = &sm8550_data, >> + }, >> + { >> + .compatible = "qcom,sm8250-venus", >> + .data = &sm8250_data, > > You just added this. No, please do not add code which is immediatly > incorrect. It's not incorrect, in earlier patch we only added the compatible strings and with this patch introducing the platform data and APIs to get it. > >> + }, >> { }, >> }; >> MODULE_DEVICE_TABLE(of, iris_dt_match); >> diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c >> new file mode 100644 >> index 000000000000..57c6f9f3449b >> --- /dev/null >> +++ b/drivers/media/platform/qcom/iris/iris_resources.c >> @@ -0,0 +1,171 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/interconnect.h> >> +#include <linux/pm_domain.h> >> +#include <linux/pm_opp.h> >> +#include <linux/reset.h> >> + >> +#include "iris_core.h" >> +#include "iris_resources.h" >> + >> +static int iris_init_icc(struct iris_core *core) >> +{ >> + const struct icc_info *icc_tbl; >> + u32 ret, i = 0; >> + >> + icc_tbl = core->iris_platform_data->icc_tbl; >> + >> + core->icc_count = core->iris_platform_data->icc_tbl_size; >> + core->icc_tbl = devm_kzalloc(core->dev, >> + sizeof(struct icc_bulk_data) * core->icc_count, >> + GFP_KERNEL); >> + if (!core->icc_tbl) >> + return -ENOMEM; >> + >> + for (i = 0; i < core->icc_count; i++) { >> + core->icc_tbl[i].name = icc_tbl[i].name; >> + core->icc_tbl[i].avg_bw = icc_tbl[i].bw_min_kbps; >> + core->icc_tbl[i].peak_bw = 0; >> + } >> + >> + ret = devm_of_icc_bulk_get(core->dev, core->icc_count, core->icc_tbl); >> + if (ret) >> + dev_err(core->dev, "failed to get interconnect paths, NoC will stay unconfigured!\n"); >> + >> + return ret; >> +} >> + >> +static int iris_pd_get(struct iris_core *core) >> +{ >> + int ret; >> + >> + struct dev_pm_domain_attach_data iris_pd_data = { >> + .pd_names = core->iris_platform_data->pmdomain_tbl, >> + .num_pd_names = core->iris_platform_data->pmdomain_tbl_size, >> + .pd_flags = PD_FLAG_NO_DEV_LINK, >> + }; >> + >> + ret = devm_pm_domain_attach_list(core->dev, &iris_pd_data, &core->pmdomain_tbl); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int iris_opp_pd_get(struct iris_core *core) >> +{ >> + int ret; >> + >> + struct dev_pm_domain_attach_data iris_opp_pd_data = { >> + .pd_names = core->iris_platform_data->opp_pd_tbl, >> + .num_pd_names = core->iris_platform_data->opp_pd_tbl_size, >> + .pd_flags = PD_FLAG_DEV_LINK_ON, >> + }; >> + >> + ret = devm_pm_domain_attach_list(core->dev, &iris_opp_pd_data, &core->opp_pmdomain_tbl); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int iris_init_power_domains(struct iris_core *core) >> +{ >> + const struct platform_clk_data *clk_tbl; >> + u32 clk_cnt, i; >> + int ret; >> + >> + ret = iris_pd_get(core); >> + if (ret) >> + return ret; >> + >> + ret = iris_opp_pd_get(core); >> + if (ret) >> + return ret; >> + >> + clk_tbl = core->iris_platform_data->clk_tbl; >> + clk_cnt = core->iris_platform_data->clk_tbl_size; >> + >> + for (i = 0; i < clk_cnt; i++) { >> + if (clk_tbl[i].clk_type == IRIS_HW_CLK) { >> + ret = devm_pm_opp_set_clkname(core->dev, clk_tbl[i].clk_name); >> + if (ret) >> + return ret; >> + } >> + } >> + >> + ret = devm_pm_opp_of_add_table(core->dev); >> + if (ret) { >> + dev_err(core->dev, "failed to add opp table\n"); >> + return ret; >> + } >> + >> + return ret; >> +} >> + >> +static int iris_init_clocks(struct iris_core *core) >> +{ >> + int ret; >> + >> + ret = devm_clk_bulk_get_all(core->dev, &core->clock_tbl); >> + if (ret < 0) { >> + dev_err(core->dev, "failed to get bulk clock\n"); > > Syntax is: > return dev_err_probe(). If this is probe path. Is it? > Sure will fix. >> + return ret; >> + } >> + >> + core->clk_count = ret; >> + >> + return 0; >> +} >> + >> +static int iris_init_resets(struct iris_core *core) >> +{ >> + const char * const *rst_tbl; >> + u32 rst_tbl_size; >> + u32 i = 0, ret; >> + >> + rst_tbl = core->iris_platform_data->clk_rst_tbl; >> + rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size; >> + >> + core->resets = devm_kzalloc(core->dev, >> + sizeof(*core->resets) * rst_tbl_size, >> + GFP_KERNEL); >> + if (rst_tbl_size && !core->resets) >> + return -ENOMEM; >> + >> + for (i = 0; i < rst_tbl_size; i++) >> + core->resets[i].id = rst_tbl[i]; >> + >> + ret = devm_reset_control_bulk_get_exclusive(core->dev, rst_tbl_size, core->resets); >> + if (ret) { >> + dev_err(core->dev, "failed to get resets\n"); > > Syntax is: > return dev_err_probe(). If this is probe path. Is it? > Sure, will fix >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +int iris_init_resources(struct iris_core *core) >> +{ >> + int ret; >> + >> + ret = iris_init_icc(core); >> + if (ret) >> + return ret; >> + >> + ret = iris_init_power_domains(core); >> + if (ret) >> + return ret; >> + >> + ret = iris_init_clocks(core); >> + if (ret) >> + return ret; >> + >> + ret = iris_init_resets(core); >> + >> + return ret; >> +} > > This should be just part of of main unit file, next to probe. It is > unusual to see probe parts not next to probe. Sorry, that's wrong. > All the APIs handling(init/enable/disable) the different resources (PM domains, OPP, clocks, buses) are kept in this iris_resource.c file hence this API to init all those resources is kept here to not load iris_probe.c file. > Best regards, > Krzysztof >
On 05/09/2024 13:53, Dikshita Agarwal wrote: > > > On 8/27/2024 4:21 PM, Krzysztof Kozlowski wrote: >> On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote: >>> From: Dikshita Agarwal <quic_dikshita@quicinc.com> >>> >>> Add support for initializing Iris "resources", which are clocks, >>> interconnects, power domains, reset clocks, and clock frequencies >>> used for iris hardware. >>> >>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >>> --- >> >> ... >> >>> +struct iris_platform_data sm8550_data = { >>> + .icc_tbl = sm8550_icc_table, >>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>> + .clk_rst_tbl = sm8550_clk_reset_table, >>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table), >>> + .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), >>> +}; >>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c >>> index 0a54fdaa1ab5..2616a31224f9 100644 >>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev) >>> if (core->irq < 0) >>> return core->irq; >>> >>> + core->iris_platform_data = of_device_get_match_data(core->dev); >>> + if (!core->iris_platform_data) { >>> + ret = -ENODEV; >>> + dev_err_probe(core->dev, ret, "init platform failed\n"); >> >> That's not even possible. I would suggest dropping entire if. But if yoi >> insist, then without this weird redundant code. return -EINVAL. >> > Its possible if platform data is not initialized and this is only place we > check it, there is no further NULL check for the same. It is possible? Then point me to the code line. Or present some code flow leading to it. >>> + return ret; >>> + } >>> + >>> + ret = iris_init_resources(core); >>> + if (ret) { >>> + dev_err_probe(core->dev, ret, "init resource failed\n"); >>> + return ret; >> >> How many same errors are you printing? Not mentioning that syntax of >> dev_errp_rpboe is different... > We have these errors at multiple points to know at what point the probe > failed which is useful while debugging probe failures. Duplicating is not helpful. > But Sure we will revisit this code and fix the syntax of dev_err_probe. >> >> >>> + } >>> + >>> ret = v4l2_device_register(dev, &core->v4l2_dev); >>> if (ret) >>> return ret; >>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev) >>> } >>> >>> static const struct of_device_id iris_dt_match[] = { >>> - { .compatible = "qcom,sm8550-iris", }, >>> - { .compatible = "qcom,sm8250-venus", }, >>> + { >>> + .compatible = "qcom,sm8550-iris", >>> + .data = &sm8550_data, >>> + }, >>> + { >>> + .compatible = "qcom,sm8250-venus", >>> + .data = &sm8250_data, >> >> You just added this. No, please do not add code which is immediatly >> incorrect. > It's not incorrect, in earlier patch we only added the compatible strings > and with this patch introducing the platform data and APIs to get it. It is incorrect to immediately remove it. You keep arguing on basic stuff. Sorry, but that is not how it works. If you add code and IMMEDIATELY remove it, then it means the code was not needed. Or was not correct. Choose one. ... >> >> This should be just part of of main unit file, next to probe. It is >> unusual to see probe parts not next to probe. Sorry, that's wrong. >> > All the APIs handling(init/enable/disable) the different resources (PM > domains, OPP, clocks, buses) are kept in this iris_resource.c file hence > this API to init all those resources is kept here to not load iris_probe.c > file. You introduce your own coding style and as an argument you use just "I do this". The expected is to see resource initialization next to probe. Repeating what your code does, is not helping me to understand your design choice. Best regards, Krzysztof
Hi Krzysztof, On 9/5/2024 5:27 PM, Krzysztof Kozlowski wrote: > On 05/09/2024 13:53, Dikshita Agarwal wrote: >> >> >> On 8/27/2024 4:21 PM, Krzysztof Kozlowski wrote: >>> On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote: >>>> From: Dikshita Agarwal <quic_dikshita@quicinc.com> >>>> >>>> Add support for initializing Iris "resources", which are clocks, >>>> interconnects, power domains, reset clocks, and clock frequencies >>>> used for iris hardware. >>>> >>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >>>> --- >>> >>> ... >>> >>>> +struct iris_platform_data sm8550_data = { >>>> + .icc_tbl = sm8550_icc_table, >>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>> + .clk_rst_tbl = sm8550_clk_reset_table, >>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table), >>>> + .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), >>>> +}; >>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c >>>> index 0a54fdaa1ab5..2616a31224f9 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev) >>>> if (core->irq < 0) >>>> return core->irq; >>>> >>>> + core->iris_platform_data = of_device_get_match_data(core->dev); >>>> + if (!core->iris_platform_data) { >>>> + ret = -ENODEV; >>>> + dev_err_probe(core->dev, ret, "init platform failed\n"); >>> >>> That's not even possible. I would suggest dropping entire if. But if yoi >>> insist, then without this weird redundant code. return -EINVAL. >>> >> Its possible if platform data is not initialized and this is only place we >> check it, there is no further NULL check for the same. > > It is possible? Then point me to the code line. Or present some code > flow leading to it. Lets go with return -EINVAL in this if block. > >>>> + return ret; >>>> + } >>>> + >>>> + ret = iris_init_resources(core); >>>> + if (ret) { >>>> + dev_err_probe(core->dev, ret, "init resource failed\n"); >>>> + return ret; >>> >>> How many same errors are you printing? Not mentioning that syntax of >>> dev_errp_rpboe is different... >> We have these errors at multiple points to know at what point the probe >> failed which is useful while debugging probe failures. > > Duplicating is not helpful. > >> But Sure we will revisit this code and fix the syntax of dev_err_probe. > >>> >>> >>>> + } >>>> + >>>> ret = v4l2_device_register(dev, &core->v4l2_dev); >>>> if (ret) >>>> return ret; >>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev) >>>> } >>>> >>>> static const struct of_device_id iris_dt_match[] = { >>>> - { .compatible = "qcom,sm8550-iris", }, >>>> - { .compatible = "qcom,sm8250-venus", }, >>>> + { >>>> + .compatible = "qcom,sm8550-iris", >>>> + .data = &sm8550_data, >>>> + }, >>>> + { >>>> + .compatible = "qcom,sm8250-venus", >>>> + .data = &sm8250_data, >>> >>> You just added this. No, please do not add code which is immediatly >>> incorrect. >> It's not incorrect, in earlier patch we only added the compatible strings >> and with this patch introducing the platform data and APIs to get it. > > It is incorrect to immediately remove it. You keep arguing on basic > stuff. Sorry, but that is not how it works. If you add code and > IMMEDIATELY remove it, then it means the code was not needed. Or was not > correct. Choose one. I think it is not removing it. It is adding platform data to compatibles introduced in previous patch. Maybe it appears as if it is removing it. > > ... > >>> >>> This should be just part of of main unit file, next to probe. It is >>> unusual to see probe parts not next to probe. Sorry, that's wrong. >>> >> All the APIs handling(init/enable/disable) the different resources (PM >> domains, OPP, clocks, buses) are kept in this iris_resource.c file hence >> this API to init all those resources is kept here to not load iris_probe.c >> file. > > You introduce your own coding style and as an argument you use just "I > do this". > > The expected is to see resource initialization next to probe. Repeating > what your code does, is not helping me to understand your design choice. I see your point and it would be good to have the resources initialized as part of probe. > Best regards, > Krzysztof Regards, Vikash
On 06/09/2024 13:21, Vikash Garodia wrote: >>>> >>>>> + } >>>>> + >>>>> ret = v4l2_device_register(dev, &core->v4l2_dev); >>>>> if (ret) >>>>> return ret; >>>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev) >>>>> } >>>>> >>>>> static const struct of_device_id iris_dt_match[] = { >>>>> - { .compatible = "qcom,sm8550-iris", }, >>>>> - { .compatible = "qcom,sm8250-venus", }, >>>>> + { >>>>> + .compatible = "qcom,sm8550-iris", >>>>> + .data = &sm8550_data, >>>>> + }, >>>>> + { >>>>> + .compatible = "qcom,sm8250-venus", >>>>> + .data = &sm8250_data, >>>> >>>> You just added this. No, please do not add code which is immediatly >>>> incorrect. >>> It's not incorrect, in earlier patch we only added the compatible strings >>> and with this patch introducing the platform data and APIs to get it. >> >> It is incorrect to immediately remove it. You keep arguing on basic >> stuff. Sorry, but that is not how it works. If you add code and >> IMMEDIATELY remove it, then it means the code was not needed. Or was not >> correct. Choose one. > I think it is not removing it. It is adding platform data to compatibles > introduced in previous patch. Maybe it appears as if it is removing it. I know how the diff works. The way you avoid solving the problem with trivial responses is not helping. We already have been there with another patchset from different person and it lead to annoying all DT maintainers and (usually very patient) some of networking folks. I ask you to approach to the review seriously. NAK. Best regards, Krzysztof
Hi, On 9/6/2024 5:34 PM, Krzysztof Kozlowski wrote: > On 06/09/2024 13:21, Vikash Garodia wrote: >>>>> >>>>>> + } >>>>>> + >>>>>> ret = v4l2_device_register(dev, &core->v4l2_dev); >>>>>> if (ret) >>>>>> return ret; >>>>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev) >>>>>> } >>>>>> >>>>>> static const struct of_device_id iris_dt_match[] = { >>>>>> - { .compatible = "qcom,sm8550-iris", }, >>>>>> - { .compatible = "qcom,sm8250-venus", }, >>>>>> + { >>>>>> + .compatible = "qcom,sm8550-iris", >>>>>> + .data = &sm8550_data, >>>>>> + }, >>>>>> + { >>>>>> + .compatible = "qcom,sm8250-venus", >>>>>> + .data = &sm8250_data, >>>>> >>>>> You just added this. No, please do not add code which is immediatly >>>>> incorrect. >>>> It's not incorrect, in earlier patch we only added the compatible strings >>>> and with this patch introducing the platform data and APIs to get it. >>> >>> It is incorrect to immediately remove it. You keep arguing on basic >>> stuff. Sorry, but that is not how it works. If you add code and >>> IMMEDIATELY remove it, then it means the code was not needed. Or was not >>> correct. Choose one. >> I think it is not removing it. It is adding platform data to compatibles >> introduced in previous patch. Maybe it appears as if it is removing it. > > I know how the diff works. Perhaps, i have misunderstood. Are you suggesting to add compat data and compatible string together in single patch rather than splitting it in 2 patches ? If so, that would essentially end up squashing patch #3 and #4. Let me know if that would address your comment and we will plan to do that. > The way you avoid solving the problem with trivial responses is not > helping. We already have been there with another patchset from different > person and it lead to annoying all DT maintainers and (usually very > patient) some of networking folks. I ask you to approach to the review > seriously. Please be assured that all comments are valued upon and are being taken seriously. > NAK. > > Best regards, > Krzysztof > Regards, Vikash
On 06/09/2024 21:47, Vikash Garodia wrote: > Hi, > > On 9/6/2024 5:34 PM, Krzysztof Kozlowski wrote: >> On 06/09/2024 13:21, Vikash Garodia wrote: >>>>>> >>>>>>> + } >>>>>>> + >>>>>>> ret = v4l2_device_register(dev, &core->v4l2_dev); >>>>>>> if (ret) >>>>>>> return ret; >>>>>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev) >>>>>>> } >>>>>>> >>>>>>> static const struct of_device_id iris_dt_match[] = { >>>>>>> - { .compatible = "qcom,sm8550-iris", }, >>>>>>> - { .compatible = "qcom,sm8250-venus", }, >>>>>>> + { >>>>>>> + .compatible = "qcom,sm8550-iris", >>>>>>> + .data = &sm8550_data, >>>>>>> + }, >>>>>>> + { >>>>>>> + .compatible = "qcom,sm8250-venus", >>>>>>> + .data = &sm8250_data, >>>>>> >>>>>> You just added this. No, please do not add code which is immediatly >>>>>> incorrect. >>>>> It's not incorrect, in earlier patch we only added the compatible strings >>>>> and with this patch introducing the platform data and APIs to get it. >>>> >>>> It is incorrect to immediately remove it. You keep arguing on basic >>>> stuff. Sorry, but that is not how it works. If you add code and >>>> IMMEDIATELY remove it, then it means the code was not needed. Or was not >>>> correct. Choose one. >>> I think it is not removing it. It is adding platform data to compatibles >>> introduced in previous patch. Maybe it appears as if it is removing it. >> >> I know how the diff works. > Perhaps, i have misunderstood. Are you suggesting to add compat data and > compatible string together in single patch rather than splitting it in 2 patches > ? If so, that would essentially end up squashing patch #3 and #4. Let me know if > that would address your comment and we will plan to do that. You are supposed to organize your patches so they have logical order. I already explained why this order is wrong. What's more, previous patch of two equal compatibles does not have much value. Devices cannot work and code is confusing. Best regards, Krzysztof
diff --git a/drivers/media/platform/qcom/iris/Makefile b/drivers/media/platform/qcom/iris/Makefile index 913da225486b..3e8474d064f4 100644 --- a/drivers/media/platform/qcom/iris/Makefile +++ b/drivers/media/platform/qcom/iris/Makefile @@ -1,3 +1,6 @@ -iris-objs += iris_probe.o \ +iris-objs += iris_platform_sm8250.o \ + iris_platform_sm8550.o \ + iris_probe.o \ + iris_resources.o \ obj-$(CONFIG_VIDEO_QCOM_IRIS) += iris.o diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h index 402f0aaef995..a1afd4387b3a 100644 --- a/drivers/media/platform/qcom/iris/iris_core.h +++ b/drivers/media/platform/qcom/iris/iris_core.h @@ -6,8 +6,12 @@ #ifndef _IRIS_CORE_H_ #define _IRIS_CORE_H_ +#include <linux/types.h> #include <media/v4l2-device.h> +#include "iris_platform_common.h" +#include "iris_resources.h" + /** * struct iris_core - holds core parameters valid for all instances * @@ -16,6 +20,14 @@ * @irq: iris irq * @v4l2_dev: a holder for v4l2 device structure * @vdev_dec: iris video device structure for decoder + * @icc_tbl: table of iris interconnects + * @icc_count: count of iris interconnects + * @pmdomain_tbl: table of iris power domains + * @opp_pmdomain_tbl: table of opp power domains + * @clock_tbl: table of iris clocks + * @clk_count: count of iris clocks + * @resets: table of iris reset clocks + * @iris_platform_data: a structure for platform data */ struct iris_core { @@ -24,6 +36,14 @@ struct iris_core { int irq; struct v4l2_device v4l2_dev; struct video_device *vdev_dec; + struct icc_bulk_data *icc_tbl; + u32 icc_count; + struct dev_pm_domain_list *pmdomain_tbl; + struct dev_pm_domain_list *opp_pmdomain_tbl; + struct clk_bulk_data *clock_tbl; + u32 clk_count; + struct reset_control_bulk_data *resets; + const struct iris_platform_data *iris_platform_data; }; #endif diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h new file mode 100644 index 000000000000..293fb7e904b0 --- /dev/null +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h @@ -0,0 +1,36 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#ifndef _IRIS_PLATFORM_COMMON_H_ +#define _IRIS_PLATFORM_COMMON_H_ + +extern struct iris_platform_data sm8550_data; +extern struct iris_platform_data sm8250_data; + +enum platform_clk_type { + IRIS_AXI_CLK, + IRIS_CTRL_CLK, + IRIS_HW_CLK, +}; + +struct platform_clk_data { + enum platform_clk_type clk_type; + const char *clk_name; +}; + +struct iris_platform_data { + const struct icc_info *icc_tbl; + unsigned int icc_tbl_size; + const char * const *pmdomain_tbl; + unsigned int pmdomain_tbl_size; + const char * const *opp_pd_tbl; + unsigned int opp_pd_tbl_size; + const struct platform_clk_data *clk_tbl; + unsigned int clk_tbl_size; + const char * const *clk_rst_tbl; + unsigned int clk_rst_tbl_size; +}; + +#endif diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8250.c b/drivers/media/platform/qcom/iris/iris_platform_sm8250.c new file mode 100644 index 000000000000..b6f08fc327b5 --- /dev/null +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8250.c @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include "iris_core.h" +#include "iris_platform_common.h" +#include "iris_resources.h" + +static const struct icc_info sm8250_icc_table[] = { + { "cpu-cfg", 1000, 1000 }, + { "video-mem", 1000, 15000000 }, +}; + +static const char * const sm8250_clk_reset_table[] = { "bus", "core" }; + +static const char * const sm8250_pmdomain_table[] = { "venus", "vcodec0" }; + +static const char * const sm8250_opp_pd_table[] = { "mx" }; + +static const struct platform_clk_data sm8250_clk_table[] = { + {IRIS_AXI_CLK, "iface" }, + {IRIS_CTRL_CLK, "core" }, + {IRIS_HW_CLK, "vcodec0_core" }, +}; + +struct iris_platform_data sm8250_data = { + .icc_tbl = sm8250_icc_table, + .icc_tbl_size = ARRAY_SIZE(sm8250_icc_table), + .clk_rst_tbl = sm8250_clk_reset_table, + .clk_rst_tbl_size = ARRAY_SIZE(sm8250_clk_reset_table), + .pmdomain_tbl = sm8250_pmdomain_table, + .pmdomain_tbl_size = ARRAY_SIZE(sm8250_pmdomain_table), + .opp_pd_tbl = sm8250_opp_pd_table, + .opp_pd_tbl_size = ARRAY_SIZE(sm8250_opp_pd_table), + .clk_tbl = sm8250_clk_table, + .clk_tbl_size = ARRAY_SIZE(sm8250_clk_table), +}; diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c new file mode 100644 index 000000000000..f2f9e6f6775f --- /dev/null +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include "iris_core.h" +#include "iris_platform_common.h" +#include "iris_resources.h" + +static const struct icc_info sm8550_icc_table[] = { + { "cpu-cfg", 1000, 1000 }, + { "video-mem", 1000, 15000000 }, +}; + +static const char * const sm8550_clk_reset_table[] = { "bus" }; + +static const char * const sm8550_pmdomain_table[] = { "venus", "vcodec0" }; + +static const char * const sm8550_opp_pd_table[] = { "mxc", "mmcx" }; + +static const struct platform_clk_data sm8550_clk_table[] = { + {IRIS_AXI_CLK, "iface" }, + {IRIS_CTRL_CLK, "core" }, + {IRIS_HW_CLK, "vcodec0_core" }, +}; + +struct iris_platform_data sm8550_data = { + .icc_tbl = sm8550_icc_table, + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), + .clk_rst_tbl = sm8550_clk_reset_table, + .clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table), + .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), +}; diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c index 0a54fdaa1ab5..2616a31224f9 100644 --- a/drivers/media/platform/qcom/iris/iris_probe.c +++ b/drivers/media/platform/qcom/iris/iris_probe.c @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev) if (core->irq < 0) return core->irq; + core->iris_platform_data = of_device_get_match_data(core->dev); + if (!core->iris_platform_data) { + ret = -ENODEV; + dev_err_probe(core->dev, ret, "init platform failed\n"); + return ret; + } + + ret = iris_init_resources(core); + if (ret) { + dev_err_probe(core->dev, ret, "init resource failed\n"); + return ret; + } + ret = v4l2_device_register(dev, &core->v4l2_dev); if (ret) return ret; @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev) } static const struct of_device_id iris_dt_match[] = { - { .compatible = "qcom,sm8550-iris", }, - { .compatible = "qcom,sm8250-venus", }, + { + .compatible = "qcom,sm8550-iris", + .data = &sm8550_data, + }, + { + .compatible = "qcom,sm8250-venus", + .data = &sm8250_data, + }, { }, }; MODULE_DEVICE_TABLE(of, iris_dt_match); diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c new file mode 100644 index 000000000000..57c6f9f3449b --- /dev/null +++ b/drivers/media/platform/qcom/iris/iris_resources.c @@ -0,0 +1,171 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include <linux/clk.h> +#include <linux/interconnect.h> +#include <linux/pm_domain.h> +#include <linux/pm_opp.h> +#include <linux/reset.h> + +#include "iris_core.h" +#include "iris_resources.h" + +static int iris_init_icc(struct iris_core *core) +{ + const struct icc_info *icc_tbl; + u32 ret, i = 0; + + icc_tbl = core->iris_platform_data->icc_tbl; + + core->icc_count = core->iris_platform_data->icc_tbl_size; + core->icc_tbl = devm_kzalloc(core->dev, + sizeof(struct icc_bulk_data) * core->icc_count, + GFP_KERNEL); + if (!core->icc_tbl) + return -ENOMEM; + + for (i = 0; i < core->icc_count; i++) { + core->icc_tbl[i].name = icc_tbl[i].name; + core->icc_tbl[i].avg_bw = icc_tbl[i].bw_min_kbps; + core->icc_tbl[i].peak_bw = 0; + } + + ret = devm_of_icc_bulk_get(core->dev, core->icc_count, core->icc_tbl); + if (ret) + dev_err(core->dev, "failed to get interconnect paths, NoC will stay unconfigured!\n"); + + return ret; +} + +static int iris_pd_get(struct iris_core *core) +{ + int ret; + + struct dev_pm_domain_attach_data iris_pd_data = { + .pd_names = core->iris_platform_data->pmdomain_tbl, + .num_pd_names = core->iris_platform_data->pmdomain_tbl_size, + .pd_flags = PD_FLAG_NO_DEV_LINK, + }; + + ret = devm_pm_domain_attach_list(core->dev, &iris_pd_data, &core->pmdomain_tbl); + if (ret < 0) + return ret; + + return 0; +} + +static int iris_opp_pd_get(struct iris_core *core) +{ + int ret; + + struct dev_pm_domain_attach_data iris_opp_pd_data = { + .pd_names = core->iris_platform_data->opp_pd_tbl, + .num_pd_names = core->iris_platform_data->opp_pd_tbl_size, + .pd_flags = PD_FLAG_DEV_LINK_ON, + }; + + ret = devm_pm_domain_attach_list(core->dev, &iris_opp_pd_data, &core->opp_pmdomain_tbl); + if (ret < 0) + return ret; + + return 0; +} + +static int iris_init_power_domains(struct iris_core *core) +{ + const struct platform_clk_data *clk_tbl; + u32 clk_cnt, i; + int ret; + + ret = iris_pd_get(core); + if (ret) + return ret; + + ret = iris_opp_pd_get(core); + if (ret) + return ret; + + clk_tbl = core->iris_platform_data->clk_tbl; + clk_cnt = core->iris_platform_data->clk_tbl_size; + + for (i = 0; i < clk_cnt; i++) { + if (clk_tbl[i].clk_type == IRIS_HW_CLK) { + ret = devm_pm_opp_set_clkname(core->dev, clk_tbl[i].clk_name); + if (ret) + return ret; + } + } + + ret = devm_pm_opp_of_add_table(core->dev); + if (ret) { + dev_err(core->dev, "failed to add opp table\n"); + return ret; + } + + return ret; +} + +static int iris_init_clocks(struct iris_core *core) +{ + int ret; + + ret = devm_clk_bulk_get_all(core->dev, &core->clock_tbl); + if (ret < 0) { + dev_err(core->dev, "failed to get bulk clock\n"); + return ret; + } + + core->clk_count = ret; + + return 0; +} + +static int iris_init_resets(struct iris_core *core) +{ + const char * const *rst_tbl; + u32 rst_tbl_size; + u32 i = 0, ret; + + rst_tbl = core->iris_platform_data->clk_rst_tbl; + rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size; + + core->resets = devm_kzalloc(core->dev, + sizeof(*core->resets) * rst_tbl_size, + GFP_KERNEL); + if (rst_tbl_size && !core->resets) + return -ENOMEM; + + for (i = 0; i < rst_tbl_size; i++) + core->resets[i].id = rst_tbl[i]; + + ret = devm_reset_control_bulk_get_exclusive(core->dev, rst_tbl_size, core->resets); + if (ret) { + dev_err(core->dev, "failed to get resets\n"); + return ret; + } + + return 0; +} + +int iris_init_resources(struct iris_core *core) +{ + int ret; + + ret = iris_init_icc(core); + if (ret) + return ret; + + ret = iris_init_power_domains(core); + if (ret) + return ret; + + ret = iris_init_clocks(core); + if (ret) + return ret; + + ret = iris_init_resets(core); + + return ret; +} diff --git a/drivers/media/platform/qcom/iris/iris_resources.h b/drivers/media/platform/qcom/iris/iris_resources.h new file mode 100644 index 000000000000..b0217399030a --- /dev/null +++ b/drivers/media/platform/qcom/iris/iris_resources.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#ifndef _IRIS_RESOURCES_H_ +#define _IRIS_RESOURCES_H_ + +struct iris_core; + +struct icc_info { + const char *name; + u32 bw_min_kbps; + u32 bw_max_kbps; +}; + +int iris_init_resources(struct iris_core *core); + +#endif