Devicetree

* Re: [PATCH v2 4/5] drivers: bus: add driver for initializing the SSC bus on (some) qcom SoCs
From: Philipp Zabel @ 2022-01-24 12:22 UTC (permalink / raw)
  To: michael.srba, Andy Gross, Bjorn Andersson, Rob Herring,
	Stephen Boyd
  Cc: Linus Walleij, Florian Fainelli, Arnd Bergmann,
	Greg Kroah-Hartman, Saravana Kannan, linux-arm-msm, devicetree
In-Reply-To: <20220124112740.22790-1-michael.srba@seznam.cz>

Hi Michael,

On Mon, 2022-01-24 at 12:27 +0100, michael.srba@seznam.cz wrote:
[...]
> +
> +static int qcom_ssc_block_bus_init(struct device *dev)
> +{
> +	int ret;
> +
> +	struct qcom_ssc_block_bus_data *data = dev_get_drvdata(dev);
> +
> +	clk_prepare_enable(data->xo_clk);
> +	clk_prepare_enable(data->aggre2_clk);
> +
> +	clk_prepare_enable(data->gcc_im_sleep_clk);

Those should be disabled on failure below, otherwise they could
accumulate enable_counts with multiple failed block_bus_init calls.
(Assuming that the reset_control_* can fail in practice.)

> +
> +	reg32_clear_bits(data->reg_mpm_sscaon_config0, BIT(4) | BIT(5));
> +	reg32_clear_bits(data->reg_mpm_sscaon_config1, BIT(31));
> +
> +	clk_disable(data->aggre2_north_clk);

Where was this clock enabled before?

> +
> +	ret = reset_control_deassert(data->ssc_reset);
> +	if (ret) {
> +		dev_err(dev, "error deasserting ssc_reset: %d\n", ret);
> +		return ret;

And leave the aggre2_north_clk disabled? See below.

> +	}
> +
> +	clk_prepare_enable(data->aggre2_north_clk);
> +
> +	ret = reset_control_deassert(data->ssc_bcr);
> +	if (ret) {
> +		dev_err(dev, "error deasserting ssc_bcr: %d\n", ret);
> +		return ret;

And leave the aggre2_north_clk enabled? This needs to be consistent.

> +	}
> +
> +	regmap_write(data->halt_map, data->ssc_axi_halt + AXI_HALTREQ_REG, 0);
> +
> +	clk_prepare_enable(data->ssc_xo_clk);
> +
> +	clk_prepare_enable(data->ssc_ahbs_clk);
> +
> +	return 0;
> +}
> +
> +static int qcom_ssc_block_bus_deinit(struct device *dev)

Why does this return int when its result is never checked?

> +{
> +	int ret;
> +
> +	struct qcom_ssc_block_bus_data *data = dev_get_drvdata(dev);
> +
> +	clk_disable(data->ssc_xo_clk);
> +	clk_disable(data->ssc_ahbs_clk);
> +
> +	ret = reset_control_assert(data->ssc_bcr);
> +	if (ret) {
> +		dev_err(dev, "error asserting ssc_bcr: %d\n", ret);
> +		return ret;

And leave clocks below enabled?

> +	}
> +
> +	regmap_write(data->halt_map, data->ssc_axi_halt + AXI_HALTREQ_REG, 1);
> +
> +	reg32_set_bits(data->reg_mpm_sscaon_config1, BIT(31));
> +	reg32_set_bits(data->reg_mpm_sscaon_config0, BIT(4) | BIT(5));
> +
> +	ret = reset_control_assert(data->ssc_reset);
> +	if (ret) {
> +		dev_err(dev, "error asserting ssc_reset: %d\n", ret);
> +		return ret;

Same as above.

> +	}
> +
> +	clk_disable(data->gcc_im_sleep_clk);
> +
> +	clk_disable(data->aggre2_north_clk);
> +
> +	clk_disable(data->aggre2_clk);
> +	clk_disable(data->xo_clk);
> +
> +	return 0;
> +}
[...]
> +static int qcom_ssc_block_bus_probe(struct platform_device *pdev)
> +{
> +	struct qcom_ssc_block_bus_data *data;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct of_phandle_args halt_args;
> +	struct resource *res;
> +	int ret;
> +
> +	if (np)
> +		of_platform_populate(np, NULL, NULL, &pdev->dev);
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	data->pd_names = qcom_ssc_block_pd_names;
> +	data->num_pds = ARRAY_SIZE(qcom_ssc_block_pd_names);
> +
> +	ret = qcom_ssc_block_bus_pds_attach(&pdev->dev, data->pds, data->pd_names, data->num_pds);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "error when attaching power domains: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = qcom_ssc_block_bus_pds_enable(data->pds, data->num_pds);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "error when enabling power domains: %d\n", ret);
> +		return ret;
> +	}
> +
> +	// the meaning of the bits in these two registers is sadly not documented,
> +	// the set/clear operations are just copying what qcom does
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpm_sscaon_config0");
> +	data->reg_mpm_sscaon_config0 = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->reg_mpm_sscaon_config0)) {
> +		ret = PTR_ERR(data->reg_mpm_sscaon_config0);
> +		dev_err(&pdev->dev, "failed to ioremap mpm_sscaon_config0 (err: %d)\n", ret);
> +		return ret;
> +	}
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpm_sscaon_config0");
> +	data->reg_mpm_sscaon_config1 = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->reg_mpm_sscaon_config1)) {
> +		ret = PTR_ERR(data->reg_mpm_sscaon_config1);
> +		dev_err(&pdev->dev, "failed to ioremap mpm_sscaon_config1 (err: %d)\n", ret);
> +		return ret;
> +	}
> +
> +	data->ssc_bcr = devm_reset_control_get_exclusive(&pdev->dev, "ssc_bcr");
> +	if (IS_ERR(data->ssc_bcr)) {
> +		ret = PTR_ERR(data->ssc_bcr);
> +		dev_err(&pdev->dev, "failed to acquire reset: scc_bcr (err: %d)\n", ret);
> +		return ret;

Why check for -EPROBE_DEFER for the clocks but not here?
You could use dev_err_probe() to simplify all these error returns.

regards
Philipp

^ permalink raw reply