| Message ID | 1644851994-22732-8-git-send-email-quic_srivasam@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add pin control support for lpass sc7280 | expand |
On 14/02/2022 15:19, Srinivasa Rao Mandadapu wrote: > Update bulk clock voting to optional voting as ADSP bypass platform doesn't > need macro and decodec clocks, these are maintained as power domains and > operated from lpass audio core cc. > > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> > Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com> > Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com> > --- > drivers/pinctrl/qcom/pinctrl-lpass-lpi.c | 16 +++++++++------- > drivers/pinctrl/qcom/pinctrl-lpass-lpi.h | 1 + > drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c | 1 + > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c > index 8a82fd9..103f0a6c 100644 > --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c > +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c > @@ -407,13 +407,15 @@ int lpi_pinctrl_probe(struct platform_device *pdev) > return dev_err_probe(dev, PTR_ERR(pctrl->slew_base), > "Slew resource not provided\n"); > > - ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks); > - if (ret) > - return dev_err_probe(dev, ret, "Can't get clocks\n"); > - > - ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks); > - if (ret) > - return dev_err_probe(dev, ret, "Can't enable clocks\n"); > + if (!data->is_clk_optional) { > + ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks); > + if (ret) > + return dev_err_probe(dev, ret, "Can't get clocks\n"); > + > + ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks); > + if (ret) > + return dev_err_probe(dev, ret, "Can't enable clocks\n"); > + } > > pctrl->desc.pctlops = &lpi_gpio_pinctrl_ops; > pctrl->desc.pmxops = &lpi_gpio_pinmux_ops; > diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h > index a511d72..c1079bf 100644 > --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h > +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h > @@ -77,6 +77,7 @@ struct lpi_pinctrl_variant_data { > int ngroups; > const struct lpi_function *functions; > int nfunctions; > + int is_clk_optional; > }; > > int lpi_pinctrl_probe(struct platform_device *pdev); > diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c > index 5bf30d97..4277e31 100644 > --- a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c > +++ b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c > @@ -143,6 +143,7 @@ static const struct lpi_pinctrl_variant_data sc7280_lpi_data = { > .ngroups = ARRAY_SIZE(sc7280_groups), > .functions = sc7280_functions, > .nfunctions = ARRAY_SIZE(sc7280_functions), > + .is_clk_optional = 1, This is forcefully set assuming that sc7280 is always used in ADSP bypass mode. Which is not correct. Can't you use devm_clk_bulk_get_optional instead? --srini > }; > > static const struct of_device_id lpi_pinctrl_of_match[] = {
On 2/16/2022 7:50 PM, Srinivas Kandagatla wrote: Thanks for Your Time Srini!!! > > On 14/02/2022 15:19, Srinivasa Rao Mandadapu wrote: >> Update bulk clock voting to optional voting as ADSP bypass platform >> doesn't >> need macro and decodec clocks, these are maintained as power domains and >> operated from lpass audio core cc. >> >> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> >> Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com> >> Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com> >> --- >> drivers/pinctrl/qcom/pinctrl-lpass-lpi.c | 16 +++++++++------- >> drivers/pinctrl/qcom/pinctrl-lpass-lpi.h | 1 + >> drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c | 1 + >> 3 files changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c >> b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c >> index 8a82fd9..103f0a6c 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c >> +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c >> @@ -407,13 +407,15 @@ int lpi_pinctrl_probe(struct platform_device >> *pdev) >> return dev_err_probe(dev, PTR_ERR(pctrl->slew_base), >> "Slew resource not provided\n"); >> - ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks); >> - if (ret) >> - return dev_err_probe(dev, ret, "Can't get clocks\n"); >> - >> - ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks); >> - if (ret) >> - return dev_err_probe(dev, ret, "Can't enable clocks\n"); >> + if (!data->is_clk_optional) { >> + ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks); >> + if (ret) >> + return dev_err_probe(dev, ret, "Can't get clocks\n"); >> + >> + ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks); >> + if (ret) >> + return dev_err_probe(dev, ret, "Can't enable clocks\n"); >> + } >> pctrl->desc.pctlops = &lpi_gpio_pinctrl_ops; >> pctrl->desc.pmxops = &lpi_gpio_pinmux_ops; >> diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h >> b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h >> index a511d72..c1079bf 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h >> +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h >> @@ -77,6 +77,7 @@ struct lpi_pinctrl_variant_data { >> int ngroups; >> const struct lpi_function *functions; >> int nfunctions; >> + int is_clk_optional; >> }; >> int lpi_pinctrl_probe(struct platform_device *pdev); >> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >> b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >> index 5bf30d97..4277e31 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >> +++ b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >> @@ -143,6 +143,7 @@ static const struct lpi_pinctrl_variant_data >> sc7280_lpi_data = { >> .ngroups = ARRAY_SIZE(sc7280_groups), >> .functions = sc7280_functions, >> .nfunctions = ARRAY_SIZE(sc7280_functions), >> + .is_clk_optional = 1, > > This is forcefully set assuming that sc7280 is always used in ADSP > bypass mode. Which is not correct. > > Can't you use devm_clk_bulk_get_optional instead? Yes. Agreed. Initially used devm_clk_bulk_get_optional, but Bjorn suggested for conditional check instead of optional. Again Shall we go for optional clock voting? > > --srini > >> }; >> static const struct of_device_id lpi_pinctrl_of_match[] = {
On 16/02/2022 14:41, Srinivasa Rao Mandadapu wrote: > > On 2/16/2022 7:50 PM, Srinivas Kandagatla wrote: > Thanks for Your Time Srini!!! >> >> On 14/02/2022 15:19, Srinivasa Rao Mandadapu wrote: >>> Update bulk clock voting to optional voting as ADSP bypass platform >>> doesn't >>> need macro and decodec clocks, these are maintained as power domains and >>> operated from lpass audio core cc. >>> >>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> >>> Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com> >>> Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com> >>> --- >>> drivers/pinctrl/qcom/pinctrl-lpass-lpi.c | 16 +++++++++------- >>> drivers/pinctrl/qcom/pinctrl-lpass-lpi.h | 1 + >>> drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c | 1 + >>> 3 files changed, 11 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c >>> b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c >>> index 8a82fd9..103f0a6c 100644 >>> --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c >>> +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c >>> @@ -407,13 +407,15 @@ int lpi_pinctrl_probe(struct platform_device >>> *pdev) >>> return dev_err_probe(dev, PTR_ERR(pctrl->slew_base), >>> "Slew resource not provided\n"); >>> - ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks); >>> - if (ret) >>> - return dev_err_probe(dev, ret, "Can't get clocks\n"); >>> - >>> - ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks); >>> - if (ret) >>> - return dev_err_probe(dev, ret, "Can't enable clocks\n"); >>> + if (!data->is_clk_optional) { >>> + ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks); >>> + if (ret) >>> + return dev_err_probe(dev, ret, "Can't get clocks\n"); >>> + >>> + ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks); >>> + if (ret) >>> + return dev_err_probe(dev, ret, "Can't enable clocks\n"); >>> + } >>> pctrl->desc.pctlops = &lpi_gpio_pinctrl_ops; >>> pctrl->desc.pmxops = &lpi_gpio_pinmux_ops; >>> diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h >>> b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h >>> index a511d72..c1079bf 100644 >>> --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h >>> +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h >>> @@ -77,6 +77,7 @@ struct lpi_pinctrl_variant_data { >>> int ngroups; >>> const struct lpi_function *functions; >>> int nfunctions; >>> + int is_clk_optional; >>> }; >>> int lpi_pinctrl_probe(struct platform_device *pdev); >>> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >>> b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >>> index 5bf30d97..4277e31 100644 >>> --- a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >>> +++ b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >>> @@ -143,6 +143,7 @@ static const struct lpi_pinctrl_variant_data >>> sc7280_lpi_data = { >>> .ngroups = ARRAY_SIZE(sc7280_groups), >>> .functions = sc7280_functions, >>> .nfunctions = ARRAY_SIZE(sc7280_functions), >>> + .is_clk_optional = 1, >> >> This is forcefully set assuming that sc7280 is always used in ADSP >> bypass mode. Which is not correct. >> >> Can't you use devm_clk_bulk_get_optional instead? > > Yes. Agreed. Initially used devm_clk_bulk_get_optional, but Bjorn > suggested for conditional check instead of optional. > > Again Shall we go for optional clock voting? That means that the condition has to be dynamic based on the platform using DSP or not. Which is impossible to deduce without some help from DT. I would prefer to stay with optional clock unless Bjorn has some strong objection on not using int. --srini > >> >> --srini >> >>> }; >>> static const struct of_device_id lpi_pinctrl_of_match[] = {
Quoting Srinivas Kandagatla (2022-02-16 07:38:02) > > > On 16/02/2022 14:41, Srinivasa Rao Mandadapu wrote: > > > > On 2/16/2022 7:50 PM, Srinivas Kandagatla wrote: > > Thanks for Your Time Srini!!! > >> > >> On 14/02/2022 15:19, Srinivasa Rao Mandadapu wrote: > >>> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c > >>> b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c > >>> index 5bf30d97..4277e31 100644 > >>> --- a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c > >>> +++ b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c > >>> @@ -143,6 +143,7 @@ static const struct lpi_pinctrl_variant_data > >>> sc7280_lpi_data = { > >>> .ngroups = ARRAY_SIZE(sc7280_groups), > >>> .functions = sc7280_functions, > >>> .nfunctions = ARRAY_SIZE(sc7280_functions), > >>> + .is_clk_optional = 1, > >> > >> This is forcefully set assuming that sc7280 is always used in ADSP > >> bypass mode. Which is not correct. > >> > >> Can't you use devm_clk_bulk_get_optional instead? > > > > Yes. Agreed. Initially used devm_clk_bulk_get_optional, but Bjorn > > suggested for conditional check instead of optional. > > > > Again Shall we go for optional clock voting? > > That means that the condition has to be dynamic based on the platform > using DSP or not. Which is impossible to deduce without some help from DT. > > I would prefer to stay with optional clock unless Bjorn has some strong > objection on not using int. I think we need the combination of optional API and bool flag. My understanding is it's optional on sc7280, but not on the previous revision, so we want to be very strict on previous revision and less strict on sc7280. Hence the flag. Maybe we should change it to clk_required and then assume optional going forward. Then the callsite can use one or the other API?
On 2/19/2022 8:16 AM, Stephen Boyd wrote: Thanks for Your time Stephen!!! > Quoting Srinivas Kandagatla (2022-02-16 07:38:02) >> >> On 16/02/2022 14:41, Srinivasa Rao Mandadapu wrote: >>> On 2/16/2022 7:50 PM, Srinivas Kandagatla wrote: >>> Thanks for Your Time Srini!!! >>>> On 14/02/2022 15:19, Srinivasa Rao Mandadapu wrote: >>>>> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >>>>> b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >>>>> index 5bf30d97..4277e31 100644 >>>>> --- a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >>>>> +++ b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c >>>>> @@ -143,6 +143,7 @@ static const struct lpi_pinctrl_variant_data >>>>> sc7280_lpi_data = { >>>>> .ngroups = ARRAY_SIZE(sc7280_groups), >>>>> .functions = sc7280_functions, >>>>> .nfunctions = ARRAY_SIZE(sc7280_functions), >>>>> + .is_clk_optional = 1, >>>> This is forcefully set assuming that sc7280 is always used in ADSP >>>> bypass mode. Which is not correct. >>>> >>>> Can't you use devm_clk_bulk_get_optional instead? >>> Yes. Agreed. Initially used devm_clk_bulk_get_optional, but Bjorn >>> suggested for conditional check instead of optional. >>> >>> Again Shall we go for optional clock voting? >> That means that the condition has to be dynamic based on the platform >> using DSP or not. Which is impossible to deduce without some help from DT. >> >> I would prefer to stay with optional clock unless Bjorn has some strong >> objection on not using int. > I think we need the combination of optional API and bool flag. My > understanding is it's optional on sc7280, but not on the previous > revision, so we want to be very strict on previous revision and less > strict on sc7280. Hence the flag. Maybe we should change it to > clk_required and then assume optional going forward. Then the callsite > can use one or the other API? Okay.Will change accordingly.
diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c index 8a82fd9..103f0a6c 100644 --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.c @@ -407,13 +407,15 @@ int lpi_pinctrl_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(pctrl->slew_base), "Slew resource not provided\n"); - ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks); - if (ret) - return dev_err_probe(dev, ret, "Can't get clocks\n"); - - ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks); - if (ret) - return dev_err_probe(dev, ret, "Can't enable clocks\n"); + if (!data->is_clk_optional) { + ret = devm_clk_bulk_get(dev, MAX_LPI_NUM_CLKS, pctrl->clks); + if (ret) + return dev_err_probe(dev, ret, "Can't get clocks\n"); + + ret = clk_bulk_prepare_enable(MAX_LPI_NUM_CLKS, pctrl->clks); + if (ret) + return dev_err_probe(dev, ret, "Can't enable clocks\n"); + } pctrl->desc.pctlops = &lpi_gpio_pinctrl_ops; pctrl->desc.pmxops = &lpi_gpio_pinmux_ops; diff --git a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h index a511d72..c1079bf 100644 --- a/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h +++ b/drivers/pinctrl/qcom/pinctrl-lpass-lpi.h @@ -77,6 +77,7 @@ struct lpi_pinctrl_variant_data { int ngroups; const struct lpi_function *functions; int nfunctions; + int is_clk_optional; }; int lpi_pinctrl_probe(struct platform_device *pdev); diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c index 5bf30d97..4277e31 100644 --- a/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c +++ b/drivers/pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c @@ -143,6 +143,7 @@ static const struct lpi_pinctrl_variant_data sc7280_lpi_data = { .ngroups = ARRAY_SIZE(sc7280_groups), .functions = sc7280_functions, .nfunctions = ARRAY_SIZE(sc7280_functions), + .is_clk_optional = 1, }; static const struct of_device_id lpi_pinctrl_of_match[] = {