diff mbox

[RFC,1/2,MMC-4.5] Disable emulation

Message ID 1336574901-26579-1-git-send-email-saugata.das@stericsson.com
State New
Headers show

Commit Message

Saugata Das May 9, 2012, 2:48 p.m. UTC
From: Saugata Das <saugata.das@linaro.org>

This patch adds the support for large sector size of 4KB by disabling emulation.
This patch passes eMMC DATA_SECTOR_SIZE as the logical block size during
mmc_blk_alloc_req.

In order to use this patch for 4KB sector size, ensure that USE_NATIVE_SECTOR
is enabled, partition table is 4KB sector size aligned and file system block
size is 4KB.

Signed-off-by: Saugata Das <saugata.das@linaro.org>
---
 drivers/mmc/card/block.c |    6 +++++-
 drivers/mmc/core/mmc.c   |    2 ++
 2 files changed, 7 insertions(+), 1 deletions(-)

Comments

Subhash Jadavani May 14, 2012, 10:25 a.m. UTC | #1
On 5/9/2012 8:18 PM, Saugata Das wrote:
> From: Saugata Das<saugata.das@linaro.org>
>
> This patch adds the support for large sector size of 4KB by disabling emulation.
> This patch passes eMMC DATA_SECTOR_SIZE as the logical block size during
> mmc_blk_alloc_req.
>
> In order to use this patch for 4KB sector size, ensure that USE_NATIVE_SECTOR
> is enabled, partition table is 4KB sector size aligned and file system block
> size is 4KB.
>
> Signed-off-by: Saugata Das<saugata.das@linaro.org>
> ---
>   drivers/mmc/card/block.c |    6 +++++-
>   drivers/mmc/core/mmc.c   |    2 ++
>   2 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index a7c75d8..0e54118e 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1517,7 +1517,11 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>   	snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
>   		 "mmcblk%d%s", md->name_idx, subname ? subname : "");
>
> -	blk_queue_logical_block_size(md->queue.queue, 512);
> +	if (mmc_card_mmc(card))
> +		blk_queue_logical_block_size(md->queue.queue,
> +			card->ext_csd.data_sector_size);
Shouldn't we also set the physical block size to NATIVE_SECTOR_SIZE value?

Other question,
Did you find any eMMC device which 4K native sector size? If yes, please 
share the test results.

Other than that, this patch looks good to me.

Regards,
Subhash
> +	else
> +		blk_queue_logical_block_size(md->queue.queue, 512);
>   	set_capacity(md->disk, size);
>
>   	if (mmc_host_cmd23(card->host)) {
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 02914d6..8dcbe995 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -533,6 +533,8 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
>   		} else {
>   			card->ext_csd.data_tag_unit_size = 0;
>   		}
> +	} else {
> +		card->ext_csd.data_sector_size = 512;
>   	}
>
>   out:
Saugata Das May 14, 2012, 2:51 p.m. UTC | #2
On 14 May 2012 15:55, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> On 5/9/2012 8:18 PM, Saugata Das wrote:
>>
>> From: Saugata Das<saugata.das@linaro.org>
>>
>> This patch adds the support for large sector size of 4KB by disabling
>> emulation.
>> This patch passes eMMC DATA_SECTOR_SIZE as the logical block size during
>> mmc_blk_alloc_req.
>>
>> In order to use this patch for 4KB sector size, ensure that
>> USE_NATIVE_SECTOR
>> is enabled, partition table is 4KB sector size aligned and file system
>> block
>> size is 4KB.
>>
>> Signed-off-by: Saugata Das<saugata.das@linaro.org>
>> ---
>>  drivers/mmc/card/block.c |    6 +++++-
>>  drivers/mmc/core/mmc.c   |    2 ++
>>  2 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index a7c75d8..0e54118e 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -1517,7 +1517,11 @@ static struct mmc_blk_data
>> *mmc_blk_alloc_req(struct mmc_card *card,
>>        snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
>>                 "mmcblk%d%s", md->name_idx, subname ? subname : "");
>>
>> -       blk_queue_logical_block_size(md->queue.queue, 512);
>> +       if (mmc_card_mmc(card))
>> +               blk_queue_logical_block_size(md->queue.queue,
>> +                       card->ext_csd.data_sector_size);
>
> Shouldn't we also set the physical block size to NATIVE_SECTOR_SIZE value?
>

The physical block size gets set from blk_queue_logical_block_size

> Other question,
> Did you find any eMMC device which 4K native sector size? If yes, please
> share the test results.
>

I have not yet seen the eMMC device implementing 4KB sector. But MMC
vendors are promising this feature, so I prepared this patch. So far,
I have tested by setting "data_sector_size" to 4KB (doing a hack in
mmc_read_ext_csd), configuring file system to have 4KB block size and
mounting it.

