Messages in this thread | | | Date | Tue, 29 Nov 2022 09:24:47 +0100 | From | Neil Armstrong <> | Subject | Re: [PATCH v2 3/6] soc: qcom: geni-se: add desc struct to specify clocks from device match data |
| |
On 18/11/2022 10:06, Konrad Dybcio wrote: > > > On 18/11/2022 09:45, Neil Armstrong wrote: >> The I2C Master Hub is a stripped down version of the GENI Serial Engine >> QUP Wrapper Controller but only supporting I2C serial engines without >> DMA support. >> >> Prepare support for the I2C Master Hub variant by moving the required >> clocks list to a new desc struct then passing it through the compatible >> match data. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> drivers/soc/qcom/qcom-geni-se.c | 59 +++++++++++++++++++++++++++++++---------- >> 1 file changed, 45 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c >> index a0ceeede450f..ced2a2932eda 100644 >> --- a/drivers/soc/qcom/qcom-geni-se.c >> +++ b/drivers/soc/qcom/qcom-geni-se.c >> @@ -81,19 +81,31 @@ >> */ >> #define MAX_CLK_PERF_LEVEL 32 >> -#define NUM_AHB_CLKS 2 >> +#define MAX_CLKS 2 >> /** >> * struct geni_wrapper - Data structure to represent the QUP Wrapper Core >> * @dev: Device pointer of the QUP wrapper core >> * @base: Base address of this instance of QUP wrapper core >> - * @ahb_clks: Handle to the primary & secondary AHB clocks >> + * @clks: Handle to the primary & optional secondary AHB clocks >> + * @num_clks: Count of clocks >> * @to_core: Core ICC path >> */ >> struct geni_wrapper { >> struct device *dev; >> void __iomem *base; >> - struct clk_bulk_data ahb_clks[NUM_AHB_CLKS]; >> + struct clk_bulk_data clks[MAX_CLKS]; >> + unsigned int num_clks; >> +}; >> + >> +/** >> + * struct geni_se_desc - Data structure to represent the QUP Wrapper resources >> + * @clks: Name of the primary & optional secondary AHB clocks >> + * @num_clks: Count of clock names >> + */ >> +struct geni_se_desc { >> + unsigned int num_clks; >> + const char * const *clks; >> }; >> static const char * const icc_path_names[] = {"qup-core", "qup-config", >> @@ -496,8 +508,7 @@ static void geni_se_clks_off(struct geni_se *se) >> struct geni_wrapper *wrapper = se->wrapper; >> clk_disable_unprepare(se->clk); >> - clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks), >> - wrapper->ahb_clks); >> + clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks); >> } >> /** >> @@ -528,15 +539,13 @@ static int geni_se_clks_on(struct geni_se *se) >> int ret; >> struct geni_wrapper *wrapper = se->wrapper; >> - ret = clk_bulk_prepare_enable(ARRAY_SIZE(wrapper->ahb_clks), >> - wrapper->ahb_clks); >> + ret = clk_bulk_prepare_enable(wrapper->num_clks, wrapper->clks); >> if (ret) >> return ret; >> ret = clk_prepare_enable(se->clk); >> if (ret) >> - clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks), >> - wrapper->ahb_clks); >> + clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks); >> return ret; >> } >> @@ -887,11 +896,23 @@ static int geni_se_probe(struct platform_device *pdev) >> return PTR_ERR(wrapper->base); >> if (!has_acpi_companion(&pdev->dev)) { >> - wrapper->ahb_clks[0].id = "m-ahb"; >> - wrapper->ahb_clks[1].id = "s-ahb"; >> - ret = devm_clk_bulk_get(dev, NUM_AHB_CLKS, wrapper->ahb_clks); >> + const struct geni_se_desc *desc; >> + int i; >> + >> + desc = device_get_match_data(&pdev->dev); >> + if (!desc) >> + return -EINVAL; >> + >> + wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS); >> + if (wrapper->num_clks < desc->num_clks) > This will never execute (except if somebody adding a third desc would make a mistake or not update MAX_CLKS), as wrapper->num_clks will only be < desc->num_clks if desc->num_clks > MAX_CLKS.
You're right, I did read too fast.
> > I was thinking about getting the number of actual clocks passed to the device in the DT, but I can't find a helper function for that, so it would probably require some kind of manual looping.. I guess we can drop this. Or leave it to save somebody pulling their hair out in an unlikely event. I guess I'm fine with both.
This would be:
of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells")
but ultimately if the number of clocks is lower than requested, it will fail in the call to devm_clk_bulk_get().
Would we warn if the DT clocks count is higher ? or simply fail if lower ?
Neil
> > >> + dev_warn(dev, "too much clocks described in DT\n") > If you leave it, s/too much/Too many/ > > > Konrad >> + >> + for (i = 0; i < wrapper->num_clks; ++i) >> + wrapper->clks[i].id = desc->clks[i]; >> + >> + ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks); >> if (ret) { >> - dev_err(dev, "Err getting AHB clks %d\n", ret); >> + dev_err(dev, "Err getting clks %d\n", ret); >> return ret; >> } >> } >> @@ -901,8 +922,18 @@ static int geni_se_probe(struct platform_device *pdev) >> return devm_of_platform_populate(dev); >> } >> +static const char * const qup_clks[] = { >> + "m-ahb", >> + "s-ahb", >> +}; >> + >> +static const struct geni_se_desc qup_desc = { >> + .clks = qup_clks, >> + .num_clks = ARRAY_SIZE(qup_clks), >> +}; >> + >> static const struct of_device_id geni_se_dt_match[] = { >> - { .compatible = "qcom,geni-se-qup", }, >> + { .compatible = "qcom,geni-se-qup", .data = &qup_desc }, >> {} >> }; >> MODULE_DEVICE_TABLE(of, geni_se_dt_match); >>
| |