| Message ID | 20221114-narmstrong-sm8550-upstream-i2c-master-hub-v2-3-aadaa6997b28@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: qcom: add support for the I2C Master Hub | expand |
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. 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. > + 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); >
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); >>
On 29.11.2022 09:24, Neil Armstrong wrote: > 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 ? Just "fail if lower" sounds good. Konrad > > 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); >>> >
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) + dev_warn(dev, "too much clocks described in DT\n"); + + 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);