> Other than that, this patch looks good to me.
>
> Regards,
> Subhash
>
>> +       else
>> +               blk_queue_logical_block_size(md->queue.queue, 512);
>>        set_capacity(md->disk, size);
>>
>>        if (mmc_host_cmd23(card->host)) {
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 02914d6..8dcbe995 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -533,6 +533,8 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8
>> *ext_csd)
>>                } else {
>>                        card->ext_csd.data_tag_unit_size = 0;
>>                }
>> +       } else {
>> +               card->ext_csd.data_sector_size = 512;
>>        }
>>
>>  out:
>
>
Subhash Jadavani May 15, 2012, 6:40 a.m. UTC | #3
On 5/14/2012 8:21 PM, Saugata Das wrote:
> On 14 May 2012 15:55, Subhash Jadavani<subhashj@codeaurora.org>  wrote:
>> On 5/9/2012 8:18 PM, Saugata Das wrote:
>>> From: Saugata Das<saugata.das@linaro.org>
>>>
>>> This patch adds the support for large sector size of 4KB by disabling
>>> emulation.
>>> This patch passes eMMC DATA_SECTOR_SIZE as the logical block size during
>>> mmc_blk_alloc_req.
>>>
>>> In order to use this patch for 4KB sector size, ensure that
>>> USE_NATIVE_SECTOR
>>> is enabled, partition table is 4KB sector size aligned and file system
>>> block
>>> size is 4KB.
>>>
>>> Signed-off-by: Saugata Das<saugata.das@linaro.org>
>>> ---
>>>   drivers/mmc/card/block.c |    6 +++++-
>>>   drivers/mmc/core/mmc.c   |    2 ++
>>>   2 files changed, 7 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>> index a7c75d8..0e54118e 100644
>>> --- a/drivers/mmc/card/block.c
>>> +++ b/drivers/mmc/card/block.c
>>> @@ -1517,7 +1517,11 @@ static struct mmc_blk_data
>>> *mmc_blk_alloc_req(struct mmc_card *card,
>>>         snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
>>>                  "mmcblk%d%s", md->name_idx, subname ? subname : "");
>>>
>>> -       blk_queue_logical_block_size(md->queue.queue, 512);
>>> +       if (mmc_card_mmc(card))
>>> +               blk_queue_logical_block_size(md->queue.queue,
>>> +                       card->ext_csd.data_sector_size);
>> Shouldn't we also set the physical block size to NATIVE_SECTOR_SIZE value?
>>
> The physical block size gets set from blk_queue_logical_block_size
>
>> Other question,
>> Did you find any eMMC device which 4K native sector size? If yes, please
>> share the test results.
>>
> I have not yet seen the eMMC device implementing 4KB sector. But MMC
> vendors are promising this feature, so I prepared this patch. So far,
> I have tested by setting "data_sector_size" to 4KB (doing a hack in
> mmc_read_ext_csd), configuring file system to have 4KB block size and
> mounting it.
Ok. So basically you are emulating the 512B native sector device as 4K 
native sector device and always read/write this device in 4K 
granularity. So this is just a proof of concept that setting the block 
queue logical sector size to 4K works or not?
Are vendors promising any performance improvement if we run the device 
in 4K sector size mode rather than emulation mode?

I have few other doubts (from specification point of view) when the 
device starts running in native sector size mode:

1. According spec, "A large sector device shall not support partial 
access and shall not support reliable write mode EN_REL_WR=0."
         Shouldn't this patch make ensure this?

2.  Some internal sizes reported by the device may change after 
successfully disabling of the emulation mode.
         Shouldn't  we take of this as well?

3. After a successful disabling of the emulation mode, the content of 
the User Data Area is undefined.
         If the contents of the user data area is undefined after 
disabling the emulation mode, i don't think device will be able to boot 
to kernel after power cycle. So does this mean we have to flash the 
kernel/file system images
         again? Basically i don't see a use of disabling the emulation 
mode from kernel. It should be done by basic build/image flashing 
utility which could flash the images after setting the use_native_sector 
field and power cycle.

4. Data transfers on the bus are still using 512B CRC-protected blocks, 
but data shall only be transferred in multiple of 8 such blocks (always 
multiples of 4KB)
         So this means host controller still will be configured in 512B 
sector mode but amount of data will be in multple of 8 such 512B blocks?

5. Sector addressing is still used, but sector addresses shall always be 
aligned to 8 (4KB)
         How the sector addressing work here? will the sector address 
(at argument of read/write commands) of byte address 0x1000 be 1 or 8?

         512B sector addressing:
                 (byte address)         (sector address)
                 0x0000                => 0
                 0x0200                => 1
                 0x0400                => 2
                 0x0600                => 3
                 0x0800                => 4
                 0x0A00                => 5
                 0x0C00                => 6
                 0x0E00                => 7
                 0x1000                => 8

         4K sector addressing:
                 (byte address)         (sector address)
                 0x0000                => 0
                 0x0200                => NA
                 0x0400                => NA
                 0x0600                => NA
                 0x0800                => NA
                 0x0A00                => NA
                 0x0C00                => NA
                 0x0E00                => NA
                 0x1000                => 1

6. Arguments for read commands (CMD17/18) and write commands (CMD24/25) 
shall always be aligned to 8 (4KB)
         What will be meaning of the single block read now? Should it be 
use single 512B block or single 4KB block? If CMD17 and CMD24 now mean 
that access to 4KB memory area, don't you need to take of the same in MMC
         block driver?

7. what should be the block length set by CMD16 (SET_BLOCK_LENGTH) command?
         I would guess that in 4KB native sector size mode, this command 
is redundant and will be ignored by the card but it's no where 
explicitly mentioned in specification.

Regards,
Subhash

