netdev - Re: [PATCH v5] can: add Renesas R-Car CAN driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <531074F5.8090702@pengutronix.de>
Date:	Fri, 28 Feb 2014 12:37:25 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	netdev@...r.kernel.org, wg@...ndegger.com,
	linux-can@...r.kernel.org
CC:	linux-sh@...r.kernel.org, vksavl@...il.com
Subject: Re: [PATCH v5] can: add Renesas R-Car CAN driver

On 02/28/2014 12:16 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 28-02-2014 13:08, Marc Kleine-Budde wrote:
> 
>>>>> 1. According to documentation BCR is the 24-bit register.
>>>>> Actually we can consider some 32-bit register that combines BCR and
>>>>> CLKR but according to documentation there are two separate registers.
>>>>> 2. BCR has 8- ,16-, and 32-bit access (according to documentation).
>>>>> 3. This is the algorithm that the documentation suggests.
>>>>> 4. We had a driver version with byte access but 32-bit access seems
>>>>> shorter.
> 
>>>> Please use a normal read-modify-write 32 bit access.
> 
>>>     IMO, reading 32-bits is futile, as we're going to completely
>>> overwrite those 24 bits that constitute BCR. So I kept the 8-bit CLKR
>>> read but removed the CLKR write in the end. I've also added a comment
>>> clarifying why CLKR is positioned in the LSBs of 32-bit word (while it's
>>> address would assume MSBs).
>>> The host bus is big-endian but byte-swaps at least 16- and 32-bit
>>> accesses, so that read[wl]()/write[wl]() work. 8-bit accesses are not
>>> byte swapped, despite what the figure in the manual shows.
> 
>> A 32 bit read/modify/write is a standard operation, nothing special, no
>> need to worry about byte swapping or anything like this.
> 
>    Oh, really? 8-)
>    Don't you know that read[bwlq]() assume little-endian memory layout
> and to read from big-endian 32-bit register one normally needs readl_be()?

I assume you are on little endian ARM only (for now).

If you use a standard 32 bit read, then modify the correct bits in that
32 bit word and write it back, with the corresponding 32 bit write
everything should be fine. For this usecase you just have yo figure out
which 24 of the 32 bit are the one you have to change and which are the
8 that must not be modified.

Looking at the register layout:

> +	u8 bcr[3];	/* Bit Configuration Register */
> +	u8 clkr;	/* Clock Select Register */

I think clkr would be the lowest 8 bit and bcr[] are the upper 24.
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   |


Download attachment "signature.asc" of type "application/pgp-signature" (243 bytes)