thr3ads.net - Nouveau - [Nouveau] [PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory clients [Jan 2015]

If this information is useful, please help other people find it:
Share via:

Thierry Reding

2015-Jan-06 14:30 UTC

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

On Tue, Dec 23, 2014 at 06:39:56PM +0800, Vince Hsu
wrote:> Signed-off-by: Vince Hsu <vinceh at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20150106/9c6f4da2/attachment.sig>

Vince Hsu

2015-Jan-06 15:07 UTC

head link

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

On 03:30:00PM Jan 06, Thierry Reding wrote:> * PGP Signed by an unknown key
> 
> On Tue, Dec 23, 2014 at 06:39:56PM +0800, Vince Hsu wrote:
> > Signed-off-by: Vince Hsu <vinceh at 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.Oops. Thanks for catching this. Will fix.
> 
> > +	{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?I recall it's due to a HW bug that there might be a gitch if we program
the ctrl reg and then read the status reg in a short window. This function
is to make sure we have a stable status.
> 
> > +	*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.Maybe no. We might need some spin lock here to ensure only one flushing
operation requested and no race could happen.
> 
> Would it be difficult to implement this for Tegra30 and Tegra114?No. But I have to check the detail in Tegra30 and Tegra114. And the biggest
problem is I don't have the boards to test.

Thanks,
Vince

Thierry Reding

2015-Jan-06 15:27 UTC

head link

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

On Tue, Jan 06, 2015 at 11:07:45PM +0800, Vince Hsu
wrote:> On 03:30:00PM Jan 06, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Dec 23, 2014 at 06:39:56PM +0800, Vince Hsu wrote:
> > > Signed-off-by: Vince Hsu <vinceh at 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.
> Oops. Thanks for catching this. Will fix.
> 
> > 
> > > +	{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?
> I recall it's due to a HW bug that there might be a gitch if we program
> the ctrl reg and then read the status reg in a short window. This function
> is to make sure we have a stable status.
This warrants a comment, then.
> > > +	*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.
> Maybe no. We might need some spin lock here to ensure only one flushing
> operation requested and no race could happen.
We should use a mutex, then. There's no saying how long this will take
and busy-looping indefinitely is a bad idea. Though it seems to me like
we don't need to lock around the polling loop here since we're merely
reading a status register. We would only need to lock around accesses to
the control register to make sure two processes can't step on each
others's toes.
> > Would it be difficult to implement this for Tegra30 and Tegra114?
> No. But I have to check the detail in Tegra30 and Tegra114. And the biggest
> problem is I don't have the boards to test.
I can help with testing. Though that raises the question of how this can
be tested. It seems like this feature is used to make sure that all
outstanding memory requests from clients are flushed before resetting a
module. Typically Linux assumes that devices do that anyway, so if a
device is suspended or shut down, the corresponding function should
ensure that all outstanding transfers have been cancelled and are
flushed.

Also we've managed just fine so far, so I'm beginning to wonder whether
we actually need this feature on Linux. If not, how do we test that this
is indeed doing what it should? How to trigger a condition that requires
flushing and how do we determine that flushing actually fixes things?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20150106/3171b15d/attachment.sig>

Apparently Analagous Threads

Search for more seemingly similar threads

Nouveau - Jan 2015 - [PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory clients

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

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

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

Apparently Analagous Threads