RE: [PATCH] ssi: xilinx_spips: Filter the non spi registers transactions

[Sai Pavan Boddu]

Hi Alistair,

> -----Original Message-----
> From: Alistair Francis <address@hidden>
> Sent: Thursday, October 17, 2019 4:39 AM
> To: Sai Pavan Boddu <address@hidden>
> Cc: Alistair Francis <address@hidden>; Peter Maydell
> <address@hidden>; address@hidden Developers <qemu-
> address@hidden>
> Subject: Re: [PATCH] ssi: xilinx_spips: Filter the non spi registers 
> transactions
> 
> On Sun, Oct 13, 2019 at 11:51 PM Sai Pavan Boddu
> <address@hidden> wrote:
> >
> > ZynqMP/Versal specific qspi registers should be handled inside
> > zynqmp_qspi_read/write calls. When few of these transactions are
> > handled by spi hooks we see state change in spi bus unexpectedly.
> >
> > Signed-off-by: Sai Pavan Boddu <address@hidden>
> > ---
> >  hw/ssi/xilinx_spips.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c index
> > a309c71..4f9f8e0 100644
> > --- a/hw/ssi/xilinx_spips.c
> > +++ b/hw/ssi/xilinx_spips.c
> > @@ -109,6 +109,7 @@
> >  #define R_GPIO              (0x30 / 4)
> >  #define R_LPBK_DLY_ADJ      (0x38 / 4)
> >  #define R_LPBK_DLY_ADJ_RESET (0x33)
> > +#define R_IOU_TAPDLY_BYPASS (0x3C / 4)
> >  #define R_TXD1              (0x80 / 4)
> >  #define R_TXD2              (0x84 / 4)
> >  #define R_TXD3              (0x88 / 4)
> > @@ -139,6 +140,8 @@
> >  #define R_LQSPI_STS         (0xA4 / 4)
> >  #define LQSPI_STS_WR_RECVD      (1 << 1)
> >
> > +#define R_DUMMY_CYCLE_EN    (0xC8 / 4)
> > +#define R_ECO               (0xF8 / 4)
> >  #define R_MOD_ID            (0xFC / 4)
> >
> >  #define R_GQSPI_SELECT          (0x144 / 4)
> > @@ -938,7 +941,16 @@ static uint64_t xlnx_zynqmp_qspips_read(void
> *opaque,
> >      int shortfall;
> >
> >      if (reg <= R_MOD_ID) {
> > -        return xilinx_spips_read(opaque, addr, size);
> > +        switch (addr) {
> > +        case R_GPIO:
> > +        case R_LPBK_DLY_ADJ:
> > +        case R_IOU_TAPDLY_BYPASS:
> > +        case R_DUMMY_CYCLE_EN:
> > +        case R_ECO:
> > +            return s->regs[addr / 4];
> > +        default:
> > +            return xilinx_spips_read(opaque, addr, size);
> 
> This doesn't seem right. This should have no functional change for the read
> function and has the consequence of not printing the memory accesses. If
> you try to debug this code now you won't see all of these operations in the
> log.
[Sai Pavan Boddu] Yeah reads do not have any issue. But I see your point of 
debug prints.
> 
> > +        }
> >      } else {
> >          switch (reg) {
> >          case R_GQSPI_RXD:
> > @@ -1063,7 +1075,17 @@ static void xlnx_zynqmp_qspips_write(void
> *opaque, hwaddr addr,
> >      uint32_t reg = addr / 4;
> >
> >      if (reg <= R_MOD_ID) {
> > -        xilinx_qspips_write(opaque, addr, value, size);
> > +        switch (reg) {
> > +        case R_GPIO:
> > +        case R_LPBK_DLY_ADJ:
> > +        case R_IOU_TAPDLY_BYPASS:
> > +        case R_DUMMY_CYCLE_EN:
> > +        case R_ECO:
> > +            s->regs[addr] = value;
> > +            break;
> > +        default:
> > +            xilinx_qspips_write(opaque, addr, value, size);
> > +        }
> 
> For the write code it looks like this skips the "no_reg_update" goto.
> Maybe that is the issue that you are seeing?
[Sai Pavan Boddu] yes, no_reg_update triggers update of cs lines.
We can also put a check there to skip no_reg_update when it’s a zynqmp qspi.
I will try that and send a V2.

Thanks
Sai Pavan
> 
> Alistair
> 
> >      } else {
> >          switch (reg) {
> >          case R_GQSPI_CNFG:
> > --
> > 2.7.4
> >
> >