Re: [PATCH 4/7] pinctrl: samsung: Add GPFx support of Exynos5433

[Tomasz Figa]

Hi Chanwoo,

2016-08-19 18:07 GMT+09:00 Chanwoo Choi <cw00.choi@xxxxxxxxxxx>:
> Hi Tomasz Figa,
>
> Due to wrong setting of email client,
> your reply is deleted on my email client at the company.

I used Gmail (in plain text mode) to reply, was that related?

> I'm so sorry. So, I add your comment on below and then
> I reply the detailed description.

No problem. Thanks for description.

>
> On 2016ë 08ì 16ì 15:27, Chanwoo Choi wrote:
>> From: Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>
>>
>> This patch add the support of GPFx pin of Exynos5433 SoC. Exynos5433 has
>> different memory map of GPFx from previous Exynos SoC. Exynos GPIO has
>> following register to control gpio funciton. Usually, all registers of GPIO
>> are included in same domain.
>> - CON / DAT / PUD / DRV / CONPDN / PUDPDN
>> - EINT_CON/ EINT_FLTCON0, EINT_FLTCON1 / EINT_MASK / EINT_PEND
>>
>> But, GPFx are included in two domain as following. So, this patch supports
>> the GPFx pin which handle the on separate two domains.
>> - ALIVE domain : CON / DAT / PUD / DRV / CONPDN / PUDPDN
>> - IMEM domain  : EINT_CON/ EINT_FLTCON0, EINT_FLTCON1 / EINT_MASK / EINT_PEND
>
> ---------
> I'm afraid I don't get anything from the description above. Could you
> describe the layout of registers in memory map and IRQ routing of the
> pins?
>
> Best regards,
> Tomasz
> ----------
>
> On this patch, I'm sorry that I described the wrong information about GFP1~5.
> I explained the memory map of GPF[1-5] the oppositely. Following compositions
> are correct.
> - ALIVE : WEINT_FLTCONx, WEINT_MASK, WEING_PEND
> - IMEM : CON, DAT, PUD, DRV, CONPDN, PUDPDN
> And, I add the memory map for GPF[1~5][2] and the wakeup interrupt information[1].
>
> [1] Memory map for GPF1~5
> [ALIVE]
> WEINT_GPA0_CON         : 0x1058_0000 (ALIVE) + (0x0700 = 0x0700 + 0x0)
> WEINT_GPA1_CON         : 0x1058_0000 (ALIVE) + (0x0704 = 0x0700 + 0x4)
> WEINT_GPA2_CON         : 0x1058_0000 (ALIVE) + (0x0708 = 0x0700 + 0x8)
> WEINT_GPA3_CON         : 0x1058_0000 (ALIVE) + (0x070C = 0x0700 + 0xC)
>
> WEINT_GPF1_CON         : 0x1058_0000 (ALIVE) + (0x1704 = 0x0700 + 0x1004)
> WEINT_GPF2_CON         : 0x1058_0000 (ALIVE) + (0x1708 = 0x0700 + 0x1008)
> WEINT_GPF3_CON         : 0x1058_0000 (ALIVE) + (0x170C = 0x0700 + 0x100C)
> WEINT_GPF4_CON         : 0x1058_0000 (ALIVE) + (0x1710 = 0x0700 + 0x10010)
> WEINT_GPF5_CON         : 0x1058_0000 (ALIVE) + (0x1714 = 0x0700 + 0x1014)
>
> WEINT_GPF[x]_MASK     : 0x1058_0000 (ALIVE) + (0x1900 + (x * 4))
> WEINT_GPF[x]_PEND     : 0x1058_0000 (ALIVE) + (0x1A00 + (x * 4))
> (x : 1 ~ 5)
>
> [IMEM]
> GPF1_CON          : 0X1109_0000 (IMEM) + 0x0020
> GPF1_DAT           : 0X1109_0000 (IMEM) + 0x0024
> GPF1_PUD          : 0X1109_0000 (IMEM) + 0x0028
> GPF1_DRV          : 0X1109_0000 (IMEM) + 0x002C
> GPF1_CONPDN  : 0X1109_0000 (IMEM) + 0x0030
> GPF1_PUDPDN  : 0X1109_0000 (IMEM) + 0x0034
>
> GPF2_CON         : 0X1109_0000 (IMEM) + 0x0040
> ...
> GPF3_CON         : 0X1109_0000 (IMEM) + 0x0060
> ...
> GPF4_CON         : 0X1109_0000 (IMEM) + 0x0080
> ...
> GPF5_CON         : 0X1109_0000 (IMEM) + 0x00A0
>
> [2] Interrput pin information
> - the total number of wakeup external IRQ is 64.
> ----------------------------------------------------------------------------------
> domain| gpio : nr  | wakeup interrput name   | SPI number       |
> -----------------------------------------------------------------------------------
> ALIVE | GPA0 : 8 | INTREQ__EINT[0~7]     | SPI[0] ~ SPI[7]   |
> ALIVE | GPA1 : 8 | INTREQ__EINT[8~15]   | SPI[8] ~ SPI[15] |
> ALIVE | GPA2 : 8 | INTREQ__EINT_16_63 | SPI[16]               |
> ALIVE | GPA3 : 8 | INTREQ__EINT_16_63 | SPI[16]               |
> -----------------------------------------------------------------------------------
> ALIVE | GPF1 : 8 | INTREQ__EINT_16_63 | SPI[16]               |
> ALIVE | GPF2 : 4 | INTREQ__EINT_16_63 | SPI[16]               |
> ALIVE | GPF3 : 4 | INTREQ__EINT_16_63 | SPI[16]               |
> ALIVE | GPF4 : 8 | INTREQ__EINT_16_63 | SPI[16]               |
> ALIVE | GPF5 : 8 | INTREQ__EINT_16_63 | SPI[16]               |
> -----------------------------------------------------------------------------------
>
> In summary,
> If gpf[1-5] handle the CON/DAT/PUD/DRV register,
> the driver will use the drvdata->ext_base (IMEM base address)
> instead of drvdata->virt_base(ALIVE base address)
> because the CON/DAT/PUD/DRV register of gpf[1-5] are included
> in the IMEM domain.
>
> If gpf[1-5] handle the WEINT_* register,
> the driver will use the drvdata->virt_base(ALIVE base address)
> because the WEINT_* registers of gpf[1-5] are included in the ALIVE domain.

Okay, so Krzysztof's suggestion doesn't apply because it's not the
eint base which is displaced, but the pinctrl base. I'd suggest the
following solution then:

- make samsung_pinctrl_drv_data::virt_base an array and save there all
mapped iomem resources,

- add unsigned pctl_base_res_idx to samsung_pin_bank_data that would
be the index of iomem resource into which the
samsung_pin_bank_data::pctl_offset is an offfset, Existing
EXYNOS_PIN_BANK* macros don't need to be changed, because the field
would be 0 by default. Then only the new bank type macro would have
another argument that would be the resource index,

- replace samsung_pin_bank::pctl_offset with void __iomem *pctl_base
and precalculate the addresses at probe time for each bank (pctl_base
= virt_base[pctl_base_res_idx] + samsung_pin_bank_data::pctl_offset).
Since currently there is only a problem with pctl_base and eint_base
seems to be only one, EINT code can simply use virt_base[0] all the
time for now.

Also you should document the second regs entry in the DT binding.

What do you think?

Best regards,
Tomasz