It will help when moving this around for qtesting in the next commit.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/sd/sd.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 27a70896cd..06607115a7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
return shift_reg;
}
+enum {
+ F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
+ F136_CONTENT_LENGTH = 15,
+};
+
+static uint8_t sd_frame48_calc_checksum(const void *content)
+{
+ return sd_crc7(content, F48_CONTENT_LENGTH);
+}
+
+static uint8_t sd_frame136_calc_checksum(const void *content)
+{
+ return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
+}
+
#define OCR_POWER_DELAY_NS 500000 /* 0.5ms */
FIELD(OCR, VDD_VOLTAGE_WINDOW, 0, 24)
@@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
sd->cid[13] = 0x00 | /* Manufacture date (MDT) */
((MDT_YR - 2000) / 10);
sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
- sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
+ sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
}
#define HWBLOCK_SHIFT 9 /* 512 bytes */
@@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
sd->csd[13] = 0x40;
sd->csd[14] = 0x00;
}
- sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
+ sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
}
static void sd_set_rca(SDState *sd)
@@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
buffer[0] = 0x40 | req->cmd;
stl_be_p(&buffer[1], req->arg);
return 0;
- return sd_crc7(buffer, 5) != req->crc; /* TODO */
+ return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
}
static void sd_response_r1_make(SDState *sd, uint8_t *response)
--
2.17.0
On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> It will help when moving this around for qtesting in the next commit.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/sd/sd.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 27a70896cd..06607115a7 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
> return shift_reg;
> }
>
> +enum {
> + F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
> + F136_CONTENT_LENGTH = 15,
> +};
> +
> +static uint8_t sd_frame48_calc_checksum(const void *content)
> +{
> + return sd_crc7(content, F48_CONTENT_LENGTH);
> +}
> +
> +static uint8_t sd_frame136_calc_checksum(const void *content)
> +{
> + return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
> +}
> +
> #define OCR_POWER_DELAY_NS 500000 /* 0.5ms */
>
> FIELD(OCR, VDD_VOLTAGE_WINDOW, 0, 24)
> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
> sd->cid[13] = 0x00 | /* Manufacture date (MDT) */
> ((MDT_YR - 2000) / 10);
> sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
> - sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
> + sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
> }
>
> #define HWBLOCK_SHIFT 9 /* 512 bytes */
> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
> sd->csd[13] = 0x40;
> sd->csd[14] = 0x00;
> }
> - sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
> + sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
> }
>
> static void sd_set_rca(SDState *sd)
> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
> buffer[0] = 0x40 | req->cmd;
> stl_be_p(&buffer[1], req->arg);
> return 0;
> - return sd_crc7(buffer, 5) != req->crc; /* TODO */
> + return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
This 5 has changed to a 15. Is that on purpose? It should be mentioned
in the commit message if it is.
Alistair
> }
>
> static void sd_response_r1_make(SDState *sd, uint8_t *response)
> --
> 2.17.0
>
>
Hi Alistair,
On 05/09/2018 03:00 PM, Alistair Francis wrote:
> On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> It will help when moving this around for qtesting in the next commit.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/sd/sd.c | 21 ++++++++++++++++++---
>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 27a70896cd..06607115a7 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>> return shift_reg;
>> }
>>
>> +enum {
>> + F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
>> + F136_CONTENT_LENGTH = 15,
>> +};
>> +
>> +static uint8_t sd_frame48_calc_checksum(const void *content)
>> +{
>> + return sd_crc7(content, F48_CONTENT_LENGTH);
>> +}
>> +
>> +static uint8_t sd_frame136_calc_checksum(const void *content)
>> +{
>> + return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>> +}
>> +
>> #define OCR_POWER_DELAY_NS 500000 /* 0.5ms */
>>
>> FIELD(OCR, VDD_VOLTAGE_WINDOW, 0, 24)
>> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>> sd->cid[13] = 0x00 | /* Manufacture date (MDT) */
>> ((MDT_YR - 2000) / 10);
>> sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
>> - sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>> + sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>> }
>>
>> #define HWBLOCK_SHIFT 9 /* 512 bytes */
>> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>> sd->csd[13] = 0x40;
>> sd->csd[14] = 0x00;
>> }
>> - sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>> + sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>> }
>>
>> static void sd_set_rca(SDState *sd)
>> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>> buffer[0] = 0x40 | req->cmd;
>> stl_be_p(&buffer[1], req->arg);
>> return 0;
>> - return sd_crc7(buffer, 5) != req->crc; /* TODO */
>> + return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
>
> This 5 has changed to a 15. Is that on purpose? It should be mentioned
> in the commit message if it is.
I just extracted this function:
static uint8_t sd_frame48_calc_checksum(const void *content)
{
return sd_crc7(content, F48_CONTENT_LENGTH);
}
Having:
enum {
F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
So F48_CONTENT_LENGTH = 5 as previous.
This function is later verified with tests from patch 12 of this series.
>
> Alistair
>
>> }
>>
>> static void sd_response_r1_make(SDState *sd, uint8_t *response)
>> --
>> 2.17.0
>>
>>
On Wed, May 9, 2018 at 12:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Alistair,
>
> On 05/09/2018 03:00 PM, Alistair Francis wrote:
>> On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> It will help when moving this around for qtesting in the next commit.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> hw/sd/sd.c | 21 ++++++++++++++++++---
>>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 27a70896cd..06607115a7 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>>> return shift_reg;
>>> }
>>>
>>> +enum {
>>> + F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
>>> + F136_CONTENT_LENGTH = 15,
>>> +};
>>> +
>>> +static uint8_t sd_frame48_calc_checksum(const void *content)
>>> +{
>>> + return sd_crc7(content, F48_CONTENT_LENGTH);
>>> +}
>>> +
>>> +static uint8_t sd_frame136_calc_checksum(const void *content)
>>> +{
>>> + return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>>> +}
>>> +
>>> #define OCR_POWER_DELAY_NS 500000 /* 0.5ms */
>>>
>>> FIELD(OCR, VDD_VOLTAGE_WINDOW, 0, 24)
>>> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>>> sd->cid[13] = 0x00 | /* Manufacture date (MDT) */
>>> ((MDT_YR - 2000) / 10);
>>> sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
>>> - sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>>> + sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>>> }
>>>
>>> #define HWBLOCK_SHIFT 9 /* 512 bytes */
>>> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>> sd->csd[13] = 0x40;
>>> sd->csd[14] = 0x00;
>>> }
>>> - sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>>> + sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>>> }
>>>
>>> static void sd_set_rca(SDState *sd)
>>> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>>> buffer[0] = 0x40 | req->cmd;
>>> stl_be_p(&buffer[1], req->arg);
>>> return 0;
>>> - return sd_crc7(buffer, 5) != req->crc; /* TODO */
>>> + return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
>>
>> This 5 has changed to a 15. Is that on purpose? It should be mentioned
>> in the commit message if it is.
>
> I just extracted this function:
>
> static uint8_t sd_frame48_calc_checksum(const void *content)
> {
> return sd_crc7(content, F48_CONTENT_LENGTH);
> }
>
> Having:
>
> enum {
> F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
>
> So F48_CONTENT_LENGTH = 5 as previous.
Ah, I missed the '+ 4 '. I just stopped reading at the comment.
Looks good then:
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
>
> This function is later verified with tests from patch 12 of this series.
>
>>
>> Alistair
>>
>>> }
>>>
>>> static void sd_response_r1_make(SDState *sd, uint8_t *response)
>>> --
>>> 2.17.0
>>>
>>>
On 05/09/2018 08:04 PM, Alistair Francis wrote:
> On Wed, May 9, 2018 at 12:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Alistair,
>>
>> On 05/09/2018 03:00 PM, Alistair Francis wrote:
>>> On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> It will help when moving this around for qtesting in the next commit.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>> hw/sd/sd.c | 21 ++++++++++++++++++---
>>>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>> index 27a70896cd..06607115a7 100644
>>>> --- a/hw/sd/sd.c
>>>> +++ b/hw/sd/sd.c
>>>> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>>>> return shift_reg;
>>>> }
>>>>
>>>> +enum {
>>>> + F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
>>>> + F136_CONTENT_LENGTH = 15,
>>>> +};
>>>> +
>>>> +static uint8_t sd_frame48_calc_checksum(const void *content)
>>>> +{
>>>> + return sd_crc7(content, F48_CONTENT_LENGTH);
>>>> +}
>>>> +
>>>> +static uint8_t sd_frame136_calc_checksum(const void *content)
>>>> +{
>>>> + return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>>>> +}
>>>> +
>>>> #define OCR_POWER_DELAY_NS 500000 /* 0.5ms */
>>>>
>>>> FIELD(OCR, VDD_VOLTAGE_WINDOW, 0, 24)
>>>> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>>>> sd->cid[13] = 0x00 | /* Manufacture date (MDT) */
>>>> ((MDT_YR - 2000) / 10);
>>>> sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
>>>> - sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>>>> + sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>>>> }
>>>>
>>>> #define HWBLOCK_SHIFT 9 /* 512 bytes */
>>>> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>>> sd->csd[13] = 0x40;
>>>> sd->csd[14] = 0x00;
>>>> }
>>>> - sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>>>> + sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>>>> }
>>>>
>>>> static void sd_set_rca(SDState *sd)
>>>> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>>>> buffer[0] = 0x40 | req->cmd;
>>>> stl_be_p(&buffer[1], req->arg);
>>>> return 0;
>>>> - return sd_crc7(buffer, 5) != req->crc; /* TODO */
>>>> + return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
>>>
>>> This 5 has changed to a 15. Is that on purpose? It should be mentioned
>>> in the commit message if it is.
>>
>> I just extracted this function:
>>
>> static uint8_t sd_frame48_calc_checksum(const void *content)
>> {
>> return sd_crc7(content, F48_CONTENT_LENGTH);
>> }
>>
>> Having:
>>
>> enum {
>> F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
>>
>> So F48_CONTENT_LENGTH = 5 as previous.
>
> Ah, I missed the '+ 4 '. I just stopped reading at the comment.
This way looked clearer to me, but it might not be...
Would this be clearer?
F48_CONTENT_LENGTH = 1 + 4 /* command + argument */,
>
> Looks good then:
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Thanks for your review time :)
>
> Alistair
>
>>
>> This function is later verified with tests from patch 12 of this series.
>>
>>>
>>> Alistair
>>>
>>>> }
>>>>
>>>> static void sd_response_r1_make(SDState *sd, uint8_t *response)
>>>> --
>>>> 2.17.0
>>>>
>>>>
On Wed, May 9, 2018 at 5:16 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 05/09/2018 08:04 PM, Alistair Francis wrote:
>> On Wed, May 9, 2018 at 12:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Hi Alistair,
>>>
>>> On 05/09/2018 03:00 PM, Alistair Francis wrote:
>>>> On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>> It will help when moving this around for qtesting in the next commit.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>> hw/sd/sd.c | 21 ++++++++++++++++++---
>>>>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>> index 27a70896cd..06607115a7 100644
>>>>> --- a/hw/sd/sd.c
>>>>> +++ b/hw/sd/sd.c
>>>>> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>>>>> return shift_reg;
>>>>> }
>>>>>
>>>>> +enum {
>>>>> + F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
>>>>> + F136_CONTENT_LENGTH = 15,
>>>>> +};
>>>>> +
>>>>> +static uint8_t sd_frame48_calc_checksum(const void *content)
>>>>> +{
>>>>> + return sd_crc7(content, F48_CONTENT_LENGTH);
>>>>> +}
>>>>> +
>>>>> +static uint8_t sd_frame136_calc_checksum(const void *content)
>>>>> +{
>>>>> + return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>>>>> +}
>>>>> +
>>>>> #define OCR_POWER_DELAY_NS 500000 /* 0.5ms */
>>>>>
>>>>> FIELD(OCR, VDD_VOLTAGE_WINDOW, 0, 24)
>>>>> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>>>>> sd->cid[13] = 0x00 | /* Manufacture date (MDT) */
>>>>> ((MDT_YR - 2000) / 10);
>>>>> sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
>>>>> - sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>>>>> + sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>>>>> }
>>>>>
>>>>> #define HWBLOCK_SHIFT 9 /* 512 bytes */
>>>>> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>>>> sd->csd[13] = 0x40;
>>>>> sd->csd[14] = 0x00;
>>>>> }
>>>>> - sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>>>>> + sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>>>>> }
>>>>>
>>>>> static void sd_set_rca(SDState *sd)
>>>>> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>>>>> buffer[0] = 0x40 | req->cmd;
>>>>> stl_be_p(&buffer[1], req->arg);
>>>>> return 0;
>>>>> - return sd_crc7(buffer, 5) != req->crc; /* TODO */
>>>>> + return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
>>>>
>>>> This 5 has changed to a 15. Is that on purpose? It should be mentioned
>>>> in the commit message if it is.
>>>
>>> I just extracted this function:
>>>
>>> static uint8_t sd_frame48_calc_checksum(const void *content)
>>> {
>>> return sd_crc7(content, F48_CONTENT_LENGTH);
>>> }
>>>
>>> Having:
>>>
>>> enum {
>>> F48_CONTENT_LENGTH = 1 /* command */ + 4 /* argument */,
>>>
>>> So F48_CONTENT_LENGTH = 5 as previous.
>>
>> Ah, I missed the '+ 4 '. I just stopped reading at the comment.
>
> This way looked clearer to me, but it might not be...
> Would this be clearer?
>
> F48_CONTENT_LENGTH = 1 + 4 /* command + argument */,
I think this is clearer, but the way you have it now is fine as well.
>
>>
>> Looks good then:
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Thanks for your review time :)
No worries :)
Alistair
>
>>
>> Alistair
>>
>>>
>>> This function is later verified with tests from patch 12 of this series.
>>>
>>>>
>>>> Alistair
>>>>
>>>>> }
>>>>>
>>>>> static void sd_response_r1_make(SDState *sd, uint8_t *response)
>>>>> --
>>>>> 2.17.0
>>>>>
>>>>>
© 2016 - 2025 Red Hat, Inc.