Patchwork [V4,04/12] hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase

login
register
mail settings
Submitter Mitsyanko Igor
Date July 27, 2012, 7:29 p.m.
Message ID <1343417387-13953-5-git-send-email-i.mitsyanko@samsung.com>
Download mbox | patch
Permalink /patch/173772/
State New
Headers show

Comments

Mitsyanko Igor - July 27, 2012, 7:29 p.m.
Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
and CMD33, we have to account for this.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Markus Armbruster - July 31, 2012, 9:29 a.m.
Igor Mitsyanko <i.mitsyanko@samsung.com> writes:

> Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
> block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
> and CMD33, we have to account for this.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/sd.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index d0674d5..f7aa580 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
>          return;
>      }
>  
> -    start = sd_addr_to_wpnum(sd->erase_start);
> -    end = sd_addr_to_wpnum(sd->erase_end);
> +    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
> +            (uint64_t)sd->erase_start * 512 : sd->erase_start);
> +    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
> +            (uint64_t)sd->erase_end * 512 : sd->erase_end);
>      sd->erase_start = 0;
>      sd->erase_end = 0;
>      sd->csd[14] |= 0x40;

Is this a bug fix?

If yes, I recommend to state that clearly in the subject, say "hw/sd.c:
Fix erase for high capacity cards".
Mitsyanko Igor - July 31, 2012, 10:19 a.m.
On 07/31/2012 01:29 PM, Markus Armbruster wrote:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>
>> Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
>> block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
>> and CMD33, we have to account for this.
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>>   hw/sd.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index d0674d5..f7aa580 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
>>           return;
>>       }
>>   
>> -    start = sd_addr_to_wpnum(sd->erase_start);
>> -    end = sd_addr_to_wpnum(sd->erase_end);
>> +    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
>> +            (uint64_t)sd->erase_start * 512 : sd->erase_start);
>> +    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
>> +            (uint64_t)sd->erase_end * 512 : sd->erase_end);
>>       sd->erase_start = 0;
>>       sd->erase_end = 0;
>>       sd->csd[14] |= 0x40;
> Is this a bug fix?
>
> If yes, I recommend to state that clearly in the subject, say "hw/sd.c:
> Fix erase for high capacity cards".
>
Yes it is, I'll change subject in next version then.
Peter Maydell - July 31, 2012, 2:34 p.m.
On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
> block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
> and CMD33, we have to account for this.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/sd.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index d0674d5..f7aa580 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
>          return;
>      }
>
> -    start = sd_addr_to_wpnum(sd->erase_start);
> -    end = sd_addr_to_wpnum(sd->erase_end);
> +    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
> +            (uint64_t)sd->erase_start * 512 : sd->erase_start);
> +    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
> +            (uint64_t)sd->erase_end * 512 : sd->erase_end);

I think this would be a little clearer phrased as:

    uint64_t erase_start = sd->erase_start;
    uint64_t erase_end = sd->erase_end;

    [...]

    if (extract32(sd->ocr, 30, 1)) {
        /* High capacity memory card: erase units are 512 byte blocks */
        erase_start *= 512;
        erase_end *= 512;
    }

    start = sd_addr_to_wpnum(erase_start);
    end = sd_addr_to_wpnum(erase_end);

(I don't insist on the extract32() if you don't like it, but I
would like to avoid the repeated inline ?: ops.)

-- PMM
Mitsyanko Igor - July 31, 2012, 3:13 p.m.
On 07/31/2012 06:34 PM, Peter Maydell wrote:
> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
>> block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
>> and CMD33, we have to account for this.
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>>   hw/sd.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index d0674d5..f7aa580 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
>>           return;
>>       }
>>
>> -    start = sd_addr_to_wpnum(sd->erase_start);
>> -    end = sd_addr_to_wpnum(sd->erase_end);
>> +    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
>> +            (uint64_t)sd->erase_start * 512 : sd->erase_start);
>> +    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
>> +            (uint64_t)sd->erase_end * 512 : sd->erase_end);
>
> I think this would be a little clearer phrased as:
>
>      uint64_t erase_start = sd->erase_start;
>      uint64_t erase_end = sd->erase_end;
>
>      [...]
>
>      if (extract32(sd->ocr, 30, 1)) {
>          /* High capacity memory card: erase units are 512 byte blocks */
>          erase_start *= 512;
>          erase_end *= 512;
>      }
>
>      start = sd_addr_to_wpnum(erase_start);
>      end = sd_addr_to_wpnum(erase_end);
>
> (I don't insist on the extract32() if you don't like it, but I
> would like to avoid the repeated inline ?: ops.)
>
> -- PMM
>
>

I like it, I extensively use your new fancy extract/deposit api in my 
local work.

Patch

diff --git a/hw/sd.c b/hw/sd.c
index d0674d5..f7aa580 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -482,8 +482,10 @@  static void sd_erase(SDState *sd)
         return;
     }
 
-    start = sd_addr_to_wpnum(sd->erase_start);
-    end = sd_addr_to_wpnum(sd->erase_end);
+    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
+            (uint64_t)sd->erase_start * 512 : sd->erase_start);
+    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
+            (uint64_t)sd->erase_end * 512 : sd->erase_end);
     sd->erase_start = 0;
     sd->erase_end = 0;
     sd->csd[14] |= 0x40;