Patchwork [U-Boot,9/9] mx23: Use PIO mode support for MMC

login
register
mail settings
Submitter Otavio Salvador
Date Jan. 22, 2013, 1:47 a.m.
Message ID <1358819250-31625-10-git-send-email-otavio@ossystems.com.br>
Download mbox | patch
Permalink /patch/214311/
State Not Applicable
Headers show

Comments

Otavio Salvador - Jan. 22, 2013, 1:47 a.m.
Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 drivers/mmc/mxsmmc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
Marek Vasut - Jan. 22, 2013, 1:50 a.m.
Dear Otavio Salvador,

NAK. DMA works on MX28 and works in Linux on MX23, therefore the DMA is not 
broken on either. Thus there's no reason to introduce such "force_pio" stuff.

Best regards,
Marek Vasut
Otavio Salvador - Jan. 22, 2013, 1:56 a.m.
On Mon, Jan 21, 2013 at 11:50 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Otavio Salvador,
>
> NAK. DMA works on MX28 and works in Linux on MX23, therefore the DMA is not
> broken on either. Thus there's no reason to introduce such "force_pio" stuff.

Lack of time to work on it is one reason. I won't be able to work on
it for some days and I prefer a working solution than skip it for
another release.

--
Otavio Salvador                             O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br
Marek Vasut - Jan. 22, 2013, 1:59 a.m.
Dear Otavio Salvador,

> On Mon, Jan 21, 2013 at 11:50 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Otavio Salvador,
> > 
> > NAK. DMA works on MX28 and works in Linux on MX23, therefore the DMA is
> > not broken on either. Thus there's no reason to introduce such
> > "force_pio" stuff.
> 
> Lack of time to work on it is one reason. I won't be able to work on
> it for some days and I prefer a working solution than skip it for
> another release.

The solution you propose adds undocumented feature and is not working, this is 
clearly a workaround.

Best regards,
Marek Vasut
Stefano Babic - Jan. 23, 2013, 9:04 a.m.
On 22/01/2013 02:56, Otavio Salvador wrote:
> On Mon, Jan 21, 2013 at 11:50 PM, Marek Vasut <marex@denx.de> wrote:
>> Dear Otavio Salvador,
>>
>> NAK. DMA works on MX28 and works in Linux on MX23, therefore the DMA is not
>> broken on either. Thus there's no reason to introduce such "force_pio" stuff.
> 
> Lack of time to work on it is one reason. I won't be able to work on
> it for some days and I prefer a working solution than skip it for
> another release.
> 
> --
> Otavio Salvador                             O.S. Systems
> E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
> Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br
> 

Hi Otavio, hi Marek,

