Upgrading litedram cause issue "TypeError: unsupported operand type(s) for /: 'NoneType' and 'int'"

[mithro]

- make firmware check (atlys base lm32 firmware)
---------------------------------------------
mkdir -p build/atlys_base_lm32/
time python -u ./make.py --platform=atlys --target=base --cpu-type=lm32 --iprange=192.168.100     --no-compile-gateware \
		2>&1 | tee -a build/atlys_base_lm32//output.20181001-223522.log; (exit ${PIPESTATUS[0]})
Traceback (most recent call last):
  File "./make.py", line 164, in <module>
    main()
  File "./make.py", line 123, in main
    soc = get_soc(args, platform)
  File "./make.py", line 57, in get_soc
    soc = SoC(platform, ident=SoC.__name__, **soc_sdram_argdict(args), **dict(args.target_option))
  File "/home/travis/build/mithro/litex-buildenv/targets/atlys/base.py", line 233, in __init__
    controller_settings=controller_settings)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litex/litex/soc/integration/soc_sdram.py", line 60, in register_sdram
    phy, geom_settings, timing_settings, **kwargs)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litex/litex/soc/integration/soc_sdram.py", line 27, in __init__
    phy.settings, geom_settings, timing_settings, **kwargs)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litedram/litedram/core/controller.py", line 66, in __init__
    settings)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litedram/litedram/core/bankmachine.py", line 86, in __init__
    write_latency = math.ceil(settings.phy.cwl / settings.phy.nphases)
TypeError: unsupported operand type(s) for /: 'NoneType' and 'int'

[mithro]

litedram changed from ea1ac4d to 41a8a24

     * litedram changed from ea1ac4d to 41a8a24
        * 41a8a24 - modules: express tFAW in ns <Florent Kermarrec>
        * 7062068 - modules: split DDR3 in 2 categories: Chips and SO-DIMMs <Florent Kermarrec>
        * 0f46dc4 - modules: add DDR3-800 timings for MT41J128M16 and use it on arty example <Florent Kermarrec>
        * 426ae23 - examples/litedram_gen: add sdram_module_speedgrade parameter <Florent Kermarrec>
        * 1bc016c - test: add test_examples <Florent Kermarrec>
        * f7f8169 - test: update downconverter/upconverter <Florent Kermarrec>
        * 8de1d91 - core: add with_bank paramteter to NativePort (cause issues on adaptation is bank is always exposed) <Florent Kermarrec>
        *   70516c4 - Merge branch 'master' of https://github.com/enjoy-digital/litedram <>
        |\
        | * 5820970 - frontend/crossbar: fix #49 <Florent Kermarrec>
        * | 71f78d9 - Fix reordering controller rejecting all commands <>
        * | 8f14211 - Account for CWL in write to read timing <>
        |/
        * 5fb8afe - frontend/axi: omit bank in rdata connect <Florent Kermarrec>
        *   06ca53d - Merge pull request #48 from enjoy-digital/staging <enjoy-digital>
        |\
        | *   5a4d063 - Merge branch 'master' into staging <enjoy-digital>
        | |\
        | |/
        |/|
        * | 5984eaa - core: change api for out-of-order. (with_reordering passed to controller and not ports). <Florent Kermarrec>
        * | 6e10dae - core/bankmachine/write to precharge: indicate that AL=0 <Florent Kermarrec>
        * |   869c8ee - Merge pull request #46 from enjoy-digital/WritePrechargeFix <enjoy-digital>
        |\ \
        | * | 0405f41 - Update the write-to-precharge timings so it works with 1:2 <>
        * | | 30c32f5 - example/litedram_gen: simplify clocking with new S7PLL module, a lot easier :) <Florent Kermarrec>
        |/ /
        * | 2a3cacb - core/bankmachine: minor cleanup on trc/tras <Florent Kermarrec>
        * |   42ccf05 - Merge pull request #45 from enjoy-digital/tRAS_FIX <enjoy-digital>
        |\ \
        | * | 79b1421 - Auto precharge is too pessimistic, it will wait on its own for a valid time to execute <John Sully>
        | * | 177d739 - Implement tRAS <John Sully>
        * | |   5902027 - Merge pull request #44 from enjoy-digital/tRC_Fix <enjoy-digital>
        |\ \ \
        | |/ /
        | * | 5f6b857 - This adds support for tRC timing parameters <John Sully>
        |/ /
        * |   1777720 - Merge pull request #42 from enjoy-digital/HalfRateSequentialFix <enjoy-digital>
        |\ \
        | * | 06c8c2a - The actual fix <John Sully>
        | * | e22580f - remove unnecessary file <John Sully>
        | * | c028786 - Fix overflow bug from code review <John Sully>
        | * | 8447d69 - We wait an extra cycle for no reason <John Sully>
        |/ /
        | *   04aa04d - Merge pull request #43 from enjoy-digital/EfficencyFixes <enjoy-digital>
        | |\
        |/ /
        | * c4bd842 - Fix many bugs <John Sully>
        | * fa0f3b2 - Use the ready signal for cas_allowed so that arbitrators know not to iterate <John Sully>
        |/
        * c12404e - README: Add ECC <Florent Kermarrec>
        * 3f4c14b - frontend/ecc: expose incident bits, change clear register name <Florent Kermarrec>
        * b9aadf1 - frontend/axi: remove write buffer reservation (not needed) <Florent Kermarrec>