>
>> Other than that, this patch looks good to me.
>>
>> Regards,
>> Subhash
>>
>>> +       else
>>> +               blk_queue_logical_block_size(md->queue.queue, 512);
>>>         set_capacity(md->disk, size);
>>>
>>>         if (mmc_host_cmd23(card->host)) {
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 02914d6..8dcbe995 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -533,6 +533,8 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8
>>> *ext_csd)
>>>                 } else {
>>>                         card->ext_csd.data_tag_unit_size = 0;
>>>                 }
>>> +       } else {
>>> +               card->ext_csd.data_sector_size = 512;
>>>         }
>>>
>>>   out:
>>
> --
> 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
Luca Porzio (lporzio) May 15, 2012, 9:41 a.m. UTC | #4
Hallo Subhash,

One general comment: native sector size is mandatory (as of Jedec spec) 
for >256GB devices. For other devices native sector size is optional.
This feature is not meant for performance improvement. Anyway if you 
start using your device with 4K alignment, you should already see 
some performance improvements on currently available market devices.

Other Comments below.

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org]
> On Behalf Of Subhash Jadavani
> Sent: Monday, May 14, 2012 11:40 PM
> To: Saugata Das
> Cc: Saugata Das; linux-mmc@vger.kernel.org; patches@linaro.org
> Subject: Re: [RFC 1/2] [MMC-4.5] Disable emulation
> 
> On 5/14/2012 8:21 PM, Saugata Das wrote:
> > On 14 May 2012 15:55, Subhash Jadavani<subhashj@codeaurora.org>  wrote:
> >> On 5/9/2012 8:18 PM, Saugata Das wrote:
> >>> From: Saugata Das<saugata.das@linaro.org>
> >>>
> >>> This patch adds the support for large sector size of 4KB by disabling
> >>> emulation.
> >>> This patch passes eMMC DATA_SECTOR_SIZE as the logical block size during
> >>> mmc_blk_alloc_req.
> >>>
> >>> In order to use this patch for 4KB sector size, ensure that
> >>> USE_NATIVE_SECTOR
> >>> is enabled, partition table is 4KB sector size aligned and file system
> >>> block
> >>> size is 4KB.
> >>>
> >>> Signed-off-by: Saugata Das<saugata.das@linaro.org>
> >>> ---
> >>>   drivers/mmc/card/block.c |    6 +++++-
> >>>   drivers/mmc/core/mmc.c   |    2 ++
> >>>   2 files changed, 7 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> >>> index a7c75d8..0e54118e 100644
> >>> --- a/drivers/mmc/card/block.c
> >>> +++ b/drivers/mmc/card/block.c
> >>> @@ -1517,7 +1517,11 @@ static struct mmc_blk_data
> >>> *mmc_blk_alloc_req(struct mmc_card *card,
> >>>         snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
> >>>                  "mmcblk%d%s", md->name_idx, subname ? subname : "");
> >>>
> >>> -       blk_queue_logical_block_size(md->queue.queue, 512);
> >>> +       if (mmc_card_mmc(card))
> >>> +               blk_queue_logical_block_size(md->queue.queue,
> >>> +                       card->ext_csd.data_sector_size);
> >> Shouldn't we also set the physical block size to NATIVE_SECTOR_SIZE value?
> >>
> > The physical block size gets set from blk_queue_logical_block_size
> >
> >> Other question,
> >> Did you find any eMMC device which 4K native sector size? If yes, please
> >> share the test results.
> >>
> > I have not yet seen the eMMC device implementing 4KB sector. But MMC
> > vendors are promising this feature, so I prepared this patch. So far,
> > I have tested by setting "data_sector_size" to 4KB (doing a hack in
> > mmc_read_ext_csd), configuring file system to have 4KB block size and
> > mounting it.
> Ok. So basically you are emulating the 512B native sector device as 4K
> native sector device and always read/write this device in 4K
> granularity. So this is just a proof of concept that setting the block
> queue logical sector size to 4K works or not?
> Are vendors promising any performance improvement if we run the device
> in 4K sector size mode rather than emulation mode?
> 
> I have few other doubts (from specification point of view) when the
> device starts running in native sector size mode:
> 
> 1. According spec, "A large sector device shall not support partial
> access and shall not support reliable write mode EN_REL_WR=0."
>          Shouldn't this patch make ensure this?
> 

I agree. Reliable write shall be disabled for native sector size 
but also I would suggest for performances (reliable writes act
As per Jedec spec as the FUA bit so unless this is intended behavior
I would suggest to remove it)

> 2.  Some internal sizes reported by the device may change after
> successfully disabling of the emulation mode.
>          Shouldn't  we take of this as well?
> 
> 3. After a successful disabling of the emulation mode, the content of
> the User Data Area is undefined.
>          If the contents of the user data area is undefined after
> disabling the emulation mode, i don't think device will be able to boot
> to kernel after power cycle. So does this mean we have to flash the
> kernel/file system images
>          again? Basically i don't see a use of disabling the emulation
> mode from kernel. It should be done by basic build/image flashing
> utility which could flash the images after setting the use_native_sector
> field and power cycle.
> 

I agree that use_native_sector size and its settings is something that 
Basically needs to happen in factory or any kind of "controlled environment".

Nevertheless my understanding here is that Saugata provided only the support 
within the Kernel to recognize the native sector setting. In my understanding
the userspace tool is more like a reference code for factory or development 
than something to be used for production. 

