atsamd: Add support for SAMC21 #6009

Wulfsta · 2023-01-23T20:16:25Z

Quick and dirty addition of samc21, missing the sdadc. Whoever reviews this, please have a close look at the clocks - I don't know if they're up to Klipper's standards.

Signed-off-by: Luke Vuksta <wulfstawulfsta@gmail.com>

KevinOConnor

Thanks. In general it looks good to me. I have a few minor comments below.

Separately, can you give a brief overview of what the "sdadc" is and how it is used? I don't think it is a roadblock for this PR, but a highlevel description would help give me some context.

-Kevin

src/atsamd/Kconfig

src/atsamd/adc.c

KevinOConnor · 2023-01-26T01:37:01Z

src/atsamd/fdcan.c

+#if CONFIG_HAVE_SAMD_USB
    if (!CONFIG_USB) {
        // The FDCAN peripheral only seems to run if at least one
        // other peripheral is also enabled.
        enable_pclock(USB_GCLK_ID, ID_USB);
        USB->DEVICE.CTRLA.reg = USB_CTRLA_ENABLE;
    }
+#endif


It's only necessary to enable one other device - if ID_USB isn't defined, we can choose some other device. (This whole thing is some weird quirk of the samd power enable code on sam5x.)

Do you know if this is true for the samc21? This chip does not have usb, so we’d have to enable something else.

@KevinOConnor can you provide more details about this? I’m having issues with CAN, and am wondering where I might investigate this further for the samc21.

src/atsamd/samc21_clock.c

src/atsamd/Makefile

Wulfsta · 2023-01-26T03:20:48Z

Separately, can you give a brief overview of what the "sdadc" is and how it is used? I don't think it is a roadblock for this PR, but a highlevel description would help give me some context.

The sdadc is a different data structure, so an additional field in gpio_adc will be needed to indicate if this is the sdadc or not. I also might be misunderstanding, but the specification seems to suggest that the input is over a pair of pins? I haven't worked with adcs before and am a bit confused about why the circuit on the toolboard looks like it does...

KevinOConnor · 2023-01-26T04:09:08Z

I'm a little confused - what pins on that Duet board are using the sdadc?

-Kevin

Wulfsta · 2023-01-26T05:09:21Z

TEMP_0, the intended thermistor input, uses the sdadc. I don’t understand why there are two VSSA_MON_n though, both of those pins are negative sdadc inputs. Maybe it’s PB09 and PB08 as a pair of inputs?

Edit: accidentally closed, oops.

KevinOConnor · 2023-01-26T05:47:43Z

Okay, thanks for the info. FWIW, the duet3 use of the sdadc looks to me to be a "micro optimization" that requires "high complexity". My suggestion would be to not add any sdadc code and just use the traditional adc measuring of PB8 for TEMP_0.

As background, the sigma-delta adc (sdadc) is capable of calculating the voltage difference between the two "negative" and "positive" input lines. (As opposed to a traditional ADC which measures a voltage as a ratio between VSSA and VREF.) The sdadc has 16 bit resolution, while the traditional adc has 12 bit resolution. So, it seems to me that the duet3 is just trying to get a few more bits of resolution (though because the comparison is always positive, it's at best a 15 bit resolution, and it's not clear to me that the additional bits would be of much value due to noise in the system).

-Kevin

Wulfsta · 2023-01-26T05:53:32Z

Okay, I understand it now. I'll think about how I can hack it in when I get a few minutes to clean up this other stuff; it seems like a shame to waste the hardware if it's there.

Wulfsta · 2023-01-27T06:06:27Z

@KevinOConnor Don't merge this just yet, I should do some more testing.

Wulfsta · 2023-01-28T00:25:03Z

I think this is good, but I'm not around hardware to test it right now. I should be able to in a few days.

KevinOConnor · 2023-02-03T23:41:11Z

What's the current status of this PR?

-Kevin

Wulfsta · 2023-02-04T00:40:55Z

What's the current status of this PR?

-Kevin

Untested, on vacation. I’ll test once I’m back and have a few hours.

Wulfsta · 2023-02-18T00:13:05Z

So I've figured out how to flash Duet Toolboards greater than revision 1.0 from a Pi 4, but am not seeing the mcu from a waveshare hat. I didn't look too carefully at the fdcan.c file, but it probably needs some work done to get it correct for this board.

Wulfsta · 2023-02-18T07:20:29Z

src/atsamd/samc21_clock.c

