Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix buildwarnings

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7018 bytes --]

On 10/27/2010 01:27 PM, Tomoya MORINAGA wrote:
> On Wednesday, October 27, 2010 3:52 AM :  Marc Kleine-Budde and Wolfgang Grandegge wrote:

>> Do I understand your code correctly? You have a big loop, but only do
>>  two different things at certain values of the loop? Smells fishy.
> Uh, I can't understand your intention.
> Please show in detail.

It's easier to talk about code when we can see it, pelase don't delete :)

>> +static void pch_can_config_rx_tx_buffers(struct pch_can_priv *priv)
>> > +{
>> > +	int i;
>> > +	unsigned long flags;
>> > +
>> > +	spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>> > +
>> > +	for (i = 0; i < PCH_OBJ_NUM; i++) {
>> > +		if (priv->msg_obj[i] == MSG_OBJ_RX) {
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > +			iowrite32(CAN_CMASK_RX_TX_GET,
>> > +				&priv->regs->if1_cmask);
>> > +			pch_can_check_if_busy(&priv->regs->if1_creq, i+1);
>> > +
>> > +			iowrite32(0x0, &priv->regs->if1_id1);
>> > +			iowrite32(0x0, &priv->regs->if1_id2);
>> > +
>> > +			pch_can_bit_set(&priv->regs->if1_mcont,
>> > +					CAN_IF_MCONT_UMASK);
>> > +
>> > +			/* Set FIFO mode set to 0 except last Rx Obj*/
>> > +			pch_can_bit_clear(&priv->regs->if1_mcont,
>> > +					  CAN_IF_MCONT_EOB);
>> > +			/* In case FIFO mode, Last EoB of Rx Obj must be 1 */
>> > +			if (i == (PCH_RX_OBJ_NUM - 1))
>> > +				pch_can_bit_set(&priv->regs->if1_mcont,
>> > +						  CAN_IF_MCONT_EOB);
>> > +
>> > +			iowrite32(0, &priv->regs->if1_mask1);
>> > +			pch_can_bit_clear(&priv->regs->if1_mask2,
>> > +					  0x1fff | CAN_MASK2_MDIR_MXTD);
>> > +
>> > +			/* Setting CMASK for writing */
>> > +			iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK |
>> > +				  CAN_CMASK_ARB | CAN_CMASK_CTRL,
>> > +				  &priv->regs->if1_cmask);
>> > +
>> > +			pch_can_check_if_busy(&priv->regs->if1_creq, i+1);
>> > +		} else if (priv->msg_obj[i] == MSG_OBJ_TX) {
>                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Do I understand your code correctly? You have a big loop, but only do
> two different things at certain values of the loop? Smells fishy.

Looking again at the code it makes sense as it is :) Sorry for the
confusion.

>> > +			iowrite32(CAN_CMASK_RX_TX_GET,
>> > +				&priv->regs->if2_cmask);
>> > +			pch_can_check_if_busy(&priv->regs->if2_creq, i+1);
>> > +
>> > +			/* Resetting DIR bit for reception */
>> > +			iowrite32(0x0, &priv->regs->if2_id1);
>> > +			iowrite32(0x0, &priv->regs->if2_id2);
>> > +			pch_can_bit_set(&priv->regs->if2_id2, CAN_ID2_DIR);
>> > +
>> > +			/* Setting EOB bit for transmitter */
>> > +			iowrite32(CAN_IF_MCONT_EOB, &priv->regs->if2_mcont);
>> > +
>> > +			pch_can_bit_set(&priv->regs->if2_mcont,
>> > +					CAN_IF_MCONT_UMASK);
>> > +
>> > +			iowrite32(0, &priv->regs->if2_mask1);
>> > +			pch_can_bit_clear(&priv->regs->if2_mask2, 0x1fff);
>> > +
>> > +			/* Setting CMASK for writing */
>> > +			iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK |
>> > +				  CAN_CMASK_ARB | CAN_CMASK_CTRL,
>> > +				  &priv->regs->if2_cmask);
>> > +
>> > +			pch_can_check_if_busy(&priv->regs->if2_creq, i+1);
>> > +		}
>> > +	}
>> > +	spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>> > +}

> This processing does configuration for all message objects.

Yeah, got it. However I think you can get rid of the priv->msg_obj
variable altogether. Let me recapitulate:
- you setup priv->msg_obj[] in the probe function, which defines if a
  msg_obj is a rx or tx
- this definition is never changed
- all objects of one kind are in a row

So you can identify the purpose of a msg_obj by simply looking at it's
number. If you need to loop over them you can even define helper
functions like, for_each_rx_obj().

>> what does this loop do? why is it nessecarry? I don't like delay loops
>>   in the hot path of a driver.
> This loop is for waiting for all tx Message Object completion.
> This is Topcliff CAN HW specification.

Can you give us a pointer into intel's documentation?
I think Wolfgang already suggested to check if the chip is busy _before_
accessing it instead of waiting the chip to finish after accessing.

>> If you figured out how to use the endianess conversion functions from
>> the cpu_to_{le,be}-{le,to}_to_cpup family use them here, too.
> Uh,le32_to_cpu have been used already here.

Let's look at the code:

>> +		for (i = 0, j = 0; i < cf->can_dlc; j++) {
>> > +			reg = ioread32(&priv->regs->if1_dataa1 + j*4);
>> > +			cf->data[i++] = cpu_to_le32(reg & 0xff);
>> > +			if (i == cf->can_dlc)
>> > +				break;
>> > +			cf->data[i++] = cpu_to_le32((reg >> 8) & 0xff);
>> > +		}

What does the code do? It swaps bytes because the data bytes in the can
core is arranged differently compared to the data in the struct can_frame.

According to the datasheet if_dataa1 holds 1st byte in bits 07:00 and
2nd byte in 15:08. (The rest is reserved.) So in the memory it looks
like this:

  xx xx byte1 byte0

The can_frame has a different layout:

	__u8    data[8] __attribute__((aligned(8)));

which is in memory:

  byte0 byte1 byte2 byte3  byte4 byte5 byte6 byte7

This is why you swap. However in Linux no need to do this by hand.

The if_dataXX have a little endian layout, while the can frame has a big
endian layout. Further if_dataXX has only 16 bit of can data.

I think it should look like this:

	for (i = 0; i < cf->can_dlc; i += 2) {
		reg = ioread32(&priv->regs->if1_data[i >> 1]);
		*(__be16 *)cf->data[i] = cpu_to_be16(reg);
	}

You have to change the definition of the regs struct a bit:

>	u32 if1_mcont;
>	u32 if1_data[4];
>	u32 reserve2;

Totally untested, though.
BTW: Where can I get this Intel Hardware to improve and test the driver?

> I can't understand your intention.
> Please show in detail.

Above we have the RX-Path, the TX-path would probably use a
"be16_to_cpup", have a look at the flexcan driver. It uses the whole 32
bit for candata, though.

>>> All these check if busy in the code make me a bit nervous, can you
>>> please explain why they are needed. A pointer to the manual is okay, too.
>> Me too. I already ask in my previous mail how long that functions
>> usually blocks.
> When accessing read/write from/to Message RAM,
> Since it takes much time for transferring between Register and Message RAM,
> SW must check busy flag of CAN register.
> This is a Topcliff HW specification.

see above.

>> is there some pdev->name instead of KBUILD_MODNAME that can be used?
> I can't understand your intention.
> pdev(struct pci_dev) doesn't have "name" member. 

I was just asking :) As it doesn't have a name, KBUILD_MODNAME is fine.

regards,
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]