Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 18 Aug 2022 17:13:54 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Petlozu Pravareshwar <petlozup@nvidia.com>
Cc:     jonathanh@nvidia.com, p.zabel@pengutronix.de,
        dmitry.osipenko@collabora.com, ulf.hansson@linaro.org,
        kkartik@nvidia.com, cai.huoqing@linux.dev, spatra@nvidia.com,
        linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] soc/tegra: pmc: Add IO Pad table for tegra234
Message-ID: <Yv5XMt5M7IGm4/Gx@orome>
References: <20220808201420.3451111-1-petlozup@nvidia.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="GqRZS6dITKd9nBWe"
Content-Disposition: inline
In-Reply-To: <20220808201420.3451111-1-petlozup@nvidia.com>
User-Agent: Mutt/2.2.7 (2022-08-07)
Precedence: bulk


--GqRZS6dITKd9nBWe
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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.
>=20
> Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 109 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 105 insertions(+), 4 deletions(-)
>=20
> 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;
>  };
> =20
> +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;
>  };
> =20
>  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;
>  };
> =20
>  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;
>  };
> =20
>  /**
> @@ -1546,6 +1560,14 @@ static int tegra_io_pad_get_dpd_register_bit(struc=
t tegra_pmc *pmc,
>  	if (pad->dpd =3D=3D UINT_MAX)
>  		return -ENOTSUPP;
> =20
> +	if (pmc->soc->has_reorg_hw_dpd_reg_impl) {
> +		*mask =3D BIT(pad->dpd);
> +		*status =3D pmc->soc->regs->reorg_dpd_status[pad->reg_index];
> +		*request =3D pmc->soc->regs->reorg_dpd_req[pad->reg_index];
> +
> +		goto done;
> +	}
> +
>  	*mask =3D BIT(pad->dpd % 32);
> =20
>  	if (pad->dpd < 32) {
> @@ -1556,6 +1578,7 @@ static int tegra_io_pad_get_dpd_register_bit(struct=
 tegra_pmc *pmc,
>  		*request =3D pmc->soc->regs->dpd2_req;
>  	}
> =20
> +done:
>  	return 0;
>  }
> =20

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

--GqRZS6dITKd9nBWe
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmL+VzIACgkQ3SOs138+
s6G1Uw/+Ifn5D/RMqFUWRn+dGPtPI7RrBqU6AfT+cwbtE1d3lT6RNgVpj+h+iZBI
B6JmgjQlIyfQQq22JCK78bQ8pwW6GXSCU1yvatJPYiwfzEu/w8lsZCCG4RkhYsjJ
1FNs+vIqnHvCu8/xOWiCbKwYjBX1xSndUVUeOcrJKpezNdC+KPjM21wavJsGUGFW
KNzR4kwpTBEdJ1Vb2CsI7qu2f7wy9DN34giYy2LAbyNQe/PYqVXffli2W9mSFXir
Qmr8PzzldfW41vJnbLp9FDIHOm5I9vjEVtQNKJhMfGEwLNYQJ4uuk2HU2yTE98yr
/i2SOlLi73oLoXHGfzwYeJBvkBr/8XlTpEqSX6RpaZBAVtfAkslVmWEQpubxN2u8
4DTd33Qzo9/ZNe3D4s1H8nFeadN0FpJmLbUC0Gzs2V7zR6U+ExTeg7o9u525lm8z
pRypv0n3BFI9YOu6AowgYEMaMIGTMNs+maQDyQUp2q7RS4G5KXeeDXdrPmv2hTwz
neMPtDqSKfL5NnTK35DgG6VK74O2wa4Z66+AvH52W5+C7zKt+VQ8muQdwmVpyUnK
lduzHIXbSBTEolP8TQQCgJEPx/n2DOh5bTitbQ8uBXbwKinKUHUhbIMUmKrEPIuX
lStgJQA8nlPmzh3myIIpc6DPFHApoGWyH4ikT8kMlrvCxQ8wcVA=
=61el
-----END PGP SIGNATURE-----

--GqRZS6dITKd9nBWe--