diff mbox series

[v2,10/12] pinctrl: samsung: add exynosautov920 pinctrl

Message ID 20231115095609.39883-11-jaewon02.kim@samsung.com (mailing list archive)
State New, archived
Headers show
Series Introduce ExynosAutov920 SoC and SADK board | expand

Commit Message

Jaewon Kim Nov. 15, 2023, 9:56 a.m. UTC
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@samsung.com>
---
 .../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(-)

Comments

Krzysztof Kozlowski Nov. 15, 2023, 12:28 p.m. UTC | #1
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@samsung.com>
> ---
>  .../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.

...

> @@ -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...

Anyway first patch is to rework driver to support new register layout.
Second patch is to add new variant.

Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 15, 2023, 12:42 p.m. UTC | #2
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.

Checkpatch points some warnings:

CHECK: Alignment should match open parenthesis
CHECK: Lines should not end with a '('
CHECK: Macro argument reuse 'reg' - possible side-effects?

Best regards,
Krzysztof
Jaewon Kim Nov. 16, 2023, 3:50 a.m. UTC | #3
On 23. 11. 15. 21:42, 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.
> Checkpatch points some warnings:
>
> CHECK: Alignment should match open parenthesis
> CHECK: Lines should not end with a '('
> CHECK: Macro argument reuse 'reg' - possible side-effects?

I don`t know this happens.

When I did the checkpatch, there were no problems as shown below.

---

./scripts/checkpatch.pl 
0010-pinctrl-samsung-add-exynosautov920-pinctrl.patch
total: 0 errors, 0 warnings, 416 lines checked

0010-pinctrl-samsung-add-exynosautov920-pinctrl.patch has no obvious 
style problems and is ready for submission.

---

>
> Best regards,
> Krzysztof
>
>

Thanks

Jaewon Kim
Jaewon Kim Nov. 16, 2023, 7:18 a.m. UTC | #4
Hi all,


I sent it again because the mail format was broken due to the HTML part.


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@samsung.com>
>> ---
>>   .../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.

I did not work on downstream source.

Kernel version is different, and there are numerous SoCs, So bringing 
the patch

form the downstream make conflicts. Don`t worry about it.

Only GPIO struct was copied in the downstream code and the 'const' was 
missing.

Anyway, I will add const in next version.


>
> ...
>
>> @@ -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 more information about new architect.

-------------------------------------------------
| 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|
| 0x24  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 GPB_CON	 |
| 0x704 EINT_GPB_CON	| 0x1004 GPB_DAT	 |
|-----------------------| 0x1008 GPB_PUD	 |
| 0x800 EINT_GPA_FLTCON	| 0x100c GPB_DRV	 |
| 0x804 EINT_GPB_FLTCON	| 0x1018 EINT_GPB_CON	 |
|-----------------------| 0x101c EINT_GPB_FLTCON0|
| 0x900 EINT_GPA_MASK	| 0x1020 EINT_GPB_FLTCON1|
| 0x904 EINT_GPB_MASK	| 0x1024 EINT_GPB_MASK	 |
|-----------------------| 0x1028 EINT_GPB_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.

If there is more reasonable name, i will change it.

And I will also change the description of the variable.


EINT registers related to the entire group(e.g SVC) were at the end of

the GPIO block 0xb00 and now it has been moved to 0xf000.


>
> Anyway first patch is to rework driver to support new register layout.
> Second patch is to add new variant.

Okay, I will divide the patch in the next version.

>
> Best regards,
> Krzysztof
>
>

Thanks

