Thread (34 messages) 34 messages, 6 authors, 2014-02-28

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

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: 2014-02-28 11:37:25
Also in: linux-can, linux-sh

On 02/28/2014 12:16 PM, Sergei Shtylyov wrote:
Hello.

On 28-02-2014 13:08, Marc Kleine-Budde wrote:
quoted
quoted
quoted
quoted
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.
quoted
quoted
quoted
Please use a normal read-modify-write 32 bit access.
quoted
quoted
    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.
quoted
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   |

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help