[Xenomai-core] CAN updates & questions

[Xenomai-core] CAN updates & questions

[Jan Kiszka]

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

Hi Wolfgang,

as one result of a hacking session on a PPC405 with SJA1000 on board I
applied two minor fixes to rtcan_isa.c to SVN, see end of mail for
reference. The first one gave an "interesting" effect on big-endian
because irq is an integer, the second one reveals that we should do some
multiport test with the ISA driver soon.

Questions arose regarding the meaning of rtcan_device.can_sys_clock
(BTW, comment in rtcan_dev.h is wrong). What clock rate does it
describe? For the SJA1000 drivers its obviously clock/2. I'm asking
because of a) the output in rtcan_calc_bit_time() and b) the module
parameter of rtcan_isa and its description. We first hacked 16 MHz into
the latter yesterday as I didn't recall quickly enough that our Phytec
board also runs at 16 MHZ, thus the default value of 8 MHZ was already
correct. Confused? At least we were...

And another suggestion: In order make module names shorter, what about
the renaming xeno_rtcan* -> xeno_can*?

Then we will soon have to discuss how to deal with a rtcan_isa
derivative that uses ioremapped memory instead of ports (naming,
separation or integration).

Jan


--

Index: ksrc/drivers/can/sja1000/rtcan_isa.c
===================================================================
--- ksrc/drivers/can/sja1000/rtcan_isa.c	(Revision 1564)
+++ ksrc/drivers/can/sja1000/rtcan_isa.c	(Arbeitskopie)
@@ -56,7 +56,7 @@ static u8 ocr[RTCAN_ISA_MAX_DEV];
 static u8 cdr[RTCAN_ISA_MAX_DEV];

 compat_module_short_param_array(isa, RTCAN_ISA_MAX_DEV);
-compat_module_byte_param_array(irq, RTCAN_ISA_MAX_DEV);
+compat_module_int_param_array(irq, RTCAN_ISA_MAX_DEV);
 compat_module_int_param_array(clock, RTCAN_ISA_MAX_DEV);
 compat_module_byte_param_array(ocr, RTCAN_ISA_MAX_DEV);
 compat_module_byte_param_array(cdr, RTCAN_ISA_MAX_DEV);
@@ -187,9 +187,10 @@ static void __exit rtcan_isa_exit(void)
     int i;
     struct rtcan_device *dev;

-    for (i = 0, dev = rtcan_isa_devs[i];
-	 i < RTCAN_ISA_MAX_DEV && dev != NULL;
-	 i++) {
+    for (i = 0; i < RTCAN_ISA_MAX_DEV; i++) {
+	dev = rtcan_isa_devs[i];
+	if (!dev)
+	    continue;
 	rtcan_sja1000_unregister(dev);
     }
 }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

[Xenomai-core] Re: CAN updates & questions

[Wolfgang Grandegger]

Jan Kiszka wrote:
> Hi Wolfgang,
> 
> as one result of a hacking session on a PPC405 with SJA1000 on board I
> applied two minor fixes to rtcan_isa.c to SVN, see end of mail for
> reference. The first one gave an "interesting" effect on big-endian
> because irq is an integer, the second one reveals that we should do some
> multiport test with the ISA driver soon.

OK, I see. Unfortunately I do not have a ISA CAN device for testing.

> Questions arose regarding the meaning of rtcan_device.can_sys_clock
> (BTW, comment in rtcan_dev.h is wrong). What clock rate does it
> describe? For the SJA1000 drivers its obviously clock/2. I'm asking
> because of a) the output in rtcan_calc_bit_time() and b) the module
> parameter of rtcan_isa and its description. We first hacked 16 MHz into
> the latter yesterday as I didn't recall quickly enough that our Phytec
> board also runs at 16 MHZ, thus the default value of 8 MHZ was already
> correct. Confused? At least we were...

You have to define the real CAN system clock, which is 16/2 = 8 Mhz for 
most SJA 1000 hardware even if the oscillator is running at 16 MHz. I 
will add some reasonable note to rtcan_dev.h

> And another suggestion: In order make module names shorter, what about
> the renaming xeno_rtcan* -> xeno_can*?

Fine for me.

> Then we will soon have to discuss how to deal with a rtcan_isa
> derivative that uses ioremapped memory instead of ports (naming,
> separation or integration).

We could add a generic device similar to ISA (or extend ISA accordingly).

> 
> Jan
> 

Wolfgang.

