Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932277AbcKRTap (ORCPT ); Fri, 18 Nov 2016 14:30:45 -0500 Received: from mail-pf0-f182.google.com ([209.85.192.182]:32857 "EHLO mail-pf0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932178AbcKRTan (ORCPT ); Fri, 18 Nov 2016 14:30:43 -0500 Date: Fri, 18 Nov 2016 11:30:39 -0800 From: Bjorn Andersson To: Avaneesh Kumar Dwivedi Cc: linux-kernel@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-arm-msm@vger.kernel.org Subject: Re: [PATCH v3 2/3] remoteproc: qcom: Hexagon resource handling Message-ID: <20161118193039.GL28340@tuxbot> References: <1478522240-11036-1-git-send-email-akdwived@codeaurora.org> <1478522240-11036-3-git-send-email-akdwived@codeaurora.org> <20161108064907.GJ25787@tuxbot> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8920 Lines: 248 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