| Message ID | 1483979087-32663-12-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze: > When doing fast read, a certain amount of dummy bytes should be sent > before the read. This number is configurable in the controler CE0 > Control Register and needs to be modeled using fake transfers the > flash module. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > --- > hw/ssi/aspeed_smc.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > Changes since v1: > > - splitted user mode from command mode support. > > diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c > index 28d563a5800f..7a76d3344df2 100644 > --- a/hw/ssi/aspeed_smc.c > +++ b/hw/ssi/aspeed_smc.c > @@ -69,7 +69,9 @@ > #define R_CTRL0 (0x10 / 4) > #define CTRL_CMD_SHIFT 16 > #define CTRL_CMD_MASK 0xff > +#define CTRL_DUMMY_HIGH_SHIFT 14 > #define CTRL_AST2400_SPI_4BYTE (1 << 13) > +#define CTRL_DUMMY_LOW_SHIFT 6 /* 2 bits [7:6] */ > #define CTRL_CE_STOP_ACTIVE (1 << 2) > #define CTRL_CMD_MODE_MASK 0x3 > #define CTRL_READMODE 0x0 > @@ -141,6 +143,7 @@ > > /* Flash opcodes. */ > #define SPI_OP_READ 0x03 /* Read data bytes (low frequency) */ > +#define SPI_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */ > > /* > * Default segments mapping addresses and size for each slave per > @@ -490,10 +493,21 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl, > return addr; > } > > +static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl) > +{ > + const AspeedSMCState *s = fl->controller; > + uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id]; > + uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1; > + uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3; > + > + return ((dummy_high << 2) | dummy_low) * 8; > +} > + > static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr) > { > const AspeedSMCState *s = fl->controller; > uint8_t cmd = aspeed_smc_flash_cmd(fl); > + int i; > > /* Flash access can not exceed CS segment */ > addr = aspeed_smc_check_segment_addr(fl, addr); > @@ -506,6 +520,13 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr) > ssi_transfer(s->spi, (addr >> 16) & 0xff); > ssi_transfer(s->spi, (addr >> 8) & 0xff); > ssi_transfer(s->spi, (addr & 0xff)); > + > + /* Handle dummies in case of fast read */ > + if (cmd == SPI_OP_READ_FAST) { I feel this is wrong. It indicates that the controller knows FAST_READ op code. I believe controller always try to send dummy cycles, and do not do this when their count is set to 0 (so you do not need above if). Thanks, Marcin > + for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) { > + ssi_transfer(fl->controller->spi, 0xFF); > + } > + } > } > > static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
On 01/11/2017 07:20 PM, mar.krzeminski wrote: > W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze: >> When doing fast read, a certain amount of dummy bytes should be sent >> before the read. This number is configurable in the controler CE0 >> Control Register and needs to be modeled using fake transfers the >> flash module. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> Reviewed-by: Andrew Jeffery <andrew@aj.id.au> >> --- >> hw/ssi/aspeed_smc.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> Changes since v1: >> >> - splitted user mode from command mode support. >> >> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c >> index 28d563a5800f..7a76d3344df2 100644 >> --- a/hw/ssi/aspeed_smc.c >> +++ b/hw/ssi/aspeed_smc.c >> @@ -69,7 +69,9 @@ >> #define R_CTRL0 (0x10 / 4) >> #define CTRL_CMD_SHIFT 16 >> #define CTRL_CMD_MASK 0xff >> +#define CTRL_DUMMY_HIGH_SHIFT 14 >> #define CTRL_AST2400_SPI_4BYTE (1 << 13) >> +#define CTRL_DUMMY_LOW_SHIFT 6 /* 2 bits [7:6] */ >> #define CTRL_CE_STOP_ACTIVE (1 << 2) >> #define CTRL_CMD_MODE_MASK 0x3 >> #define CTRL_READMODE 0x0 >> @@ -141,6 +143,7 @@ >> >> /* Flash opcodes. */ >> #define SPI_OP_READ 0x03 /* Read data bytes (low frequency) */ >> +#define SPI_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */ >> >> /* >> * Default segments mapping addresses and size for each slave per >> @@ -490,10 +493,21 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl, >> return addr; >> } >> >> +static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl) >> +{ >> + const AspeedSMCState *s = fl->controller; >> + uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id]; >> + uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1; >> + uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3; >> + >> + return ((dummy_high << 2) | dummy_low) * 8; >> +} >> + >> static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr) >> { >> const AspeedSMCState *s = fl->controller; >> uint8_t cmd = aspeed_smc_flash_cmd(fl); >> + int i; >> >> /* Flash access can not exceed CS segment */ >> addr = aspeed_smc_check_segment_addr(fl, addr); >> @@ -506,6 +520,13 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr) >> ssi_transfer(s->spi, (addr >> 16) & 0xff); >> ssi_transfer(s->spi, (addr >> 8) & 0xff); >> ssi_transfer(s->spi, (addr & 0xff)); >> + >> + /* Handle dummies in case of fast read */ >> + if (cmd == SPI_OP_READ_FAST) { > I feel this is wrong. It indicates that the controller knows FAST_READ op code. you are right. I should not be testing the SPI OP command code but the controller mode : CTRL_FREADMODE > I believe controller always try to send dummy cycles, and do not do this when > their count is set to 0 (so you do not need above if). Indeed. I will check If I can just drop the test then. Thanks, C. > Thanks, > Marcin >> + for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) { >> + ssi_transfer(fl->controller->spi, 0xFF); >> + } >> + } >> } >> >> static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size) >
W dniu 11.01.2017 o 19:55, Cédric Le Goater pisze: > On 01/11/2017 07:20 PM, mar.krzeminski wrote: >> W dniu 09.01.2017 o 17:24, Cédric Le Goater pisze: >>> When doing fast read, a certain amount of dummy bytes should be sent >>> before the read. This number is configurable in the controler CE0 >>> Control Register and needs to be modeled using fake transfers the >>> flash module. >>> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au> >>> --- >>> hw/ssi/aspeed_smc.c | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >>> >>> Changes since v1: >>> >>> - splitted user mode from command mode support. >>> >>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c >>> index 28d563a5800f..7a76d3344df2 100644 >>> --- a/hw/ssi/aspeed_smc.c >>> +++ b/hw/ssi/aspeed_smc.c >>> @@ -69,7 +69,9 @@ >>> #define R_CTRL0 (0x10 / 4) >>> #define CTRL_CMD_SHIFT 16 >>> #define CTRL_CMD_MASK 0xff >>> +#define CTRL_DUMMY_HIGH_SHIFT 14 >>> #define CTRL_AST2400_SPI_4BYTE (1 << 13) >>> +#define CTRL_DUMMY_LOW_SHIFT 6 /* 2 bits [7:6] */ >>> #define CTRL_CE_STOP_ACTIVE (1 << 2) >>> #define CTRL_CMD_MODE_MASK 0x3 >>> #define CTRL_READMODE 0x0 >>> @@ -141,6 +143,7 @@ >>> >>> /* Flash opcodes. */ >>> #define SPI_OP_READ 0x03 /* Read data bytes (low frequency) */ >>> +#define SPI_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */ >>> >>> /* >>> * Default segments mapping addresses and size for each slave per >>> @@ -490,10 +493,21 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl, >>> return addr; >>> } >>> >>> +static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl) >>> +{ >>> + const AspeedSMCState *s = fl->controller; >>> + uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id]; >>> + uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1; >>> + uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3; >>> + >>> + return ((dummy_high << 2) | dummy_low) * 8; >>> +} >>> + >>> static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr) >>> { >>> const AspeedSMCState *s = fl->controller; >>> uint8_t cmd = aspeed_smc_flash_cmd(fl); >>> + int i; >>> >>> /* Flash access can not exceed CS segment */ >>> addr = aspeed_smc_check_segment_addr(fl, addr); >>> @@ -506,6 +520,13 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr) >>> ssi_transfer(s->spi, (addr >> 16) & 0xff); >>> ssi_transfer(s->spi, (addr >> 8) & 0xff); >>> ssi_transfer(s->spi, (addr & 0xff)); >>> + >>> + /* Handle dummies in case of fast read */ >>> + if (cmd == SPI_OP_READ_FAST) { >> I feel this is wrong. It indicates that the controller knows FAST_READ op code. > you are right. I should not be testing the SPI OP command code > but the controller mode : > > CTRL_FREADMODE > >> I believe controller always try to send dummy cycles, and do not do this when >> their count is set to 0 (so you do not need above if). > Indeed. I will check If I can just drop the test then. I did not notice that this function is also called in writes, isn't it? If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE needs to be tested. Thanks, Marcin > > Thanks, > > C. > >> Thanks, >> Marcin >>> + for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) { >>> + ssi_transfer(fl->controller->spi, 0xFF); >>> + } >>> + } >>> } >>> >>> static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size) >
> I did not notice that this function is also called in writes, isn't it? > If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE > needs to be tested. yes. I can take care of that in a follow up patchset for dummy support. Dummies in user mode is a bit painful to implement, as I had to snoop into the command flow to catch the fast read op. Not sure this is the right approach so I kept it for later. Did you have time to take look at the other patches adding Command mode and extending the tests ? I should have addressed your comments there. Thanks, C.
W dniu 16.01.2017 o 09:18, Cédric Le Goater pisze: >> I did not notice that this function is also called in writes, isn't it? >> If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE >> needs to be tested. > yes. I can take care of that in a follow up patchset for > dummy support. > > Dummies in user mode is a bit painful to implement, as I had > to snoop into the command flow to catch the fast read op. > Not sure this is the right approach so I kept it for later. Definitelly wrong, controller should not be aware of tha, fix is still on me :( > > > Did you have time to take look at the other patches adding > Command mode and extending the tests ? I should have addressed > your comments there. Yes, there is one more thing that could be important. It popped out in Sabrelite SPI model. The question is does SCM support different CS active (so device is active at CS high). Your code assume that SMC will always use CS LOW to activate device. If this is not true you might be interested in update this too. Thanks, Marcin > > Thanks, > > C. >
On 01/16/2017 07:58 PM, mar.krzeminski wrote: > > > W dniu 16.01.2017 o 09:18, Cédric Le Goater pisze: >>> I did not notice that this function is also called in writes, isn't it? >>> If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE >>> needs to be tested. >> yes. I can take care of that in a follow up patchset for >> dummy support. >> Dummies in user mode is a bit painful to implement, as I had >> to snoop into the command flow to catch the fast read op. >> Not sure this is the right approach so I kept it for later. > Definitelly wrong, controller should not be aware of tha, fix is still on me :( ok. np. I will send the support for command mode though, as this is a must have for booting. >> >> >> Did you have time to take look at the other patches adding >> Command mode and extending the tests ? I should have addressed >> your comments there. > Yes, there is one more thing that could be important. It popped out in Sabrelite > SPI model. The question is does SCM support different CS active (so device > is active at CS high). Your code assume that SMC will always use CS LOW to activate > device. If this is not true you might be interested in update this too. Aspeed SoC does not let you configure the chip select polarity (nor the clock phase ) So it is active low only. Thanks, C.
W dniu 17.01.2017 o 09:37, Cédric Le Goater pisze: > On 01/16/2017 07:58 PM, mar.krzeminski wrote: >> >> W dniu 16.01.2017 o 09:18, Cédric Le Goater pisze: >>>> I did not notice that this function is also called in writes, isn't it? >>>> If yes, dummy cycles are used only during reads so probably CTRL_FREADMODE >>>> needs to be tested. >>> yes. I can take care of that in a follow up patchset for >>> dummy support. >>> Dummies in user mode is a bit painful to implement, as I had >>> to snoop into the command flow to catch the fast read op. >>> Not sure this is the right approach so I kept it for later. >> Definitelly wrong, controller should not be aware of tha, fix is still on me :( > ok. np. I will send the support for command mode though, > as this is a must have for booting. > >>> >>> Did you have time to take look at the other patches adding >>> Command mode and extending the tests ? I should have addressed >>> your comments there. >> Yes, there is one more thing that could be important. It popped out in Sabrelite >> SPI model. The question is does SCM support different CS active (so device >> is active at CS high). Your code assume that SMC will always use CS LOW to activate >> device. If this is not true you might be interested in update this too. > Aspeed SoC does not let you configure the chip select polarity > (nor the clock phase ) So it is active low only. Thanks. > > Thanks, > > C. > >
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index 28d563a5800f..7a76d3344df2 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -69,7 +69,9 @@ #define R_CTRL0 (0x10 / 4) #define CTRL_CMD_SHIFT 16 #define CTRL_CMD_MASK 0xff +#define CTRL_DUMMY_HIGH_SHIFT 14 #define CTRL_AST2400_SPI_4BYTE (1 << 13) +#define CTRL_DUMMY_LOW_SHIFT 6 /* 2 bits [7:6] */ #define CTRL_CE_STOP_ACTIVE (1 << 2) #define CTRL_CMD_MODE_MASK 0x3 #define CTRL_READMODE 0x0 @@ -141,6 +143,7 @@ /* Flash opcodes. */ #define SPI_OP_READ 0x03 /* Read data bytes (low frequency) */ +#define SPI_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */ /* * Default segments mapping addresses and size for each slave per @@ -490,10 +493,21 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl, return addr; } +static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl) +{ + const AspeedSMCState *s = fl->controller; + uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id]; + uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1; + uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3; + + return ((dummy_high << 2) | dummy_low) * 8; +} + static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr) { const AspeedSMCState *s = fl->controller; uint8_t cmd = aspeed_smc_flash_cmd(fl); + int i; /* Flash access can not exceed CS segment */ addr = aspeed_smc_check_segment_addr(fl, addr); @@ -506,6 +520,13 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr) ssi_transfer(s->spi, (addr >> 16) & 0xff); ssi_transfer(s->spi, (addr >> 8) & 0xff); ssi_transfer(s->spi, (addr & 0xff)); + + /* Handle dummies in case of fast read */ + if (cmd == SPI_OP_READ_FAST) { + for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) { + ssi_transfer(fl->controller->spi, 0xFF); + } + } } static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)