As such I think Saugata patch is ok because lets the kernel be aware of it.

> 4. Data transfers on the bus are still using 512B CRC-protected blocks,
> but data shall only be transferred in multiple of 8 such blocks (always
> multiples of 4KB)
>          So this means host controller still will be configured in 512B
> sector mode but amount of data will be in multple of 8 such 512B blocks?
>

Yes. Basically in the attempt to not be disruptive with legacy host 
Controllers, CRC, sectors and addressing will all still be in terms of 512B
sectors.
Actually my understanding is that as long as you guarantee a 4K alignment, 
Nothing changes at all with or without enabling native sector size.
 
> 5. Sector addressing is still used, but sector addresses shall always be
> aligned to 8 (4KB)
>          How the sector addressing work here? will the sector address
> (at argument of read/write commands) of byte address 0x1000 be 1 or 8?
> 
>          512B sector addressing:
>                  (byte address)         (sector address)
>                  0x0000                => 0
>                  0x0200                => 1
>                  0x0400                => 2
>                  0x0600                => 3
>                  0x0800                => 4
>                  0x0A00                => 5
>                  0x0C00                => 6
>                  0x0E00                => 7
>                  0x1000                => 8
> 

This is right

>          4K sector addressing:
>                  (byte address)         (sector address)
>                  0x0000                => 0
>                  0x0200                => NA
>                  0x0400                => NA
>                  0x0600                => NA
>                  0x0800                => NA
>                  0x0A00                => NA
>                  0x0C00                => NA
>                  0x0E00                => NA
>                  0x1000                => 1
> 

This is wrong.
As above, addressing is still in 512B sectors to guarantee retro-compatibility.

> 6. Arguments for read commands (CMD17/18) and write commands (CMD24/25)
> shall always be aligned to 8 (4KB)
>          What will be meaning of the single block read now? Should it be
> use single 512B block or single 4KB block? If CMD17 and CMD24 now mean
> that access to 4KB memory area, don't you need to take of the same in MMC
>          block driver?
> 

Single block read/write as per jedec spec cannot be used anymore as such they 
should be disabled on those devices where native sector size has been enabled.

> 7. what should be the block length set by CMD16 (SET_BLOCK_LENGTH) command?
>          I would guess that in 4KB native sector size mode, this command
> is redundant and will be ignored by the card but it's no where
> explicitly mentioned in specification.
> 
> Regards,
> Subhash
> 
> >
> >> Other than that, this patch looks good to me.
> >>
> >> Regards,
> >> Subhash
> >>
> >>> +       else
> >>> +               blk_queue_logical_block_size(md->queue.queue, 512);
> >>>         set_capacity(md->disk, size);
> >>>
> >>>         if (mmc_host_cmd23(card->host)) {
> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> >>> index 02914d6..8dcbe995 100644
> >>> --- a/drivers/mmc/core/mmc.c
> >>> +++ b/drivers/mmc/core/mmc.c
> >>> @@ -533,6 +533,8 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8
> >>> *ext_csd)
> >>>                 } else {
> >>>                         card->ext_csd.data_tag_unit_size = 0;
> >>>                 }
> >>> +       } else {
> >>> +               card->ext_csd.data_sector_size = 512;
> >>>         }
> >>>
> >>>   out:
> >>
> > --
> > 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
Venkatraman S May 15, 2012, 10:17 a.m. UTC | #5
On Tue, May 15, 2012 at 12:10 PM, Subhash Jadavani
<subhashj@codeaurora.org> wrote:
> On 5/14/2012 8:21 PM, Saugata Das wrote:
>>
>> On 14 May 2012 15:55, Subhash Jadavani<subhashj@codeaurora.org>  wrote:
>>>
>>> On 5/9/2012 8:18 PM, Saugata Das wrote:
>>>>
>>>> From: Saugata Das<saugata.das@linaro.org>
>>>>
>>>> This patch adds the support for large sector size of 4KB by disabling
>>>> emulation.
>>>> This patch passes eMMC DATA_SECTOR_SIZE as the logical block size during
>>>> mmc_blk_alloc_req.
>>>>
>>>> In order to use this patch for 4KB sector size, ensure that
>>>> USE_NATIVE_SECTOR
>>>> is enabled, partition table is 4KB sector size aligned and file system
>>>> block
>>>> size is 4KB.
>>>>
>>>> Signed-off-by: Saugata Das<saugata.das@linaro.org>
>>>> ---
>>>>  drivers/mmc/card/block.c |    6 +++++-
>>>>  drivers/mmc/core/mmc.c   |    2 ++
>>>>  2 files changed, 7 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>> index a7c75d8..0e54118e 100644
>>>> --- a/drivers/mmc/card/block.c
>>>> +++ b/drivers/mmc/card/block.c
>>>> @@ -1517,7 +1517,11 @@ static struct mmc_blk_data
>>>> *mmc_blk_alloc_req(struct mmc_card *card,
>>>>        snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
>>>>                 "mmcblk%d%s", md->name_idx, subname ? subname : "");
>>>>
>>>> -       blk_queue_logical_block_size(md->queue.queue, 512);
>>>> +       if (mmc_card_mmc(card))
>>>> +               blk_queue_logical_block_size(md->queue.queue,
>>>> +                       card->ext_csd.data_sector_size);
>>>
>>> Shouldn't we also set the physical block size to NATIVE_SECTOR_SIZE
>>> value?
>>>
>> The physical block size gets set from blk_queue_logical_block_size
>>
>>> Other question,
>>> Did you find any eMMC device which 4K native sector size? If yes, please
>>> share the test results.
>>>
>> I have not yet seen the eMMC device implementing 4KB sector. But MMC
>> vendors are promising this feature, so I prepared this patch. So far,
>> I have tested by setting "data_sector_size" to 4KB (doing a hack in
>> mmc_read_ext_csd), configuring file system to have 4KB block size and
>> mounting it.
>
> Ok. So basically you are emulating the 512B native sector device as 4K
> native sector device and always read/write this device in 4K granularity. So
> this is just a proof of concept that setting the block queue logical sector
> size to 4K works or not?
> Are vendors promising any performance improvement if we run the device in 4K
> sector size mode rather than emulation mode?
>
> I have few other doubts (from specification point of view) when the device
> starts running in native sector size mode:
>
> 1. According spec, "A large sector device shall not support partial access
> and shall not support reliable write mode EN_REL_WR=0."
>        Shouldn't this patch make ensure this?
>
> 2.  Some internal sizes reported by the device may change after successfully
> disabling of the emulation mode.
>        Shouldn't  we take of this as well?
This would be because if a single 512byte sector was marked as bad
(unusable) initially, the entire
aligned 4K block would be marked as bad now after switching to 4K
sector size, resulting in a slight capacity shrinkage.
  This would only matter if the switch happens for a fairly old
