Add SAM E70 support #5366

eamaclean · 2022-03-19T18:25:44Z

This adds support for the SAME70Q20B used on the Duet 3 6HC.

Lightly tested by users in Discord, but a blind port without personal hardware.

Currently PWM1 is not supported as hard PWM support seems little used, and to my knowledge PWM and TC are both untested.

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

KevinOConnor · 2022-03-20T15:41:38Z

Thanks. At a high-level, it looks good to me.

I did notice a couple of minor things:

Is the deprecated/ folder needed in lib/atsame70b/include/component directory?
I find the same70_compat.h header file to be confusing. I think it would be preferable to place the defines in the c files that need them.
If hard_pwm.c hasn't been tested, then I think we should separate out those changes to a separate PR. (In the interim, use select HAVE_GPIO_HARD_PWM if !MACH_SAME70Q in Kconfig).
It would help if the users that have tested the patch could add some comments here.

Not a blocker for merging, but I also noticed this code adds several #if checks. In general it's preferable to reduce the number of compile time definitions. I think a number of places could reduce these definitions (eg, by using regular c if (CONFIG_XXX) statements, by combining existing #if CONFIG_XXX blocks, by converting gpio.c digital_regs to a struct that stores both the regs and id, etc.).

-Kevin

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

eamaclean · 2022-03-21T20:31:56Z

1. Is the deprecated/ folder needed in lib/atsame70b/include/component directory?

Unfortunately yes, several deprecated defines are used by the USB driver and the SAM3 headers do not contain the new style.

2. I find the same70_compat.h header file to be confusing.  I think it would be preferable to place the defines in the c files that need them.

I felt that resulted in excessive cluttering (over 50 lines of it for USB, for example), but I can do so if you're certain that's better.

3. If hard_pwm.c hasn't been tested, then I think we should separate out those changes to a separate PR.  (In the interim, use `select HAVE_GPIO_HARD_PWM if !MACH_SAME70Q` in Kconfig).

Clipped that out, will send a PR once someone is able to test properly.

4. It would help if the users that have tested the patch could add some comments here.

I've asked several people to chime in.

Not a blocker for merging, but I also noticed this code adds several #if checks. In general it's preferable to reduce the number of compile time definitions. I think a number of places could reduce these definitions (eg, by using regular c if (CONFIG_XXX) statements, by combining existing #if CONFIG_XXX blocks, by converting gpio.c digital_regs to a struct that stores both the regs and id, etc.).

The gpio.c bit was particularly ugly, my apologies for that. Have refactored with more caffeine. Where else looked like a candidate for culling?

KevinOConnor · 2022-03-21T23:52:12Z

I find the same70_compat.h header file to be confusing. I think it would be preferable to place the defines in the c files that need them.

I felt that resulted in excessive cluttering (over 50 lines of it for USB, for example), but I can do so if you're certain that's better.

I'm leery of large include files with dependencies that aren't clear. If you think the defines are too much for sam3_usb.c then perhaps create a same70_usb.h file with just its usb redefinitions. (Along with a comment that the header is just for register compatibility.)

Where else looked like a candidate for culling?

I'll send a separate message with some examples. Again, nothing critical.

-Kevin

KevinOConnor · 2022-03-21T23:54:23Z

src/atsam/Kconfig

+    config MACH_SAME70Q20B
+        bool "SAME70Q20B (Duet 3 6HC)"
+        select MACH_SAME70Q


Why does kconfig report "same70" to the user, while the lib directory is same70b, while the define here is same70q?

The includes are for revision B (SAME70xxxB), revision A is not (readily?) supportable with the same binary. The 6HC is revision B and I think it unlikely anyone will develop a controller using any revision A part. Q(xxx) is the package, and I simply haven't checked if any peripherals have jumped pin on the 100-pin (N), although it's not likely. We're.. not likely to see the 64-pin (J), the USB peripheral on it doesn't function.

I could just drop the Q, and express it as SAME70xxxB in Kconfig?

The MACH_SAME70Q20B seems fine to me. The MACH_SAME70Q seems odd. I'd have thought the families would be SAM3, SAM4, or SAME70. Not a big deal either way - just curious.

-Kevin

KevinOConnor · 2022-03-21T23:56:20Z

src/atsam/sam4e_afec.c

+    #if CONFIG_MACH_SAM4E
    afec->AFE_MR = AFE_MR_ANACH_ALLOWED | \
                    AFE_MR_PRESCAL(pclk / (2 * ADC_FREQ_MAX) - 1) | \
                    AFE_MR_SETTLING_AST3 | \
                    AFE_MR_TRACKTIM(2) | \
                    AFE_MR_TRANSFER(1) | \
                    AFE_MR_STARTUP_SUT64;
