Hi Philippe, On Sun, Jan 17, 2021 at 12:12 AM Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > On 1/16/21 4:59 PM, Bin Meng wrote: > > Hi Philippe, > > > > On Sat, Jan 16, 2021 at 11:21 PM Philippe Mathieu-Daudé <f4...@amsat.org> > > wrote: > >> > >> Hi Bin, > >> > >> On 1/16/21 2:57 PM, Bin Meng wrote: > >>> On Fri, Jan 15, 2021 at 11:37 PM Philippe Mathieu-Daudé <f4...@amsat.org> > >>> wrote: > >>>> > >>>> When the block is disabled, only the ECSPI_CONREG register can > >>>> be modified. Setting the EN bit enabled the device, clearing it > >>> > >>> I don't know how this conclusion came out. The manual only says the > >>> following 2 registers ignore the write when the block is disabled. > >>> > >>> ECSPI_TXDATA, ECSPI_INTREG > >> > >> 21.4.5 Reset > >> > >> Whenever a device reset occurs, a reset is performed on the > >> ECSPI, resetting all registers to their default values. > >> > >> My understanding is it is pointless to update them when the > >> device is in reset, as they will get their default value when > >> going out of reset. > > > > I have a different understanding. When ECSPI_CONREG[EN] is cleared, > > it's like a hardware reset, and the ECSPI takes the following action: > > > > "Whenever a device reset occurs, a reset is performed on the > > ECSPI, resetting all registers to their default values." > > > > Chapter 21.4.5 Reset does not mention what's the hardware behavior > > afterwards. > > > > So my understanding is: afterwards, the software can still write to > > various registers, unless the register description tells us it's > > ignored. > > > >> > >>> > >>>> "disables the block and resets the internal logic with the > >>>> exception of the ECSPI_CONREG" register. > >>>> > >>>> Move the imx_spi_is_enabled() check earlier. > >>>> > >>>> Ref: i.MX 6DQ Applications Processor Reference Manual (IMX6DQRM), > >>>> chapter 21.7.3: Control Register (ECSPIx_CONREG) > >>>> > >>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> > >>>> --- > >>>> hw/ssi/imx_spi.c | 26 ++++++++++++++++---------- > >>>> 1 file changed, 16 insertions(+), 10 deletions(-) > >>>> > >>>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c > >>>> index ba7d3438d87..f06bbf317e2 100644 > >>>> --- a/hw/ssi/imx_spi.c > >>>> +++ b/hw/ssi/imx_spi.c > >>>> @@ -322,6 +322,21 @@ static void imx_spi_write(void *opaque, hwaddr > >>>> offset, uint64_t value, > >>>> DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index), > >>>> (uint32_t)value); > >>>> > >>>> + if (!imx_spi_is_enabled(s)) { > >>>> + /* Block is disabled */ > >>>> + if (index != ECSPI_CONREG) { > >>>> + /* Ignore access */ > >>>> + return; > >>>> + } > >>>> + s->regs[ECSPI_CONREG] = value; > >> > >> [*] > >> > >>>> + if (!(value & ECSPI_CONREG_EN)) { > >>>> + /* Keep disabled */ > >>> > >>> So other bits except ECSPI_CONREG_EN are discarded? The manual does > >>> not explicitly mention this but this looks suspicious. > >> > >> See in [*], all bits from the register are updated. We simply check > >> ECSPI_CONREG_EN to see if we need to go out of reset. > > > > Oops, I missed the [*] line. Now I have read this carefully, and found > > there is one problem: > > > > Now with the new logic the device reset activity has been postponed > > until next time a device register is written. This is wrong. > > Yes, I just realized that in the imx_spi_read() function. > > > > >> > >> See: > >> > >> 21.5 Initialization > >> > >> This section provides initialization information for ECSPI. > >> > >> To initialize the block: > >> > >> 1. Clear the EN bit in ECSPI_CONREG to reset the block. > >> 2. Enable the clocks for ECSPI. > >> 3. Set the EN bit in ECSPI_CONREG to put ECSPI out of reset. > >> 4. Configure corresponding IOMUX for ECSPI external signals. > >> 5 Configure registers of ECSPI properly according to the > >> specifications of the external SPI device. > >> > >> And ECSPI_CONREG_EN bit description: > >> > >> SPI Block Enable Control. This bit enables the ECSPI. This bit > >> must be set before writing to other registers or initiating an > >> exchange. Writing zero to this bit disables the block and resets > >> the internal logic with the exception of the ECSPI_CONREG. The > >> block's internal clocks are gated off whenever the block is > >> disabled. > >> > >> > >> I simply wanted to help you. I don't want to delay your work, so > >> if you think my approach is incorrect, suggest Peter to queue your > >> v5 or resend it (once riscv-next is merged) as v8. > > > > Thank you for the help. I mentioned in an earlier thread before, that > > my view was not to fix it until it's broken as the v5 series can > > satisfy my work. But since you pointed out various spec violation > > stuff related to device reset, I do think your findings make sense. So > > let's improve this model together. :) > > I'm not mad, just I'm doing too many things and I should rather review > your ssi-sd series. I don't have the physical hardware (neither know the > firmware using it) so it is a bit dumb of me to code blindly with no > possibility of testing. If you think this series is going the good way, > it would be great if you can give it another try, and I will be happy > to review.
Sure I will see if I can find a hardware to verify the register write behavior when ECSPI is disabled. Regards, Bin