RTEMS | can: add support for SJA1000 CAN controller (!1183)

[Michal Lenc (@michallenc)]

Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183 was reviewed by Michal Lenc

--
  
Michal Lenc commented on a discussion on cpukit/include/dev/can/sja1000.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148056

 > + * @return Pointer to CAN chip structure on success, NULL otherwise.
 > + */
 > +struct rtems_can_chip *rtems_sja1000_initialize(

The naming logic is same as in CTU CAN FD driver - `rtems_ctucanfd_initialize`. Though I agree it should probably be `rtems_can_sja1000_initialize` and `rtems_can_ctucanfd_initialize`. I will change it here and submit the change for CTU CAN FD as well if you agree - it's an incompatible API change though.

The function is called either from the application or board support package to initialize the controller. It's the standard char dev driver initialization, for example

```c
struct rtems_can_bus bus;
bus.chip = rtems_sja1000_initialize(
    0x43c90000,
    RTEMS_SJA1000_HW_REG_SPAN_4,
    64,
    SJA1000_WORKER_PRIORITY,
    RTEMS_INTERRUPT_UNIQUE,
    100000000
);

int ret = rtems_can_bus_register( &bus, "/dev/can0" );
```

--
  
Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000_regs.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148057

 > +
 > +/* PeliCAN mode */
 > +enum sja1000_regs {

Fixed

--
  
Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148058

 > +    rtems_build_name( 'C', 'A', 'N', 'S' ),
 > +    worker_priority,
 > +    RTEMS_MINIMUM_STACK_SIZE + 0x1000,

This is the same issue with CTU CAN FD controller #5193. Me and @ppisa plan to do stack analysis for both controllers, but haven't got to it yet. I would expect 4096 which is the value of `RTEMS_MINIMUM_STACK_SIZE` for almost all platforms should be enough though. What do you think @ppisa?

--
  
Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148060

 > +  ret = sja1000_enable_configuration( internal );
 > +  if ( ret < 0)
 > +    goto start_chip_unlock;

Yes, replaced with copy-pasted unlock and return.

--
  
Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148061

 > +  struct rtems_can_queue_ends_dev *qends_dev = chip->qends_dev;
 > +  struct rtems_can_queue_ends *qends = &qends_dev->base;
 > +  while ( 1 ) {

Ok, changed.

--
  
Michal Lenc commented on a discussion on cpukit/dev/can/sja1000/sja1000.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183#note_148062

 > +  atomic_fetch_and( &internal->isr, ~( REG_INT_EPI ) );
 > +
 > +  if ( FIELD_GET( REG_INT_EI, isr ) ) {

I am not sure if it makes the function more comprehensible. Error frame `err_frame` is used in all errors and is cumulative as it can return more that one error type in single message. I could pass a pointer to the error frame to specific functions, but the current implementation imho better indicates the variable is modified in all if statements.


-- 
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1183
You're receiving this email because of your account on gitlab.rtems.org.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20260404/fcae064b/attachment-0001.htm>