Re: [PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory clients

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3409 bytes --]

On Tue, Dec 23, 2014 at 06:39:56PM +0800, Vince Hsu wrote:
> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> ---
>  drivers/memory/tegra/tegra124.c | 82 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
> index 278d40b854c1..036935743a0a 100644
> --- a/drivers/memory/tegra/tegra124.c
> +++ b/drivers/memory/tegra/tegra124.c
> @@ -6,6 +6,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/mm.h>
>  
> @@ -959,7 +960,85 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
>  	{ .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
>  };
>  
> +static const struct tegra_mc_hr tegra124_mc_hr[] = {
> +	{TEGRA_SWGROUP_AFI,        0x200, 0x200,  0},
> +	{TEGRA_SWGROUP_AVPC,       0x200, 0x200,  1},
> +	{TEGRA_SWGROUP_DC,         0x200, 0x200,  2},
> +	{TEGRA_SWGROUP_DCB,        0x200, 0x200,  3},
> +	{TEGRA_SWGROUP_HC,         0x200, 0x200,  6},
> +	{TEGRA_SWGROUP_HDA,        0x200, 0x200,  7},
> +	{TEGRA_SWGROUP_ISP2,       0x200, 0x200,  8},
> +	{TEGRA_SWGROUP_MPCORE,     0x200, 0x200,  9},
> +	{TEGRA_SWGROUP_MPCORELP,   0x200, 0x200, 10},
> +	{TEGRA_SWGROUP_MSENC,      0x200, 0x200, 11},
> +	{TEGRA_SWGROUP_PPCS,       0x200, 0x200, 14},
> +	{TEGRA_SWGROUP_SATA,       0x200, 0x200, 15},
> +	{TEGRA_SWGROUP_VDE,        0x200, 0x200, 16},
> +	{TEGRA_SWGROUP_VI,         0x200, 0x200, 17},
> +	{TEGRA_SWGROUP_VIC,        0x200, 0x200, 18},
> +	{TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x200, 19},
> +	{TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x200, 20},
> +	{TEGRA_SWGROUP_TSEC,       0x200, 0x200, 22},
> +	{TEGRA_SWGROUP_SDMMC1A,    0x200, 0x200, 29},
> +	{TEGRA_SWGROUP_SDMMC2A,    0x200, 0x200, 30},
> +	{TEGRA_SWGROUP_SDMMC3A,    0x200, 0x200, 31},

The documentation that I have says that the status register for these is
0x204.

> +	{TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
> +	{TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
> +	{TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
> +};
> +
>  #ifdef CONFIG_ARCH_TEGRA_124_SOC
> +
> +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
> +		u32 reg, u32 *stat)
> +{
> +	int i;
> +	u32 cur_stat;
> +	u32 prv_stat;
> +
> +	prv_stat = mc_readl(mc, reg);
> +	for (i = 0; i < 5; i++) {
> +		cur_stat = mc_readl(mc, reg);
> +		if (cur_stat != prv_stat)
> +			return false;
> +	}

Why this loop? The function is already called in a polling loop below.
Also why compare to the previous value of the register? Isn't the only
thing we're interested in the value of the specific bit?

> +	*stat = cur_stat;
> +	return true;
> +}
> +
> +static int tegra124_mc_flush(struct tegra_mc *mc,
> +		const struct tegra_mc_hr *hr_client, bool enable)
> +{
> +	u32 val;
> +
> +	if (!mc || !hr_client)
> +		return -EINVAL;
> +
> +	val = mc_readl(mc, hr_client->ctrl);
> +	if (enable)
> +		val |= BIT(hr_client->bit);
> +	else
> +		val &= ~BIT(hr_client->bit);
> +	mc_writel(mc, val, hr_client->ctrl);
> +	mc_readl(mc, hr_client->ctrl);
> +
> +	/* poll till the flush is done */
> +	if (enable) {
> +		do {
> +			udelay(10);

This should probably be usleep_range(10, 20) or something.

Would it be difficult to implement this for Tegra30 and Tegra114?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]