Re: [PATCH v2 10/12] pinctrl: samsung: add exynosautov920 pinctrl
From: Jaewon Kim
Date: Fri Nov 17 2023 - 02:39:36 EST
On 23. 11. 16. 20:21, Krzysztof Kozlowski wrote:
> On 16/11/2023 06:39, Jaewon Kim wrote:
>> On 23. 11. 15. 21:28, Krzysztof Kozlowski wrote:
>>
>>> On 15/11/2023 10:56, Jaewon Kim wrote:
>>>> ExynosAutov920 GPIO has a different register structure.
>>>> In the existing Exynos series, EINT control register enumerated after
>>>> a specific offset (e.g EXYNOS_GPIO_ECON_OFFSET).
>>>> However, in ExynosAutov920 SoC, the register that controls EINT belongs
>>>> to each GPIO group, and each GPIO group has 0x1000 align.
>>>>
>>>> This is a structure to protect the GPIO group with S2MPU in VM environment,
>>>> and will only be applied in ExynosAuto series SoCs.
>>>>
>>>> Example)
>>>> -------------------------------------------------
>>>> | original | ExynosAutov920 |
>>>> |-----------------------------------------------|
>>>> | 0x0 GPIO_CON | 0x0 GPIO_CON |
>>>> | 0x4 GPIO_DAT | 0x4 GPIO_DAT |
>>>> | 0x8 GPIO_PUD | 0x8 GPIO_PUD |
>>>> | 0xc GPIO_DRV | 0xc GPIO_DRV |
>>>> | 0x700 EINT_CON | 0x18 EINT_CON |
>>>> | 0x800 EINT_FLTCON | 0x1c EINT_FLTCON0 |
>>>> | 0x900 EINT_MASK | 0x20 EINT_FLTCON1 |
>>>> | 0xa00 EINT_PEND | 0x24 EINT_MASK |
>>>> | | 0x28 EINT_PEND |
>>>> -------------------------------------------------
>>>>
>>>> Pinctrl data for ExynosAutoV920 SoC.
>>>> - GPA0,GPA1 (10): External wake up interrupt
>>>> - GPQ0 (2): SPMI (PMIC I/F)
>>>> - GPB0,GPB1,GPB2,GPB3,GPB4,GPB5,GPB6 (47): I2S Audio
>>>> - GPH0,GPH1,GPH2,GPH3,GPH4,GPH5,GPH6,GPH8 (49): PCIE, UFS, Ethernet
>>>> - GPG0,GPG1,GPG2,GPG3,GPG4,GPG5 (29): General purpose
>>>> - GPP0,GPP1,GPP2,GPP3,GPP4,GPP5,GPP6,GPP7,GPP8,GPP9,GPP10 (77): USI
>>>>
>>>> Signed-off-by: Jaewon Kim<jaewon02.kim@xxxxxxxxxxx>
>>>> ---
>>>> .../pinctrl/samsung/pinctrl-exynos-arm64.c | 140 ++++++++++++++++++
>>>> drivers/pinctrl/samsung/pinctrl-exynos.c | 102 ++++++++++++-
>>>> drivers/pinctrl/samsung/pinctrl-exynos.h | 27 ++++
>>>> drivers/pinctrl/samsung/pinctrl-samsung.c | 5 +
>>>> drivers/pinctrl/samsung/pinctrl-samsung.h | 13 ++
>>>> 5 files changed, 280 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
>>>> index cb965cf93705..cf86722a70a3 100644
>>>> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
>>>> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
>>>> @@ -796,3 +796,143 @@ const struct samsung_pinctrl_of_match_data fsd_of_data __initconst = {
>>>> .ctrl = fsd_pin_ctrl,
>>>> .num_ctrl = ARRAY_SIZE(fsd_pin_ctrl),
>>>> };
>>>> +
>>>> +/* pin banks of exynosautov920 pin-controller 0 (ALIVE) */
>>>> +static struct samsung_pin_bank_data exynosautov920_pin_banks0[] = {
>>> So you created patch from some downstream code? No, please work on
>>> upstream. Take upstream code and customize it to your needs. That way
>>> you won't introduce same mistakes fixes years ago.
>>>
>>> Missing const.
>> Thanks for the guide.
>>
>> I didn`t work on downstream source, but when I copy/paste
>>
>> the struct enumerations from downstream, it seemed like
> That's what I am talking about. Don't do like this.
>
> We fixed several things in Linux kernel, so copying unfixed code is
> wasting of everyone's time. Don't work on downstream. Don't copy
> anything from downstream. You *MUST CUSTOMIZE* upstream file, not
> downstream.
Got it. I will not copy from downstream code.
>
>
>> 'const' was missing.
>>
>>> ...
>>>
>>>> @@ -31,6 +31,7 @@
>>>> #define EXYNOS7_WKUP_EMASK_OFFSET 0x900
>>>> #define EXYNOS7_WKUP_EPEND_OFFSET 0xA00
>>>> #define EXYNOS_SVC_OFFSET 0xB08
>>>> +#define EXYNOSAUTOV920_SVC_OFFSET 0xF008
>>>>
>>> ...
>>>
>>>> #ifdef CONFIG_PINCTRL_S3C64XX
>>>> { .compatible = "samsung,s3c64xx-pinctrl",
>>>> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
>>>> index 9b3db50adef3..cbb78178651b 100644
>>>> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
>>>> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
>>>> @@ -122,6 +122,9 @@ struct samsung_pin_bank_type {
>>>> * @eint_type: type of the external interrupt supported by the bank.
>>>> * @eint_mask: bit mask of pins which support EINT function.
>>>> * @eint_offset: SoC-specific EINT register or interrupt offset of bank.
>>>> + * @mask_offset: SoC-specific EINT mask register offset of bank.
>>>> + * @pend_offset: SoC-specific EINT pend register offset of bank.
>>>> + * @combine: EINT register is adjacent to the GPIO control register.
>>> I don't understand it. Adjacent? Are you sure? GPIO control register has
>>> 0xF004 (EXYNOSAUTOV920_SVC_OFFSET + 0x4)? Anyway, this does not scale.
>>> What if next revision comes with not-adjacent. There will be
>>> "combine_plus"? Also name confuses me - combine means together.
>>>
>>> Also your first map of registers does not have it adjacent...
>> I think I should have added a little more information about new struct.
>>
>> -------------------------------------------------
>> | original | ExynosAutov920 |
>> |-----------------------------------------------|
>> | 0x0 GPA_CON | 0x0 GPA_CON |
>> | 0x4 GPA_DAT | 0x4 GPA_DAT |
>> | 0x8 GPA_PUD | 0x8 GPA_PUD |
>> | 0xc GPA_DRV | 0xc GPA_DRV |
>> |----------------------| 0x18 EINT_GPA_CON |
>> | 0x20 GPB_CON | 0x1c EINT_GPA_FLTCON0|
>> | 0x4 GPB_DAT | 0x20 EINT_GPA_FLTCON1|
>> | 0x28 GPB_PUD | 0x24 EINT_GPA_MASK |
>> | 0x2c GPB_DRV | 0x28 EINT_GPA_PEND |
>> |----------------------|------------------------|
>> | 0x700 EINT_GPA_CON | 0x1000 GPA_CON |
>> | 0x704 EINT_GPB_CON | 0x1004 GPA_DAT |
>> |----------------------| 0x1008 GPA_PUD |
>> | 0x800 EINT_GPA_FLTCON| 0x100c GPA_DRV |
>> | 0x804 EINT_GPB_FLTCON| 0x1018 EINT_GPA_CON |
>> |----------------------| 0x101c EINT_GPA_FLTCON0|
>> | 0x900 EINT_GPA_MASK | 0x1020 EINT_GPA_FLTCON1|
>> | 0x904 EINT_GPB_MASK | 0x1024 EINT_GPA_MASK |
>> |----------------------| 0x1028 EINT_GPA_PEND |
>> | 0xa00 EINT_GPA_PEND |------------------------|
>> | 0xa04 EINT_GPB_PEND | |
>> ------------------------------------------------|
>> | 0xb08 SVC | 0xf008 SVC |
>> -------------------------------------------------
>>
>> The reason why I chose variable name 'combine' is that EINT registers was
>> separated from gpio control address. However, in exynosautov920 EINT
>> registers combined with GPx group. So I chose "combine" word.
> What does it mean "the GPx group"? Combined means the same place, the
> same register. I could imagine offset is 0x4, what I wrote last time.
>
> Is the offset 0x4?
>
>
>> Is another reasonable word, I will change it.
>
> Why you cannot store the offset?
>
>> EINT registers related to the entire group(e.g SVC) were at the end of
>> the GPIO block and are now moved to 0xf000.
> So not in the same register, not combined?
>
Okay,
Instead of the word combine, I will think of a better word in next version.
Thanks
Jaewon Kim