+    #elif CONFIG_MACH_SAME70Q
+    afec->AFE_MR = AFEC_MR_ONE | \
+                    AFE_MR_PRESCAL (pclk / (ADC_FREQ_MAX) -1) | \
+                    AFE_MR_TRANSFER(2) | \
+                    AFE_MR_STARTUP_SUT64;
+    #endif


Could be merged with existing AFEC1_START #ifdef, by creating a new CFG_AFEC_MR.

KevinOConnor · 2022-03-21T23:56:38Z

src/atsam/sam4e_afec.c

+    #if CONFIG_MACH_SAME70Q
+    afec->AFE_ACR = AFE_ACR_IBCTL(1) | AFEC_ACR_PGA0EN | AFEC_ACR_PGA1EN;
+    #else
    afec->AFE_ACR = AFE_ACR_IBCTL(1);
+    #endif


KevinOConnor · 2022-03-21T23:57:02Z

src/atsam/sam4e_afec.c

-    afec->AFE_IDR = 0xDF00803F;
+    #if CONFIG_MACH_SAM4E
+    afec->AFE_IDR = 0xDF00FFFF;
+    #elif CONFIG_MACH_SAME70Q
+    afec->AFE_IDR = 0x47000FFF;
+    #endif


Same. Or, could be if (CONFIG_MACH_SAM4E) ... else ....

KevinOConnor · 2022-03-21T23:57:28Z

src/atsam/sam4e_afec.c

+    #if CONFIG_MACH_SAM4E
    reg |= 1 << (2 * afec_chan);
+    #endif


if (CONFIG_MACH_SAM4E)

On a second look I think I should just remove that entirely and leave both bits cleared on either target. DIFFx is explicitly cleared, so there should be no need to set GAINx to 1 on SAM4E. Will verify, I have that hardware.

KevinOConnor · 2022-03-21T23:58:16Z

src/atsam/sam4e_afec.c

+    #if CONFIG_MACH_SAM4E
    afec->AFE_COCR = (0x800 & AFE_COCR_AOFF_Msk);
+    #elif CONFIG_MACH_SAME70Q
+    afec->AFE_COCR = (0x200 & AFE_COCR_AOFF_Msk);
+    #endif


if (CONFIG_MACH_SAM4E) or create a new define in previous #ifdef .

KevinOConnor · 2022-03-22T00:01:07Z

src/atsam/sam3_usb.c

+    #if CONFIG_MACH_SAM3
    UOTGHS->UOTGHS_CTRL = (UOTGHS_CTRL_UIMOD | UOTGHS_CTRL_OTGPADE
                           | UOTGHS_CTRL_USBE);
+    #elif CONFIG_MACH_SAME70Q
+    UOTGHS->UOTGHS_CTRL = (UOTGHS_CTRL_UIMOD | UOTGHS_CTRL_USBE);
+    #endif


Can create a new #define CFG_UOTGHS_CTRL in existing #if CONFIG_MACH_SAME70 block.

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

KevinOConnor · 2022-03-26T16:01:25Z

Thanks. I squashed and merged this (commits 99c2bf0 and 8049243).

-Kevin

whimsycwd · 2022-06-16T00:19:40Z

Hi, @eamaclean @KevinOConnor I am trying to setup Klipper using DUET3MB6HC. Very appreciated your effort make this possible.

I am having a issue that, when I trying to configure TMC5160, the SPI wire diagram make me hard to follow the configuration tutorial, I don't know how to configure the printer.cfg to make X axis move. Can I have an example config or some hint how to do that?

eamaclean · 2022-06-17T23:46:55Z

Hi, @eamaclean @KevinOConnor I am trying to setup Klipper using DUET3MB6HC. Very appreciated your effort make this possible.

I am having a issue that, when I trying to configure TMC5160, the SPI wire diagram make me hard to follow the configuration tutorial, I don't know how to configure the printer.cfg to make X axis move. Can I have an example config or some hint how to do that?

For each driver you will need a:

[tmc5160 stepper_n]
cs_pin: PD17
spi_bus: usart1
chain_position: 1
chain_length: 6

Just increment chain_position for each driver. See https://www.klipper3d.org/Config_Reference.html#tmc5160

szkrisz · 2023-03-20T13:05:52Z

Hi !

I try to port the FW to SAM E70Q21A-AN CPU but failed. I have custom made printer control board (working with other a custom FW).
Should the SAME70Q21B config work on the SAME70Q21A CPU?
Only I had to change change the serial port (FTDI connected):

Serial port is on PA9, PA10 MUX A port

The error is:

