Re: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller

[Wolfgang Grandegger <wg@grandegger.com>]

On 04/03/2012 05:58 PM, AnilKumar, Chimata wrote:
> On Tue, Apr 03, 2012 at 21:03:40, Marc Kleine-Budde wrote:
>> On 04/03/2012 04:29 PM, AnilKumar, Chimata wrote:
>>>>>> Please explain why this CAN controller cannot be handled by the existing
>>>>>> C_CAN driver, eventually with some extensions. The register layout seems
>>>>>> almost identical, at least.
>>>>>>
>>>>>> Wolfgang.
>>>>>>
>>>>>
>>>>> These are the some of the pointers I can say, why I had gone for separate
>>>>> file instead of existing driver:
>>>>> * In case of D_CAN driver we can see all the registers are 32bit length
>>>>>   but in case of C_CAN registers are in 16bit length.
>>>>
>>>> How many bits in these 32 bit registers are used?
>>>
>>> In some cases (D_CAN_TXRQ, D_CAN_INTPND, D_CAN_MSGVAL) I have used all the
>>> bits, in some cases used few bits.
>>>
>>> Roughly I can say that its (higher 16bits) % of usages is similar as compare
>>> to 16bits 
>>>
>>> While checking the status of TXRequest registers and INT pending register,
>>> which is a hot code path, we have to put if checks for register access.
>>
>> The c_can already has a c_can_read_reg32() function. It's for example
>> used in the rx_poll function. You can make it a function pointer (i.e.
>> pric->read_reg32()) for easy abstraction.
> 
> This won't fit for D_CAN case because offsets are different in c_can compared
> to d_can. For example if I read CONTROL_REG register (0x0) in case of d_can,
> which will read only control register. In case of c_can it will read
> CONTROL_REG + STATUS register values in single read
> 
>>
>>>>> * Some of the registers, bit masks are different, so we have to add
>>>>>   checks on every API for differentiating the kind of device
>>>>
>>>> Which registers are this? Can you give us an example?
>>>
>>> I am pointing out some of the resisters
>>> * Single registers in case of D_CAN but multiple register in case of C_CAN
>>>   So masks will change MASK, ARB, INTPND
>>> * D_CAN_IFCMD is the combination of COMM request and COMM mask registers
>>
>> Maybe you can use the read_reg32 function on both c_can and d_can.
> 
> Above comment applies here as well

I did look into the manual. Unfortunately, a direct mapping of the C_CAN
to the D_CAN registers seems not possible. It's not just a different
alignment but sometimes two 16-bit C_CAN registers are folded into *one*
32-bit D_CAN register. Therefore we need something more clever, e.g.
using a separate struct or union or handling those register separately.
I still think, if feasible, we should avoid an extra driver for the
D_CAN controller, also because we sooner than later need the same
infrastructure (register_c_can_dev etc.).

Wolfgang.