| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cbccbcbe-5006-cdc9-f3af-39c53d87b8a5@linaro.org>
Date: Tue, 29 Nov 2022 09:24:47 +0100
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Andy Gross <agross@...nel.org>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-i2c@...r.kernel.org
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@...aro.org>
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@...aro.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);
>>
Powered by blists - more mailing lists