| Message ID | 20250506180232.1299-4-quic_ptalari@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable QUPs and Serial on SA8255p Qualcomm platforms | expand |
On 06/05/2025 19:02, Praveen Talari wrote: > On the sa8255p platform, resources such as clocks,interconnects > and TLMM (GPIO) configurations are managed by firmware. > > Introduce a platform data function callback to distinguish whether > resource control is performed by firmware or directly by the driver > in linux. > > The refactor ensures clear differentiation of resource > management mechanisms, improving maintainability and flexibility > in handling platform-specific configurations. > > Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com> > --- > v3 -> v4 > - declared an empty struct for sa8255p and added check as num clks. > - Added version log after --- > > v1 -> v2 > - changed datatype of i from int to unsigned int as per comment. > --- > drivers/soc/qcom/qcom-geni-se.c | 73 ++++++++++++++++++++------------- > 1 file changed, 45 insertions(+), 28 deletions(-) > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > index 4cb959106efa..b6e90bac55fe 100644 > --- a/drivers/soc/qcom/qcom-geni-se.c > +++ b/drivers/soc/qcom/qcom-geni-se.c > @@ -105,6 +105,8 @@ struct geni_wrapper { > struct geni_se_desc { > unsigned int num_clks; > const char * const *clks; > + int (*geni_se_rsc_init)(struct geni_wrapper *wrapper, > + const struct geni_se_desc *desc); > }; > > static const char * const icc_path_names[] = {"qup-core", "qup-config", > @@ -891,10 +893,44 @@ int geni_icc_disable(struct geni_se *se) > } > EXPORT_SYMBOL_GPL(geni_icc_disable); > > +static int geni_se_resource_init(struct geni_wrapper *wrapper, > + const struct geni_se_desc *desc) > +{ > + struct device *dev = wrapper->dev; > + int ret; > + unsigned int i; > + > + wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS); It should be an error to depend on more clocks - which are specified in a descriptor down the bottom of this file than MAX_CLKS allows. > + > + for (i = 0; i < wrapper->num_clks; ++i) > + wrapper->clks[i].id = desc->clks[i]; > + > + ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells"); > + if (ret < 0) { > + dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node); > + return ret; return dev_err_probe(); > + } > + > + if (ret < wrapper->num_clks) { > + dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n", > + dev->of_node, wrapper->num_clks); > + return -EINVAL; > + } > + > + ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks); > + if (ret) { > + dev_err(dev, "Err getting clks %d\n", ret); > + return ret; > + } > + > + return ret; > +} > + > static int geni_se_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct geni_wrapper *wrapper; > + const struct geni_se_desc *desc; > int ret; > > wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL); > @@ -906,36 +942,12 @@ static int geni_se_probe(struct platform_device *pdev) > if (IS_ERR(wrapper->base)) > return PTR_ERR(wrapper->base); > > - if (!has_acpi_companion(&pdev->dev)) { > - const struct geni_se_desc *desc; > - int i; > + desc = device_get_match_data(&pdev->dev); > > - desc = device_get_match_data(&pdev->dev); > - if (!desc) > + if (!has_acpi_companion(&pdev->dev) && desc->num_clks) { There is no desc in this file that has !num_clks I don't think the conjunction is justified. > + ret = desc->geni_se_rsc_init(wrapper, desc); > + if (ret) > return -EINVAL; > - > - wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS); > - > - for (i = 0; i < wrapper->num_clks; ++i) > - wrapper->clks[i].id = desc->clks[i]; > - > - ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells"); > - if (ret < 0) { > - dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node); > - return ret; > - } > - > - if (ret < wrapper->num_clks) { > - dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n", > - dev->of_node, wrapper->num_clks); > - return -EINVAL; > - } > - > - ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks); > - if (ret) { > - dev_err(dev, "Err getting clks %d\n", ret); > - return ret; > - } > } > > dev_set_drvdata(dev, wrapper); > @@ -951,8 +963,11 @@ static const char * const qup_clks[] = { > static const struct geni_se_desc qup_desc = { > .clks = qup_clks, > .num_clks = ARRAY_SIZE(qup_clks), > + .geni_se_rsc_init = geni_se_resource_init, > }; > > +static const struct geni_se_desc sa8255p_qup_desc; > + > static const char * const i2c_master_hub_clks[] = { > "s-ahb", > }; > @@ -960,11 +975,13 @@ static const char * const i2c_master_hub_clks[] = { > static const struct geni_se_desc i2c_master_hub_desc = { > .clks = i2c_master_hub_clks, > .num_clks = ARRAY_SIZE(i2c_master_hub_clks), > + .geni_se_rsc_init = geni_se_resource_init, > }; > > static const struct of_device_id geni_se_dt_match[] = { > { .compatible = "qcom,geni-se-qup", .data = &qup_desc }, > { .compatible = "qcom,geni-se-i2c-master-hub", .data = &i2c_master_hub_desc }, > + { .compatible = "qcom,sa8255p-geni-se-qup", .data = &sa8255p_qup_desc }, > {} > }; > MODULE_DEVICE_TABLE(of, geni_se_dt_match); > -- > 2.17.1 > > Other than those minor details looks pretty good. Please include me in v6 and I will review further. --- bod
Hi Bryan, Thank you for review. On 6/3/2025 7:51 PM, Bryan O'Donoghue wrote: > On 06/05/2025 19:02, Praveen Talari wrote: >> On the sa8255p platform, resources such as clocks,interconnects >> and TLMM (GPIO) configurations are managed by firmware. >> >> Introduce a platform data function callback to distinguish whether >> resource control is performed by firmware or directly by the driver >> in linux. >> >> The refactor ensures clear differentiation of resource >> management mechanisms, improving maintainability and flexibility >> in handling platform-specific configurations. >> >> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com> >> --- >> v3 -> v4 >> - declared an empty struct for sa8255p and added check as num clks. >> - Added version log after --- >> >> v1 -> v2 >> - changed datatype of i from int to unsigned int as per comment. >> --- >> drivers/soc/qcom/qcom-geni-se.c | 73 ++++++++++++++++++++------------- >> 1 file changed, 45 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/soc/qcom/qcom-geni-se.c >> b/drivers/soc/qcom/qcom-geni-se.c >> index 4cb959106efa..b6e90bac55fe 100644 >> --- a/drivers/soc/qcom/qcom-geni-se.c >> +++ b/drivers/soc/qcom/qcom-geni-se.c >> @@ -105,6 +105,8 @@ struct geni_wrapper { >> struct geni_se_desc { >> unsigned int num_clks; >> const char * const *clks; >> + int (*geni_se_rsc_init)(struct geni_wrapper *wrapper, >> + const struct geni_se_desc *desc); >> }; >> >> static const char * const icc_path_names[] = {"qup-core", "qup-config", >> @@ -891,10 +893,44 @@ int geni_icc_disable(struct geni_se *se) >> } >> EXPORT_SYMBOL_GPL(geni_icc_disable); >> >> +static int geni_se_resource_init(struct geni_wrapper *wrapper, >> + const struct geni_se_desc *desc) >> +{ >> + struct device *dev = wrapper->dev; >> + int ret; >> + unsigned int i; >> + >> + wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS); > > It should be an error to depend on more clocks - which are specified in > a descriptor down the bottom of this file than MAX_CLKS allows. should i keep condition before assign num_clks to wrapper->num_clks as below right? if(desc->num_clks > MAX_CLKS){ { return dev_err_probe(dev, " to many clocks defined in descriptor:%u (max allowed: %u)\n", desc->num_clks, MAX_CLKS); } Please correct me if i am wrong. Thanks, Praveen Talari > >> + >> + for (i = 0; i < wrapper->num_clks; ++i) >> + wrapper->clks[i].id = desc->clks[i]; >> + >> + ret = of_count_phandle_with_args(dev->of_node, "clocks", >> "#clock-cells"); >> + if (ret < 0) { >> + dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node); >> + return ret; > > return dev_err_probe(); > >> + } >> + >> + if (ret < wrapper->num_clks) { >> + dev_err(dev, "invalid clocks count at %pOF, expected %d >> entries\n", >> + dev->of_node, wrapper->num_clks); >> + return -EINVAL; >> + } >> + >> + ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks); >> + if (ret) { >> + dev_err(dev, "Err getting clks %d\n", ret); >> + return ret; >> + } >> + >> + return ret; >> +} >> + >> static int geni_se_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct geni_wrapper *wrapper; >> + const struct geni_se_desc *desc; >> int ret; >> >> wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL); >> @@ -906,36 +942,12 @@ static int geni_se_probe(struct platform_device >> *pdev) >> if (IS_ERR(wrapper->base)) >> return PTR_ERR(wrapper->base); >> >> - if (!has_acpi_companion(&pdev->dev)) { >> - const struct geni_se_desc *desc; >> - int i; >> + desc = device_get_match_data(&pdev->dev); >> >> - desc = device_get_match_data(&pdev->dev); >> - if (!desc) >> + if (!has_acpi_companion(&pdev->dev) && desc->num_clks) { > > There is no desc in this file that has !num_clks I don't think the > conjunction is justified. there is empty struct defined below sa8255p_qup_desc. > >> + ret = desc->geni_se_rsc_init(wrapper, desc); >> + if (ret) >> return -EINVAL; >> - >> - wrapper->num_clks = min_t(unsigned int, desc->num_clks, >> MAX_CLKS); >> - >> - for (i = 0; i < wrapper->num_clks; ++i) >> - wrapper->clks[i].id = desc->clks[i]; >> - >> - ret = of_count_phandle_with_args(dev->of_node, "clocks", >> "#clock-cells"); >> - if (ret < 0) { >> - dev_err(dev, "invalid clocks property at %pOF\n", >> dev->of_node); >> - return ret; >> - } >> - >> - if (ret < wrapper->num_clks) { >> - dev_err(dev, "invalid clocks count at %pOF, expected %d >> entries\n", >> - dev->of_node, wrapper->num_clks); >> - return -EINVAL; >> - } >> - >> - ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks); >> - if (ret) { >> - dev_err(dev, "Err getting clks %d\n", ret); >> - return ret; >> - } >> } >> >> dev_set_drvdata(dev, wrapper); >> @@ -951,8 +963,11 @@ static const char * const qup_clks[] = { >> static const struct geni_se_desc qup_desc = { >> .clks = qup_clks, >> .num_clks = ARRAY_SIZE(qup_clks), >> + .geni_se_rsc_init = geni_se_resource_init, >> }; >> >> +static const struct geni_se_desc sa8255p_qup_desc; >> + >> static const char * const i2c_master_hub_clks[] = { >> "s-ahb", >> }; >> @@ -960,11 +975,13 @@ static const char * const i2c_master_hub_clks[] = { >> static const struct geni_se_desc i2c_master_hub_desc = { >> .clks = i2c_master_hub_clks, >> .num_clks = ARRAY_SIZE(i2c_master_hub_clks), >> + .geni_se_rsc_init = geni_se_resource_init, >> }; >> >> static const struct of_device_id geni_se_dt_match[] = { >> { .compatible = "qcom,geni-se-qup", .data = &qup_desc }, >> { .compatible = "qcom,geni-se-i2c-master-hub", .data = >> &i2c_master_hub_desc }, >> + { .compatible = "qcom,sa8255p-geni-se-qup", .data = >> &sa8255p_qup_desc }, >> {} >> }; >> MODULE_DEVICE_TABLE(of, geni_se_dt_match); >> -- >> 2.17.1 >> >> > Other than those minor details looks pretty good. > Please include me in v6 and I will review further. > --- > bod >
On 04/06/2025 08:28, Praveen Talari wrote: > should i keep condition before assign num_clks to wrapper->num_clks as > below right? > > if(desc->num_clks > MAX_CLKS){ > { > return dev_err_probe(dev, " to many clocks defined in descriptor:%u > (max allowed: %u)\n", desc->num_clks, MAX_CLKS); > } Its up to you if you think this check is justified and/or necessary but, if you decide you want it then it should be an error if the specified size exceeds your defined MAX. --- bod
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c index 4cb959106efa..b6e90bac55fe 100644 --- a/drivers/soc/qcom/qcom-geni-se.c +++ b/drivers/soc/qcom/qcom-geni-se.c @@ -105,6 +105,8 @@ struct geni_wrapper { struct geni_se_desc { unsigned int num_clks; const char * const *clks; + int (*geni_se_rsc_init)(struct geni_wrapper *wrapper, + const struct geni_se_desc *desc); }; static const char * const icc_path_names[] = {"qup-core", "qup-config", @@ -891,10 +893,44 @@ int geni_icc_disable(struct geni_se *se) } EXPORT_SYMBOL_GPL(geni_icc_disable); +static int geni_se_resource_init(struct geni_wrapper *wrapper, + const struct geni_se_desc *desc) +{ + struct device *dev = wrapper->dev; + int ret; + unsigned int i; + + wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS); + + for (i = 0; i < wrapper->num_clks; ++i) + wrapper->clks[i].id = desc->clks[i]; + + ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells"); + if (ret < 0) { + dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node); + return ret; + } + + if (ret < wrapper->num_clks) { + dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n", + dev->of_node, wrapper->num_clks); + return -EINVAL; + } + + ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks); + if (ret) { + dev_err(dev, "Err getting clks %d\n", ret); + return ret; + } + + return ret; +} + static int geni_se_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct geni_wrapper *wrapper; + const struct geni_se_desc *desc; int ret; wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL); @@ -906,36 +942,12 @@ static int geni_se_probe(struct platform_device *pdev) if (IS_ERR(wrapper->base)) return PTR_ERR(wrapper->base); - if (!has_acpi_companion(&pdev->dev)) { - const struct geni_se_desc *desc; - int i; + desc = device_get_match_data(&pdev->dev); - desc = device_get_match_data(&pdev->dev); - if (!desc) + if (!has_acpi_companion(&pdev->dev) && desc->num_clks) { + ret = desc->geni_se_rsc_init(wrapper, desc); + if (ret) return -EINVAL; - - wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS); - - for (i = 0; i < wrapper->num_clks; ++i) - wrapper->clks[i].id = desc->clks[i]; - - ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells"); - if (ret < 0) { - dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node); - return ret; - } - - if (ret < wrapper->num_clks) { - dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n", - dev->of_node, wrapper->num_clks); - return -EINVAL; - } - - ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks); - if (ret) { - dev_err(dev, "Err getting clks %d\n", ret); - return ret; - } } dev_set_drvdata(dev, wrapper); @@ -951,8 +963,11 @@ static const char * const qup_clks[] = { static const struct geni_se_desc qup_desc = { .clks = qup_clks, .num_clks = ARRAY_SIZE(qup_clks), + .geni_se_rsc_init = geni_se_resource_init, }; +static const struct geni_se_desc sa8255p_qup_desc; + static const char * const i2c_master_hub_clks[] = { "s-ahb", }; @@ -960,11 +975,13 @@ static const char * const i2c_master_hub_clks[] = { static const struct geni_se_desc i2c_master_hub_desc = { .clks = i2c_master_hub_clks, .num_clks = ARRAY_SIZE(i2c_master_hub_clks), + .geni_se_rsc_init = geni_se_resource_init, }; static const struct of_device_id geni_se_dt_match[] = { { .compatible = "qcom,geni-se-qup", .data = &qup_desc }, { .compatible = "qcom,geni-se-i2c-master-hub", .data = &i2c_master_hub_desc }, + { .compatible = "qcom,sa8255p-geni-se-qup", .data = &sa8255p_qup_desc }, {} }; MODULE_DEVICE_TABLE(of, geni_se_dt_match);
On the sa8255p platform, resources such as clocks,interconnects and TLMM (GPIO) configurations are managed by firmware. Introduce a platform data function callback to distinguish whether resource control is performed by firmware or directly by the driver in linux. The refactor ensures clear differentiation of resource management mechanisms, improving maintainability and flexibility in handling platform-specific configurations. Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com> --- v3 -> v4 - declared an empty struct for sa8255p and added check as num clks. - Added version log after --- v1 -> v2 - changed datatype of i from int to unsigned int as per comment. --- drivers/soc/qcom/qcom-geni-se.c | 73 ++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 28 deletions(-)