Hello Marc, thanks for interrest to clean the code. On Wednesday 10 of August 2022 22:00:43 Marc Kleine-Budde wrote: > I was about to convert these macros into static inline bool functions > and thinking the naming can be improved a bit: > > On 22.03.2022 00:32:30, Pavel Pisa wrote: > > +#define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF, > > ctucan_read32(priv, CTUCANFD_STATUS))) > > +#define CTU_CAN_FD_ENABLED(priv) > > (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE))) > > [...] > > The common prefix for functions seems to be "ctucan_", so if I convert > CTU_CAN_FD_ENABLED -> ctucan_fd_enabled() the unfamiliar reader might > think this functions tests if the controller is in FD mode. > > As far as I understand the code, the test is if the controller is > enabled at all. This is done via the ctucan_chip_start() function and > undone via ctucan_chip_stop(). So a better function names might be: > - ctucan_chip_started() > - ctucan_chip_is_started() > - ctucan_chip_enabled() > - ctucan_chip_is_enabled() All are understandable. When I think about meaning then I have come to these two ctucan_operation_is_enabled() ctucan_communication_enabled() As for TXNF, it meaning is TX buffers are not full or there is at least one buffer available to fill/prepare another Tx request. So some examples of descriptive names ctucan_empty_txbuf_available() or if negated then ctucan_no_empty_txbuf() If you want to clear that yourself then go on, you have my ACK in advance, I am not available on Internet for next week. You can left it to the Matej Vasilevski prepared patches series. I hope he finds some time to prepare it to mainline his thesis work. But if he does not have time I takeover results of his work and attempt to form his path and add some mine followups (clock etc..) to include them in network next during September... As for the basic driver functionality testing, it is possible with mainline QEMU easily https://www.qemu.org/docs/master/system/devices/can.html The setup to run commits against ARM kernel environment automatically on Zynq HW is available but requires access rights to our GitLab repositories and actual kernel version is 5.10.19-rt32-00004-gd19e6d98b097 #1 SMP PREEMPT_RT I can add access to you to our GitLab but I do not expect that is is of much interrest. I plan to update base but it is used on more education and other systems, so we want to keep wider compatability. Matej Vasilevski has HW for local testing and tests on mainline. Pavel Hronek will continue on his work with goal to have another setups with automatic testing, including latencies which would follow actual mainline. Ideally in cooperation with OSADL to be used on systems with other controllers on hardware available there. But that is longer goal and we look for OSADL members who have interrest for such continuous RT and CAN latencies QA farm testing. > > +/** > > + * ctucan_set_btr() - Sets CAN bus bit timing in CTU CAN FD > > + * @ndev: Pointer to net_device structure > > + * @bt: Pointer to Bit timing structure > > + * @nominal: True - Nominal bit timing, False - Data bit timing > > + * > > + * Return: 0 - OK, -%EPERM if controller is enabled > > + */ > > +static int ctucan_set_btr(struct net_device *ndev, struct can_bittiming > > *bt, bool nominal) +{ > > + struct ctucan_priv *priv = netdev_priv(ndev); > > + int max_ph1_len = 31; > > + u32 btr = 0; > > + u32 prop_seg = bt->prop_seg; > > + u32 phase_seg1 = bt->phase_seg1; > > + > > + if (CTU_CAN_FD_ENABLED(priv)) { > > + netdev_err(ndev, "BUG! Cannot set bittiming - CAN is enabled\n"); > > I would want to replace "CAN" with "Chip" or "Controller". May it be BUG! Cannot set bittiming when operation is enabled > > + return -EPERM; > > + } > > + > > + if (nominal) > > + max_ph1_len = 63; > > + > > + /* The timing calculation functions have only constraints on tseg1, > > which is prop_seg + + * phase1_seg combined. tseg1 is then split in half > > and stored into prog_seg and phase_seg1. + * In CTU CAN FD, PROP is 6/7 > > bits wide but PH1 only 6/5, so we must re-distribute the + * values > > here. > > + */ > > + if (phase_seg1 > max_ph1_len) { > > + prop_seg += phase_seg1 - max_ph1_len; > > + phase_seg1 = max_ph1_len; > > + bt->prop_seg = prop_seg; > > + bt->phase_seg1 = phase_seg1; > > + } > > + > > + if (nominal) { > > + btr = FIELD_PREP(REG_BTR_PROP, prop_seg); > > + btr |= FIELD_PREP(REG_BTR_PH1, phase_seg1); > > + btr |= FIELD_PREP(REG_BTR_PH2, bt->phase_seg2); > > + btr |= FIELD_PREP(REG_BTR_BRP, bt->brp); > > + btr |= FIELD_PREP(REG_BTR_SJW, bt->sjw); > > + > > + ctucan_write32(priv, CTUCANFD_BTR, btr); > > + } else { > > + btr = FIELD_PREP(REG_BTR_FD_PROP_FD, prop_seg); > > + btr |= FIELD_PREP(REG_BTR_FD_PH1_FD, phase_seg1); > > + btr |= FIELD_PREP(REG_BTR_FD_PH2_FD, bt->phase_seg2); > > + btr |= FIELD_PREP(REG_BTR_FD_BRP_FD, bt->brp); > > + btr |= FIELD_PREP(REG_BTR_FD_SJW_FD, bt->sjw); > > + > > + ctucan_write32(priv, CTUCANFD_BTR_FD, btr); > > + } > > + > > + return 0; > > +} > > [...] > > > +/** > > + * ctucan_start_xmit() - Starts the transmission > > + * @skb: sk_buff pointer that contains data to be Txed > > + * @ndev: Pointer to net_device structure > > + * > > + * Invoked from upper layers to initiate transmission. Uses the next > > available free TXT Buffer and + * populates its fields to start the > > transmission. > > + * > > + * Return: %NETDEV_TX_OK on success, %NETDEV_TX_BUSY when no free TXT > > buffer is available, + * negative return values reserved for > > error cases > > + */ > > +static netdev_tx_t ctucan_start_xmit(struct sk_buff *skb, struct > > net_device *ndev) +{ > > + struct ctucan_priv *priv = netdev_priv(ndev); > > + struct canfd_frame *cf = (struct canfd_frame *)skb->data; > > + u32 txtb_id; > > + bool ok; > > + unsigned long flags; > > + > > + if (can_dropped_invalid_skb(ndev, skb)) > > + return NETDEV_TX_OK; > > + > > + if (unlikely(!CTU_CAN_FD_TXTNF(priv))) { > > I'm also looking for nicer names for "CTU_CAN_FD_TXTNF". > What about ctucan_txt_buffer_full() and reverse the logic? possible as well > > + netif_stop_queue(ndev); > > + netdev_err(ndev, "BUG!, no TXB free when queue awake!\n"); > > + return NETDEV_TX_BUSY; > > + } > > Marc Best wishes, Pavel -- Pavel Pisa phone: +420 603531357 e-mail: pisa@xxxxxxxxxxxxxxxx Department of Control Engineering FEE CVUT Karlovo namesti 13, 121 35, Prague 2 university: http://control.fel.cvut.cz/ personal: http://cmp.felk.cvut.cz/~pisa projects: https://www.openhub.net/accounts/ppisa CAN related:http://canbus.pages.fel.cvut.cz/ RISC-V education: https://comparch.edu.cvut.cz/ Open Technologies Research Education and Exchange Services https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home
