Message-ID: <20161118193039.GL28340@tuxbot>
Date:   Fri, 18 Nov 2016 11:30:39 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Avaneesh Kumar Dwivedi <akdwived@...eaurora.org>
Cc:     linux-kernel@...r.kernel.org, linux-remoteproc@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling

On Wed 16 Nov 07:17 PST 2016, Avaneesh Kumar Dwivedi wrote:

> 
> 
> On 11/8/2016 12:19 PM, Bjorn Andersson wrote:
> >On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
> >
> >>Handling of clock and regulator resources as well as reset
> >>register programing differ according to version of hexagon
> >>dsp hardware. Different version require different resources
> >>and different parameters for same resource. Hence it is
> >>needed that resource handling is generic and independent of
> >>hexagon dsp version.
> >>
> >It would be much easier to review this if you didn't do all three
> >changes in the same patch, and at the same time changed the function
> >names. There's large parts of this patch where it's not obvious what the
> >actual changes are.
> 
> OK, have broken patches in very small set of function now.
> but patches has increased from 3 to 9.
> sorry for inconvenience caused.

I will have a look once we have agreed on below issues.

[..]
> >>+	struct regulator **active_regs;
> >Keeping these as statically sized arrays, potentially with unused
> >elements at the end removes the need for allocating the storage and the
> >double pointers.
> since i do not know how many resource of a particular type will be needed on
> new version of new class of hexagon that is why i am working with pointers.
> have removed many entries from above resource struct, it will lok much
> cleaner in next patchset.

Just pick the largest number we support right now and then if future
versions need more we increment that number.

> >
> >>  	struct completion start_done;
> >>  	struct completion stop_done;
> >>@@ -147,67 +150,245 @@ struct q6v5 {
> >>  	phys_addr_t mpss_reloc;
> >>  	void *mpss_region;
> >>  	size_t mpss_size;
> >>+	struct mutex q6_lock;
> >>+	bool proxy_unvote_reg;
> >>+	bool proxy_unvote_clk;
> >I still don't see the need for these 3 attributes.
> I agree, Have removed them.
> >
> >>  };
> >>-enum {
> >>-	Q6V5_SUPPLY_CX,
> >>-	Q6V5_SUPPLY_MX,
> >>-	Q6V5_SUPPLY_MSS,
> >>-	Q6V5_SUPPLY_PLL,
> >>-};
> >>+static int q6_regulator_init(struct q6v5 *qproc)
> >>+{
> >>+	struct regulator **reg_arr;
> >>+	int i;
> >>+
> >>+	if (qproc->q6_rproc_res->proxy_reg_cnt) {
> >If you keep proxy_regs and active_regs as arrays you don't need this
> >check.
> Agree, have removed check.
> >
> >>+		reg_arr = devm_kzalloc(qproc->dev,
> >>+		sizeof(reg_arr) * qproc->q6_rproc_res->proxy_reg_cnt,
> >>+		GFP_KERNEL);
> >>+
> >>+		for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> >>+			reg_arr[i] = devm_regulator_get(qproc->dev,
> >>+			qproc->q6_rproc_res->proxy_regs[i]);
> >>+			if (IS_ERR(reg_arr[i]))
> >>+				return PTR_ERR(reg_arr[i]);
> >>+			qproc->proxy_regs = reg_arr;
> >>+		}
> >>+	}
> >>+
> >>+	if (qproc->q6_rproc_res->active_reg_cnt) {
> >>+		reg_arr = devm_kzalloc(qproc->dev,
> >>+		sizeof(reg_arr) * qproc->q6_rproc_res->active_reg_cnt,
> >>+		GFP_KERNEL);
> >>+
> >>+		for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
> >>+			reg_arr[i] = devm_regulator_get(qproc->dev,
> >>+			qproc->q6_rproc_res->active_regs[i]);
> >>+
> >>+			if (IS_ERR(reg_arr[i]))
> >>+				return PTR_ERR(reg_arr[i]);
> >>+			qproc->active_regs = reg_arr;
> >>+		}
> >>+	}
> >Please keep active_regs and proxy_regs as regulator_bulk_data and
> >continue to use devm_regulator_bulk_get(), it should make this code
> >cleaner.
> the way i have reorganized code in next patchset i found using
> devm_regulator_get() more convenient, can i keep using them? as i am reading
> string one by one and based on read string filling a regulator struct with
> its voltage and load and handle info.

If it's cleaner, then sure.

