linux-kernel - Re: [PATCH v2 3/7] pinctrl: samsung: Add the support the multiple IORESOURCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-id: <57CD27FC.2020201@samsung.com>
Date:   Mon, 05 Sep 2016 17:08:28 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Tomasz Figa <tomasz.figa@...il.com>
Cc:     Krzysztof Kozłowski <k.kozlowski@...sung.com>,
        Kukjin Kim <kgene@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Jaehoon Chung <jh80.chung@...sung.com>,
        "sw0312.kim" <sw0312.kim@...sung.com>,
        Joonyoung Shim <jy0922.shim@...sung.com>,
        InKi Dae <inki.dae@...sung.com>,
        Jonghwa Lee <jonghwa3.lee@...sung.com>,
        Beomho Seo <beomho.seo@...sung.com>, jaewon02.kim@...sung.com,
        human.hwang@...sung.com, Inha Song <ideal.song@...sung.com>,
        ingi2.kim@...sung.com, Marek Szyprowski <m.szyprowski@...sung.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Sylwester Nawrocki <s.nawrocki@...sung.com>,
        chanwoo@...nel.org, Linus Walleij <linus.walleij@...aro.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v2 3/7] pinctrl: samsung: Add the support the multiple
 IORESOURCE_MEM for one pin-bank

Hi Tomasz,

I'm sorry for late reply.

On 2016년 08월 25일 23:41, Tomasz Figa wrote:
> 2016-08-25 23:30 GMT+09:00 Tomasz Figa <tomasz.figa@...il.com>:
>>> +       }
>>> +
>>> +#define EXYNOS_PIN_BANK_EINTN_EXT(pins, reg, id, pctl_idx, eint_idx)   \
>>> +       {                                               \
>>> +               .type           = &bank_type_off,       \
>>> +               .pctl_offset    = reg,                  \
>>> +               .nr_pins        = pins,                 \
>>> +               .eint_type      = EINT_TYPE_NONE,       \
>>> +               .name           = id,                   \
>>> +               .pctl_res_idx   = pctl_idx,             \
>>> +               .eint_res_idx   = eint_dix              \
>>> +       }
>>
>> Your patch 4/7 doesn't seem to use this one, so this is dead code for
>> the time being. Please add when there is real need for it.
>>
>> Also it doesn't really make much sense to have index for both pctl and
>> eint. Please define first entry of regs property as always pointing to
>> pctl registers and by also eint registers for usual controllers. Then
>> second regs entry would be eint registers for controllers with
>> separate register blocks. Then there is only a need to have
>> eint_res_idx in the driver and no need for pctl_res_idx, because it
>> would be always 0.
> 
> Ah, sorry, I got confused again by which registers are where in these
> GPF banks. Let's make it the other way around and make DT contain eint
> registers in first regs entry and hardcode eint_res_idx to 0 for the
> time being.

I got with slight confusion.
Do you mean that you want to remove the 'eint_res_idx' because
it is always zero(0) as your comment. And do you agree to add 'pctl_res_idx'?

Also, as you commented, the eint_res_idx for both GPA and GPFx is zero(0).

Example:
	pinctrl_alive: pinctrl@...80000 {
		compatible = "samsung,exynos5433-pinctrl";
                      /* ALIVE domain    ,  IMEM domain  */
		reg = <0x10580000 0x1a20>, <0x11090000 0x100>;

		wakeup-interrupt-controller {
			compatible = "samsung,exynos7-wakeup-eint";
			interrupts = <GIC_SPI 16 0>;
		};
	};

	GPA's eint_res_idx is 0
	GPA's pctl_res_idx is 0

	GPFx's eint_res_idx is 0
	GPFx's pctl_res_idx is 1


 However it should be still beneficial to refactor the code
> and calculate per-bank eint_base to avoid adding the offset every
> time, similarly to pctl_base/offset, from my suggestion below.

I agree. I'll modify it according to your comment.

> 
>>> @@ -345,7 +346,8 @@ static void pin_to_reg_bank(struct samsung_pinctrl_drv_data *drvdata,
>>>                         ((b->pin_base + b->nr_pins - 1) < pin))
>>>                 b++;
>>>
>>> -       *reg = drvdata->virt_base + b->pctl_offset;
>>> +       pctl_res_idx = b->pctl_res_idx;
>>> +       *reg = drvdata->virt_base[pctl_res_idx] + b->pctl_offset;
>>
>> I suggested something slightly different. Instead of
>> bank::pctl_res_idx, I proposed bank::pctl_base.
>> bank_info::pctl_res_idx would be specified only in init driver data
>> and bank::pctl_base would be calculated at probe time as
>> drvdata->virt_base[bank_info->pctl_res_idx] + bank_info->pctl_offset.
>> This would eliminate the need to do any indexing and adding further in
>> the code and make things simpler.
>>
>> Taking my other comments above, pctl part would be unchanged and only
>> eint addresses and offsets would be affected.
> 
> Ah, scratch this one sentence. I got confused with the register layout
> again, sorry. Please refactor both eint and pctl as I suggested in the
> upper paragraph.
> 
> Best regards,
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

-- 
Best Regards,
Chanwoo Choi