> --
> 
> Index: ksrc/drivers/can/sja1000/rtcan_isa.c
> ===================================================================
> --- ksrc/drivers/can/sja1000/rtcan_isa.c	(Revision 1564)
> +++ ksrc/drivers/can/sja1000/rtcan_isa.c	(Arbeitskopie)
> @@ -56,7 +56,7 @@ static u8 ocr[RTCAN_ISA_MAX_DEV];
>  static u8 cdr[RTCAN_ISA_MAX_DEV];
> 
>  compat_module_short_param_array(isa, RTCAN_ISA_MAX_DEV);
> -compat_module_byte_param_array(irq, RTCAN_ISA_MAX_DEV);
> +compat_module_int_param_array(irq, RTCAN_ISA_MAX_DEV);
>  compat_module_int_param_array(clock, RTCAN_ISA_MAX_DEV);
>  compat_module_byte_param_array(ocr, RTCAN_ISA_MAX_DEV);
>  compat_module_byte_param_array(cdr, RTCAN_ISA_MAX_DEV);
> @@ -187,9 +187,10 @@ static void __exit rtcan_isa_exit(void)
>      int i;
>      struct rtcan_device *dev;
> 
> -    for (i = 0, dev = rtcan_isa_devs[i];
> -	 i < RTCAN_ISA_MAX_DEV && dev != NULL;
> -	 i++) {
> +    for (i = 0; i < RTCAN_ISA_MAX_DEV; i++) {
> +	dev = rtcan_isa_devs[i];
> +	if (!dev)
> +	    continue;
>  	rtcan_sja1000_unregister(dev);
>      }
>  }
> 

Re: [Xenomai-core] Re: CAN updates & questions

[Matthias Fuchs]

Hi Wolfgang,

Wolfgang Grandegger wrote:
> You have to define the real CAN system clock, which is 16/2 = 8 Mhz for
> most SJA 1000 hardware even if the oscillator is running at 16 MHz. I
> will add some reasonable note to rtcan_dev.h
Is there any special reason for this? Wouldn't it be more meaningful to pass 
the SJA1000 externally applied clock frequency? I can imagine that others 
will also run into this issue as Jan and I did yesterday ;-)
>
> > Then we will soon have to discuss how to deal with a rtcan_isa
> > derivative that uses ioremapped memory instead of ports (naming,
> > separation or integration).
>
> We could add a generic device similar to ISA (or extend ISA accordingly).
The SJA1000 isa driver is very simple - and so is the modified version for the 
memory mapped SJA1000. I think that merging both ways of access to the 
SJA1000 into a single driver will make the code much more dirty. I would 
prefer different source files (= different drivers). 

It would be a compromise to add support for boards that use some kind of 
indirect addressing to access the SJA1000 (address + data register) into the 
isa and mem versions of the driver.

One more proposal: I think many (old) ISA drivers name the module parameter 
for the ISA io port "io" instead of "isa". For the memory mapped SJA1000 
driver, I'd like to call the parameter "mem".

Matthias

Re: [Xenomai-core] Re: CAN updates & questions

[Wolfgang Grandegger]

Hi Matthias,

Matthias Fuchs wrote:
> Hi Wolfgang,
> 
> Wolfgang Grandegger wrote:
>> You have to define the real CAN system clock, which is 16/2 = 8 Mhz for
>> most SJA 1000 hardware even if the oscillator is running at 16 MHz. I
>> will add some reasonable note to rtcan_dev.h
> Is there any special reason for this? Wouldn't it be more meaningful to pass 
> the SJA1000 externally applied clock frequency? I can imagine that others 
> will also run into this issue as Jan and I did yesterday ;-)

The frequency is used to calculate reasonable bit-timing parameters, not 
only for SJA 1000. It's used in a similar way in the Linux socket CAN 
driver.

>>> Then we will soon have to discuss how to deal with a rtcan_isa
>>> derivative that uses ioremapped memory instead of ports (naming,
>>> separation or integration).
>> We could add a generic device similar to ISA (or extend ISA accordingly).
> The SJA1000 isa driver is very simple - and so is the modified version for the 
> memory mapped SJA1000. I think that merging both ways of access to the 
> SJA1000 into a single driver will make the code much more dirty. I would 
> prefer different source files (= different drivers). 

I agree.

> It would be a compromise to add support for boards that use some kind of 
> indirect addressing to access the SJA1000 (address + data register) into the 
> isa and mem versions of the driver.

Fine if this could be done in a generic way. We also should add a 
generic PCI driver sooner than later.

> One more proposal: I think many (old) ISA drivers name the module parameter 
> for the ISA io port "io" instead of "isa". For the memory mapped SJA1000 
> driver, I'd like to call the parameter "mem".

Well, I cannot remember why I used the name "isa". Common is "io" or 
"port" or "ioport", indeed. "io" and "mem" or "ioport" and "iomem" would 
be fine for me.

Wolfgang.

Re: [Xenomai-core] Re: CAN updates & questions