INFO:root:Starting serial connect
Loaded 102 commands (v0.11.0-122-ge6ef48cd / gcc: (15:8-2019-q3-1+b1) 8.3.1 20190703 (release) [gcc-8-branch revision 273027] binutils: (2.35.2-2+14+b2) 2.35.2)
MCU config: ADC_MAX=4095 BUS_PINS_spi0=PD20,PD21,PD22 BUS_PINS_twihs0=PA4,PA3 BUS_PINS_twihs1=PB5,PB4 BUS_PINS_twihs2=PD28,PD27 BUS_PINS_usart0=PB0,PB1,PB13 BUS_PINS_usart1=PA21,PB4,PA23 BUS_PINS_usart2=PD15,PD16,PD17 CLOCK_FREQ=300000000 MCU=same70q20b STATS_SUMSQ_BASE=256 STEPPER_BOTH_EDGE=1
INFO:root:Resetting prediction variance 16752.971: freq=300000000 diff=-15688250 stddev=300000.000
Traceback (most recent call last):
  File "/home/pi/./klipper/klippy/console.py", line 286, in <module>
    main()
  File "/home/pi/./klipper/klippy/console.py", line 281, in main
    r.run()
  File "/home/pi/klipper/klippy/reactor.py", line 292, in run
    g_next.switch()
  File "/home/pi/klipper/klippy/reactor.py", line 340, in _dispatch_loop
    timeout = self._check_timers(eventtime, busy)
  File "/home/pi/klipper/klippy/reactor.py", line 158, in _check_timers
    t.waketime = waketime = t.callback(eventtime)
  File "/home/pi/klipper/klippy/reactor.py", line 48, in invoke
    res = self.callback(eventtime)
  File "/home/pi/./klipper/klippy/console.py", line 74, in connect
    self.clocksync.connect(self.ser)
  File "/home/pi/klipper/klippy/clocksync.py", line 45, in connect
    self._handle_clock(params)
  File "/home/pi/klipper/klippy/clocksync.py", line 118, in _handle_clock
    self.serial.set_clock_est(new_freq, self.time_avg + TRANSMIT_EXTRA,
  File "/home/pi/klipper/klippy/serialhdl.py", line 205, in set_clock_est
    self.ffi_lib.serialqueue_set_clock_est(
OverflowError: can't convert negative number to unsigned

Thank You!

KevinOConnor · 2023-03-20T14:52:16Z

The error is

That looks similar to an error reported at #6094 for a different board - you can look at that PR to see how that submitter fixed the issue.

-Kevin

lib: Add Atmel SAM E70 headers

0c0bfc2

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

atsam: Add support for SAM E70

261a8cd

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

eamaclean force-pushed the atsame70 branch from ba0d091 to 261a8cd Compare March 21, 2022 20:30

KevinOConnor reviewed Mar 22, 2022

View reviewed changes

eamaclean added 4 commits March 22, 2022 16:53

atsam: Rename MACH_SAME70Q to MACH_SAME70

c34ba36

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

atsam: sam4e_afec: reduce preprocessor abuse

972698c

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

atsam: sam3_usb: reduce preprocessor abuse

e66b595

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

atsam: Split up SAME70 header redefinitions

f901ac9

Signed-off-by: Alex Maclean <monkeh@monkeh.net>

KevinOConnor closed this Mar 26, 2022

github-actions bot locked and limited conversation to collaborators Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SAM E70 support #5366

Add SAM E70 support #5366

eamaclean commented Mar 19, 2022

KevinOConnor commented Mar 20, 2022

eamaclean commented Mar 21, 2022

KevinOConnor commented Mar 21, 2022

KevinOConnor Mar 21, 2022

eamaclean Mar 22, 2022

KevinOConnor Mar 22, 2022

KevinOConnor Mar 21, 2022

KevinOConnor Mar 21, 2022

KevinOConnor Mar 21, 2022

KevinOConnor Mar 21, 2022

eamaclean Mar 22, 2022

KevinOConnor Mar 21, 2022

KevinOConnor Mar 22, 2022

KevinOConnor commented Mar 26, 2022

whimsycwd commented Jun 16, 2022

eamaclean commented Jun 17, 2022

szkrisz commented Mar 20, 2023

KevinOConnor commented Mar 20, 2023

Add SAM E70 support #5366

Add SAM E70 support #5366

Conversation

eamaclean commented Mar 19, 2022

KevinOConnor commented Mar 20, 2022

eamaclean commented Mar 21, 2022

KevinOConnor commented Mar 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinOConnor commented Mar 26, 2022

whimsycwd commented Jun 16, 2022

eamaclean commented Jun 17, 2022

szkrisz commented Mar 20, 2023

KevinOConnor commented Mar 20, 2023