Re: [PATCH v3] soc/tegra: pmc: Add IO Pad table for tegra234

From: Thierry Reding
Date: Wed Sep 14 2022 - 10:51:23 EST

Next message: Jan Kara: "Re: [PATCH v2 4/7] iov_iter: new iov_iter_pin_pages*() routines"
Previous message: Krishna Chaitanya Chundru: "Re: [PATCH v6 3/5] phy: core: Add support for phy power down & power up"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Aug 24, 2022 at 08:27:23PM +0000, Petlozu Pravareshwar wrote:
> Add IO PAD table for tegra234 to allow configuring dpd mode
> and switching the pins to 1.8V or 3.3V as needed.
>
> In tegra234, DPD registers are reorganized such that there is
> a DPD_REQ register and a DPD_STATUS register per pad group.
> This change accordingly updates the PMC driver.
>
> Signed-off-by: Petlozu Pravareshwar <petlozup@xxxxxxxxxx>
> ---
> v3:
> * Update to make the code more readable and avoid using extra flags.
> ---
> drivers/soc/tegra/pmc.c | 163 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 151 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 5611d14d3ba2..495d16a4732c 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -266,11 +266,23 @@ struct tegra_powergate {
> struct reset_control *reset;
> };
>
> +enum tegra_io_pad_dpd {
> + TEGRA_PMC_IO_LEGACY_DPD,
> + TEGRA_PMC_IO_CSI_DPD,
> + TEGRA_PMC_IO_DISP_DPD,
> + TEGRA_PMC_IO_QSPI_DPD,
> + TEGRA_PMC_IO_UFS_DPD,
> + TEGRA_PMC_IO_EDP_DPD,
> + TEGRA_PMC_IO_SDMMC1_HV_DPD,
> + TEGRA_PMC_IO_INVALID_DPD,
> +};
> +
> struct tegra_io_pad_soc {
> enum tegra_io_pad id;
> unsigned int dpd;
> unsigned int voltage;
> const char *name;
> + enum tegra_io_pad_dpd dpd_index;
> };
>
> struct tegra_pmc_regs {
> @@ -284,6 +296,18 @@ struct tegra_pmc_regs {
> unsigned int rst_source_mask;
> unsigned int rst_level_shift;
> unsigned int rst_level_mask;
> + unsigned int csi_dpd_req;
> + unsigned int csi_dpd_status;
> + unsigned int disp_dpd_req;
> + unsigned int disp_dpd_status;
> + unsigned int qspi_dpd_req;
> + unsigned int qspi_dpd_status;
> + unsigned int ufs_dpd_req;
> + unsigned int ufs_dpd_status;
> + unsigned int edp_dpd_req;
> + unsigned int edp_dpd_status;
> + unsigned int sdmmc1_hv_dpd_req;
> + unsigned int sdmmc1_hv_dpd_status;

This seems a bit complicated because there's an extra level of
indirection (via that dpd_index variable). I think it'd be a bit more
trivial to directly add offsets for the req and status registers to the
I/O pad entry definitions. So I'm thinking something like this:

struct tegra_io_pad_soc {
enum tegra_io_pad id;
unsigned int dpd;
unsigned int request; /* these two are the new */
unsigned int status; /* register offsets */
unsigned int voltage;
const char *name;
};

And then...

> };
>
> struct tegra_wake_event {
> @@ -1536,6 +1560,7 @@ static int tegra_io_pad_get_dpd_register_bit(struct tegra_pmc *pmc,
> u32 *mask)
> {
> const struct tegra_io_pad_soc *pad;
> + int ret = 0;
>
> pad = tegra_io_pad_find(pmc, id);
> if (!pad) {
> @@ -1546,17 +1571,63 @@ static int tegra_io_pad_get_dpd_register_bit(struct tegra_pmc *pmc,
> if (pad->dpd == UINT_MAX)
> return -ENOTSUPP;
>
> - *mask = BIT(pad->dpd % 32);
>
> - if (pad->dpd < 32) {
> - *status = pmc->soc->regs->dpd_status;
> - *request = pmc->soc->regs->dpd_req;
> - } else {
> - *status = pmc->soc->regs->dpd2_status;
> - *request = pmc->soc->regs->dpd2_req;
> + switch (pad->dpd_index) {
> + case TEGRA_PMC_IO_LEGACY_DPD:
> + *mask = BIT(pad->dpd % 32);
> +
> + if (pad->dpd < 32) {
> + *status = pmc->soc->regs->dpd_status;
> + *request = pmc->soc->regs->dpd_req;
> + } else {
> + *status = pmc->soc->regs->dpd2_status;
> + *request = pmc->soc->regs->dpd2_req;
> + }
> + break;
> +
> + case TEGRA_PMC_IO_CSI_DPD:
> + *mask = BIT(pad->dpd);
> + *status = pmc->soc->regs->csi_dpd_status;
> + *request = pmc->soc->regs->csi_dpd_req;
> + break;
> +
> + case TEGRA_PMC_IO_DISP_DPD:
> + *mask = BIT(pad->dpd);
> + *status = pmc->soc->regs->disp_dpd_status;
> + *request = pmc->soc->regs->disp_dpd_req;
> + break;
> +
> + case TEGRA_PMC_IO_QSPI_DPD:
> + *mask = BIT(pad->dpd);
> + *status = pmc->soc->regs->qspi_dpd_status;
> + *request = pmc->soc->regs->qspi_dpd_req;
> + break;
> +
> + case TEGRA_PMC_IO_UFS_DPD:
> + *mask = BIT(pad->dpd);
> + *status = pmc->soc->regs->ufs_dpd_status;
> + *request = pmc->soc->regs->ufs_dpd_req;
> + break;
> +
> + case TEGRA_PMC_IO_EDP_DPD:
> + *mask = BIT(pad->dpd);
> + *status = pmc->soc->regs->edp_dpd_status;
> + *request = pmc->soc->regs->edp_dpd_req;
> + break;
> +
> + case TEGRA_PMC_IO_SDMMC1_HV_DPD:
> + *mask = BIT(pad->dpd);
> + *status = pmc->soc->regs->sdmmc1_hv_dpd_status;
> + *request = pmc->soc->regs->sdmmc1_hv_dpd_req;
> + break;
> +
> + default:
> + dev_err(pmc->dev, "invalid DPD reg index %u\n", pad->dpd_index);
> + ret = -ENOENT;
> + break;
> }

All of these can simply become:

*request = pad->request;
*status = pad->status;
*mask = BIT(pad->dpd);

At which point it might even be worth dropping that helper function
altogether and inlining the values there. Perhaps do the pad lookup at a
level higher up, such as in tegra_io_pad_power_enable() and pass a const
struct tegra_io_pad_soc * to tegra_io_pad_prepare() and
tegra_io_pad_poll().

This has the disadvantage that it requires existing tables to be
reworked a little and they will grow somewhat because of the extra
fields, but ultimately the code becomes much simpler and decreases in
size to compensate for the extra data. We also won't need special I/O
pad initialization macros like TEGRA234_IO_PAD below.

If there's a *big* concern about the data size, we could avoid the worst
by special-casing the legacy cases like you've done here. But I think it
is really not worth it. On the plus side, the new version will be
flexible enough to handle any additions and/or changes to the set of
request and status registers in future SoCs.

Thierry

>
> - return 0;
> + return ret;
> }
>
> static int tegra_io_pad_prepare(struct tegra_pmc *pmc, enum tegra_io_pad id,
> @@ -3291,6 +3362,7 @@ static const u8 tegra124_cpu_powergates[] = {
> .dpd = (_dpd), \
> .voltage = (_voltage), \
> .name = (_name), \
> + .dpd_index = TEGRA_PMC_IO_LEGACY_DPD, \
> })
>
> #define TEGRA_IO_PIN_DESC(_id, _dpd, _voltage, _name) \
> @@ -3791,6 +3863,61 @@ static const struct tegra_pmc_soc tegra194_pmc_soc = {
> .has_usb_sleepwalk = false,
> };
>
> +#define TEGRA234_IO_PAD(_id, _dpd, _voltage, _name, _dpd_index) \
> + ((struct tegra_io_pad_soc) { \
> + .id = (_id), \
> + .dpd = (_dpd), \
> + .voltage = (_voltage), \
> + .name = (_name), \
> + .dpd_index = (_dpd_index), \
> + })
> +
> +#define TEGRA234_IO_PIN_DESC(_id, _dpd, _voltage, _name, _dpd_index) \
> + ((struct pinctrl_pin_desc) { \
> + .number = (_id), \
> + .name = (_name) \
> + })
> +
> +#define TEGRA234_IO_PAD_TABLE(_pad) { \
> + /* (id, dpd, voltage, name, dpd_index) */ \
> + _pad(TEGRA_IO_PAD_CSIA, 0, UINT_MAX, "csia", \
> + TEGRA_PMC_IO_CSI_DPD), \
> + _pad(TEGRA_IO_PAD_CSIB, 1, UINT_MAX, "csib", \
> + TEGRA_PMC_IO_CSI_DPD), \
> + _pad(TEGRA_IO_PAD_HDMI_DP0, 0, UINT_MAX, "hdmi-dp0", \
> + TEGRA_PMC_IO_DISP_DPD), \
> + _pad(TEGRA_IO_PAD_CSIC, 2, UINT_MAX, "csic", \
> + TEGRA_PMC_IO_CSI_DPD), \
> + _pad(TEGRA_IO_PAD_CSID, 3, UINT_MAX, "csid", \
> + TEGRA_PMC_IO_CSI_DPD), \
> + _pad(TEGRA_IO_PAD_CSIE, 4, UINT_MAX, "csie", \
> + TEGRA_PMC_IO_CSI_DPD), \
> + _pad(TEGRA_IO_PAD_CSIF, 5, UINT_MAX, "csif", \
> + TEGRA_PMC_IO_CSI_DPD), \
> + _pad(TEGRA_IO_PAD_UFS, 0, UINT_MAX, "ufs", \
> + TEGRA_PMC_IO_UFS_DPD), \
> + _pad(TEGRA_IO_PAD_EDP, 1, UINT_MAX, "edp", \
> + TEGRA_PMC_IO_EDP_DPD), \
> + _pad(TEGRA_IO_PAD_SDMMC1_HV, 0, 4, "sdmmc1-hv", \
> + TEGRA_PMC_IO_SDMMC1_HV_DPD), \
> + _pad(TEGRA_IO_PAD_SDMMC3_HV, UINT_MAX, 6, "sdmmc3-hv", \
> + TEGRA_PMC_IO_INVALID_DPD), \
> + _pad(TEGRA_IO_PAD_AUDIO_HV, UINT_MAX, 1, "audio-hv", \
> + TEGRA_PMC_IO_INVALID_DPD), \
> + _pad(TEGRA_IO_PAD_AO_HV, UINT_MAX, 0, "ao-hv", \
> + TEGRA_PMC_IO_INVALID_DPD), \
> + _pad(TEGRA_IO_PAD_CSIG, 6, UINT_MAX, "csig", \
> + TEGRA_PMC_IO_CSI_DPD), \
> + _pad(TEGRA_IO_PAD_CSIH, 7, UINT_MAX, "csih", \
> + TEGRA_PMC_IO_CSI_DPD) \
> + }
> +
> +static const struct tegra_io_pad_soc tegra234_io_pads[] =
> + TEGRA234_IO_PAD_TABLE(TEGRA234_IO_PAD);
> +
> +static const struct pinctrl_pin_desc tegra234_pin_descs[] =
> + TEGRA234_IO_PAD_TABLE(TEGRA234_IO_PIN_DESC);
> +
> static const struct tegra_pmc_regs tegra234_pmc_regs = {
> .scratch0 = 0x2000,
> .dpd_req = 0,
> @@ -3802,6 +3929,18 @@ static const struct tegra_pmc_regs tegra234_pmc_regs = {
> .rst_source_mask = 0xfc,
> .rst_level_shift = 0x0,
> .rst_level_mask = 0x3,
> + .csi_dpd_req = 0xe0c0,
> + .csi_dpd_status = 0xe0c4,
> + .disp_dpd_req = 0xe0d0,
> + .disp_dpd_status = 0xe0d4,
> + .qspi_dpd_req = 0xe074,
> + .qspi_dpd_status = 0xe078,
> + .ufs_dpd_req = 0xe064,
> + .ufs_dpd_status = 0xe068,
> + .edp_dpd_req = 0xe05c,
> + .edp_dpd_status = 0xe060,
> + .sdmmc1_hv_dpd_req = 0xe054,
> + .sdmmc1_hv_dpd_status = 0xe058,
> };
>
> static const char * const tegra234_reset_sources[] = {
> @@ -3861,10 +4000,10 @@ static const struct tegra_pmc_soc tegra234_pmc_soc = {
> .needs_mbist_war = false,
> .has_impl_33v_pwr = true,
> .maybe_tz_only = false,
> - .num_io_pads = 0,
> - .io_pads = NULL,
> - .num_pin_descs = 0,
> - .pin_descs = NULL,
> + .num_io_pads = ARRAY_SIZE(tegra234_io_pads),
> + .io_pads = tegra234_io_pads,
> + .num_pin_descs = ARRAY_SIZE(tegra234_pin_descs),
> + .pin_descs = tegra234_pin_descs,
> .regs = &tegra234_pmc_regs,
> .init = NULL,
> .setup_irq_polarity = tegra186_pmc_setup_irq_polarity,
> --
> 2.17.1
>

Attachment: signature.asc
Description: PGP signature

Next message: Jan Kara: "Re: [PATCH v2 4/7] iov_iter: new iov_iter_pin_pages*() routines"
Previous message: Krishna Chaitanya Chundru: "Re: [PATCH v6 3/5] phy: core: Add support for phy power down & power up"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]