Re: [PATCH v2 3/6] soc: qcom: geni-se: add desc struct to specify clocks from device match data

[Neil Armstrong]

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@xxxxxxxxxx>
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> > ---
> >   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);