device. Typically, the sector size change should happen during factory
initialization, so there wouldn't be too many bad blocks, so it
shouldn't be much of an issue now.
  Of course, this is just one aspect of a problem for the "dynamic
device capacity" feature of eMMC4.5, and can be solved if we get to
that..


>
> 3. After a successful disabling of the emulation mode, the content of the
> User Data Area is undefined.
>        If the contents of the user data area is undefined after disabling
> the emulation mode, i don't think device will be able to boot to kernel
> after power cycle. So does this mean we have to flash the kernel/file system
> images
>        again? Basically i don't see a use of disabling the emulation mode
> from kernel. It should be done by basic build/image flashing utility which
> could flash the images after setting the use_native_sector field and power
> cycle.
>
> 4. Data transfers on the bus are still using 512B CRC-protected blocks, but
> data shall only be transferred in multiple of 8 such blocks (always
> multiples of 4KB)
>        So this means host controller still will be configured in 512B sector
> mode but amount of data will be in multple of 8 such 512B blocks?
>
> 5. Sector addressing is still used, but sector addresses shall always be
> aligned to 8 (4KB)
>        How the sector addressing work here? will the sector address (at
> argument of read/write commands) of byte address 0x1000 be 1 or 8?
>
>        512B sector addressing:
>                (byte address)         (sector address)
>                0x0000                => 0
>                0x0200                => 1
>                0x0400                => 2
>                0x0600                => 3
>                0x0800                => 4
>                0x0A00                => 5
>                0x0C00                => 6
>                0x0E00                => 7
>                0x1000                => 8
>
>        4K sector addressing:
>                (byte address)         (sector address)
>                0x0000                => 0
>                0x0200                => NA
>                0x0400                => NA
>                0x0600                => NA
>                0x0800                => NA
>                0x0A00                => NA
>                0x0C00                => NA
>                0x0E00                => NA
>                0x1000                => 1
>
> 6. Arguments for read commands (CMD17/18) and write commands (CMD24/25)
> shall always be aligned to 8 (4KB)
>        What will be meaning of the single block read now? Should it be use
> single 512B block or single 4KB block? If CMD17 and CMD24 now mean that
> access to 4KB memory area, don't you need to take of the same in MMC
>        block driver?
>
> 7. what should be the block length set by CMD16 (SET_BLOCK_LENGTH) command?
>        I would guess that in 4KB native sector size mode, this command is
> redundant and will be ignored by the card but it's no where explicitly
> mentioned in specification.
>
> Regards,
> Subhash
>
>
>>
>>> Other than that, this patch looks good to me.
>>>
>>> Regards,
>>> Subhash
>>>
>>>> +       else
>>>> +               blk_queue_logical_block_size(md->queue.queue, 512);
>>>>        set_capacity(md->disk, size);
>>>>
>>>>        if (mmc_host_cmd23(card->host)) {
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index 02914d6..8dcbe995 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -533,6 +533,8 @@ static int mmc_read_ext_csd(struct mmc_card *card,
>>>> u8
>>>> *ext_csd)
>>>>                } else {
>>>>                        card->ext_csd.data_tag_unit_size = 0;
>>>>                }
>>>> +       } else {
>>>> +               card->ext_csd.data_sector_size = 512;
>>>>        }
>>>>
>>>>  out:
>>>
>>>
>> --
>> 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
Saugata Das May 15, 2012, 2:49 p.m. UTC | #6
On 15 May 2012 12:10, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> On 5/14/2012 8:21 PM, Saugata Das wrote:
>>
>> On 14 May 2012 15:55, Subhash Jadavani<subhashj@codeaurora.org>  wrote:
>>>
>>> On 5/9/2012 8:18 PM, Saugata Das wrote:
>>>>
>>>> From: Saugata Das<saugata.das@linaro.org>
>>>>
>>>> This patch adds the support for large sector size of 4KB by disabling
>>>> emulation.
>>>> This patch passes eMMC DATA_SECTOR_SIZE as the logical block size during
>>>> mmc_blk_alloc_req.
>>>>
>>>> In order to use this patch for 4KB sector size, ensure that
>>>> USE_NATIVE_SECTOR
>>>> is enabled, partition table is 4KB sector size aligned and file system
>>>> block
>>>> size is 4KB.
>>>>
>>>> Signed-off-by: Saugata Das<saugata.das@linaro.org>
>>>> ---
>>>>  drivers/mmc/card/block.c |    6 +++++-
>>>>  drivers/mmc/core/mmc.c   |    2 ++
>>>>  2 files changed, 7 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>> index a7c75d8..0e54118e 100644
>>>> --- a/drivers/mmc/card/block.c
>>>> +++ b/drivers/mmc/card/block.c
>>>> @@ -1517,7 +1517,11 @@ static struct mmc_blk_data
>>>> *mmc_blk_alloc_req(struct mmc_card *card,
>>>>        snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
>>>>                 "mmcblk%d%s", md->name_idx, subname ? subname : "");
>>>>
>>>> -       blk_queue_logical_block_size(md->queue.queue, 512);
>>>> +       if (mmc_card_mmc(card))
>>>> +               blk_queue_logical_block_size(md->queue.queue,
>>>> +                       card->ext_csd.data_sector_size);
>>>
>>> Shouldn't we also set the physical block size to NATIVE_SECTOR_SIZE
>>> value?
>>>
>> The physical block size gets set from blk_queue_logical_block_size
>>
>>> Other question,
>>> Did you find any eMMC device which 4K native sector size? If yes, please
>>> share the test results.
>>>
>> I have not yet seen the eMMC device implementing 4KB sector. But MMC
>> vendors are promising this feature, so I prepared this patch. So far,
>> I have tested by setting "data_sector_size" to 4KB (doing a hack in
>> mmc_read_ext_csd), configuring file system to have 4KB block size and
>> mounting it.
>
> Ok. So basically you are emulating the 512B native sector device as 4K
> native sector device and always read/write this device in 4K granularity. So
> this is just a proof of concept that setting the block queue logical sector
> size to 4K works or not?

