Re: [PATCH v2 10/12] pinctrl: samsung: add exynosautov920 pinctrl

[Jaewon Kim <jaewon02.kim@xxxxxxxxxxx>]

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