+uint32_t
+get_pclock_frequency(uint32_t pclk_id)
+{
+    return CLKGEN_MAIN;


Well that's a bug.

KevinOConnor · 2023-02-27T18:49:30Z

@KevinOConnor can you provide more details about this? I’m having issues with CAN, and am wondering where I might investigate this further for the samc21.

Unfortunately, I don't know why that limitation exists on the atsame chips. I just know that when I was running Klipper in usb-to-can bridge mode everything worked and that it wouldn't work if regular canbus mode. Ultimately I found that if I just enabled the usb peripheral clock it was enough to get canbus working in normal standalone mode. I saw a brief blurb in the documentation about the can hw on atsame not requesting clocks (or some such) and so I just forced it to work by enabling some other clock. My recollection is that enabling any other clock worked.

Separately, what is the state of this PR?

If there is an issue with fdcan then I think it would help if we could separate out the fdcan changes from the rest of the PR. If atsamc is working in regular serial mode then we can get that merged. That will make the review process for the fdcan parts a lot smaller (and thus simpler).

Cheers,
-Kevin

Wulfsta · 2023-02-28T23:01:37Z

Ah, thank you for the info. I'm lacking 5v hardware to hook this up to UART right now, so I'm inclined to continue until I have this working with CAN, so as to not accidentally push any bugs upstream. I've been using gdb and hardcoded calls to validate various functionality so far. I can convert this to a draft if you aren't happy with it sitting open while I investigate why I can't connect over CAN.

github-actions · 2023-03-27T00:06:55Z

It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

KevinOConnor · 2023-03-27T23:45:40Z

What was the final results of the work on the samc21?

-Kevin

Wulfsta · 2023-03-28T00:56:30Z

Still working on it, CAN Tx works, but I can’t seem to get Rx to. Going to hook up some more hardware and try to catch what’s happening in gdb. If you want to reopen feel free, I will get this to work eventually. I’d also really appreciate it if you could take a hard look at the fdcan.c file - you might catch something I haven’t…

Wulfsta · 2023-03-31T21:49:25Z

@KevinOConnor Can you comment out the USB initialization in fdcan.c, hack assigned_id to a set value upon initialization, and tell me if the board you originally wrote this CAN file for transmits status messages? I’m wondering if this is a related problem, and the Rx code in particular is what breaks when it’s the first thing initialized…

Wulfsta · 2023-04-22T05:17:40Z

@KevinOConnor I added 12MHz clocks for v1.0 of the Toolboard, dug a Jetson TX2 out of storage, and tested with those two pieces of hardware. It worked immediately, so the issue was the hardware I was testing with. You can go ahead and merge.

KevinOConnor

Thanks. I have some review comments and questions. See below.

-Kevin

KevinOConnor · 2023-04-27T03:06:18Z

src/atsamd/fdcan.c

+#if CONFIG_MACH_SAMC21
+    MCLK->AHBMASK.reg |= MCLK_AHBMASK_CANx;
+#endif


Can you explain what this does?

This bus clock for CAN is not enabled by default on this chip (annoyingly, see section 17.8.6 of the data sheet). This turns it on. Notice that it's not the same as the one in the clock code.

src/atsamd/chipid.c

src/atsamd/Makefile

src/atsamd/Kconfig

src/atsamd/serial.c

KevinOConnor · 2023-04-27T03:14:18Z

src/atsamd/timer.c

+#if CONFIG_MACH_SAMC21
+    TCp->COUNT32.CTRLBSET.reg |= TC_CTRLBSET_CMD(TC_CTRLBCLR_CMD_READSYNC_Val);
+    while (TCp->COUNT32.SYNCBUSY.reg & TC_SYNCBUSY_COUNT)
+        ;
+#endif


Can you explain what this does? The timer_read_time() is a "fast path" function - overhead here will have notable impact on performance and benchmark results. Is there a way to just read the counter value without having to perform a write (or at least without having to perform a read-modify-write and a loop)?

There is a note hidden in the data sheet under the 32 bit register summary for this register (section 35.7.3.13) that states this synchronization must happen prior to any read access.

Signed-off-by: Luke Vuksta <wulfstawulfsta@gmail.com>

KevinOConnor · 2023-05-01T17:59:09Z

Thanks. I merged this PR.

In regard to timer_read_time(), have you tried without the synchronization? It may just be that one will get a duplicate value without the sync, and that isn't a huge issue (less of an issue than going dramatically slower).

Along those lines, it would be interesting to see the benchmark results (see docs/Benchmarks.md) for this chip.

Also, would probably be a good idea to update the regression build tests (test/configs/) to compile test this chip.

-Kevin

lib: Add Atmel SAM C21 headers

1748d04

Signed-off-by: Luke Vuksta <wulfstawulfsta@gmail.com>

Wulfsta force-pushed the samc21 branch from c23cfb2 to 7e64221 Compare January 23, 2023 20:19

KevinOConnor reviewed Jan 26, 2023

View reviewed changes

Wulfsta closed this Jan 26, 2023

Wulfsta reopened this Jan 26, 2023

Wulfsta force-pushed the samc21 branch 2 times, most recently from 78f37f4 to c305fa5 Compare January 27, 2023 03:23

Wulfsta force-pushed the samc21 branch from c305fa5 to 3085d47 Compare January 27, 2023 23:19

KevinOConnor added the pending feedback Topic is pending feedback from submitter label Jan 31, 2023

Wulfsta commented Feb 18, 2023

View reviewed changes

Wulfsta force-pushed the samc21 branch 10 times, most recently from 19bc5cf to 588732d Compare February 20, 2023 18:09

Wulfsta force-pushed the samc21 branch 4 times, most recently from ccce4b3 to 87865b8 Compare February 26, 2023 00:31

Wulfsta force-pushed the samc21 branch 3 times, most recently from bac6311 to 0df7798 Compare March 6, 2023 00:04

github-actions bot added the inactive Not currently being worked on label Mar 27, 2023

github-actions bot closed this Mar 27, 2023

KevinOConnor reopened this Mar 28, 2023

KevinOConnor removed the inactive Not currently being worked on label Apr 19, 2023

Wulfsta force-pushed the samc21 branch from 0df7798 to c0d711f Compare April 22, 2023 04:51

KevinOConnor reviewed Apr 27, 2023

View reviewed changes

Wulfsta force-pushed the samc21 branch 2 times, most recently from 21599cf to 8205ef5 Compare April 27, 2023 06:00

atsamd: Add support for SAMC21

7afbf78

Signed-off-by: Luke Vuksta <wulfstawulfsta@gmail.com>

Wulfsta force-pushed the samc21 branch from 8205ef5 to 7afbf78 Compare April 27, 2023 06:04

KevinOConnor merged commit 3b0729c into Klipper3d:master May 1, 2023
1 check passed

github-actions bot locked and limited conversation to collaborators May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

atsamd: Add support for SAMC21 #6009

atsamd: Add support for SAMC21 #6009

Wulfsta commented Jan 23, 2023

KevinOConnor left a comment

KevinOConnor Jan 26, 2023

Wulfsta Jan 26, 2023

Wulfsta Feb 18, 2023

Wulfsta commented Jan 26, 2023 •

edited

KevinOConnor commented Jan 26, 2023

Wulfsta commented Jan 26, 2023 •

edited

KevinOConnor commented Jan 26, 2023

Wulfsta commented Jan 26, 2023

Wulfsta commented Jan 27, 2023

Wulfsta commented Jan 28, 2023

KevinOConnor commented Feb 3, 2023

Wulfsta commented Feb 4, 2023

Wulfsta commented Feb 18, 2023 •

edited

Wulfsta Feb 18, 2023

KevinOConnor commented Feb 27, 2023

Wulfsta commented Feb 28, 2023

github-actions bot commented Mar 27, 2023

KevinOConnor commented Mar 27, 2023

Wulfsta commented Mar 28, 2023

Wulfsta commented Mar 31, 2023

Wulfsta commented Apr 22, 2023

KevinOConnor left a comment

KevinOConnor Apr 27, 2023

Wulfsta Apr 27, 2023 •

edited

KevinOConnor Apr 27, 2023

Wulfsta Apr 27, 2023

KevinOConnor commented May 1, 2023

atsamd: Add support for SAMC21 #6009

atsamd: Add support for SAMC21 #6009

Conversation

Wulfsta commented Jan 23, 2023

KevinOConnor left a comment

Choose a reason for hiding this comment

KevinOConnor Jan 26, 2023

Choose a reason for hiding this comment

Wulfsta Jan 26, 2023

Choose a reason for hiding this comment

Wulfsta Feb 18, 2023

Choose a reason for hiding this comment

Wulfsta commented Jan 26, 2023 • edited

KevinOConnor commented Jan 26, 2023

Wulfsta commented Jan 26, 2023 • edited

KevinOConnor commented Jan 26, 2023

Wulfsta commented Jan 26, 2023

Wulfsta commented Jan 27, 2023

Wulfsta commented Jan 28, 2023

KevinOConnor commented Feb 3, 2023

Wulfsta commented Feb 4, 2023

Wulfsta commented Feb 18, 2023 • edited

Wulfsta Feb 18, 2023

Choose a reason for hiding this comment

KevinOConnor commented Feb 27, 2023

Wulfsta commented Feb 28, 2023

github-actions bot commented Mar 27, 2023

KevinOConnor commented Mar 27, 2023

Wulfsta commented Mar 28, 2023

Wulfsta commented Mar 31, 2023

Wulfsta commented Apr 22, 2023

KevinOConnor left a comment

Choose a reason for hiding this comment

KevinOConnor Apr 27, 2023

Choose a reason for hiding this comment

Wulfsta Apr 27, 2023 • edited

Choose a reason for hiding this comment

KevinOConnor Apr 27, 2023

Choose a reason for hiding this comment

Wulfsta Apr 27, 2023

Choose a reason for hiding this comment

KevinOConnor commented May 1, 2023

Wulfsta commented Jan 26, 2023 •

edited

Wulfsta commented Jan 26, 2023 •

edited

Wulfsta commented Feb 18, 2023 •

edited

Wulfsta Apr 27, 2023 •

edited