by Dwivedi, Avaneesh Kumar (avani)

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] remoteproc: qcom: Embed private data structure for hexagon dsp.

On 11/19/2016 12:27 AM, Bjorn Andersson wrote:
> On Wed 16 Nov 06:41 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> i have been a little delayed for posting replies to patch comments,
>> hopefully onward it should be quick and fast.
>>
> I would greatly appreciate if you allow for a discussion before posting
> new revisions of the patchset. I will respond to your comments here and
> ignore v4 for now.

Ok, i will resend v4 patches with modification after consensus.
>
>> On 11/8/2016 12:38 PM, Bjorn Andersson wrote:
>>> On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote:
> [..]
>>>> +char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk",
>>>> + "snoc_axi_clk", "mnoc_axi_clk"};
>>> All these needs to be static, but I would prefer if you just put the
>>> values directly into the resource structs below.
>> If i have to initialize directly into resource struct, i need to declare
>> individual elements as array of fixed size
>> but number of resource lets say number of proxy regulator not being same
>> for different q6 chips, made me to
>> work with double pointer which can be assigned with address of an array of
>> string pointer as per need when new version need to be supported.
>>
> Using a termination sentinel to indicate end of lists is quite common in
> the kernel, so you can do this:
>
> .active_clks = (char*[]){
> "iface",
> "bus",
> ...,
> NULL
> },
OK
>
>> Though i have made above elements as static in next patch.
>>>> +
>>>> +static const struct q6_rproc_res msm_8996_res = {
>>>> + .proxy_clks = proxy_8x96_clk_str,
>>>> + .proxy_clk_cnt = 3,
>>> It's common practice to use a NULL terminator in the definition list
>>> rather than keeping separate count of the number of items.
>>>
>>> While acquiring the resources you would have to "calculate" the number
>>> and store it in the q6v5 struct, but this would make turn this struct
>>> into something only used during probe() - which is nice.
>> Yes, have modified as per suggestion.
>>>> + .active_clks = active_8x96_clk_str,
>>>> + .active_clk_cnt = 6,
>>>> + .proxy_regs = proxy_8x96_reg_str,
>>>> + .active_regs = NULL,
>>>> + .proxy_reg_action = (int **)proxy_8x96_reg_action,
>>>> + .proxy_reg_load = (int *)proxy_8x96_reg_load,
>>>> + .active_reg_action = NULL,
>>>> + .active_reg_load = NULL,
>>>> + .proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage,
>>>> + .active_reg_voltage = NULL,
>>>> + .proxy_reg_cnt = 3,
>>>> + .active_reg_cnt = 0,
>>>> + .q6_reset_init = q6v56_init_reset,
>>>> + .q6_version = "v56",
>>> q6_version would be better to have as a enum.
>> have removed this entry.
>> each class of q6 lets say "v56" have again many version of hexagon with
>> minor differences wrt each other.
> Okay, I'm fine with us sticking to classes, but I would like for them to
> make sense - and be listed as an enum instead of a string, to simplify
> the code.
OK, i will use one compatible string for each msm platform with enum
denoting the class and version of hexagon chip. something like

.compatible = "qcom,q6v56-1-5-pil", for msm 8996?
and version will carry a unique enum value based on compatible.

>
>> for example msm8996 use "v56" 1.5.0 while MSM8952 uses 1.8.0, reset sequence
> In msm-3.18 8996 is listed to be v55.
The version and class that i am referring are strictly as per HPG.
for 8996, hexagon core for MSS is "v56" with version 1.5.0
>
>> and some other operation differ wrt to this version in terms of order of
>> register programming. so i have introduced one variable in q6v5 struct per
>> q6 chip supported, if this is defined then we can check and carry out
>> version specific instruction.
>> will this be OK?
>>
> Generally in the Linux kernel it's frowned upon to carry the version
> information and then do conditional operation on this.
OK
>
> It's preferred to carry explicit flags through the implementation, e.g.
> carrying "mba.mbn" vs "mba.b00" rather than switching based on "version"
> at the point of use of this data.
If i have one enum for each version (which will require one compatible
for each platform) then i can easily pass firmware binary name to probe.
> But I'm not sure if the other differences has reasonable names, e.g. how
> to we denote the differences in reset sequence?
i have one option that should initialize one function pointer for each
compatible to perform reset sequence, but this way there will be huge
duplicity of code as more than 50% code in reset sequence remain same,
else based on check something like below i can perform certain unique
register operations for a particular hexagon core.

if (drvdata->hexagon_flag & 0x2)
do this
else
do that

where hexagon_flag will be initialized with enum based on compatible.??
>
>>>> + .q6_mba_image = "mba.mbn",
>>>> +};
>>>> +
>>>> +char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"};
>>>> +char *active_8x16_reg_str[] = {"mss"};
>>>> +int proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} };
>>>> +int active_8x16_reg_action[1][2] = { {1, 1} };
>>>> +int proxy_8x16_reg_load[] = {100000, 0, 100000, 100000};
>>>> +int active_8x16_reg_load[] = {100000};
>>>> +int proxy_8x16_reg_min_voltage[] = {1050000, 0, 0};
>>>> +int active_8x16_reg_min_voltage[] = {1000000};
>>>> +char *proxy_8x16_clk_str[] = {"xo"};
>>>> +char *active_8x16_clk_str[] = {"iface", "bus", "mem"};
>>>> +
>>>> +static const struct q6_rproc_res msm_8916_res = {
>>>> + .proxy_clks = proxy_8x16_clk_str,
>>>> + .proxy_clk_cnt = 1,
>>>> + .active_clks = active_8x16_clk_str,
>>>> + .active_clk_cnt = 3,
>>>> + .proxy_regs = proxy_8x16_reg_str,
>>>> + .active_regs = active_8x16_reg_str,
>>>> + .proxy_reg_action = (int **)proxy_8x16_reg_action,
>>>> + .proxy_reg_load = (int *)proxy_8x16_reg_load,
>>>> + .active_reg_action = (int **)active_8x16_reg_action,
>>>> + .active_reg_load = (int *)active_8x16_reg_load,
>>>> + .proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage,
>>>> + .active_reg_voltage = active_8x16_reg_min_voltage,
>>>> + .proxy_reg_cnt = 3,
>>>> + .active_reg_cnt = 1,
>>>> + .q6_reset_init = q6v5_init_reset,
>>>> + .q6_version = "v5",
>>>> + .q6_mba_image = "mba.b00",
>>> q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00.
>> Again this is a case where compatible string can not help, we should have
>> single compatible string i.e. "q6v5" for msm8916 and msm8974 since both are
>> from same class of q6 chip with different version.
>> so if we cant initialize mdt name based on compatible string alone.
> msm-3.4, msm-3.10 and msm-3.18 states that they are not the same and as
> such I don't think we have a problem.
>
> The more I look at this, the more convinced I am that we got 8916 wrong,
> i.e. we specified the wrong class and it just happens to work.
>
>>>> +};
>>>> +
>>>> +static const struct of_device_id q6_of_match[] = {
>>>> + { .compatible = "qcom,q6v5-pil", .data = &msm_8916_res},
>>> As I was looking in the CAF tree, I believe the correct version for 8916
>>> is 5.5 and v5 was used in 8974.
>>>
>>> But I presume these versions are not strictly tied to certain platforms,
>>> so please name the resource structs based on the q6v5 version, rather
>>> than platforms.
>> Yes, resource are tied to compatible and version of q6.
>> have used compatible to initialize major resources but using DT specified
>> version string for minor deviations where needed.
>> Should this be fine?
>> as far as version on 8916 and 8974 are concern they are as below.
>>
>> msm8974 q6v5 core version 5.0.0
>> msm8916 q6v5 core version 5.1.1
> If there are differences within a class then that just forces us to use
> the version number. There's very little overhead in carrying one
> compatible per platform, if that's what we need.

Yes i will use one compatible per msm platform.
>
>>>> + { .compatible = "qcom,q6v56-pil", .data = &msm_8996_res},
>>>> { },
>>>> };
> Regards,
> Bjorn

2016-11-21 19:29:21

by Dwivedi, Avaneesh Kumar (avani)

[permalink] [raw]

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

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