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 |