[RFC] mmc: dw_mmc: Fix the max_blk_count in IDMAC

[RFC] mmc: dw_mmc: Fix the max_blk_count in IDMAC

[Seungwon Jeon]

Hi all,

Even though 1MB is reserved for descriptor table in IDMAC,
the dw_mmc host driver is allowed to receive only maximum
128KB block length in one request. This is caused by setting
improper max_blk_count. It needs to be e adjusted so that
descriptor table is used fully.

It is found that the performance is improved with the increased the
max_blk_count. In my case, there is 82%(6%) improvement in sequential
write(read). Is there anybody who knows the reason about the old setting?

Best regards,
Seungwon Jeon.

Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
---
 drivers/mmc/host/dw_mmc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index bf3c9b4..af2b901 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1804,9 +1804,9 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 #ifdef CONFIG_MMC_DW_IDMAC
 		mmc->max_segs = host->ring_size;
 		mmc->max_blk_size = 65536;
-		mmc->max_blk_count = host->ring_size;
 		mmc->max_seg_size = 0x1000;
-		mmc->max_req_size = mmc->max_seg_size * mmc->max_blk_count;
+		mmc->max_req_size = mmc->max_seg_size * host->ring_size;
+		mmc->max_blk_count = mmc->max_req_size / 512;
 #else
 		mmc->max_segs = 64;
 		mmc->max_blk_size = 65536; /* BLKSIZ is 16 bits */
-- 
1.7.0.4

Re: [RFC] mmc: dw_mmc: Fix the max_blk_count in IDMAC

[Will Newton]

2012/2/23 Seungwon Jeon <tgih.jun@samsung.com>:
> Hi all,
>
> Even though 1MB is reserved for descriptor table in IDMAC,
> the dw_mmc host driver is allowed to receive only maximum
> 128KB block length in one request. This is caused by setting
> improper max_blk_count. It needs to be e adjusted so that
> descriptor table is used fully.
>
> It is found that the performance is improved with the increased the
> max_blk_count. In my case, there is 82%(6%) improvement in sequential
> write(read). Is there anybody who knows the reason about the old setting?
>
> Best regards,
> Seungwon Jeon.
>
> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> ---
>  drivers/mmc/host/dw_mmc.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index bf3c9b4..af2b901 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1804,9 +1804,9 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>  #ifdef CONFIG_MMC_DW_IDMAC
>                mmc->max_segs = host->ring_size;
>                mmc->max_blk_size = 65536;
> -               mmc->max_blk_count = host->ring_size;
>                mmc->max_seg_size = 0x1000;
> -               mmc->max_req_size = mmc->max_seg_size * mmc->max_blk_count;
> +               mmc->max_req_size = mmc->max_seg_size * host->ring_size;
> +               mmc->max_blk_count = mmc->max_req_size / 512;

I am not sure this is correct, it seems like most drivers calculate
max_req_size like this:

mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;

And indeed in the external DMA case dw_mmc does that too.

It should probably be safe to set max_seg_size to max_req_size too.

>  #else
>                mmc->max_segs = 64;
>                mmc->max_blk_size = 65536; /* BLKSIZ is 16 bits */
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC] mmc: dw_mmc: Fix the max_blk_count in IDMAC

[Seungwon Jeon]

Will Newton <will.newton@gmail.com> wrote:
> 2012/2/23 Seungwon Jeon <tgih.jun@samsung.com>:
> > Hi all,
> >
> > Even though 1MB is reserved for descriptor table in IDMAC,
> > the dw_mmc host driver is allowed to receive only maximum
> > 128KB block length in one request. This is caused by setting
> > improper max_blk_count. It needs to be e adjusted so that
> > descriptor table is used fully.
> >
> > It is found that the performance is improved with the increased the
> > max_blk_count. In my case, there is 82%(6%) improvement in sequential
> > write(read). Is there anybody who knows the reason about the old setting?
> >
> > Best regards,
> > Seungwon Jeon.
> >
> > Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> > ---
> >  drivers/mmc/host/dw_mmc.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index bf3c9b4..af2b901 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -1804,9 +1804,9 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> >  #ifdef CONFIG_MMC_DW_IDMAC
> >                mmc->max_segs = host->ring_size;
> >                mmc->max_blk_size = 65536;
> > -               mmc->max_blk_count = host->ring_size;
> >                mmc->max_seg_size = 0x1000;
> > -               mmc->max_req_size = mmc->max_seg_size * mmc->max_blk_count;
> > +               mmc->max_req_size = mmc->max_seg_size * host->ring_size;
> > +               mmc->max_blk_count = mmc->max_req_size / 512;
> 
> I am not sure this is correct, it seems like most drivers calculate
> max_req_size like this:
> 
> mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;

Could you explain the following origin code in dw_mmc?
We need to change like most other drivers?
  mmc->max_req_size = mmc->max_seg_size * mmc->max_blk_count;
  - mmc->max_blk_count is set as host->ring_size.
  - max_seg_size seems to represent max_blk_size.

I think max_req_size means the total transfer size through DMA descriptors.
max_blk_size just is limited to 16 bits for BLKSIZ register. Actually 512 is used.
Host driver doesn't inform block layer of this size.
Also, block layer doesn't utilize max_blk_size.

Thanks,
Seungwon Jeon.
> 
> And indeed in the external DMA case dw_mmc does that too.
> 
> It should probably be safe to set max_seg_size to max_req_size too.
> 

> >  #else
> >                mmc->max_segs = 64;
> >                mmc->max_blk_size = 65536; /* BLKSIZ is 16 bits */
> > --
> > 1.7.0.4
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] mmc: dw_mmc: Fix the max_blk_count in IDMAC

[Will Newton]