Jaewon Kim
Krzysztof Kozlowski Nov. 16, 2023, 11:17 a.m. UTC | #5
On 16/11/2023 04:50, Jaewon Kim wrote:
> 
> On 23. 11. 15. 21:42, 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.
>> Checkpatch points some warnings:
>>
>> CHECK: Alignment should match open parenthesis
>> CHECK: Lines should not end with a '('
>> CHECK: Macro argument reuse 'reg' - possible side-effects?
> 
> I don`t know this happens.
> 
> When I did the checkpatch, there were no problems as shown below.

Didn't you miss some arguments? Lime --strict?

Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 16, 2023, 11:21 a.m. UTC | #6
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@samsung.com>
>>> ---
>>>   .../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.


> 
> '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?

Best regards,
Krzysztof
Jaewon Kim Nov. 17, 2023, 7:36 a.m. UTC | #7
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@samsung.com>
>>>> ---
>>>>    .../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
Jaewon Kim Nov. 17, 2023, 8:07 a.m. UTC | #8
On 23. 11. 16. 20:17, Krzysztof Kozlowski wrote:
> On 16/11/2023 04:50, Jaewon Kim wrote:
>> On 23. 11. 15. 21:42, 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.
>>> Checkpatch points some warnings:
>>>
>>> CHECK: Alignment should match open parenthesis
>>> CHECK: Lines should not end with a '('
>>> CHECK: Macro argument reuse 'reg' - possible side-effects?
>> I don`t know this happens.
>>
>> When I did the checkpatch, there were no problems as shown below.
> Didn't you miss some arguments? Lime --strict?
>
 From next time, I will check with --strict option.

Thanks Krzysztof.


Thanks

Jaewon Kim
Krzysztof Kozlowski Nov. 17, 2023, 10:48 a.m. UTC | #9
On 17/11/2023 08:36, Jaewon Kim wrote:
>>> 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.

I want to know answer to:

"Why you cannot store the offset?"

Best regards,
Krzysztof
Jaewon Kim Nov. 18, 2023, 7:43 a.m. UTC | #10
On 23. 11. 17. 19:48, Krzysztof Kozlowski wrote:
> On 17/11/2023 08:36, Jaewon Kim wrote:
>>>> 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?

If you are asking about the offset of GPIO control register and EINT 
control register, 0x4 is correct.

There is no empty space between the two register.


0x0 CON

0x4 DAT

0x8 PUD

0xc DRV

0x10 CONPDN

0x14 PUDPDN

0x18 EINT_CON

0x1c EINT_FLTCON

0x20 or 0x24 EINT_MASK (The size of FLTCON register depending on the 
number of gpio)

0x24 or 0x28 EINT_PEND


>>>
>>>
>>>> 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.
> I want to know answer to:
>
> "Why you cannot store the offset?"
>
I did not understand exactly what you said, but if i guess,,

you want to get rid of the offs because the value of the offs is always 
the same?

#define EXYNOSV920_PIN_BANK_EINTG(pins, reg, id, offs, mask_offs, pend_offs)


Thanks

Jaewon Kim
Krzysztof Kozlowski Nov. 21, 2023, 1:51 p.m. UTC | #11
On 18/11/2023 08:43, Jaewon Kim wrote:
> 
> On 23. 11. 17. 19:48, Krzysztof Kozlowski wrote:
>> On 17/11/2023 08:36, Jaewon Kim wrote:
>>>>> 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?
> 
> If you are asking about the offset of GPIO control register and EINT 
> control register, 0x4 is correct.
> 
> There is no empty space between the two register.
> 
> 
> 0x0 CON
> 
> 0x4 DAT
> 
> 0x8 PUD
> 
> 0xc DRV
> 
> 0x10 CONPDN
> 
> 0x14 PUDPDN
> 
> 0x18 EINT_CON
> 
> 0x1c EINT_FLTCON
> 
> 0x20 or 0x24 EINT_MASK (The size of FLTCON register depending on the 
> number of gpio)
> 
> 0x24 or 0x28 EINT_PEND
> 
> 
>>>>
>>>>
>>>>> 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.
>> I want to know answer to:
>>
>> "Why you cannot store the offset?"
>>
> I did not understand exactly what you said, but if i guess,,
> 
> you want to get rid of the offs because the value of the offs is always 
> the same?
> 
> #define EXYNOSV920_PIN_BANK_EINTG(pins, reg, id, offs, mask_offs, pend_offs)

I meant that it looks possible to store the offset and use it directly,
instead of storing bool telling that offset is different.

