Re: [PATCH 07/18] hw/sd: sd: Fix incorrect populated function switch status data structure

[Sai Pavan Boddu]

Hi Philippe,

First two patch of SD look good. Tested them over zynqmp and versal
platforms.

Thanks,
Sai Pavan
On Tue, Aug 18, 2020 at 04:30:15PM +0000, Sai Pavan Boddu wrote:
> Thanks Philippe,
> 
> I would test this and get back.
> 
> Regards,
> Sai Pavan
> 
> > -----Original Message-----
> > From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On
> > Behalf Of Philippe Mathieu-Daudé
> > Sent: Saturday, August 15, 2020 1:29 PM
> > To: Bin Meng <bmeng.cn@gmail.com>; Alistair Francis
> > <Alistair.Francis@wdc.com>; Bastian Koppelmann <kbastian@mail.uni-
> > paderborn.de>; Palmer Dabbelt <palmerdabbelt@google.com>; Sagar
> > Karandikar <sagark@eecs.berkeley.edu>; qemu-devel@nongnu.org; qemu-
> > riscv@nongnu.org; Sai Pavan Boddu <saipava@xilinx.com>
> > Cc: Bin Meng <bin.meng@windriver.com>; qemu-block@nongnu.org
> > Subject: Re: [PATCH 07/18] hw/sd: sd: Fix incorrect populated function 
> > switch
> > status data structure
> > 
> > +Sai Pavan
> > 
> > On 8/14/20 6:40 PM, Bin Meng wrote:
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > At present the function switch status data structure bit [399:376] are
> > > wrongly pupulated. These 3 bytes encode function switch status for the
> > > 6 function groups, with 4 bits per group, starting from function group
> > > 6 at bit 399, then followed by function group 5 at bit 395, and so on.
> > >
> > > However the codes mistakenly fills in the function group 1 status at
> > > bit 399. This fixes the code logic.
> > >
> > 
> > Fixes: a1bb27b1e9 ("SD card emulation (initial implementation)")
> > 
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > ---
> > >
> > >  hw/sd/sd.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > > index fad9cf1..51f5900 100644
> > > --- a/hw/sd/sd.c
> > > +++ b/hw/sd/sd.c
> > > @@ -806,11 +806,15 @@ static void sd_function_switch(SDState *sd,
> > uint32_t arg)
> > >      sd->data[11] = 0x43;
> > >      sd->data[12] = 0x80; /* Supported group 1 functions */
> > >      sd->data[13] = 0x03;
> > > +
> > > +    sd->data[14] = 0;
> > > +    sd->data[15] = 0;
> > > +    sd->data[16] = 0;
> > 
> > Pointless zero initialization.
> > 
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > 
> > I'll wait until next week in case Sai Pavan wants to test this patch (I 
> > don't have
> > access to the Xilinx images anymore) then I'll queue this via my sd-next 
> > tree.
> > 
> > Regards,
> > 
> > Phil.
> > 
> > >      for (i = 0; i < 6; i ++) {
> > >          new_func = (arg >> (i * 4)) & 0x0f;
> > >          if (mode && new_func != 0x0f)
> > >              sd->function_group[i] = new_func;
> > > -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
> > > +        sd->data[16 - (i >> 1)] |= new_func << ((i % 2) * 4);
> > >      }
> > >      memset(&sd->data[17], 0, 47);
> > >      stw_be_p(sd->data + 64, sd_crc16(sd->data, 64));
> > >
>