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

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

[David Miller]

From: Tomoya <tomoya-linux-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>
Date: Tue, 26 Oct 2010 09:04:43 +0900

> Hi David,
> 
>> This patch has been corrupted by your email client.
> 
> Sorry, it seems my e-mail client setting was reset to default.

There are still problems.

This is beyond frustrating.

> index 55ec324..2889e11 100755

Why does the pch_can.c file have execute permission in your tree?
It doesn't in net-next-2.6 and that is what you should be generating
patches against.

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

[David Miller]

From: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Date: Tue, 26 Oct 2010 10:52:06 -0700 (PDT)

> It doesn't in net-next-2.6 and that is what you should be generating
                ^^^^^^^^^^^^
> patches against.

I mean net-2.6 of course.

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

[Wolfgang Grandegger]

Hi David,

On 10/26/2010 07:55 PM, David Miller wrote:
> From: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Date: Tue, 26 Oct 2010 10:52:06 -0700 (PDT)
> 
>> It doesn't in net-next-2.6 and that is what you should be generating
>                 ^^^^^^^^^^^^
>> patches against.
> 
> I mean net-2.6 of course.

Oh, this patch has various other issues, as Marc and I have already
pointed out, which should be fixed before the driver hits the user.
Unfortunately, the CC to netdev of our reviews got lost somehow, sorry
for the inconvenience, but the original author should have received it.
Tomoya, could you (or somebody else) please also fix the remaining
issues quickly?

Thanks,

Wolfgang.

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

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 946 bytes --]

On 10/26/2010 08:27 PM, Wolfgang Grandegger wrote:

> Oh, this patch has various other issues, as Marc and I have already
> pointed out, which should be fixed before the driver hits the user.
> Unfortunately, the CC to netdev of our reviews got lost somehow, sorry

After noticing my faul I resend my review to the netdev + socketcan
mailinglists and the individual persons mentioned in the original patch.

> for the inconvenience, but the original author should have received it.
> Tomoya, could you (or somebody else) please also fix the remaining
> issues quickly?

For reference, here it is:
http://www.spinics.net/lists/netdev/msg144852.html

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

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

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

[Tomoya MORINAGA]

On Wednesday, October 27, 2010 3:52 AM :  Marc Kleine-Budde and Wolfgang Grandegge wrote:

The following is some inarticulate points I have for your questions.
Please give me more information.

> 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.
This processing does configuration for all message objects.


> 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.


> 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.
I can't understand your intention.
Please show in detail.


>> 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.


> 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. 
Please show in detail.

Thanks, Tomoya(OKI SEMICONDUCTOR CO., LTD.)

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

[Wolfgang Grandegger]

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:
> 
> The following is some inarticulate points I have for your questions.
> Please give me more information.
> 
>> 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.
> This processing does configuration for all message objects.

Not all, but just a few of them. We believe it can be implemented more
efficiently.

> 
>> 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.

What do you mean with "tx Message Object completion"? Is TX done not
signaled via interrupt? Please explain why you need to wait multiples of
500us here in the hot TX path.

>> 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.
> I can't understand your intention.
> Please show in detail.
> 
> 
>>> 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,

Much time means how many mirco-seconds?

> SW must check busy flag of CAN register.
> This is a Topcliff HW specification.

Maybe the busy check could also be done *before* the Message RAM is
accessed to avoid (or minimize) waiting.

>> 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. 
> Please show in detail.

Using KBUILD_MODNAME is OK. It does hold the name of the driver as required.

Wolfgang.

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

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1048 bytes --]

On 10/27/2010 01:57 PM, Wolfgang Grandegger wrote:
> 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:
>>
>> The following is some inarticulate points I have for your questions.
>> Please give me more information.
>>
>>> 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.
>> This processing does configuration for all message objects.
> 
> Not all, but just a few of them. We believe it can be implemented more
> efficiently.

I misread the code...sorry - I'm just writing a longer answer.

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

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

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

[Marc Kleine-Budde]

[-- Attachment #1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

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

[Tomoya MORINAGA]

Hi Wolfgang and Marc,

On Wednesday, October 27, 2010 3:27 AM, Wolfgang Grandegger wrote:
> for the inconvenience, but the original author should have received it.
> Tomoya, could you (or somebody else) please also fix the remaining
> issues quickly?

"masa-korg-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org" left this project. and this mail-address will be deleted soon.
I (tomoya-linux-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org) have taken over "pch can drive" from him.

I will modify your comments ASAP.

---
Hi David,

On Wednesday, October 27, 2010 2:52 AM, David Miller wrote:
> There are still problems.
> 
> This is beyond frustrating.
> 
> > index 55ec324..2889e11 100755
> 
> Why does the pch_can.c file have execute permission in your tree?
> It doesn't in net-next-2.6 and that is what you should be generating
> patches against.
Sorry for the inconvenience.
After updating maintainer's comments and yours, I will post again.

---
Thanks, Tomoya(OKI SEMICONDUCTOR CO., LTD.)

can: About Socket CAN with MSI issue

[Tomoya MORINAGA]

We have faced issue when our CAN diriver whose MSI is enabled, after installing the driver,
once remove the driver and install the driver again,
As a result, interupt handler of the driver is not called again.

Do you have any information or suggestion about the above issue?

As to the our latest CAN driver,
please refer to followoing patch posted by me.
[PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings

Thanks, Tomoya(OKI SEMICONDUCTOR CO., LTD.)

Re: can: About Socket CAN with MSI issue

[Wolfgang Grandegger]

On 10/27/2010 06:29 AM, Tomoya MORINAGA wrote:
> We have faced issue when our CAN diriver whose MSI is enabled, after installing the driver,
> once remove the driver and install the driver again,
> As a result, interupt handler of the driver is not called again.
> 
> Do you have any information or suggestion about the above issue?

Not really, the remove functions looks ok, apart from the fact, that
pch_can_reset() is called *after* pci_iounmap().

Wolfgang.

Re: can: About Socket CAN with MSI issue

[Dave Airlie]

On Wed, Oct 27, 2010 at 2:29 PM, Tomoya MORINAGA
<tomoya-linux@dsn.okisemi.com> wrote:
> We have faced issue when our CAN diriver whose MSI is enabled, after installing the driver,
> once remove the driver and install the driver again,
> As a result, interupt handler of the driver is not called again.
>
> Do you have any information or suggestion about the above issue?

Its a bug in the PCI layer most likely,

http://amailbox.org/mailarchive/linux-kernel/2010/10/7/4629072

Dave.

Re: can: About Socket CAN with MSI issue

[Tomoya MORINAGA]

On Wednesday, October 27, 2010 4:56 PM, Dave Airlie wrote:
> Its a bug in the PCI layer most likely,

Thank you for your information.

This issue occurrs with Socket-CAN driver only.
Previous version our CAN driver(Not Socket-CAN but Tolapai ) works well.
Thus, IMHO, PCI layer doesn't related to the issue.

Thanks, Tomoya(OKI SEMICONDUCTOR CO., LTD.)