Best regards,
Krzysztof
Jaewon Kim Nov. 23, 2023, 6:22 a.m. UTC | #12
On 23. 11. 21. 22:51, Krzysztof Kozlowski wrote:
> On 18/11/2023 08:43, Jaewon Kim wrote:
>> On 23. 11. 17. 19:48, Krzysztof Kozlowski wrote:
>>> On 17/11/2023 08:36, Jaewon Kim wrote:
>>>>>> 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?
>> If you are asking about the offset of GPIO control register and EINT
>> control register, 0x4 is correct.
>>
>> There is no empty space between the two register.
>>
>>
>> 0x0 CON
>>
>> 0x4 DAT
>>
>> 0x8 PUD
>>
>> 0xc DRV
>>
>> 0x10 CONPDN
>>
>> 0x14 PUDPDN
>>
>> 0x18 EINT_CON
>>
>> 0x1c EINT_FLTCON
>>
>> 0x20 or 0x24 EINT_MASK (The size of FLTCON register depending on the
>> number of gpio)
>>
>> 0x24 or 0x28 EINT_PEND
>>
>>
>>>>>
>>>>>> 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.
>>> I want to know answer to:
>>>
>>> "Why you cannot store the offset?"
>>>
>> I did not understand exactly what you said, but if i guess,,
>>
>> you want to get rid of the offs because the value of the offs is always
>> the same?
>>
>> #define EXYNOSV920_PIN_BANK_EINTG(pins, reg, id, offs, mask_offs, pend_offs)
> I meant that it looks possible to store the offset and use it directly,
> instead of storing bool telling that offset is different.

Thanks for your feedback. We can use offset instead of bool.

I will make v3 patch using new variable 'eint_con_offset' instead of 
'combine'.


Thanks

Jaewon Kim
diff mbox series