Yes and it works. Note that, even the partition tables need to be 4KB aligned.

> Are vendors promising any performance improvement if we run the device in 4K
> sector size mode rather than emulation mode?
>

Atleast from the explanation in the spec on emulation mode, this
promises to provide some improvement. Some vendors may already have
some proprietary optimization to optimize access which are not 4KB
aligned. But lets target this 4KB alignment to have a good performance
across devices.

> I have few other doubts (from specification point of view) when the device
> starts running in native sector size mode:
>
> 1. According spec, "A large sector device shall not support partial access
> and shall not support reliable write mode EN_REL_WR=0."
>        Shouldn't this patch make ensure this?
>

Yes, May be, we should add a check in mmc-util i.e. set
USE_NATIVE_SECTOR only if EN_REL_WR = 1 .


> 2.  Some internal sizes reported by the device may change after successfully
> disabling of the emulation mode.
>        Shouldn't  we take of this as well?
>

Which parameter size will change ? I did not come across such remark
in the specification.


> 3. After a successful disabling of the emulation mode, the content of the
> User Data Area is undefined.
>        If the contents of the user data area is undefined after disabling
> the emulation mode, i don't think device will be able to boot to kernel
> after power cycle. So does this mean we have to flash the kernel/file system
> images
>        again? Basically i don't see a use of disabling the emulation mode
> from kernel. It should be done by basic build/image flashing utility which
> could flash the images after setting the use_native_sector field and power
> cycle.

Kernel is not disabling emulation mode. The mmc-util tool is doing it.
You are right, the flashing utility will disable the emulation mode
(mmc-util change can be used as a reference for the flashing utility).
Kernel (mmc driver) just needs to understand that the mode has changed
and notify file system accordingly (blk_queue_logical_block_size)

>
> 4. Data transfers on the bus are still using 512B CRC-protected blocks, but
> data shall only be transferred in multiple of 8 such blocks (always
> multiples of 4KB)
>        So this means host controller still will be configured in 512B sector
> mode but amount of data will be in multple of 8 such 512B blocks?
>

Yes, there is no host controller change.

> 5. Sector addressing is still used, but sector addresses shall always be
> aligned to 8 (4KB)
>        How the sector addressing work here? will the sector address (at
> argument of read/write commands) of byte address 0x1000 be 1 or 8?
>
>        512B sector addressing:
>                (byte address)         (sector address)
>                0x0000                => 0
>                0x0200                => 1
>                0x0400                => 2
>                0x0600                => 3
>                0x0800                => 4
>                0x0A00                => 5
>                0x0C00                => 6
>                0x0E00                => 7
>                0x1000                => 8
>

Yes, it continues to be 512B addressing.

