RE: [PATCH v2] soc/tegra: pmc: Add IO Pad table for tegra234

[Petlozu Pravareshwar]

> 
> On Mon, Aug 08, 2022 at 08:14:20PM +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>
> > ---
> >  drivers/soc/tegra/pmc.c | 109
> > ++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 105 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index
> > 5611d14d3ba2..34d36a28f7d6 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -266,11 +266,22 @@ struct tegra_powergate {
> >  	struct reset_control *reset;
> >  };
> >
> > +enum tegra_dpd_reg {
> > +	TEGRA_PMC_IO_INVALID_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,
> > +};
> > +
> >  struct tegra_io_pad_soc {
> >  	enum tegra_io_pad id;
> >  	unsigned int dpd;
> >  	unsigned int voltage;
> >  	const char *name;
> > +	enum tegra_dpd_reg reg_index;
> >  };
> >
> >  struct tegra_pmc_regs {
> > @@ -284,6 +295,8 @@ struct tegra_pmc_regs {
> >  	unsigned int rst_source_mask;
> >  	unsigned int rst_level_shift;
> >  	unsigned int rst_level_mask;
> > +	const unsigned int *reorg_dpd_req;
> > +	const unsigned int *reorg_dpd_status;
> >  };
> >
> >  struct tegra_wake_event {
> > @@ -364,6 +377,7 @@ struct tegra_pmc_soc {
> >  	bool has_blink_output;
> >  	bool has_usb_sleepwalk;
> >  	bool supports_core_domain;
> > +	bool has_reorg_hw_dpd_reg_impl;
> >  };
> >
> >  /**
> > @@ -1546,6 +1560,14 @@ static int
> tegra_io_pad_get_dpd_register_bit(struct tegra_pmc *pmc,
> >  	if (pad->dpd == UINT_MAX)
> >  		return -ENOTSUPP;
> >
> > +	if (pmc->soc->has_reorg_hw_dpd_reg_impl) {
> > +		*mask = BIT(pad->dpd);
> > +		*status = pmc->soc->regs->reorg_dpd_status[pad-
> >reg_index];
> > +		*request = pmc->soc->regs->reorg_dpd_req[pad-
> >reg_index];
> > +
> > +		goto done;
> > +	}
> > +
> >  	*mask = BIT(pad->dpd % 32);
> >
> >  	if (pad->dpd < 32) {
> > @@ -1556,6 +1578,7 @@ static int
> tegra_io_pad_get_dpd_register_bit(struct tegra_pmc *pmc,
> >  		*request = pmc->soc->regs->dpd2_req;
> >  	}
> >
> > +done:
> >  	return 0;
> >  }
> >
> 
> All of this looks "bolted on". Can we not instead rework the existing register
> definitions to work with the new dpd_status and dpd_request arrays? It
> means that we'd probably need a bit of duplication of data since we would
> no longer programmatically determine the register offsets like we used to,
> but it would save the extra flag and make the code much more readable, in
> my opinion.
> 
> Thierry
Yes. Agree that we can rework the patch to make the code more readable and
to save the extra flags. Will update the patch.

Thanks.