let me say some words about this discussion. I agree that if there is no
support for DMA for MX23, we could add support with PIO mode. However,
there should be a very good reason, because as far as I understand,
added support for DMA is valid for mxs platform. The solution introduced
with this patchset lets also the door open for people that can use MX28
in PIO mode. I think (I have already seen in ML Marek's patchset for it)
the way to fix DMA for mx23 and then add support for MMC is the rught
way to do.

Best regards,
Stefano Babic
Otavio Salvador - Jan. 23, 2013, 11:05 a.m.
On Wed, Jan 23, 2013 at 7:04 AM, Stefano Babic <sbabic@denx.de> wrote:
> On 22/01/2013 02:56, Otavio Salvador wrote:
>> On Mon, Jan 21, 2013 at 11:50 PM, Marek Vasut <marex@denx.de> wrote:
>>> Dear Otavio Salvador,
>>>
>>> NAK. DMA works on MX28 and works in Linux on MX23, therefore the DMA is not
>>> broken on either. Thus there's no reason to introduce such "force_pio" stuff.
>>
>> Lack of time to work on it is one reason. I won't be able to work on
>> it for some days and I prefer a working solution than skip it for
>> another release.
>>
>> --
>> Otavio Salvador                             O.S. Systems
>> E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
>> Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br
>>
>
> Hi Otavio, hi Marek,
>
> let me say some words about this discussion. I agree that if there is no
> support for DMA for MX23, we could add support with PIO mode. However,
> there should be a very good reason, because as far as I understand,
> added support for DMA is valid for mxs platform. The solution introduced
> with this patchset lets also the door open for people that can use MX28
> in PIO mode. I think (I have already seen in ML Marek's patchset for it)
> the way to fix DMA for mx23 and then add support for MMC is the rught
> way to do.

So you'd like to have this patch ported to Marek's patchset so the
support for PIO mode is kept available? Instead of enabling it for
MX23 we can have a config option which allows force PIO mode and
disable DMA. This is what you want?

--
Otavio Salvador                             O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br
Marek Vasut - Jan. 23, 2013, 11:26 a.m.
Dear Otavio Salvador,

> On Wed, Jan 23, 2013 at 7:04 AM, Stefano Babic <sbabic@denx.de> wrote:
> > On 22/01/2013 02:56, Otavio Salvador wrote:
> >> On Mon, Jan 21, 2013 at 11:50 PM, Marek Vasut <marex@denx.de> wrote:
> >>> Dear Otavio Salvador,
> >>> 
> >>> NAK. DMA works on MX28 and works in Linux on MX23, therefore the DMA is
> >>> not broken on either. Thus there's no reason to introduce such
> >>> "force_pio" stuff.
> >> 
> >> Lack of time to work on it is one reason. I won't be able to work on
> >> it for some days and I prefer a working solution than skip it for
> >> another release.
> >> 
> >> --
> >> Otavio Salvador                             O.S. Systems
> >> E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
> >> Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br
> > 
> > Hi Otavio, hi Marek,
> > 
> > let me say some words about this discussion. I agree that if there is no
> > support for DMA for MX23, we could add support with PIO mode. However,
> > there should be a very good reason, because as far as I understand,
> > added support for DMA is valid for mxs platform. The solution introduced
> > with this patchset lets also the door open for people that can use MX28
> > in PIO mode. I think (I have already seen in ML Marek's patchset for it)
> > the way to fix DMA for mx23 and then add support for MMC is the rught
> > way to do.
> 
> So you'd like to have this patch ported to Marek's patchset so the
> support for PIO mode is kept available? Instead of enabling it for
> MX23 we can have a config option which allows force PIO mode and
> disable DMA. This is what you want?

Forcing PIO is nonsense, why would it be of any use?

Best regards,
Marek Vasut
Stefano Babic - Jan. 23, 2013, 11:39 a.m.
On 23/01/2013 12:26, Marek Vasut wrote:

>>> Hi Otavio, hi Marek,
>>>
>>> let me say some words about this discussion. I agree that if there is no
>>> support for DMA for MX23, we could add support with PIO mode. However,
>>> there should be a very good reason, because as far as I understand,
>>> added support for DMA is valid for mxs platform. The solution introduced
>>> with this patchset lets also the door open for people that can use MX28
>>> in PIO mode. I think (I have already seen in ML Marek's patchset for it)
>>> the way to fix DMA for mx23 and then add support for MMC is the rught
>>> way to do.
>>
>> So you'd like to have this patch ported to Marek's patchset so the
>> support for PIO mode is kept available? Instead of enabling it for
>> MX23 we can have a config option which allows force PIO mode and
>> disable DMA. This is what you want?
> 
> Forcing PIO is nonsense, why would it be of any use?

No, I agree with Marek - if DMA is then working also for MX23, we do not
need PIO at all.

Best regards,
Stefano Babic
Marek Vasut - Jan. 23, 2013, 11:48 a.m.
Dear Stefano Babic,

> On 23/01/2013 12:26, Marek Vasut wrote:
> >>> Hi Otavio, hi Marek,
> >>> 
> >>> let me say some words about this discussion. I agree that if there is
> >>> no support for DMA for MX23, we could add support with PIO mode.
> >>> However, there should be a very good reason, because as far as I
> >>> understand, added support for DMA is valid for mxs platform. The
> >>> solution introduced with this patchset lets also the door open for
> >>> people that can use MX28 in PIO mode. I think (I have already seen in
> >>> ML Marek's patchset for it) the way to fix DMA for mx23 and then add
> >>> support for MMC is the rught way to do.
> >> 
> >> So you'd like to have this patch ported to Marek's patchset so the
> >> support for PIO mode is kept available? Instead of enabling it for
> >> MX23 we can have a config option which allows force PIO mode and
> >> disable DMA. This is what you want?
> > 
> > Forcing PIO is nonsense, why would it be of any use?
> 
> No, I agree with Marek - if DMA is then working also for MX23, we do not
> need PIO at all.

Stefano, it's now using mixed mode -- PIO for small transfers, DMA for large 
transfers. That is the best pick of both worlds ;-)

Best regards,
Marek Vasut
Otavio Salvador - Jan. 23, 2013, 12:12 p.m.
On Wed, Jan 23, 2013 at 9:48 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Stefano Babic,
>
>> On 23/01/2013 12:26, Marek Vasut wrote:
>> >>> Hi Otavio, hi Marek,
>> >>>
>> >>> let me say some words about this discussion. I agree that if there is
>> >>> no support for DMA for MX23, we could add support with PIO mode.
>> >>> However, there should be a very good reason, because as far as I
>> >>> understand, added support for DMA is valid for mxs platform. The
>> >>> solution introduced with this patchset lets also the door open for
>> >>> people that can use MX28 in PIO mode. I think (I have already seen in
>> >>> ML Marek's patchset for it) the way to fix DMA for mx23 and then add
>> >>> support for MMC is the rught way to do.
>> >>
>> >> So you'd like to have this patch ported to Marek's patchset so the
>> >> support for PIO mode is kept available? Instead of enabling it for
>> >> MX23 we can have a config option which allows force PIO mode and
>> >> disable DMA. This is what you want?
>> >
>> > Forcing PIO is nonsense, why would it be of any use?
>>
>> No, I agree with Marek - if DMA is then working also for MX23, we do not
>> need PIO at all.
>
> Stefano, it's now using mixed mode -- PIO for small transfers, DMA for large
> transfers. That is the best pick of both worlds ;-)

That's fine; I just wanted to check if I had understood it right.

No problem; I will update the environments and send a v2 of my
patchset basing on top of Marek's changes.

--
Otavio Salvador                             O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br

Patch

diff --git a/drivers/mmc/mxsmmc.c b/drivers/mmc/mxsmmc.c
index 0a3f0c4..628d609 100644
--- a/drivers/mmc/mxsmmc.c
+++ b/drivers/mmc/mxsmmc.c
@@ -50,6 +50,7 @@  struct mxsmmc_priv {
 	uint32_t		buswidth;
 	int			(*mmc_is_wp)(int);
 	struct mxs_dma_desc	*desc;
+	int force_pio;
 };
 
 #define	MXSMMC_MAX_TIMEOUT	10000
@@ -184,7 +185,7 @@  mxsmmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 	if (cmd->resp_type & MMC_RSP_136)	/* It's a 136 bits response */
 		ctrl0 |= SSP_CTRL0_LONG_RESP;
 
-	if (data && (data->blocksize * data->blocks < MXSMMC_SMALL_TRANSFER))
+	if (priv->force_pio || (data && (data->blocksize * data->blocks < MXSMMC_SMALL_TRANSFER)))
 		writel(SSP_CTRL1_DMA_ENABLE, &ssp_regs->hw_ssp_ctrl1_clr);
 	else
 		writel(SSP_CTRL1_DMA_ENABLE, &ssp_regs->hw_ssp_ctrl1_set);
@@ -287,7 +288,7 @@  mxsmmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
 	if (!data)
 		return 0;
 
-	if (data->blocksize * data->blocks < MXSMMC_SMALL_TRANSFER) {
+	if (priv->force_pio || (data->blocksize * data->blocks < MXSMMC_SMALL_TRANSFER)) {
 		ret = mxsmmc_send_cmd_pio(priv, data);
 		if (ret) {
 			printf("MMC%d: Data timeout with command %d "
@@ -385,9 +386,11 @@  int mxsmmc_initialize(bd_t *bis, int id, int (*wp)(int))
 #if defined(CONFIG_MX23)
 	const unsigned int mxsmmc_max_id = 2;
 	const unsigned int mxsmmc_clk_id = 0;
+	const unsigned int mxsmmc_force_pio = 1;
 #elif defined(CONFIG_MX28)
 	const unsigned int mxsmmc_max_id = 4;
 	const unsigned int mxsmmc_clk_id = id;
+	const unsigned int mxsmmc_force_pio = 0;
 #endif
 
 	if (id >= mxsmmc_max_id)
@@ -417,6 +420,7 @@  int mxsmmc_initialize(bd_t *bis, int id, int (*wp)(int))
 	priv->mmc_is_wp = wp;
 	priv->id = id;
 	priv->regs = mxs_ssp_regs_by_bus(id);
+	priv->force_pio = mxsmmc_force_pio;
 
 	sprintf(mmc->name, "MXS MMC");
 	mmc->send_cmd = mxsmmc_send_cmd;