Patch

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[] = {
+	EXYNOSV920_PIN_BANK_EINTW(8, 0x0000, "gpa0", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTW(2, 0x1000, "gpa1", 0x18, 0x20, 0x24),
+	EXYNOS850_PIN_BANK_EINTN(2, 0x2000, "gpq0"),
+};
+
+/* pin banks of exynosautov920 pin-controller 1 (AUD) */
+static struct samsung_pin_bank_data exynosautov920_pin_banks1[] = {
+	EXYNOSV920_PIN_BANK_EINTG(7, 0x0000, "gpb0", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(6, 0x1000, "gpb1", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x2000, "gpb2", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x3000, "gpb3", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x4000, "gpb4", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(5, 0x5000, "gpb5", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(5, 0x6000, "gpb6", 0x18, 0x24, 0x28),
+};
+
+/* pin banks of exynosautov920 pin-controller 2 (HSI0) */
+static struct samsung_pin_bank_data exynosautov920_pin_banks2[] = {
+	EXYNOSV920_PIN_BANK_EINTG(6, 0x0000, "gph0", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(2, 0x1000, "gph1", 0x18, 0x20, 0x24),
+};
+
+/* pin banks of exynosautov920 pin-controller 3 (HSI1) */
+static struct samsung_pin_bank_data exynosautov920_pin_banks3[] = {
+	EXYNOSV920_PIN_BANK_EINTG(7, 0x000, "gph8", 0x18, 0x24, 0x28),
+};
+
+/* pin banks of exynosautov920 pin-controller 4 (HSI2) */
+static struct samsung_pin_bank_data exynosautov920_pin_banks4[] = {
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x0000, "gph3", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(7, 0x1000, "gph4", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x2000, "gph5", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(7, 0x3000, "gph6", 0x18, 0x24, 0x28),
+};
+
+/* pin banks of exynosautov920 pin-controller 5 (HSI2UFS) */
+static struct samsung_pin_bank_data exynosautov920_pin_banks5[] = {
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x000, "gph2", 0x18, 0x20, 0x24),
+};
+
+/* pin banks of exynosautov920 pin-controller 6 (PERIC0) */
+static struct samsung_pin_bank_data exynosautov920_pin_banks6[] = {
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x0000, "gpp0", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x1000, "gpp1", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x2000, "gpp2", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(5, 0x3000, "gpg0", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x4000, "gpp3", 0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x5000, "gpp4", 0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x6000, "gpg2", 0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x7000, "gpg5", 0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(3, 0x8000, "gpg3", 0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(5, 0x9000, "gpg4", 0x18, 0x24, 0x28),
+};
+
+/* pin banks of exynosautov920 pin-controller 7 (PERIC1) */
+static struct samsung_pin_bank_data exynosautov920_pin_banks7[] = {
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x0000, "gpp5",  0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(5, 0x1000, "gpp6",  0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x2000, "gpp10", 0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x3000, "gpp7",  0x18, 0x24, 0x28),
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x4000, "gpp8",  0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x5000, "gpp11", 0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x6000, "gpp9",  0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(4, 0x7000, "gpp12", 0x18, 0x20, 0x24),
+	EXYNOSV920_PIN_BANK_EINTG(8, 0x8000, "gpg1",  0x18, 0x24, 0x28),
+};
+
+static const struct samsung_retention_data exynosautov920_retention_data __initconst = {
+	.regs	 = NULL,
+	.nr_regs = 0,
+	.value	 = 0,
+	.refcnt	 = &exynos_shared_retention_refcnt,
+	.init	 = exynos_retention_init,
+};
+
+const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
+	{
+		/* pin-controller instance 0 ALIVE data */
+		.pin_banks	= exynosautov920_pin_banks0,
+		.nr_banks	= ARRAY_SIZE(exynosautov920_pin_banks0),
+		.eint_wkup_init	= exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynosautov920_retention_data,
+	}, {
+		/* pin-controller instance 1 AUD data */
+		.pin_banks	= exynosautov920_pin_banks1,
+		.nr_banks	= ARRAY_SIZE(exynosautov920_pin_banks1),
+	}, {
+		/* pin-controller instance 2 HSI0 data */
+		.pin_banks	= exynosautov920_pin_banks2,
+		.nr_banks	= ARRAY_SIZE(exynosautov920_pin_banks2),
+		.eint_gpio_init	= exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
+	}, {
+		/* pin-controller instance 2 HSI1 data */
+		.pin_banks	= exynosautov920_pin_banks3,
+		.nr_banks	= ARRAY_SIZE(exynosautov920_pin_banks3),
+		.eint_gpio_init	= exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
+	}, {
+		/* pin-controller instance 2 HSI2 data */
+		.pin_banks	= exynosautov920_pin_banks4,
+		.nr_banks	= ARRAY_SIZE(exynosautov920_pin_banks4),
+		.eint_gpio_init	= exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
+	}, {
+		/* pin-controller instance 2 HSI2UFS data */
+		.pin_banks	= exynosautov920_pin_banks5,
+		.nr_banks	= ARRAY_SIZE(exynosautov920_pin_banks5),
+		.eint_gpio_init	= exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
+	}, {
+		/* pin-controller instance 2 PERIC0 data */
+		.pin_banks	= exynosautov920_pin_banks6,
+		.nr_banks	= ARRAY_SIZE(exynosautov920_pin_banks6),
+		.eint_gpio_init	= exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
+	}, {
+		/* pin-controller instance 2 PERIC1 data */
+		.pin_banks	= exynosautov920_pin_banks7,
+		.nr_banks	= ARRAY_SIZE(exynosautov920_pin_banks7),
+		.eint_gpio_init	= exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
+	},
+};
+
+const struct samsung_pinctrl_of_match_data exynosautov920_of_data __initconst = {
+	.ctrl		= exynosautov920_pin_ctrl,
+	.num_ctrl	= ARRAY_SIZE(exynosautov920_pin_ctrl),
+};
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index 6b58ec84e34b..b1bf44ee09db 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -56,6 +56,9 @@  static void exynos_irq_mask(struct irq_data *irqd)
 	unsigned int mask;
 	unsigned long flags;
 
+	if (bank->combine)
+		reg_mask = bank->mask_offset;
+
 	raw_spin_lock_irqsave(&bank->slock, flags);
 
 	mask = readl(bank->eint_base + reg_mask);
@@ -72,6 +75,9 @@  static void exynos_irq_ack(struct irq_data *irqd)
 	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
 	unsigned long reg_pend = our_chip->eint_pend + bank->eint_offset;
 
+	if (bank->combine)
+		reg_pend = bank->pend_offset;
+
 	writel(1 << irqd->hwirq, bank->eint_base + reg_pend);
 }
 
@@ -95,6 +101,9 @@  static void exynos_irq_unmask(struct irq_data *irqd)
 	if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
 		exynos_irq_ack(irqd);
 
+	if (bank->combine)
+		reg_mask = bank->mask_offset;
+
 	raw_spin_lock_irqsave(&bank->slock, flags);
 
 	mask = readl(bank->eint_base + reg_mask);
@@ -221,6 +230,22 @@  static const struct exynos_irq_chip exynos_gpio_irq_chip __initconst = {
 	/* eint_wake_mask_value not used */
 };
 
+static const struct exynos_irq_chip exynosautov920_gpio_irq_chip __initconst = {
+	.chip = {
+		.name = "exynosautov920_gpio_irq_chip",
+		.irq_unmask = exynos_irq_unmask,
+		.irq_mask = exynos_irq_mask,
+		.irq_ack = exynos_irq_ack,
+		.irq_set_type = exynos_irq_set_type,
+		.irq_request_resources = exynos_irq_request_resources,
+		.irq_release_resources = exynos_irq_release_resources,
+	},
+	.eint_con = 0,
+	.eint_mask = 0,
+	.eint_pend = 0,
+	/* eint_wake_mask_value not used */
+};
+
 static int exynos_eint_irq_map(struct irq_domain *h, unsigned int virq,
 					irq_hw_number_t hw)
 {
@@ -247,7 +272,10 @@  static irqreturn_t exynos_eint_gpio_irq(int irq, void *data)
 	unsigned int svc, group, pin;
 	int ret;
 
-	svc = readl(bank->eint_base + EXYNOS_SVC_OFFSET);
+	if (bank->combine)
+		svc = readl(bank->eint_base + EXYNOSAUTOV920_SVC_OFFSET);
+	else
+		svc = readl(bank->eint_base + EXYNOS_SVC_OFFSET);
 	group = EXYNOS_SVC_GROUP(svc);
 	pin = svc & EXYNOS_SVC_NUM_MASK;
 
@@ -297,8 +325,12 @@  __init int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 		if (bank->eint_type != EINT_TYPE_GPIO)
 			continue;
 
-		bank->irq_chip = devm_kmemdup(dev, &exynos_gpio_irq_chip,
-					   sizeof(*bank->irq_chip), GFP_KERNEL);
+		if (bank->combine)
+			bank->irq_chip = devm_kmemdup(dev, &exynosautov920_gpio_irq_chip,
+						   sizeof(*bank->irq_chip), GFP_KERNEL);
+		else
+			bank->irq_chip = devm_kmemdup(dev, &exynos_gpio_irq_chip,
+						   sizeof(*bank->irq_chip), GFP_KERNEL);
 		if (!bank->irq_chip) {
 			ret = -ENOMEM;
 			goto err_domains;
@@ -456,6 +488,22 @@  static const struct exynos_irq_chip exynos7_wkup_irq_chip __initconst = {
 	.set_eint_wakeup_mask = exynos_pinctrl_set_eint_wakeup_mask,
 };
 
+static const struct exynos_irq_chip exynosautov920_wkup_irq_chip __initconst = {
+	.chip = {
+		.name = "exynosautov920_wkup_irq_chip",
+		.irq_unmask = exynos_irq_unmask,
+		.irq_mask = exynos_irq_mask,
+		.irq_ack = exynos_irq_ack,
+		.irq_set_type = exynos_irq_set_type,
+		.irq_set_wake = exynos_wkup_irq_set_wake,
+		.irq_request_resources = exynos_irq_request_resources,
+		.irq_release_resources = exynos_irq_release_resources,
+	},
+	.eint_wake_mask_value = &eint_wake_mask_value,
+	.eint_wake_mask_reg = EXYNOS5433_EINT_WAKEUP_MASK,
+	.set_eint_wakeup_mask = exynos_pinctrl_set_eint_wakeup_mask,
+};
+
 /* list of external wakeup controllers supported */
 static const struct of_device_id exynos_wkup_irq_ids[] = {
 	{ .compatible = "samsung,s5pv210-wakeup-eint",
@@ -468,6 +516,8 @@  static const struct of_device_id exynos_wkup_irq_ids[] = {
 			.data = &exynos7_wkup_irq_chip },
 	{ .compatible = "samsung,exynosautov9-wakeup-eint",
 			.data = &exynos7_wkup_irq_chip },
+	{ .compatible = "samsung,exynosautov920-wakeup-eint",
+			.data = &exynosautov920_wkup_irq_chip },
 	{ }
 };
 
@@ -655,6 +705,20 @@  static void exynos_pinctrl_suspend_bank(
 	pr_debug("%s: save    mask %#010x\n", bank->name, save->eint_mask);
 }
 
+static void exynosautov920_pinctrl_suspend_bank(
+				struct samsung_pinctrl_drv_data *drvdata,
+				struct samsung_pin_bank *bank)
+{
+	struct exynos_eint_gpio_save *save = bank->soc_priv;
+	void __iomem *regs = bank->eint_base;
+
+	save->eint_con = readl(regs + bank->eint_offset);
+	save->eint_mask = readl(regs + bank->mask_offset);
+
+	pr_debug("%s: save     con %#010x\n", bank->name, save->eint_con);
+	pr_debug("%s: save    mask %#010x\n", bank->name, save->eint_mask);
+}
+
 void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
 {
 	struct samsung_pin_bank *bank = drvdata->pin_banks;
@@ -662,8 +726,12 @@  void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
 	int i;
 
 	for (i = 0; i < drvdata->nr_banks; ++i, ++bank) {
-		if (bank->eint_type == EINT_TYPE_GPIO)
-			exynos_pinctrl_suspend_bank(drvdata, bank);
+		if (bank->eint_type == EINT_TYPE_GPIO) {
+			if (bank->combine)
+				exynosautov920_pinctrl_suspend_bank(drvdata, bank);
+			else
+				exynos_pinctrl_suspend_bank(drvdata, bank);
+		}
 		else if (bank->eint_type == EINT_TYPE_WKUP) {
 			if (!irq_chip) {
 				irq_chip = bank->irq_chip;
@@ -704,14 +772,34 @@  static void exynos_pinctrl_resume_bank(
 						+ bank->eint_offset);
 }
 
+static void exynosautov920_pinctrl_resume_bank(
+				struct samsung_pinctrl_drv_data *drvdata,
+				struct samsung_pin_bank *bank)
+{
+	struct exynos_eint_gpio_save *save = bank->soc_priv;
+	void __iomem *regs = bank->eint_base;
+
+	pr_debug("%s:     con %#010x => %#010x\n", bank->name,
+			readl(regs + bank->eint_offset), save->eint_con);
+	pr_debug("%s:    mask %#010x => %#010x\n", bank->name,
+			readl(regs + bank->mask_offset), save->eint_mask);
+
+	writel(save->eint_con, regs + bank->eint_offset);
+	writel(save->eint_mask, regs + bank->mask_offset);
+}
+
 void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 {
 	struct samsung_pin_bank *bank = drvdata->pin_banks;
 	int i;
 
 	for (i = 0; i < drvdata->nr_banks; ++i, ++bank)
-		if (bank->eint_type == EINT_TYPE_GPIO)
-			exynos_pinctrl_resume_bank(drvdata, bank);
+		if (bank->eint_type == EINT_TYPE_GPIO) {
+			if (bank->combine)
+				exynosautov920_pinctrl_resume_bank(drvdata, bank);
+			else
+				exynos_pinctrl_resume_bank(drvdata, bank);
+		}
 }
 
 static void exynos_retention_enable(struct samsung_pinctrl_drv_data *drvdata)
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
index 3ac52c2cf998..210952c7a5aa 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.h
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
@@ -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
 
 /* helpers to access interrupt service register */
 #define EXYNOS_SVC_GROUP_SHIFT		3
@@ -140,6 +141,32 @@ 
 		.name		= id				\
 	}
 
+#define EXYNOSV920_PIN_BANK_EINTG(pins, reg, id, offs, mask_offs, pend_offs)	\
+	{							\
+		.type		= &exynos850_bank_type_off,	\
+		.pctl_offset	= reg,				\
+		.nr_pins	= pins,				\
+		.eint_type	= EINT_TYPE_GPIO,		\
+		.eint_offset	= (reg + offs),			\
+		.mask_offset	= (reg + mask_offs),		\
+		.pend_offset	= (reg + pend_offs),		\
+		.combine	= true,				\
+		.name		= id				\
+	}
+
+#define EXYNOSV920_PIN_BANK_EINTW(pins, reg, id, offs, mask_offs, pend_offs)	\
+	{							\
+		.type		= &exynos850_bank_type_alive,	\
+		.pctl_offset	= reg,				\
+		.nr_pins	= pins,				\
+		.eint_type	= EINT_TYPE_WKUP,		\
+		.eint_offset	= (reg + offs),			\
+		.mask_offset	= (reg + mask_offs),		\
+		.pend_offset	= (reg + pend_offs),		\
+		.combine	= true,				\
+		.name		= id				\
+	}
+
 /**
  * struct exynos_weint_data: irq specific data for all the wakeup interrupts
  * generated by the external wakeup interrupt controller.
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 79babbb39ced..adf2d0cff438 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -1106,6 +1106,9 @@  samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
 		bank->eint_type = bdata->eint_type;
 		bank->eint_mask = bdata->eint_mask;
 		bank->eint_offset = bdata->eint_offset;
+		bank->mask_offset = bdata->mask_offset;
+		bank->pend_offset = bdata->pend_offset;
+		bank->combine = bdata->combine;
 		bank->name = bdata->name;
 
 		raw_spin_lock_init(&bank->slock);
@@ -1321,6 +1324,8 @@  static const struct of_device_id samsung_pinctrl_dt_match[] = {
 		.data = &exynosautov9_of_data },
 	{ .compatible = "tesla,fsd-pinctrl",
 		.data = &fsd_of_data },
+	{ .compatible = "samsung,exynosautov920-pinctrl",
+		.data = &exynosautov920_of_data },
 #endif
 #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.
  * @name: name to be prefixed for each pin in this pin bank.
  */
 struct samsung_pin_bank_data {
@@ -133,6 +136,9 @@  struct samsung_pin_bank_data {
 	enum eint_type	eint_type;
 	u32		eint_mask;
 	u32		eint_offset;
+	u32		mask_offset;
+	u32		pend_offset;
+	bool		combine;
 	const char	*name;
 };
 
@@ -147,6 +153,9 @@  struct samsung_pin_bank_data {
  * @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.
  * @name: name to be prefixed for each pin in this pin bank.
  * @id: id of the bank, propagated to the pin range.
  * @pin_base: starting pin number of the bank.
@@ -170,6 +179,9 @@  struct samsung_pin_bank {
 	enum eint_type	eint_type;
 	u32		eint_mask;
 	u32		eint_offset;
+	u32		mask_offset;
+	u32		pend_offset;
+	bool		combine;
 	const char	*name;
 	u32		id;
 
@@ -350,6 +362,7 @@  extern const struct samsung_pinctrl_of_match_data exynos7_of_data;
 extern const struct samsung_pinctrl_of_match_data exynos7885_of_data;
 extern const struct samsung_pinctrl_of_match_data exynos850_of_data;
 extern const struct samsung_pinctrl_of_match_data exynosautov9_of_data;
+extern const struct samsung_pinctrl_of_match_data exynosautov920_of_data;
 extern const struct samsung_pinctrl_of_match_data fsd_of_data;
 extern const struct samsung_pinctrl_of_match_data s3c64xx_of_data;
 extern const struct samsung_pinctrl_of_match_data s3c2412_of_data;