[JohnSully]

settings.phy.cwl is None. Rather unusual. Which Phy are you using? SDR/DDR/DDR2/DDR3?

[JohnSully]

The issue is in this file:
https://github.com/enjoy-digital/litedram/blob/ea1ac4d6d72ecb9a65fb884857db8ba6851f3230/litedram/phy/s6ddrphy.py

For DDR2 CWL is not set, and indeed it is not a timing parameter for DDR2. I'm researching if its safe to keep it at 0 or if some other calculation is required.

GitHub

enjoy-digital/litedram

Small footprint and configurable DRAM core. Contribute to enjoy-digital/litedram development by creating an account on GitHub.

[mithro]

Yeap, the Atlys has DDR2 ram.

[JohnSully]

For DDR2 CWL=CL. The correct fix is to assign it to the same value. Probably by default in the Phy settings class.

[mithro]

Do you want to submit a fix to litedram for us? :-P

[JohnSully]

Done.

[mithro]

Testing in #78 which should include enjoy-digital/litedram@69eaf84

[mithro]

Now getting;

mkdir -p build/atlys_base_lm32/
time python -u ./make.py --platform=atlys --target=base --cpu-type=lm32 --iprange=192.168.100     --no-compile-gateware \
		2>&1 | tee -a build/atlys_base_lm32//output.20181001-235411.log; (exit ${PIPESTATUS[0]})
Traceback (most recent call last):
  File "./make.py", line 164, in <module>
    main()
  File "./make.py", line 123, in main
    soc = get_soc(args, platform)
  File "./make.py", line 57, in get_soc
    soc = SoC(platform, ident=SoC.__name__, **soc_sdram_argdict(args), **dict(args.target_option))
  File "/home/travis/build/mithro/litex-buildenv/targets/atlys/base.py", line 233, in __init__
    controller_settings=controller_settings)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litex/litex/soc/integration/soc_sdram.py", line 60, in register_sdram
    phy, geom_settings, timing_settings, **kwargs)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litex/litex/soc/integration/soc_sdram.py", line 27, in __init__
    phy.settings, geom_settings, timing_settings, **kwargs)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litedram/litedram/core/controller.py", line 66, in __init__
    settings)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litedram/litedram/core/bankmachine.py", line 87, in __init__
    precharge_time = write_latency + settings.timing.tWR - 1 + settings.timing.tCCD # AL=0
TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'

[mithro]

settings.timing.tCCD seems to be the None value (the other two are 2).

[JohnSully]

tCCD should be (2, None) in modules.py. I should be able to check in a fix soon.

…

