On 07/31/2018 07:08 PM, Pankaj Bansal wrote:
[...]
>>> + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_FD)
>>> + reg_mcr |= FLEXCAN_MCR_FDEN;
>>
>> What does that bit do?
>> You should only switch on CAN-FD mode if the interface is configured as
>> CAN-FD.
>
> This bit configures the flexcan for FD mode.
This means the new processor will only send CAN-FD frames? This is not
an option, as the hardware supports CAN-2.0.
> This allows the MB size to be flexible. We cannot enable this bit
> based on FD mode enable or not, because we do the Message buffer
> configuration (MB count/rx offload) and bittiming info in
> flexcan_probe irrespective of FD mode enable or not. To be able to set
> this bit on FD mode, we need to move message buffer configuration and
> bittiming info from flexcan_probe to flexcan_start.
Please create a separate patch to move message buffer configuration to
flexcan_start() then. The bittiming _info_ is still assigned in the
probe function.
[...]
>>> + reg_fdctrl = priv->read(®s->fdctrl);
>>> + reg_fdctrl |= FLEXCAN_FDCTRL_MBDSR0(i) |
>>> + FLEXCAN_FDCTRL_MBDSR1(i);
>>> + reg_fdctrl &= ~FLEXCAN_FDCTRL_FDRATE;
>>
>> Does the CAN controller has some sane defaults for reg_fdctrl?
>> If the device is configured for CAN-2.0 you should use standard 8 byte
>> mailboxes....
>
> We cannot configure MB size based on FD mode enable or not, because
> we do the Message buffer configuration (MB count/rx offload) and
> bittiming info in flexcan_probe irrespective of FD mode enable or
> not To be able to set MB size based on FD mode, we need to move
> message buffer configuration and bittiming info from flexcan_probe to
> flexcan_start.
See above, please move the mailbox setup to flexcan_start().
[...]
>>> @@ -1445,6 +1584,12 @@ static int flexcan_probe(struct
>> platform_device *pdev)
>>> if (err)
>>> goto failed_offload;
>>>
>>> + if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_FD) {
>>> + priv->can.bittiming_const =
>> &extended_flexcan_bittiming_const;
>>> + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
>>
>> Does the hardware support ISO or NON-ISO CAN-FD?
>
> ISO
Just ISO. ok.
>>
>>> + priv->can.data_bittiming_const =
>> &extended_flexcan_bittiming_const;
>>
>> please name it flexcan_data_bittiming_const
>
> These are not data bittimings. These are arbitration phase bittimings
> that SHOULD be used when FD mode is to be used.
Are you sure? But you are assigning "priv->can.data_bittiming_const",
which is used during the data phase, which is _after_ the arbitration phase.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Attachment:
signature.asc
Description: OpenPGP digital signature
