[same70] CAN driver maybe broken?

[tswaehn]

I am really curious. Working with zephyr 3.0.0 CAN drivers was like charm. worked out of the box. However turning to zephyr 3.5.0 things seem to be a bit different - made all changes that come with zephyr 3.5.0 and still I cant get it running.

my test setup:

• same70q21b
• minimal zephyr program
• can init
• can test sends a single message every 100msec
• works fine in zephyr-3.0.0 - fails in zephyr-3.5.0

this is the test code for zephyr-3.5.0 below:

• maybe I am missing something important here?
• for every message I send -- I get a Remote Transmission Request (double checked with CAN sniffer)

void main(void){

    can_init(CAN_BAUDRATE);

    while(1){
        can_test();
        k_sleep(K_MSEC(100));
    }

}

void can_init(uint32_t baudrate) {
    int32_t ret;

    const struct can_filter std_filter = {
            .id = CAN_STD_ID_MASK,
            .mask = 0,
            .flags = 0,
    };

    can.dev = device_get_binding("CAN_0");
    if (can.dev == NULL) {
        LOG_ERR("unable to get CAN_0 device");
        return;
    }

    if (0 != can_set_baudrate(baudrate)) {
        return;
    }

    can_start(can.dev);
}

void can_test(){

    struct can_frame frame = {
            .flags = 0,
            .id = 0x123,
            .dlc = 5,
    };

    for (uint32_t i = 0; i < 5; i++) {
        frame.data[i] = 0x11 * i;
    }

    int32_t ret = can_send(can.dev, &frame, K_MSEC(100), NULL, NULL);

   //it always returns ret=0 !!!

    if (0 != ret){
        // error
        LOG_ERR("failed to send CAN message");
    } else {
        // success
    }

}

update

with CAN sniffer on zephyr-3.0.0

with CAN sniffer on zephyr-3.5.0

in summary it looks like the

• header is damged somehow ID and DLC is incorrect
• first byte is different, not sure what this is for

update

it looks like taking the complete wrong buffers in zephyr-3.5.0 for sending; it always sends the data, that was used before in the test with zephyr-3.0.0 which are still present in RAM.

[tswaehn]

@henrikbrixandersen from the source it looks like you are the expert on can_sam.c did you notice anything or could you confirm seeing can messages at a can-sniffer tool in general with zephy 3.5.0 (not loopback)?

[tswaehn]

it looks like this is can_sam specific. the offset register for the message ram offset is by default REG_CCFG_CAN0 = 0x20400000

however the driver comes up with sam_cfg->mram = 0x2041b40. (actually I dont know where this value comes from??)

nevertheless -- this dosnt fit together.

• we may want to correct the driver value of sam_cfg->mram
• or post update the REG_CCFG_CAN0 accordingly

this brings back writing to can in zephyr-3.5.0 to kind of normal (not yet sure about side effects)

(result of example from above)

[henrikbrixandersen]

Please retest this on the v3.5-branch (making sure commit https://github.com/zephyrproject-rtos/zephyr/commit/8415778a1a04bab98f10523fe4ca1894c79e41f5 is included in your branch when testing). Testing on main would also be nice.

[henrikbrixandersen]

• or post update the REG_CCFG_CAN0 accordingly

Yeah, I've previously raised an issue about CCFG_CAN0:CAN0DMABA and CCFG_SYSIO:CAN1DMABA being left at default values (which may or may not be correct depending on where in memory the M_CAN message RAM is placed), but I was told changing it from the default would never be needed due to the memory layout of these MCUs.

I do not have access to any SAM/SAM0 hardware with M_CAN IPs embedded myself, so unfortunately I am not able to test and confirm this problem, nor test any fix writing to CCFG_CAN0:CAN0DMABA and CCFG_SYSIO:CAN1DMABA.

You may be able to find some inspiration as to how to solve this by using the handling of MRBA in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/can/can_mcux_mcan.c#L82 as inspiration.

CC: @nandojve

[tswaehn]

the driver comes up with sam_cfg->mram = 0x2041b40

@henrikbrixandersen I am curious where this value comes from. its not the initial value. why should that be changed in first place?

[tswaehn]

Please retest this on the v3.5-branch (making sure commit 8415778 is included in your branch when testing). Testing on main would also be nice.

Please note that I already did test with zephyr 3.5; I also confirmed that patch https://github.com/zephyrproject-rtos/zephyr/commit/8415778a1a04bab98f10523fe4ca1894c79e41f5 is included.

So the issue persists and is valid - open

[henrikbrixandersen]

Please note that I already did test with zephyr 3.5; I also confirmed that patch 8415778 is included.

Do you have CONFIG_CACHE_MANAGEMENT=y and CONFIG_DCACHE=y both enabled?

[henrikbrixandersen]

the driver comes up with sam_cfg->mram = 0x2041b40

@henrikbrixandersen I am curious where this value comes from. its not the initial value. why should that be changed in first place?

The SAM/SAM0 series do not have dedicated RAM for the Bosch M_CAN message RAM, so the driver allocates a memory area for this using CAN_MCAN_DT_INST_MRAM_DEFINE(). The value, you are seeing, is the address of this area in RAM.

I am not sure what you mean by "initial value" and why you think this needs to be changed?

[nandojve]

Hi @tswaehn , @henrikbrixandersen ,

Unfortunately I can not test MCAN and don't have knowledge about CAN protocol. What I know is that there is a lot of issues around cache these days and we need to be careful. I recommend that you try after this #66524, maybe v3.6.0-RC1, to make sure you have all patches in. Since Atmel is only supported by community we only track the main branch but, eventually, there is no problem to backport to old version as soon we have a solution in main.

So if you are backport an old version I recommend you to backport to v3.6.0-rc1 at moment.

[henrikbrixandersen]

Do you have CONFIG_CACHE_MANAGEMENT=y and CONFIG_DCACHE=y both enabled?

@tswaehn Any luck?

[tswaehn]

the DMA base address (where the ATMEL controller expects the MCAN memory) for CAN0 is by default 0x2040 0000

• debugging the controller with the current zephry-3.5 can-driver gives me an MCAN base address: sam_cfg->mram = 0x2041b40
• as stated above this will not work and doesnt work as stated in the initial post -- proved.
• solution 1:
  • we need to modifiy the DMA base address in the can_init(); ex: REG_CCFG_CAN0 = sam_cfg->mram;
• solution 2:
  • we need to modify sam_cfg->mram to be at 0x2040 0000

the easier solution 1 - I tested and it works. nevertheless I am wondering if anyone can confirm or let me know why sam_cfg->mram comes up with something different from the default 0x2040 0000.

based on @henrikbrixandersen suggestion this is also the way to go https://github.com/zephyrproject-rtos/zephyr/blob/a48c958c8fb85a0985880482df9c79407a8fc55e/drivers/can/can_mcux_mcan.c#L86

nevertheless this seems to be a big change. so wondering if anyone can confirm.

[henrikbrixandersen]

Solution 1 is the way to go. You'll need to program the 16-bit MSB of the address into the register (for both CAN0 and CAN1).

[tswaehn]

just wondering if there is a more easy solution:

can_sam.c is calling here

can_mcan_configure_mram(dev, 0U, sam_cfg->mram);

where we expect the DMA base address (here):

int can_mcan_configure_mram(const struct device *dev, uintptr_t mrba, uintptr_t mram)

so instead of feeding mrba=0 we just might feed the correct value then. should be sufficient.

[tswaehn]

so actually my PR is almost ready. problem is that this of course works only for CAN0 and not CAN1 because

CAN0 mcan segment address is in REG_CCFG_CAN0

where CAN1 mcan segment address is in REG_CCFG_SYSIO

is there an elegant way of how to differentiate based on used device in device tree?

[nandojve]

Hi @tswaehn ,

CAN0 mcan segment address is in REG_CCFG_CAN0 where CAN1 mcan segment address is in REG_CCFG_SYSIO is there an elegant way of how to differentiate based on used device in device tree?

My suggestion is add this address in the DTS to map using devicetree. So, the system can proper handle with each instance. It could be a new property named dma_base_reg with a very good description. Then in the devicetree you need to fill as

CAN0 instance have CCFG_CAN0 in the address: 0x40088110 dma_base_reg = <0x40088110>

CAN1 instance have CCFG_SYSIO in the address: 0x40088114 dma_base_reg = <0x40088114>

Then you change PR code to write direct in the address defined by the dma_base_reg using some snip as below:

*config->dma_base_reg = mrba | (*config->dma_base_reg & 0x0000FFFF);

[tswaehn]

Nice, I will prepare,...

[henrikbrixandersen]

My suggestion is add this address in the DTS to map using devicetree. So, the system can proper handle with each instance. It could be a new property named dma_base_reg with a very good description.

It could also just be a new, named entry in the regs property, which is intended for this. I'd prefer that approach.

[github-actions[bot]]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[tswaehn]

It looks like there are 2 options:

option 1: "standalone config"

		can0: can@40030000 {
			compatible = "atmel,sam-can";
			reg = <0x40030000 0x100>;
			dma-base-reg = <0x40088110>;
                        ...
		};

option 2: "extending the reg config with the new register"

		can0: can@40030000 {
			compatible = "atmel,sam-can";
			reg = <0x40030000 0x100 0x40088110 0x1>;
                        ...
		};

As far as I understand the standard MCAN module produces some macros for reading the config from the CAN device tree reg property. Modifying the standard MCAN macros for SAM controller is something I dont like to do. So reading the 3rd parameter from the reg actually would be an option. nevertheless maybe standard MCAN is extending reg as well one day then this will be miss interpreted by my TAKE-3rd-VALUE() macro.

So I actually decided to go for option 1. Hope that is ok?

... just updated PR 69405

[henrikbrixandersen]

So I actually decided to go for option 1. Hope that is ok?

I am not sure what "option 1" is, but the acceptable solutions is to use an entry in regs.

Since the backend uses named registers, there is no issue adding frontend-specific regs. This is done in other drivers as well.

[tswaehn]

DTS will then look like this, correct?

		can0: can@40030000 {
			compatible = "atmel,sam-can";
			reg = <0x40030000 0x100 0x40088110 0x1>;
                        ...
		};

the first 2 values are consumed (and named) in MCAN driver.

the 3rd value is NOT a feature of MCAN and will then only consumed by SAM driver.

this requires me to add the naming of 3rd parameter to MCAN as well.

[henrikbrixandersen]

DTS will then look like this, correct?

		can0: can@40030000 {
			compatible = "atmel,sam-can";
			reg = <0x40030000 0x100 0x40088110 0x1>;
                        ...
		};

the first 2 values are consumed (and named) in MCAN driver.

the 3rd value is NOT a feature of MCAN and will then only consumed by SAM driver.

The third (and fourth) values will be consumed by the SAM driver, correct.

this requires me to add the naming of 3rd parameter to MCAN as well.

Yes, you will need to use named register addresses for this as well. Please find inspiration in how this was done in https://github.com/zephyrproject-rtos/zephyr/pull/73386 for another M_CAN frontend driver.

[tswaehn]

alright, thats new to me, understand. I will do. thank you for providing the inspiration point.

then the result should maybe look like this: https://github.com/zephyrproject-rtos/zephyr/pull/73594/files