> > 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 Agree to the comments. Will be updating the patch accordingly. Thanks. > > > > > - 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 > >