On Thu, Feb 23, 2012 at 11:58 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> Will Newton <will.newton@gmail.com> wrote:
>> 2012/2/23 Seungwon Jeon <tgih.jun@samsung.com>:
>> > Hi all,
>> >
>> > Even though 1MB is reserved for descriptor table in IDMAC,
>> > the dw_mmc host driver is allowed to receive only maximum
>> > 128KB block length in one request. This is caused by setting
>> > improper max_blk_count. It needs to be e adjusted so that
>> > descriptor table is used fully.
>> >
>> > It is found that the performance is improved with the increased the
>> > max_blk_count. In my case, there is 82%(6%) improvement in sequential
>> > write(read). Is there anybody who knows the reason about the old setting?
>> >
>> > Best regards,
>> > Seungwon Jeon.
>> >
>> > Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
>> > ---
>> >  drivers/mmc/host/dw_mmc.c |    4 ++--
>> >  1 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> > index bf3c9b4..af2b901 100644
>> > --- a/drivers/mmc/host/dw_mmc.c
>> > +++ b/drivers/mmc/host/dw_mmc.c
>> > @@ -1804,9 +1804,9 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>> >  #ifdef CONFIG_MMC_DW_IDMAC
>> >                mmc->max_segs = host->ring_size;
>> >                mmc->max_blk_size = 65536;
>> > -               mmc->max_blk_count = host->ring_size;
>> >                mmc->max_seg_size = 0x1000;
>> > -               mmc->max_req_size = mmc->max_seg_size * mmc->max_blk_count;
>> > +               mmc->max_req_size = mmc->max_seg_size * host->ring_size;
>> > +               mmc->max_blk_count = mmc->max_req_size / 512;
>>
>> I am not sure this is correct, it seems like most drivers calculate
>> max_req_size like this:
>>
>> mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
>
> Could you explain the following origin code in dw_mmc?
> We need to change like most other drivers?
>  mmc->max_req_size = mmc->max_seg_size * mmc->max_blk_count;

All the other drivers seem to set max_req_size based on blk_size and
blk_count not seg_size so the current code looks like it may be wrong.

>  - mmc->max_blk_count is set as host->ring_size.
>  - max_seg_size seems to represent max_blk_size.

I believe max_seg_size should be set to a higher value as it appears
to be only relevant for IOMMU-based drivers, possibly the same as
max_req_size.

RE: [RFC] mmc: dw_mmc: Fix the max_blk_count in IDMAC

[Seungwon Jeon]

Will Newton <will.newton@gmail.com> wrote:
> On Thu, Feb 23, 2012 at 11:58 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> > Will Newton <will.newton@gmail.com> wrote:
> >> 2012/2/23 Seungwon Jeon <tgih.jun@samsung.com>:
> >> > Hi all,
> >> >
> >> > Even though 1MB is reserved for descriptor table in IDMAC,
> >> > the dw_mmc host driver is allowed to receive only maximum
> >> > 128KB block length in one request. This is caused by setting
> >> > improper max_blk_count. It needs to be e adjusted so that
> >> > descriptor table is used fully.
> >> >
> >> > It is found that the performance is improved with the increased the
> >> > max_blk_count. In my case, there is 82%(6%) improvement in sequential
> >> > write(read). Is there anybody who knows the reason about the old setting?
> >> >
> >> > Best regards,
> >> > Seungwon Jeon.
> >> >
> >> > Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> >> > ---
> >> >  drivers/mmc/host/dw_mmc.c |    4 ++--
> >> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >> > index bf3c9b4..af2b901 100644
> >> > --- a/drivers/mmc/host/dw_mmc.c
> >> > +++ b/drivers/mmc/host/dw_mmc.c
> >> > @@ -1804,9 +1804,9 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> >> >  #ifdef CONFIG_MMC_DW_IDMAC
> >> >                mmc->max_segs = host->ring_size;
> >> >                mmc->max_blk_size = 65536;
> >> > -               mmc->max_blk_count = host->ring_size;
> >> >                mmc->max_seg_size = 0x1000;
> >> > -               mmc->max_req_size = mmc->max_seg_size * mmc->max_blk_count;
> >> > +               mmc->max_req_size = mmc->max_seg_size * host->ring_size;
> >> > +               mmc->max_blk_count = mmc->max_req_size / 512;
> >>
> >> I am not sure this is correct, it seems like most drivers calculate
> >> max_req_size like this:
> >>
> >> mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
> >
> > Could you explain the following origin code in dw_mmc?
> > We need to change like most other drivers?
> >  mmc->max_req_size = mmc->max_seg_size * mmc->max_blk_count;
> 
> All the other drivers seem to set max_req_size based on blk_size and
> blk_count not seg_size so the current code looks like it may be wrong.
Then, How can determine the blk_count?
max_req_size is a limitation which should be submitted to mmc stack.
dw_mmc can transfer the data as max_req_size with the DMA descriptors.
So I think max_seg_size and ring_size are good information for max_req_size in IDMAC
and current setting of mmc->max_req_size is fine.

> 
> >  - mmc->max_blk_count is set as host->ring_size.
> >  - max_seg_size seems to represent max_blk_size.
> 
> I believe max_seg_size should be set to a higher value as it appears
> to be only relevant for IOMMU-based drivers, possibly the same as
> max_req_size.
As we can refer kernel/block, several segments of max_seg_size make up
a request ,which has hardware sectors in the usual 512b unit.
Max hardware sectors can be offered to block layer like the following in drivers/mmc/card/queue.c.

	blk_queue_max_hw_sectors(mq->queue, min(host->max_blk_count, host->max_req_size / 512));

That means "host->max_blk_count" or "host->max_req_size / 512" decides the max request size.
So we need to set the proper max-blk_count. This RFC patch is intended for that.

Thanks,
Seungwon Jeon.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html