On Mon, Oct 1, 2018 at 9:07 PM Tim Ansell ***@***.***> wrote: settings.timing.tCCD seems to be the None value (the other two are 2). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADtuAiPMo0x2k22HeiDNsydJl7siCijyks5ugrxjgaJpZM4XDBV8> .

[mithro]

The following patch seems to fix things;

diff --git a/litedram/modules.py b/litedram/modules.py
index 62cef5d..4a1feee 100644
--- a/litedram/modules.py
+++ b/litedram/modules.py
@@ -194,6 +194,7 @@ class MT47H128M8(SDRAMModule):
     # speedgrade invariant timings
     tREFI = 64e6/8192
     tWTR = (None, 7.5)
+    tCCD  = (4, None)
     # speedgrade related timings
     tRP   = 15
     tRCD  = 15
@@ -210,6 +211,7 @@ class MT47H64M16(SDRAMModule):
     # speedgrade invariant timings
     tREFI = 64e6/8192
     tWTR = (None, 7.5)
+    tCCD  = (4, None)
     # speedgrade related timings
     tRP   = 15
     tRCD  = 15
@@ -227,6 +229,7 @@ class P3R1GE4JGF(SDRAMModule):
     # speedgrade invariant timings
     tREFI = 64e6/8192
     tWTR = (None, 7.5)
+    tCCD  = (4, None)
     # speedgrade related timings
     tRP   = 12.5
     tRCD  = 12.5

[mithro]

Fix committed here -> enjoy-digital/litedram@eddce76

[JohnSully]

For DDR2 tCCD is 2 bus cycles per the data sheet. Not 4. You can leave it at 4 but bandwidth will be cut in half.

…

On Mon, Oct 1, 2018 at 9:53 PM Tim Ansell ***@***.***> wrote: Fix committed here -> ***@***.*** <enjoy-digital/litedram@eddce76> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADtuApz1bU9VGnq0CyyyALwFUAlCBUF_ks5ugsb-gaJpZM4XDBV8> .

[mithro]

Would this be needed for LPDDR, DDR and SDR modules too?

[mithro]

Nothing to see here... It was (2, None) all the time....

enjoy-digital/litedram@6c7a804

[mithro]

Hopefully fixed in #78 now...

[JohnSully]

For ddr and sdr tCCD is 1 cycle.

LPDDR depends on the module. It’s burst length/2.

[mithro]

Created enjoy-digital/litedram#51 to remind you to fix it for LPDDR and the others

[mithro]

- make firmware check (mimasv2 base lm32 firmware)
---------------------------------------------
mkdir -p build/mimasv2_base_lm32/
time python -u ./make.py --platform=mimasv2 --target=base --cpu-type=lm32 --iprange=192.168.100     --no-compile-gateware \
		2>&1 | tee -a build/mimasv2_base_lm32//output.20181002-050444.log; (exit ${PIPESTATUS[0]})
Traceback (most recent call last):
  File "./make.py", line 164, in <module>
    main()
  File "./make.py", line 123, in main
    soc = get_soc(args, platform)
  File "./make.py", line 57, in get_soc
    soc = SoC(platform, ident=SoC.__name__, **soc_sdram_argdict(args), **dict(args.target_option))
  File "/home/travis/build/mithro/litex-buildenv/targets/mimasv2/base.py", line 241, in __init__
    controller_settings=controller_settings)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litex/litex/soc/integration/soc_sdram.py", line 60, in register_sdram
    phy, geom_settings, timing_settings, **kwargs)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litex/litex/soc/integration/soc_sdram.py", line 27, in __init__
    phy.settings, geom_settings, timing_settings, **kwargs)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litedram/litedram/core/controller.py", line 66, in __init__
    settings)
  File "/home/travis/build/mithro/litex-buildenv/third_party/litedram/litedram/core/bankmachine.py", line 87, in __init__
    precharge_time = write_latency + settings.timing.tWR - 1 + settings.timing.tCCD # AL=0
TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'

[enjoy-digital]

If should be solved now I'll make sure we run LiteX's tests before integrating changes in the future.

[mithro]

This has been fixed!