[Wolfgang Grandegger]

Wolfgang Grandegger wrote:
> Hi Matthias,
> 
> Matthias Fuchs wrote:
>> Hi Wolfgang,
>>
>> Wolfgang Grandegger wrote:
>>> You have to define the real CAN system clock, which is 16/2 = 8 Mhz for
>>> most SJA 1000 hardware even if the oscillator is running at 16 MHz. I
>>> will add some reasonable note to rtcan_dev.h
>> Is there any special reason for this? Wouldn't it be more meaningful 
>> to pass the SJA1000 externally applied clock frequency? I can imagine 
>> that others will also run into this issue as Jan and I did yesterday ;-)
> 
> The frequency is used to calculate reasonable bit-timing parameters, not 
> only for SJA 1000. It's used in a similar way in the Linux socket CAN 
> driver.

But the value passed via module parameter "clock" could be the more 
meaning full clock frequency for SJA 1000 drivers, of course.

>>>> Then we will soon have to discuss how to deal with a rtcan_isa
>>>> derivative that uses ioremapped memory instead of ports (naming,
>>>> separation or integration).
>>> We could add a generic device similar to ISA (or extend ISA 
>>> accordingly).
>> The SJA1000 isa driver is very simple - and so is the modified version 
>> for the memory mapped SJA1000. I think that merging both ways of 
>> access to the SJA1000 into a single driver will make the code much 
>> more dirty. I would prefer different source files (= different drivers). 
> 
> I agree.
> 
>> It would be a compromise to add support for boards that use some kind 
>> of indirect addressing to access the SJA1000 (address + data register) 
>> into the isa and mem versions of the driver.
> 
> Fine if this could be done in a generic way. We also should add a 
> generic PCI driver sooner than later.
> 
>> One more proposal: I think many (old) ISA drivers name the module 
>> parameter for the ISA io port "io" instead of "isa". For the memory 
>> mapped SJA1000 driver, I'd like to call the parameter "mem".
> 
> Well, I cannot remember why I used the name "isa". Common is "io" or 
> "port" or "ioport", indeed. "io" and "mem" or "ioport" and "iomem" would 
> be fine for me.
> 
> Wolfgang.
> 
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
> 
> 

Re: [Xenomai-core] Re: CAN updates & questions

[Matthias Fuchs]

Hi Wolfgang,

Wolfgang Grandegger wrote:
> Hi Matthias,
> 
> Matthias Fuchs wrote:
>> Hi Wolfgang,
>>
>> Wolfgang Grandegger wrote:
>>> You have to define the real CAN system clock, which is 16/2 = 8 Mhz for
>>> most SJA 1000 hardware even if the oscillator is running at 16 MHz. I
>>> will add some reasonable note to rtcan_dev.h
>> Is there any special reason for this? Wouldn't it be more meaningful
>> to pass the SJA1000 externally applied clock frequency? I can imagine
>> that others will also run into this issue as Jan and I did yesterday ;-)
> 
> The frequency is used to calculate reasonable bit-timing parameters, not
> only for SJA 1000. It's used in a similar way in the Linux socket CAN
> driver.
I am not very familiar with the socket CAN driver, but after a quick
glance at its svn repository it seems to me that they are also passing
the SJA1000 external clock frequency to the module (clk-parameter).

So I think other controller's drivers may use an other meaning of the
clk/clock parameter. But for SJA1000 the ext. clock frequency is best.
> 
>>>> Then we will soon have to discuss how to deal with a rtcan_isa
>>>> derivative that uses ioremapped memory instead of ports (naming,
>>>> separation or integration).
>>> We could add a generic device similar to ISA (or extend ISA
>>> accordingly).
>> The SJA1000 isa driver is very simple - and so is the modified version
>> for the memory mapped SJA1000. I think that merging both ways of
>> access to the SJA1000 into a single driver will make the code much
>> more dirty. I would prefer different source files (= different drivers). 
> 
> I agree.
> 
>> It would be a compromise to add support for boards that use some kind
>> of indirect addressing to access the SJA1000 (address + data register)
>> into the isa and mem versions of the driver.
> 
> Fine if this could be done in a generic way. We also should add a
> generic PCI driver sooner than later.
> 
>> One more proposal: I think many (old) ISA drivers name the module
>> parameter for the ISA io port "io" instead of "isa". For the memory
>> mapped SJA1000 driver, I'd like to call the parameter "mem".
> 
> Well, I cannot remember why I used the name "isa". Common is "io" or
> "port" or "ioport", indeed. "io" and "mem" or "ioport" and "iomem" would
> be fine for me.
I will use mem for the mem'mapped driver. The socket CAN project uses
base_addr - they must have an affection to a long parameter name :-)

Matthias