> >
> >>+
> >>+	return 0;
> >>+}
> >>-static int q6v5_regulator_init(struct q6v5 *qproc)
> >>+static int q6_proxy_regulator_enable(struct q6v5 *qproc)
> >>  {
> >>-	int ret;
> >>+	int i, j, ret = 0;
> >>+	int **reg_loadnvoltsetflag;
> >>+	int *reg_load;
> >>+	int *reg_voltage;
> >>+
> >>+	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
> >>+	reg_load = qproc->q6_rproc_res->proxy_reg_load;
> >>+	reg_voltage = qproc->q6_rproc_res->proxy_reg_voltage;
> >Rather then keeping these properties on int-arrays I strongly prefer
> >that you would have a struct { uV, uA } for each regulator.
> Have modified as per suggestion.
> >
> >>+
> >>+	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> >>+		for (j = 0; j <= 1; j++) {
> >>+			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
> >I'm sorry, but this is not clean. Please use the fact that we're not
> >writing assembly code and use the language to your advantage.
> Sorry for mess, have simplified and cleaned.
> >
> >>+				regulator_set_load(qproc->proxy_regs[i],
> >>+				reg_load[i]);
> >>+			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
> >>+				regulator_set_voltage(qproc->proxy_regs[i],
> >>+				reg_voltage[i], INT_MAX);
> >>+		}
> >>+	}
> >>-	qproc->supply[Q6V5_SUPPLY_CX].supply = "cx";
> >>-	qproc->supply[Q6V5_SUPPLY_MX].supply = "mx";
> >>-	qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss";
> >>-	qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll";
> >>+	for (i = 0; i < qproc->q6_rproc_res->proxy_reg_cnt; i++) {
> >>+		ret = regulator_enable(qproc->proxy_regs[i]);
> >>+		if (ret) {
> >>+			for (; i > 0; --i) {
> >>+				regulator_disable(qproc->proxy_regs[i]);
> >>+				return ret;
> >>+			}
> >>+		}
> >>+	}
> >If you just keep your regulators in a regulator_bulk_data array then you
> >can replace this with regulator_bulk_enable(proxy_reg_cnt, proxy_regs);
> As replied above i am going with getting sigle regulator handle one time.
> let me know if i can continue or need to change?
> 

The reason for using the bulk operations is that the error path becomes
cleaner, however now that I look at this again; in the event of an error
you leave the regulators with voltage and load specified. You need to
unroll this too.

But I would still prefer that you specify the loads & voltages, then
call bulk_enable() and if that fail remove all load and voltage
requests.

> >
> >>-	ret = devm_regulator_bulk_get(qproc->dev,
> >>-				      ARRAY_SIZE(qproc->supply), qproc->supply);
> >>-	if (ret < 0) {
> >>-		dev_err(qproc->dev, "failed to get supplies\n");
> >>-		return ret;
[..]
> >>+	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56")) {
> >This would be much better as an enum than a string. But I keep wonder if
> >this is only for v5.6 of the Hexagon - perhaps should we clamp different
> >things on the various versions?.
> 
> As replied elsewhere, we need a DT entry to know which version is running,
> or else many compatible string will be required. for "v56" there are
> following version, so as and when we need to support a new version we will
> require
> a new DT entry which when defined will help to take deviation where
> required.
> 1.10.0
> 1.3.0
> 1.4.0
> 1.5.0
> 1.6.0
> 1.8.0
> 

Sorry for not seeing this before I answered in the two other places,
perhaps we should just discuss this to end in one place...

But regarding my specific comment, if you want class based handling then
introduce:

enum {
	Q6V5_CLASS5,
	Q6V5_CLASS55,
	Q5V5_CLASS56
};

Then you don't have to use strcmp() to check which class you have.

> >
> >>+		/*
> >>+		 * Assert QDSP6 I/O clamp, memory wordline clamp, and compiler
> >>+		 * memory clamp as a software workaround to avoid high MX
> >>+		 * current during LPASS/MSS restart.
> >>+		 */
> >>+
> >>+		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> >>+		val |= (Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
> >>+				QDSP6v56_CLAMP_QMC_MEM);
> >>+		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> >>+		pil_mss_restart_reg(qproc, 1);
> >And by using the reset framework for mss_restart this will fall out of
> >the conditional segment and the else is gone.
> As i informed MSS RESET REGISTER was never a block control reset or BCR (a
> term used to define those reset register which control a clock or pll ) so
> clock control reset framework can not be used to do reset programming for
> MSS

But MSS RESET is a "reset" and far as this driver is concerned it should
be abstracted by the help of the reset framework. I don't want this
driver to care about the workings of the reset control.

The peripheral resets are part of the GCC block and as such I do not see
the problem with having the driver for the GCC block expose these
resets, even if though it's not a BCR - and this is how we have done it
on 8960, 8974 and 8084 so far.

> that is why i have adopted IOREMAP based mss reset programming. it is like
> any other register, may i know if any serious objection on using reset
> controller framework only? i will have to add another dummy driver just to
> do reset register programming.
> let me know please if it is mandatory?

I want this driver to consume a reset from a reset-controller, I do not
see the technical reason why we cannot just add this to the driver for
the GCC block.

Regards,
Bjorn

Powered by blists - more mailing lists