Re: [PATCH v3 14/20] pinctrl: samsung: Add gs101 SoC pinctrl configuration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sam,

On Thu, 12 Oct 2023 at 07:00, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
>
> On Wed, Oct 11, 2023 at 1:49 PM Peter Griffin <peter.griffin@xxxxxxxxxx> wrote:
> >
> > Add support for the pin-controller found on the gs101 SoC used in
> > Pixel 6 phones.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@xxxxxxxxxx>
> > ---
> >  .../pinctrl/samsung/pinctrl-exynos-arm64.c    | 163 ++++++++++++++++++
> >  drivers/pinctrl/samsung/pinctrl-exynos.c      |   2 +
> >  drivers/pinctrl/samsung/pinctrl-exynos.h      |  34 ++++
> >  drivers/pinctrl/samsung/pinctrl-samsung.c     |   2 +
> >  drivers/pinctrl/samsung/pinctrl-samsung.h     |   1 +
> >  5 files changed, 202 insertions(+)
> >
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > index cb965cf93705..db47001d1b35 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > @@ -796,3 +796,166 @@ const struct samsung_pinctrl_of_match_data fsd_of_data __initconst = {
> >         .ctrl           = fsd_pin_ctrl,
> >         .num_ctrl       = ARRAY_SIZE(fsd_pin_ctrl),
> >  };
> > +
> > +/*
> > + * bank type for non-alive type
> > + * (CON bit field: 4, DAT bit field: 1, PUD bit field: 4, DRV bit field: 4)
> > + * (CONPDN bit field: 2, PUDPDN bit field: 4)
> > + */
> > +static struct samsung_pin_bank_type gs101_bank_type_off  = {
> > +       .fld_width = { 4, 1, 4, 4, 2, 4, },
> > +       .reg_offset = { 0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, },
> > +};
> > +
> > +/*
> > + * bank type for alive type
> > + * (CON bit field: 4, DAT bit field: 1, PUD bit field: 4, DRV bit field: 4)
> > + */
> > +static const struct samsung_pin_bank_type gs101_bank_type_alive = {
> > +       .fld_width = { 4, 1, 4, 4, },
> > +       .reg_offset = { 0x00, 0x04, 0x08, 0x0c, },
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (ALIVE) */
> > +static const struct samsung_pin_bank_data gs101_pin_alive[] = {
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 8, 0x0, "gpa0", 0x00, 0x00, FLT_SELECTABLE),
>
> Here and further: please keep 80 characters per line when possible.

Will fix.

>
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 7, 0x20, "gpa1", 0x04, 0x08, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 5, 0x40, "gpa2", 0x08, 0x10, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 4, 0x60, "gpa3", 0x0c, 0x18, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 4, 0x80, "gpa4", 0x10, 0x1c, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 7, 0xa0, "gpa5", 0x14, 0x20, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 8, 0xc0, "gpa9", 0x18, 0x28, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 2, 0xe0, "gpa10", 0x1c, 0x30, FLT_SELECTABLE),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (FAR_ALIVE) */
> > +static const struct samsung_pin_bank_data gs101_pin_far_alive[] = {
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 8, 0x0, "gpa6", 0x00, 0x00, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 4, 0x20, "gpa7", 0x04, 0x08, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 8, 0x40, "gpa8", 0x08, 0x0c, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 2, 0x60, "gpa11", 0x0c, 0x14, FLT_SELECTABLE),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (GSACORE) */
> > +static const struct samsung_pin_bank_data gs101_pin_gsacore[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x0, "gps0", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 8, 0x20, "gps1", 0x04, 0x04, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 3, 0x40, "gps2", 0x08, 0x0c, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (GSACTRL) */
> > +static const struct samsung_pin_bank_data gs101_pin_gsactrl[] = {
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 6, 0x0, "gps3", 0x00, 0x00, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (PERIC0) */
> > +static const struct samsung_pin_bank_data gs101_pin_peric0[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 5, 0x0, "gpp0", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x20, "gpp1", 0x04, 0x08, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x40, "gpp2", 0x08, 0x0c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x60, "gpp3", 0x0c, 0x10, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x80, "gpp4", 0x10, 0x14, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0xa0, "gpp5", 0x14, 0x18, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0xc0, "gpp6", 0x18, 0x1c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0xe0, "gpp7", 0x1c, 0x20, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x100, "gpp8", 0x20, 0x24, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x120, "gpp9", 0x24, 0x28, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x140, "gpp10", 0x28, 0x2c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x160, "gpp11", 0x2c, 0x30, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x180, "gpp12", 0x30, 0x34, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x1a0, "gpp13", 0x34, 0x38, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x1c0, "gpp14", 0x38, 0x3c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x1e0, "gpp15", 0x3c, 0x40, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x200, "gpp16", 0x40, 0x44, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x220, "gpp17", 0x44, 0x48, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x240, "gpp18", 0x48, 0x4c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x260, "gpp19", 0x4c, 0x50, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (PERIC1) */
> > +static const struct samsung_pin_bank_data gs101_pin_peric1[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 8, 0x0, "gpp20", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x20, "gpp21", 0x04, 0x08, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x40, "gpp22", 0x08, 0x0c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 8, 0x60, "gpp23", 0x0c, 0x10, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x80, "gpp24", 0x10, 0x18, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0xa0, "gpp25", 0x14, 0x1c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 5, 0xc0, "gpp26", 0x18, 0x20, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0xe0, "gpp27", 0x1c, 0x28, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (HSI1) */
> > +static const struct samsung_pin_bank_data gs101_pin_hsi1[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 6, 0x0, "gph0", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 7, 0x20, "gph1", 0x04, 0x08, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (HSI2) */
> > +static const struct samsung_pin_bank_data gs101_pin_hsi2[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 6, 0x0, "gph2", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x20, "gph3", 0x04, 0x08, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 6, 0x40, "gph4", 0x08, 0x0c, FLT_DEFAULT),
> > +};
> > +
> > +static const struct samsung_pin_ctrl gs101_pin_ctrl[] __initconst = {
> > +       {
> > +               /* pin banks of gs101 pin-controller (ALIVE) */
> > +               .pin_banks      = gs101_pin_alive,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_alive),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .eint_wkup_init = exynos_eint_wkup_init,
>
> Is it ok to have both .eint_gpio_init and .eint_wkup_init set here and
> further? I remember doing something like that for Exynos850 before,
> only to realize further if was a mistake. Please check commit
> 96f79935015c ("pinctrl: samsung: Remove EINT handler for Exynos850
> ALIVE and CMGP gpios"). Maybe it's ok in your case.

Thanks for the hint :) You're correct eint_gpio_init is not required on alive,
far_alive, gsacore and gsactrl banks. Will update in v4.

>
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
>
> Did you manage to actually test those suspend/resume callbacks
> somehow? If so, can you please share the procedure? I guess I had some
> Power Domains and clock related problems on Exynos850 when I tried
> that before, so just curious.

You can test the callbacks using
echo mem > /sys/power/state

Thanks,

Peter.




>
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (FAR_ALIVE) */
> > +               .pin_banks      = gs101_pin_far_alive,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_far_alive),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .eint_wkup_init = exynos_eint_wkup_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (GSACORE) */
> > +               .pin_banks      = gs101_pin_gsacore,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_gsacore),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (GSACTRL) */
> > +               .pin_banks      = gs101_pin_gsactrl,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_gsactrl),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (PERIC0) */
> > +               .pin_banks      = gs101_pin_peric0,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_peric0),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (PERIC1) */
> > +               .pin_banks      = gs101_pin_peric1,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_peric1),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume = exynos_pinctrl_resume,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (HSI1) */
> > +               .pin_banks      = gs101_pin_hsi1,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_hsi1),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (HSI2) */
> > +               .pin_banks      = gs101_pin_hsi2,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_hsi2),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
> > +       },
> > +};
> > +
> > +const struct samsung_pinctrl_of_match_data gs101_of_data __initconst = {
> > +       .ctrl           = gs101_pin_ctrl,
> > +       .num_ctrl       = ARRAY_SIZE(gs101_pin_ctrl),
> > +};
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> > index 800831aa8357..014f0c37f97f 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> > @@ -533,6 +533,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 = "google,gs101-wakeup-eint",
> > +                       .data = &exynos7_wkup_irq_chip },
> >         { }
> >  };
> >
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
> > index 63b2426ad5d6..0dd013654bd2 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
> > @@ -147,6 +147,40 @@
> >                 .name           = id                            \
> >         }
> >
> > +#define EXYNOS9_PIN_BANK_EINTN(types, pins, reg, id)   \
> > +       {                                               \
> > +               .type           = &types,               \
> > +               .pctl_offset    = reg,                  \
> > +               .nr_pins        = pins,                 \
> > +               .eint_type      = EINT_TYPE_NONE,       \
> > +               .fltcon_type    = FLT_DEFAULT           \
> > +               .name           = id                    \
> > +       }
> > +
> > +#define EXYNOS9_PIN_BANK_EINTG(types, pins, reg, id, offs, fltcon_offs, fltcontype) \
> > +       {                                               \
> > +               .type           = &types,               \
> > +               .pctl_offset    = reg,                  \
> > +               .nr_pins        = pins,                 \
> > +               .eint_type      = EINT_TYPE_GPIO,       \
> > +               .eint_offset    = offs,                 \
> > +               .fltcon_type    = fltcontype,           \
> > +               .fltcon_offset  = fltcon_offs,          \
> > +               .name           = id                    \
> > +       }
> > +
> > +#define EXYNOS9_PIN_BANK_EINTW(types, pins, reg, id, offs, fltcon_offs, fltcontype) \
> > +       {                                               \
> > +               .type           = &types,               \
> > +               .pctl_offset    = reg,                  \
> > +               .nr_pins        = pins,                 \
> > +               .eint_type      = EINT_TYPE_WKUP,       \
> > +               .eint_offset    = offs,                 \
> > +               .fltcon_type    = fltcontype,           \
> > +               .fltcon_offset  = fltcon_offs,          \
> > +               .name           = id                    \
> > +       }
> > +
>
> Looks to me that instead of adding new macros the already existing
> EXYNOS850_PIN_BANK_* should be extended and re-used. Because those
> pinctrl IP-cores on all modern Exynos chips look very similar, even if
> you compare the downstream code. If EXYNOS850 prefix looks confusing,
> maybe it can be renamed to EXYNOS9 or something like that. Those
> filter parameters are also present in Exynos850 downstream kernel
> code. So I just feel like the proper way to add that feature would be
> to add that also for all modern ARM64 Exynos variants while at it.
>
> >  /**
> >   * 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 449f8109d8b5..12176f98440d 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > @@ -1321,6 +1321,8 @@ static const struct of_device_id samsung_pinctrl_dt_match[] = {
> >                 .data = &exynosautov9_of_data },
> >         { .compatible = "tesla,fsd-pinctrl",
> >                 .data = &fsd_of_data },
> > +       { .compatible = "google,gs101-pinctrl",
> > +               .data = &gs101_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 de2ca8e8b378..e62e909fb10d 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> > @@ -374,6 +374,7 @@ 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 fsd_of_data;
> > +extern const struct samsung_pinctrl_of_match_data gs101_of_data;
> >  extern const struct samsung_pinctrl_of_match_data s3c64xx_of_data;
> >  extern const struct samsung_pinctrl_of_match_data s3c2412_of_data;
> >  extern const struct samsung_pinctrl_of_match_data s3c2416_of_data;
> > --
> > 2.42.0.655.g421f12c284-goog
> >





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux