amaranth-farm / amlib

assorted library of utility cores for amaranth HDL
Other
81 stars 14 forks source link

SPI issue with CPOL=1 CPHS=1 #15

Open darkstar007 opened 1 year ago

darkstar007 commented 1 year ago

Hi, I'm using this to talk to the ADC on a ULX3S dev board - and it doesn't seem to work. My hand crafted SPI module does work. The issue is that the comms returning from the ADC are garbage - not sure if that's because tx or rx or both are being mangled. My suspicion is that its caused by the sub-SPI-clk glitch that can be seen before and after the CS line transitions.

I noticed this on my logic analyser (the values from which agree with the values the fpga returns), and they can be seen in the attached test script.

import math

from amaranth import *
from amaranth.sim import *
from amlib.io import SPIControllerInterface

if __name__ == '__main__':
    word_size = 16
    divisor = 4
    clock_polarity = 1
    clock_phase = 1
    msb_first = True
    cs_idles_high = True

    spi = SPIControllerInterface(word_size=word_size, divisor=divisor,
                                 clock_polarity=clock_polarity, clock_phase=clock_phase,
                                 msb_first=msb_first, cs_idles_high=cs_idles_high)

    m = Module()
    m.submodules.spi = spi

    sim = Simulator(m)

    def process():
        yield spi.word_out.eq(0x1234)
        yield
        yield spi.start_transfer.eq(1)
        for x in range(4*16+12):
            yield

    sim.add_sync_process(process)
    sim.add_clock(1.0/50e6)

    with sim.write_vcd("test.vcd"):
        sim.run()

glitch

hansfbaier commented 1 year ago

It would need a MultiReg to make it work out of the box, I'll add that tomorrow.

hansfbaier commented 1 year ago

@darkstar007 I just pushed a fix and the SPI unit tests still pass. So please give it a try.

darkstar007 commented 1 year ago

Hi - thanks @hansfbaier

Unfortunately that doesn't appear to quite work - although it has improved things. If I look at the values that my Siglent logic analyser thinks are being sent they are off by a factor of two (i.e. I send 0x0040 and the LA says that's 0x0020). My suspicion is that the glitch (arrowed) at the start of the transmission is making an extra zero in the MOSI payload (taken on low-high transition) - i.e. its taking bits 0:15 rather than 1:16. As the ADC isn't happy, its not echoing back commands - you have to turn that on with software - so its hard to give much to say about MISO at the moment.

It you want further debug let me know.

The pictures are from my test script above - but look very similar to what's observed on my LA.

Thanks, Matt glitch2 glitch2_markup

hansfbaier commented 1 year ago

Ah I see. OK, will look into this. You also might be able to fix this in principle, if you look at the source code.

darkstar007 commented 1 year ago

OK - but this is my first time touching amaranth so its going to take a while. (Also, pretty much my first dealings with FPGAs......).

hansfbaier commented 1 year ago

Yes SPI is actually a great first project, because it is one of the most simple protocols.

darkstar007 commented 1 year ago

Yes, SPI is a very simple protocol - the fact that spi.py is already over 1000 lines of code raises alarm bells with me.

One of the errors is with the sync domain SPIControllerInterface when CPOL=1 requires a complete clock cycle to occur before it will start to output. According to the 'documentation': "Signals in synchronous control domains change whenever a specific transition (positive or negative edge) occurs on the clock of the synchronous domain." - but I can't see how the sync domain decides what signal is going to be its clock. I also can't find out how to make it change when it make an assignment - so that changing

       if self.clock_phase == 0:
            with m.If(Rose(chip_selected, domain="sync")):
                output_one_bit(m)

to be something like

       if self.clock_phase == 0:
            with m.If(Rose(chip_selected, domain="sync")):
                output_one_bit(m)
       else:
            with m.If(Rose(chip_selected, domain="sync")):
                 output_one_bit_on_negative_edge(m))

to try and force the value of sdo to be the correct value before the incoming positive edge transition of the clock.

So, yeah...........