>        4K sector addressing:
>                (byte address)         (sector address)
>                0x0000                => 0
>                0x0200                => NA
>                0x0400                => NA
>                0x0600                => NA
>                0x0800                => NA
>                0x0A00                => NA
>                0x0C00                => NA
>                0x0E00                => NA
>                0x1000                => 1
>
> 6. Arguments for read commands (CMD17/18) and write commands (CMD24/25)
> shall always be aligned to 8 (4KB)
>        What will be meaning of the single block read now? Should it be use
> single 512B block or single 4KB block? If CMD17 and CMD24 now mean that
> access to 4KB memory area, don't you need to take of the same in MMC
>        block driver?

Yes, single block read/write is not be allowed. From file system, we
should not have 512B read/write once we have indicated 4KB logical
sector size from blk_queue_logical_block_size.

I think we can have a check on this from mmc_blk_rw_rq_prep, where a
single block read/write request will be returned with,
__blk_end_request(req, -EIO, blk_rq_cur_bytes(req));

>
> 7. what should be the block length set by CMD16 (SET_BLOCK_LENGTH) command?
>        I would guess that in 4KB native sector size mode, this command is
> redundant and will be ignored by the card but it's no where explicitly
> mentioned in specification.
>

I guess, it should be multiple of 4KB ! I see that mmc_set_blocklen is
returning the error code to the caller. So, I see nothing to be done
there.


> Regards,
> Subhash
>
>>
>>> Other than that, this patch looks good to me.
>>>
>>> Regards,
>>> Subhash
>>>
>>>> +       else
>>>> +               blk_queue_logical_block_size(md->queue.queue, 512);
>>>>        set_capacity(md->disk, size);
>>>>
>>>>        if (mmc_host_cmd23(card->host)) {
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index 02914d6..8dcbe995 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -533,6 +533,8 @@ static int mmc_read_ext_csd(struct mmc_card *card,
>>>> u8
>>>> *ext_csd)
>>>>                } else {
>>>>                        card->ext_csd.data_tag_unit_size = 0;
>>>>                }
>>>> +       } else {
>>>> +               card->ext_csd.data_sector_size = 512;
>>>>        }
>>>>
>>>>  out:
>>>
>>>
>> --
>> 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
>
>
Saugata Das May 15, 2012, 2:55 p.m. UTC | #7
On 15 May 2012 15:47, S, Venkatraman <svenkatr@ti.com> wrote:
> On Tue, May 15, 2012 at 12:10 PM, Subhash Jadavani
> <subhashj@codeaurora.org> wrote:
>> On 5/14/2012 8:21 PM, Saugata Das wrote:
>>>
>>> On 14 May 2012 15:55, Subhash Jadavani<subhashj@codeaurora.org>  wrote:
>>>>
>>>> On 5/9/2012 8:18 PM, Saugata Das wrote:
>>>>>
>>>>> From: Saugata Das<saugata.das@linaro.org>
>>>>>
>>>>> This patch adds the support for large sector size of 4KB by disabling
>>>>> emulation.
>>>>> This patch passes eMMC DATA_SECTOR_SIZE as the logical block size during
>>>>> mmc_blk_alloc_req.
>>>>>
>>>>> In order to use this patch for 4KB sector size, ensure that
>>>>> USE_NATIVE_SECTOR
>>>>> is enabled, partition table is 4KB sector size aligned and file system
>>>>> block
>>>>> size is 4KB.
>>>>>
>>>>> Signed-off-by: Saugata Das<saugata.das@linaro.org>
>>>>> ---
>>>>>  drivers/mmc/card/block.c |    6 +++++-
>>>>>  drivers/mmc/core/mmc.c   |    2 ++
>>>>>  2 files changed, 7 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>> index a7c75d8..0e54118e 100644
>>>>> --- a/drivers/mmc/card/block.c
>>>>> +++ b/drivers/mmc/card/block.c
>>>>> @@ -1517,7 +1517,11 @@ static struct mmc_blk_data
>>>>> *mmc_blk_alloc_req(struct mmc_card *card,
>>>>>        snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
>>>>>                 "mmcblk%d%s", md->name_idx, subname ? subname : "");
>>>>>
>>>>> -       blk_queue_logical_block_size(md->queue.queue, 512);
>>>>> +       if (mmc_card_mmc(card))
>>>>> +               blk_queue_logical_block_size(md->queue.queue,
>>>>> +                       card->ext_csd.data_sector_size);
>>>>
>>>> Shouldn't we also set the physical block size to NATIVE_SECTOR_SIZE
>>>> value?
>>>>
>>> The physical block size gets set from blk_queue_logical_block_size
>>>
>>>> Other question,
>>>> Did you find any eMMC device which 4K native sector size? If yes, please
>>>> share the test results.
>>>>
>>> I have not yet seen the eMMC device implementing 4KB sector. But MMC
>>> vendors are promising this feature, so I prepared this patch. So far,
>>> I have tested by setting "data_sector_size" to 4KB (doing a hack in
>>> mmc_read_ext_csd), configuring file system to have 4KB block size and
>>> mounting it.
>>
>> Ok. So basically you are emulating the 512B native sector device as 4K
>> native sector device and always read/write this device in 4K granularity. So
>> this is just a proof of concept that setting the block queue logical sector
>> size to 4K works or not?
>> Are vendors promising any performance improvement if we run the device in 4K
>> sector size mode rather than emulation mode?
>>
>> I have few other doubts (from specification point of view) when the device
>> starts running in native sector size mode:
>>
>> 1. According spec, "A large sector device shall not support partial access
>> and shall not support reliable write mode EN_REL_WR=0."
>>        Shouldn't this patch make ensure this?
>>
>> 2.  Some internal sizes reported by the device may change after successfully
>> disabling of the emulation mode.
>>        Shouldn't  we take of this as well?
> This would be because if a single 512byte sector was marked as bad
> (unusable) initially, the entire
> aligned 4K block would be marked as bad now after switching to 4K
> sector size, resulting in a slight capacity shrinkage.
>  This would only matter if the switch happens for a fairly old
> device. Typically, the sector size change should happen during factory
> initialization, so there wouldn't be too many bad blocks, so it
> shouldn't be much of an issue now.
>  Of course, this is just one aspect of a problem for the "dynamic
> device capacity" feature of eMMC4.5, and can be solved if we get to
> that..
>

