CAN: Remove GCLK requirement for Dependencies

[rnd-ash]

Summary

As discussed in Matrix, this PR will modify the clock V2 handling of the CAN Dependencies such that the GCLK argument is not required.

[kyp44]

Just for reference, here is my initial Matrix post:

@rnd-ash : Was just looking at the way the CAN uses the v2 API, and am noticing that can::Dependencies::new and can::Dependencies::free require the Gclk, and simply manually increments and decrements the consumer counter. It looks like maybe this was done in PR #797. As explained here, the Pclk should already be doing this, so that requiring the Gclk should not be necessary at. Unless I am misunderstanding something, I think this should be refactored to remove the Gclk arguments and return values.

[bradleyharden]

@kyp44 and @rnd-ash, I was going through and deleting GitHub emails this morning, and I noticed this PR. I took a quick look, and now I'm not so sure the GCLK wasn't necessary. I believe this code was written by @glaeqen, who was a primary author of clock::v2 with me. I don't think he would have done that if it weren't required.

I don't have time right now to dig deeply, but I found one old commit where the code looked different:

impl<ID: PclkId + AhbId, PS: PclkSourceId, RX, TX, CAN> Dependencies<ID, PS, RX, TX, CAN> {
    /// Create an instance of `Dependencies` struct.
    ///
    /// This struct implements [`mcan_core::Dependencies`] trait, making it
    /// possible to construct an instance of `mcan::bus::CanConfigurable`.
    pub fn new<S>(
        gclk0: S,
        pclk: Pclk<ID, PS>,
        ahbclk: AhbClk<ID>,
        rx: RX,
        tx: TX,
        can: CAN,
    ) -> (Self, S::Inc)
    where
        S: Source<Id = Gclk0Id> + Increment,
    {

Notice there is no relationship between PS and S, so there's nothing that forces pclk to be derived from Gclk0. Perhaps the CAN peripheral uses both GCLK0 and some other clock? I don't know anything about it. But I think you might want to revisit this. I think this change might have been wrong.

[rnd-ash]

I was looking at the datasheet for a while, there is no relationship between the CAN Clock and the CPU, therefore, the GCLK is not needed, since CAN does not require a relationship to the CPUs clock. I've since been using this code with CAN on GCLK6 for a few projects, and it works just fine (To verify this is the case)

[bradleyharden]

Hmm. Ok. I wonder if we can hear from @glaeqen. Why did you write the code this way. Do you remember?

[glaeqen]

I was looking at the datasheet for a while, there is no relationship between the CAN Clock and the CPU, therefore, the GCLK is not needed, since CAN does not require a relationship to the CPUs clock. I've since been using this code with CAN on GCLK6 for a few projects, and it works just fine (To verify this is the case)

I don't get it. Both manuals talk quite clearly about this but maybe I'm illiterate.

https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/m_can/mcan_users_manual_v331.pdf page 12
https://ww1.microchip.com/downloads/aemDocuments/documents/MCU32/ProductDocuments/DataSheets/SAM-D5x-E5x-Family-Data-Sheet-DS60001507.pdf page 1128

What do you mean by GCLK6? MCAN uses dual clocking, one clock (so called host clock) is used for sync operations (MMIO etc.) and Microchip wired it to AHB bus from MCLK that is hardwired to GCLK0 and it is obligatory. As a matter of fact most of the MCAN is operated from this clock. The Pclk is only used for data sampling and requires higher precision.

Have you tried to kill GCLK0 (put MCU to sleep etc.) and got MCAN to work? I'm surprised the host_clock vs can_clock check does not exist in mcan crate, must have been overlooked by me because the trait API was specifically created with this in mind.

[rnd-ash]

What do you mean by GCLK6? MCAN uses dual clocking, one clock (so called host clock) is used for sync operations (MMIO etc.) and Microchip wired it to AHB bus from MCLK that is hardwired to GCLK0 and it is obligatory. As a matter of fact most of the MCAN is operated from this clock. The Pclk is only used for data sampling and requires higher precision.

Have you tried to kill GCLK0 (put MCU to sleep etc.) and got MCAN to work? I'm surprised the host_clock vs can_clock check does not exist in mcan crate, must have been overlooked by me because the trait API was specifically created with this in mind.

Ok you raise a good point here. I'm clearly the illiterate one here, I misunderstood the data sheet, and also haven't taken a look at the Bosch one.

In my setup then, I'm derriving a PCLK for the CAN from GCLK6, and since GCLK0 is always alive (The clock V2 API is static), I never encountered an issue, apologies.

As it is tied GCLK0, then perhaps we can re-write this like I have done for the QSPI module PR? (Ensures there is an alive GCLK0 source rather than just any clock Source) - I think whichever way is taken, the other should follow so that the API is consistent when a peripheral constructor has to take in GCLK0 frequency into account

https://github.com/atsamd-rs/atsamd/pull/926/files#diff-adfda1b8541c6f9db7ecd496f40b8b958b38b9654c65e2c05d5e045a10b6356fR205

[glaeqen]

Yeah, this is how it should have been, why the heck does it accept any source hmm. All of this is documented on mcan-core btw.

https://github.com/GrepitAB/mcan/blob/0ee7ac7099574eae66c52349de7404bb5978f579/mcan-core/src/lib.rs#L174

[ianrrees]

Good find, thanks @bradleyharden and @glaeqen !