lkml.org 
[lkml]   [2010]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix buildwarnings
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 |

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2010-10-27 15:17    [W:0.076 / U:1.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site