Re: [Qemu-devel] [PATCH v3 10/12] sdcard: rename sd_set_$REG() functions called once as sd_reset_$REG()

[Philippe Mathieu-Daudé]

Hi Alistair,

On 01/31/2018 01:28 PM, Alistair Francis wrote:
> On Mon, Jan 22, 2018 at 7:21 PM, Philippe Mathieu-Daudé <address@hidden> 
> wrote:
>> All are only called once at reset.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> 
> I don't think this patch gets us anything, in the future someone might
> use the functions to set values and then this new name will be
> confusing. Can we drop this patch?

This is indeed the purpose of this patch, be explicit than these
functions are here to reset default values and not set anything (except
value at reset).

I personally prefer to use explicit self-documenting names to avoid such
confusions.

I'll drop it from this series and eventually reintroduce it in last part
where MMC functions do set values different than reset values.

> 
> Alistair
> 
>> ---
>>  hw/sd/sd.c | 31 ++++++++++++++++++-------------
>>  1 file changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index c46e9c2818..8b5022a7db 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -269,7 +269,7 @@ static uint16_t sd_crc16(void *message, size_t width)
>>      return shift_reg;
>>  }
>>
>> -static void sd_set_ocr(SDState *sd)
>> +static void sd_reset_ocr(SDState *sd)
>>  {
>>      /* All voltages OK, Standard Capacity SD Memory Card, not yet powered 
>> up */
>>      sd->ocr = 0x00ffff00;
>> @@ -285,7 +285,7 @@ static void sd_ocr_powerup(void *opaque)
>>      sd->ocr |= OCR_POWER_UP;
>>  }
>>
>> -static void sd_set_scr(SDState *sd)
>> +static void sd_reset_scr(SDState *sd)
>>  {
>>      sd->scr[0] = 0x00;         /* SCR Structure */
>>      sd->scr[1] = 0x2f;         /* SD Security Support */
>> @@ -304,7 +304,7 @@ static void sd_set_scr(SDState *sd)
>>  #define MDT_YR 2006
>>  #define MDT_MON        2
>>
>> -static void sd_set_cid(SDState *sd)
>> +static void sd_reset_cid(SDState *sd)
>>  {
>>      sd->cid[0] = MID;          /* Fake card manufacturer ID (MID) */
>>      sd->cid[1] = OID[0];       /* OEM/Application ID (OID) */
>> @@ -336,7 +336,7 @@ static const uint8_t sd_csd_rw_mask[16] = {
>>      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0xfe,
>>  };
>>
>> -static void sd_set_csd(SDState *sd, uint64_t size)
>> +static void sd_reset_csd(SDState *sd, uint64_t size)
>>  {
>>      uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1;
>>      uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1;
>> @@ -391,6 +391,11 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>      }
>>  }
>>
>> +static void sd_reset_rca(SDState *sd)
>> +{
>> +    sd->rca = 0;
>> +}
>> +
>>  static void sd_set_rca(SDState *sd)
>>  {
>>      sd->rca += 0x4567;

Here is an example where set_rsa() is not the same than reset_rsa().

>> @@ -405,12 +410,12 @@ static void sd_set_rca(SDState *sd)
>>  #define CARD_STATUS_B  0x00c01e00
>>  #define CARD_STATUS_C  0xfd39a028
>>
>> -static void sd_set_cardstatus(SDState *sd)
>> +static void sd_reset_cardstatus(SDState *sd)
>>  {
>>      sd->card_status = 0x00000100;
>>  }
>>
>> -static void sd_set_sdstatus(SDState *sd)
>> +static void sd_reset_sdstatus(SDState *sd)
>>  {
>>      memset(sd->sd_status, 0, 64);
>>  }
>> @@ -494,13 +499,13 @@ static void sd_reset(DeviceState *dev)
>>      sect = sd_addr_to_wpnum(size) + 1;
>>
>>      sd->state = sd_idle_state;
>> -    sd->rca = 0x0000;
>> -    sd_set_ocr(sd);
>> -    sd_set_scr(sd);
>> -    sd_set_cid(sd);
>> -    sd_set_csd(sd, size);
>> -    sd_set_cardstatus(sd);
>> -    sd_set_sdstatus(sd);
>> +    sd_reset_rca(sd);
>> +    sd_reset_ocr(sd);
>> +    sd_reset_scr(sd);
>> +    sd_reset_cid(sd);
>> +    sd_reset_csd(sd, size);
>> +    sd_reset_cardstatus(sd);
>> +    sd_reset_sdstatus(sd);
>>
>>      g_free(sd->wp_groups);
>>      sd->wp_switch = sd->blk ? blk_is_read_only(sd->blk) : false;
>> --
>> 2.15.1
>>
>>