Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1523579 > unrolled thread

Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling

Started byAvaneesh Kumar Dwivedi <akdwived@codeaurora.org>
First post2016-11-16 16:20 +0100
Last post2016-11-21 20:30 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> - 2016-11-16 16:20 +0100
    Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-11-18 20:40 +0100
      Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> - 2016-11-21 20:30 +0100

#1523579 — Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling

FromAvaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Date2016-11-16 16:20 +0100
SubjectRe: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling
Message-ID<sEaaf-4Bb-55@gated-at.bofh.it>

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.
>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 471 +++++++++++++++++++++++++++----------
>>   1 file changed, 344 insertions(+), 127 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index b60dff3..1fc505b 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -30,6 +30,7 @@
>>   #include <linux/reset.h>
>>   #include <linux/soc/qcom/smem.h>
>>   #include <linux/soc/qcom/smem_state.h>
>> +#include <linux/mutex.h>
>>   #include <linux/of_device.h>
>>   
>>   #include "remoteproc_internal.h"
>> @@ -93,6 +94,8 @@
>>   #define QDSS_BHS_ON			BIT(21)
>>   #define QDSS_LDO_BYP			BIT(22)
>>   
>> +#define QDSP6v56_CLAMP_WL               BIT(21)
>> +#define QDSP6v56_CLAMP_QMC_MEM          BIT(22)
>>   struct q6_rproc_res {
>>   	char **proxy_clks;
>>   	int proxy_clk_cnt;
>> @@ -129,11 +132,11 @@ struct q6v5 {
>>   	struct qcom_smem_state *state;
>>   	unsigned stop_bit;
>>   
>> -	struct regulator_bulk_data supply[4];
>>   	const struct q6_rproc_res *q6_rproc_res;
>> -	struct clk *ahb_clk;
>> -	struct clk *axi_clk;
>> -	struct clk *rom_clk;
>> +	struct clk **active_clks;
>> +	struct clk **proxy_clks;
>> +	struct regulator **proxy_regs;
>> +	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.
>
>>   
>>   	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.
>
>> +
>> +	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?

>
>>   
>> -	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;
>> +	qproc->proxy_unvote_reg = true;
> This should still not be needed.
Yes Removed
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6_active_regulator_enable(struct q6v5 *qproc)
>> +{
>> +	int i, j, ret = 0;
>> +	int **reg_loadnvoltsetflag;
>> +	int *reg_load;
>> +	int *reg_voltage;
>> +
>> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->active_reg_action;
>> +	reg_load = qproc->q6_rproc_res->active_reg_load;
>> +	reg_voltage = qproc->q6_rproc_res->active_reg_voltage;
>> +
>> +	for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
>> +		for (j = 0; j <= 1; j++) {
>> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_load(qproc->active_regs[i],
>> +				reg_load[i]);
>> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_voltage(qproc->active_regs[i],
>> +				reg_voltage[i], INT_MAX);
>> +		}
>>   	}
>>   
>> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 100000);
>> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_MSS].consumer, 100000);
>> -	regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 10000);
>> +	for (i = 0; i < qproc->q6_rproc_res->active_reg_cnt; i++) {
>> +		ret = regulator_enable(qproc->active_regs[i]);
>> +		if (ret) {
>> +			for (; i > 0; --i) {
>> +				regulator_disable(qproc->active_regs[i]);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>>   
>>   	return 0;
>>   }
>>   
>> -static int q6v5_regulator_enable(struct q6v5 *qproc)
>> +static int q6_regulator_enable(struct q6v5 *qproc)
>>   {
>> -	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
>> -	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
>>   	int ret;
>>   
>> -	/* TODO: Q6V5_SUPPLY_CX is supposed to be set to super-turbo here */
>> +	if (qproc->q6_rproc_res->proxy_reg_cnt)
>> +		ret = q6_proxy_regulator_enable(qproc);
>>   
>> -	ret = regulator_set_voltage(mx, 1050000, INT_MAX);
>> -	if (ret)
>> -		return ret;
>> +	if (qproc->q6_rproc_res->active_reg_cnt)
> q6_active_regulator_enable() is a no-op if active_reg_cnt is 0, so no
> need to check that. Rather than having two functions, try to
> parameterize the regulator enable functions so that you can have a
> single function that you pass the active or proxy list.
Agreed, have modified
>
>> +		ret = q6_active_regulator_enable(qproc);
>>   
>> -	regulator_set_voltage(mss, 1000000, 1150000);
>> +	return ret;
>> +}
>>   
>> -	return regulator_bulk_enable(ARRAY_SIZE(qproc->supply), qproc->supply);
>> +static int q6_proxy_regulator_disable(struct q6v5 *qproc)
>> +{
>> +	int i, j;
>> +	int **reg_loadnvoltsetflag;
>> +
>> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->proxy_reg_action;
>> +	if (!qproc->proxy_unvote_reg)
>> +		return 0;
>> +	for (i = qproc->q6_rproc_res->proxy_reg_cnt-1; i >= 0; i--) {
>> +		for (j = 0; j <= 1; j++) {
>> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_load(qproc->proxy_regs[i], 0);
>> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_voltage(qproc->proxy_regs[i],
>> +				0, INT_MAX);
>> +		}
>> +	}
>> +	for (i = qproc->q6_rproc_res->proxy_reg_cnt-1; i >= 0; i--)
>> +		regulator_disable(qproc->proxy_regs[i]);
>> +	qproc->proxy_unvote_reg = false;
>> +	return 0;
>>   }
>>   
>> -static void q6v5_regulator_disable(struct q6v5 *qproc)
>> +static int q6_active_regulator_disable(struct q6v5 *qproc)
>>   {
>> -	struct regulator *mss = qproc->supply[Q6V5_SUPPLY_MSS].consumer;
>> -	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
>> +	int i, j, ret = 0;
>> +	int **reg_loadnvoltsetflag;
>> +
>> +	reg_loadnvoltsetflag = qproc->q6_rproc_res->active_reg_action;
>> +
>> +	for (i = qproc->q6_rproc_res->active_reg_cnt-1; i > 0; i--) {
>> +		for (j = 0; j <= 1; j++) {
>> +			if (j == 0 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_load(qproc->active_regs[i], 0);
>> +			if (j == 1 && *(reg_loadnvoltsetflag + i*j + j))
>> +				regulator_set_voltage(qproc->active_regs[i],
>> +				0, INT_MAX);
>> +		}
>> +	}
>> +	for (i = qproc->q6_rproc_res->active_reg_cnt-1; i >= 0; i--)
>> +		ret = regulator_disable(qproc->proxy_regs[i]);
>> +	return 0;
>> +}
>> +
>> +static void q6_regulator_disable(struct q6v5 *qproc)
>> +{
>> +	if (qproc->q6_rproc_res->proxy_reg_cnt)
>> +		q6_proxy_regulator_disable(qproc);
>>   
>> -	/* TODO: Q6V5_SUPPLY_CX corner votes should be released */
>> +	if (qproc->q6_rproc_res->active_reg_cnt)
>> +		q6_active_regulator_disable(qproc);
>> +}
>>   
>> -	regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply);
>> -	regulator_set_voltage(mx, 0, INT_MAX);
>> -	regulator_set_voltage(mss, 0, 1150000);
>> +static int q6_proxy_clk_enable(struct q6v5 *qproc)
> This is really the same as active_clk_enable(), so you should just have
> a function that you pass an array of clocks and a count to - similar to
> regulator_bulk_enable().
Yes , modified.
>
>> +{
>> +	int i, ret = 0;
>> +
>> +	for (i = 0; i < qproc->q6_rproc_res->proxy_clk_cnt; i++) {
>> +		ret = clk_prepare_enable(qproc->proxy_clks[i]);
>> +		if (ret) {
>> +			for (; i > 0; --i) {
>> +				clk_disable_unprepare(qproc->proxy_clks[i]);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +	qproc->proxy_unvote_clk = true;
>> +	return 0;
>>   }
>>   
>> +static void q6_proxy_clk_disable(struct q6v5 *qproc)
>> +{
>> +	int i;
>> +
>> +	if (!qproc->proxy_unvote_clk)
>> +		return;
>> +	for (i = qproc->q6_rproc_res->proxy_clk_cnt-1; i >= 0; i--)
>> +		clk_disable_unprepare(qproc->proxy_clks[i]);
>> +	qproc->proxy_unvote_clk = false;
>> +}
>> +
>> +static int q6_active_clk_enable(struct q6v5 *qproc)
>> +{
>> +	int i, ret = 0;
> No need to initialize ret, as its first use is an assignment.
>
>> +
>> +	for (i = 0; i < qproc->q6_rproc_res->active_clk_cnt; i++) {
>> +		ret = clk_prepare_enable(qproc->active_clks[i]);
>> +		if (ret) {
> Use goto here, rather than nesting a error return in here.
OK
>
>> +			for (; i > 0; i--) {
>> +				clk_disable_unprepare(qproc->active_clks[i]);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void q6_active_clk_disable(struct q6v5 *qproc)
>> +{
>> +	int i;
>> +
>> +	for (i = qproc->q6_rproc_res->active_clk_cnt-1; i >= 0; i--)
>> +		clk_disable_unprepare(qproc->active_clks[i]);
>> +}
>> +
>> +static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
>> +{
>> +	if (qproc->restart_reg) {
>> +		writel_relaxed(mss_restart, qproc->restart_reg);
>> +		udelay(2);
>> +	}
>> +}
>>   static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>>   {
>>   	struct q6v5 *qproc = rproc->priv;
>> @@ -340,11 +521,6 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>>   	unsigned int val;
>>   	int ret;
>>   
>> -	/* Check if we're already idle */
>> -	ret = regmap_read(halt_map, offset + AXI_IDLE_REG, &val);
>> -	if (!ret && val)
>> -		return;
>> -
> Please put this in its own commit and describe why it can't be there on
> 8996 and why it's okay to drop on 8974 and 8916.
>
>>   	/* Assert halt request */
>>   	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 1);
>>   
>> @@ -366,7 +542,7 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>>   	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>>   }
>>   
>> -static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>> +static int q6_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   {
>>   	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>>   	dma_addr_t phys;
>> @@ -395,7 +571,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>>   	return ret < 0 ? ret : 0;
>>   }
>>   
>> -static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>> +static int q6_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>>   {
>>   	const struct elf32_phdr *phdrs;
>>   	const struct elf32_phdr *phdr;
>> @@ -451,7 +627,7 @@ static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
>>   	return ret < 0 ? ret : 0;
>>   }
>>   
>> -static int q6v5_mpss_load(struct q6v5 *qproc)
>> +static int q6_mpss_load(struct q6v5 *qproc)
>>   {
>>   	const struct firmware *fw;
>>   	phys_addr_t fw_addr;
>> @@ -476,7 +652,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	/* Initialize the RMB validator */
>>   	writel(0, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
>>   
>> -	ret = q6v5_mpss_init_image(qproc, fw);
>> +	ret = q6_mpss_init_image(qproc, fw);
>>   	if (ret)
>>   		goto release_firmware;
>>   
>> @@ -484,7 +660,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	if (ret)
>>   		goto release_firmware;
>>   
>> -	ret = q6v5_mpss_validate(qproc, fw);
>> +	ret = q6_mpss_validate(qproc, fw);
>>   
>>   release_firmware:
>>   	release_firmware(fw);
>> @@ -492,36 +668,41 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	return ret < 0 ? ret : 0;
>>   }
>>   
>> -static int q6v5_start(struct rproc *rproc)
>> +static int q6_start(struct rproc *rproc)
> Most of the changes in this function are renaming of functions and
> variables, please don't do this as part of a functional change.
>
> Best would be if you start with a commit that renames the necessary
> parts and where you specify that there's "no functional change".

OK, Done.
>
>>   {
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>>   
>> -	ret = q6v5_regulator_enable(qproc);
>> +	mutex_lock(&qproc->q6_lock);
> We should already be protected by the rproc->lock here, please let me
> know if there are any gaps.
Sure, Removed.
>
>> +	ret = q6_regulator_enable(qproc);
>>   	if (ret) {
>> -		dev_err(qproc->dev, "failed to enable supplies\n");
>> +		dev_err(qproc->dev, "failed to enable reg supplies\n");
> Supplies are regulators, but if you find this confusing then you
> shouldn't abbreviate regulators as "reg".
OK, Done.
>
>>   		return ret;
>>   	}
>>   
>> -	ret = reset_control_deassert(qproc->mss_restart);
> So the correct order is: enable clocks, then deassert reset?
No, deassert need to be done before Q6 clocks are enabled, Done.
>
>> +	ret = q6_proxy_clk_enable(qproc);
>>   	if (ret) {
>> -		dev_err(qproc->dev, "failed to deassert mss restart\n");
>> -		goto disable_vdd;
>> +		dev_err(qproc->dev, "failed to enable proxy_clk\n");
>> +		goto err_proxy_clk;
>>   	}
>>   
>> -	ret = clk_prepare_enable(qproc->ahb_clk);
>> -	if (ret)
>> -		goto assert_reset;
>> -
>> -	ret = clk_prepare_enable(qproc->axi_clk);
>> -	if (ret)
>> -		goto disable_ahb_clk;
>> +	ret = q6_active_clk_enable(qproc);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "failed to enable active clocks\n");
>> +		goto err_active_clks;
>> +	}
>>   
>> -	ret = clk_prepare_enable(qproc->rom_clk);
>> -	if (ret)
>> -		goto disable_axi_clk;
>> +	if (!strcmp(qproc->q6_rproc_res->q6_version, "v56"))
>> +		pil_mss_restart_reg(qproc, 0);
>> +	else {
>> +		ret = reset_control_deassert(qproc->mss_restart);
>> +		if (ret) {
>> +			dev_err(qproc->dev, "failed to deassert mss restart\n");
>> +			goto err_deassert;
>> +		}
>> +	}
>>   
>> -	writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
>> +	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
> There's no functional reason for changing writel to writel_relaxed, so
> please do this in a separate commit and motivate it well.
OK, Done.
>
>>   
>>   	ret = q6v5proc_reset(qproc);
>>   	if (ret)
>> @@ -539,13 +720,11 @@ static int q6v5_start(struct rproc *rproc)
>>   	}
>>   
>>   	dev_info(qproc->dev, "MBA booted, loading mpss\n");
>> -
>> -	ret = q6v5_mpss_load(qproc);
>> +	ret = q6_mpss_load(qproc);
>>   	if (ret)
>>   		goto halt_axi_ports;
>> -
>>   	ret = wait_for_completion_timeout(&qproc->start_done,
>> -					  msecs_to_jiffies(5000));
>> +					  msecs_to_jiffies(10000));
> Please put this in a separate commit and describe why 10 seconds is
> better than 5.
It was an experimental change made during validation, found its way in 
patch. have reverted it back.
>
>>   	if (ret == 0) {
>>   		dev_err(qproc->dev, "start timed out\n");
>>   		ret = -ETIMEDOUT;
>> @@ -553,36 +732,33 @@ static int q6v5_start(struct rproc *rproc)
>>   	}
>>   
>>   	qproc->running = true;
>> -
>>   	/* TODO: All done, release the handover resources */
>> -
>> +	q6_proxy_clk_disable(qproc);
>> +	q6_proxy_regulator_disable(qproc);
> This is good, please drop the TODO comment now that we're done.
Ok, Done.
>
>> +	mutex_unlock(&qproc->q6_lock);
>>   	return 0;
>>   
>>   halt_axi_ports:
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>> -
>> -	clk_disable_unprepare(qproc->rom_clk);
>> -disable_axi_clk:
>> -	clk_disable_unprepare(qproc->axi_clk);
>> -disable_ahb_clk:
>> -	clk_disable_unprepare(qproc->ahb_clk);
>> -assert_reset:
>> -	reset_control_assert(qproc->mss_restart);
> Don't we need to assert the reset again?
Yes needed, have corrected.
>
>> -disable_vdd:
>> -	q6v5_regulator_disable(qproc);
>> -
>> +err_deassert:
>> +	q6_active_clk_disable(qproc);
>> +err_active_clks:
>> +	q6_proxy_clk_disable(qproc);
>> +err_proxy_clk:
> It's better if the labels describe the action than the source of the
> jump, so please keep the "disable_vdd" label for this - it also makes
> your patch cleaner.
OK,  Done.
>
>> +	q6_regulator_disable(qproc);
>> +	mutex_unlock(&qproc->q6_lock);
>>   	return ret;
>>   }
>>   
>> -static int q6v5_stop(struct rproc *rproc)
>> +static int q6_stop(struct rproc *rproc)
>>   {
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>> +	u64 val;
>>   
>> -	qproc->running = false;
>> -
>> +	mutex_lock(&qproc->q6_lock);
>>   	qcom_smem_state_update_bits(qproc->state,
>>   				    BIT(qproc->stop_bit), BIT(qproc->stop_bit));
>>   
>> @@ -597,16 +773,30 @@ static int q6v5_stop(struct rproc *rproc)
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>>   
>> -	reset_control_assert(qproc->mss_restart);
>> -	clk_disable_unprepare(qproc->rom_clk);
>> -	clk_disable_unprepare(qproc->axi_clk);
>> -	clk_disable_unprepare(qproc->ahb_clk);
>> -	q6v5_regulator_disable(qproc);
>> -
>> +	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

>
>> +		/*
>> +		 * 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
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?
>
>> +	} else
>> +		reset_control_assert(qproc->mss_restart);
>> +	q6_active_clk_disable(qproc);
>> +	q6_proxy_clk_disable(qproc);
>> +	q6_proxy_regulator_disable(qproc);
>> +	q6_active_regulator_disable(qproc);
>> +	qproc->running = false;
>> +	mutex_unlock(&qproc->q6_lock);
>>   	return 0;
>>   }
>>   
> Regards,
> Bjorn

[toc] | [next] | [standalone]

#1525680

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2016-11-18 20:40 +0100
Message-ID<sEXaW-2W9-25@gated-at.bofh.it>
In reply to#1523579
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

[toc] | [prev] | [next] | [standalone]

#1527003

FromAvaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Date2016-11-21 20:30 +0100
Message-ID<sG2rU-5ky-23@gated-at.bofh.it>
In reply to#1525680

On 11/19/2016 1:00 AM, Bjorn Andersson wrote:
> 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.
OK
>
>>>>   	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
OK
>
>>>> +
>>>> +	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.
if regulator enabled failed, i am unrolling the programmed load and 
voltage setting.
but i will try to incorporate your suggestion.
>
> 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.
OK, will try to incorporate.
>
>>>> -	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.
OK, may i change enum strings as per HPG?

enum {
	Q6V5_5_0_0,--->8916
	Q6V5_5_1_1,---->8974
	Q5V56_1_5_0---->8996
};



>
>>>> +		/*
>>>> +		 * 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.
I was under impression that clock code will be up streamed by clock 
team, and they will go by downstream way.
i discussed again they said i can use GCC reset framework in upstream.
Will change patch accordingly.
>
>> 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.
Sure, will do.
>
> Regards,
> Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web