If 512byte sector becomes bad, then the complete NAND block (much
larger than 4KB) will be marked bad. As such, there should be no
difference in terms of amount of shrinkage. But yes, we need to start
thinking about "dynamic device capacity" once the number of bad NAND
blocks exceeds what eMMC can afford.

>
>>
>> 3. After a successful disabling of the emulation mode, the content of the
>> User Data Area is undefined.
>>        If the contents of the user data area is undefined after disabling
>> the emulation mode, i don't think device will be able to boot to kernel
>> after power cycle. So does this mean we have to flash the kernel/file system
>> images
>>        again? Basically i don't see a use of disabling the emulation mode
>> from kernel. It should be done by basic build/image flashing utility which
>> could flash the images after setting the use_native_sector field and power
>> cycle.
>>
>> 4. Data transfers on the bus are still using 512B CRC-protected blocks, but
>> data shall only be transferred in multiple of 8 such blocks (always
>> multiples of 4KB)
>>        So this means host controller still will be configured in 512B sector
>> mode but amount of data will be in multple of 8 such 512B blocks?
>>
>> 5. Sector addressing is still used, but sector addresses shall always be
>> aligned to 8 (4KB)
>>        How the sector addressing work here? will the sector address (at
>> argument of read/write commands) of byte address 0x1000 be 1 or 8?
>>
>>        512B sector addressing:
>>                (byte address)         (sector address)
>>                0x0000                => 0
>>                0x0200                => 1
>>                0x0400                => 2
>>                0x0600                => 3
>>                0x0800                => 4
>>                0x0A00                => 5
>>                0x0C00                => 6
>>                0x0E00                => 7
>>                0x1000                => 8
>>
>>        4K sector addressing:
>>                (byte address)         (sector address)
>>                0x0000                => 0
>>                0x0200                => NA
>>                0x0400                => NA
>>                0x0600                => NA
>>                0x0800                => NA
>>                0x0A00                => NA
>>                0x0C00                => NA
>>                0x0E00                => NA
>>                0x1000                => 1
>>
>> 6. Arguments for read commands (CMD17/18) and write commands (CMD24/25)
>> shall always be aligned to 8 (4KB)
>>        What will be meaning of the single block read now? Should it be use
>> single 512B block or single 4KB block? If CMD17 and CMD24 now mean that
>> access to 4KB memory area, don't you need to take of the same in MMC
>>        block driver?
>>
>> 7. what should be the block length set by CMD16 (SET_BLOCK_LENGTH) command?
>>        I would guess that in 4KB native sector size mode, this command is
>> redundant and will be ignored by the card but it's no where explicitly
>> mentioned in specification.
>>
>> Regards,
>> Subhash
>>
>>
>>>
>>>> Other than that, this patch looks good to me.
>>>>
>>>> Regards,
>>>> Subhash
>>>>
>>>>> +       else
>>>>> +               blk_queue_logical_block_size(md->queue.queue, 512);
>>>>>        set_capacity(md->disk, size);
>>>>>
>>>>>        if (mmc_host_cmd23(card->host)) {
>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>>> index 02914d6..8dcbe995 100644
>>>>> --- a/drivers/mmc/core/mmc.c
>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>> @@ -533,6 +533,8 @@ static int mmc_read_ext_csd(struct mmc_card *card,
>>>>> u8
>>>>> *ext_csd)
>>>>>                } else {
>>>>>                        card->ext_csd.data_tag_unit_size = 0;
>>>>>                }
>>>>> +       } else {
>>>>> +               card->ext_csd.data_sector_size = 512;
>>>>>        }
>>>>>
>>>>>  out:
>>>>
>>>>
>>> --
>>> 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
diff mbox

Patch

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index a7c75d8..0e54118e 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1517,7 +1517,11 @@  static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 	snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
 		 "mmcblk%d%s", md->name_idx, subname ? subname : "");
 
-	blk_queue_logical_block_size(md->queue.queue, 512);
+	if (mmc_card_mmc(card))
+		blk_queue_logical_block_size(md->queue.queue,
+			card->ext_csd.data_sector_size);
+	else
+		blk_queue_logical_block_size(md->queue.queue, 512);
 	set_capacity(md->disk, size);
 
 	if (mmc_host_cmd23(card->host)) {
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 02914d6..8dcbe995 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -533,6 +533,8 @@  static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
 		} else {
 			card->ext_csd.data_tag_unit_size = 0;
 		}
+	} else {
+		card->ext_csd.data_sector_size = 512;
 	}
 
 out: