- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8.2k
drivers: can: stm32fdcan #31061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drivers: can: stm32fdcan #31061
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alexanderwachter for all your effort. I have tested the driver in a classic CAN network already and can confirm that it works very well.
Still need to go through the can_mcan.c, but here are some small comments already.
6d7e1a9    to
    af2b381      
    Compare
  
    af2b381    to
    9198d49      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this PR. I would also test with it with my stm32h7 soon and go through ToDo's.
        
          
                dts/arm/st/g4/stm32g4.dtsi
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads to a warning during DTS generation:
board_name.dts.pre.tmp:281.7-324.5: Warning (simple_bus_reg): /soc/can: missing or empty reg/ranges property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbolivar any suggestions on how we can improve the DT bindings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the top-level node could be set to the general CANFD configuration register?
| can { | |
| can: can@40006500 { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It possibly misses a reg = <0>;
107a72f    to
    ecc1e3b      
    Compare
  
    b84aa20    to
    efdebda      
    Compare
  
    bd19adb    to
    fa954ff      
    Compare
  
    | 
 In my opinion it is to early to speak about h7. I've tested it on h723 and it needs some changes due to different registers structure. I'll prepare driver update as soon as your development of this branch will be in master. | 
| Hi! Thanks for your work on this branch. I think I found a problem running the  I have tried it merged with a slightly more recent zephyr (2235c71) on a nucleo_g0b1re board. For being able to use the driver on the g0b1 board I created a dts overlay for it (it is taken from your stm32g4 proposal): I then build the  Apparently it stalls on the  For the sake of not cluttering the PR, I am omitting some debug information I could provide if it can be helpful to anyone. | 
| Something I noticed:  On that same note, a number of the existing drivers depend on this value while iterating through data bytes. Though it will likely work, it's not semantically correct since DLC is no longer explicitly the number of bytes with the introduction of CAN-FD. | 
| @raul-klg do the api tests pass? | 
| Edit: -- Old post I have run three cases: 
 
 
 Please, let me know if I am doing anything wrong and how else could I help. | 
        
          
                drivers/can/can_mcan.c
              
                Outdated
          
        
      |  | ||
| if (can->ir & CAN_MCAN_IR_ARA) { | ||
| can->ir = CAN_MCAN_IR_ARA; | ||
| LOG_ERR("Acces to reserved address"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a small typo:
| LOG_ERR("Acces to reserved address"); | |
| LOG_ERR("Access to reserved address"); | 
| I have been testing this CAN implementation on a ATSAME51 using the prototype specialisations mentioned in the OP. static void can_sam0_register_state_change_isr(const struct device *dev,
						can_state_change_isr_t isr)
{
	const struct can_sam0_config *cfg = DEV_CFG(dev);
	const struct can_mcan_config *mcan_cfg = &cfg->mcan_cfg;
	struct can_mcan_reg *can = mcan_cfg->can;
	struct can_mcan_data *mcan_data = &DEV_DATA(dev)->mcan_data;
	struct can_mcan_msg_sram *msg_ram = cfg->msg_sram;
	mcan_data->state_change_isr = isr;
	if (isr == NULL) {
		can->ie &= ~CAN_MCAN_IE_EP;
	} else {
		can->ie |= CAN_MCAN_IE_EP;
	}
}I also have found that the bitrate does not seem to be working as expected with this device tree I am getting a bitrate of 370k:  | 
b6e1e6a    to
    2c03505      
    Compare
  
    | @raul-klg the test run on my stm32g474 sucessfully. | 
2c03505    to
    2f3f91a      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still finishing final can_mcan.c review, but here are some remarks already and one point that's not clear for me. Maybe you can comment already, @alexanderwachter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only got above minor comments left.
I've been using this code in classic mode for a while already without any issues.
Thanks again for all the time you put into getting this done @alexanderwachter!
| 
 Thanks for the verification (and for your additional work). I'll retry when I'm able to. Maybe I need to review how I used the branch. | 
I believe the issues have been resolved
2f3f91a    to
    8fc8b15      
    Compare
  
    Add Kconfig option to select appropriate can data field length Signed-off-by: Alexander Wachter <[email protected]>
Implementation of the Bosch M_CAN IP driver. This driver is just the base for a specific SoC implementation. Signed-off-by: Alexander Wachter <[email protected]>
This driver is the SoC specific implementation of the Bosch M_CAN IP. Signed-off-by: Alexander Wachter <[email protected]>
This commit adds the CAN nodes to the STM32g4 SoCs. Signed-off-by: Alexander Wachter <[email protected]>
This commit adds CAN support to the nucleo_g474re board. Signed-off-by: Alexander Wachter <[email protected]>
This commit enhances the CAN API test. In the send_receive tests, we are using two filters and frames at the same time to check if the frame gets dispatched to the correct filter. Signed-off-by: Alexander Wachter <[email protected]>
This commit adds tests for CAN-FD frames. Signed-off-by: Alexander Wachter <[email protected]>
STM32FDCAN implementation.
This PR implements a generic Bosch M_CAN driver that can be specialized for dedicated SoCs, such as FDCAN (STmicro), Microchip SAM, and NXP.
The specialization for FDCAN (STmicro) is included in this PR.
Tests are added to test the FD mode.
@legoabram has a prototype prototype specialization for SAM0