| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 25 Sep 2022 20:29:04 +0000
From: Petlozu Pravareshwar <petlozup@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: Jonathan Hunter <jonathanh@...dia.com>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"dmitry.osipenko@...labora.com" <dmitry.osipenko@...labora.com>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
"Kartik ." <kkartik@...dia.com>,
"cai.huoqing@...ux.dev" <cai.huoqing@...ux.dev>,
Sandipan Patra <spatra@...dia.com>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3] soc/tegra: pmc: Add IO Pad table for tegra234
>
> 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@...dia.com>
> > ---
> > 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
> >
Powered by blists - more mailing lists