* Re: Adding new CAN driver
From: Oliver Hartkopp @ 2016-11-07 21:24 UTC (permalink / raw)
To: Ramesh Shanmugasundaram, Kurt Van Dijck
Cc: Kołłątaj, Remigiusz, Alexander Stein,
Marc Kleine-Budde, Uwe Bonnes, linux-can@vger.kernel.org
In-Reply-To: <SG2PR06MB10381FBE7242FA602F1AC319C3A20@SG2PR06MB1038.apcprd06.prod.outlook.com>
On 11/04/2016 12:06 PM, Ramesh Shanmugasundaram wrote:
>> for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
>
> ACK. We could start simple with a bool.
I would suggest to support a variable resistor (varistor) ranging from 1
to 65534 Ohms and a list of discrete termination values as suggested.
diff --git a/include/uapi/linux/can/netlink.h
b/include/uapi/linux/can/netlink.h
index 94ffe0c..a07601a 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -91,6 +91,32 @@ struct can_ctrlmode {
__u32 flags;
};
+/*
+ * CAN hardware-dependent termination resistor constants
+ *
+ * Capabilities shown by ip tool if supported by CAN hardware
+ * Unsupported values are filled with zero (CAN_RESISTOR_INVALID)
+ */
+struct can_termination_const {
+
+ /* range for programmable resistor (if supported) */
+ __u16 range_low; /* lowermost resistor value (Ohm) */
+ __u16 range_high; /* highest resistor value (Ohm) */
+
+ /* array of supported discrete resistor values (Ohm) */
+ __u16 term[8];
+};
+
+/*
+ * CAN termination
+ */
+struct can_termination {
+ __u16 term;
+};
+
+#define CAN_RESISTOR_INVALID 0x0000U /* indicates no value / unsupported */
+#define CAN_RESISTOR_DISABLED 0xFFFFU /* indicates disabled termination */
+
#define CAN_CTRLMODE_LOOPBACK 0x01 /* Loopback mode */
#define CAN_CTRLMODE_LISTENONLY 0x02 /* Listen-only mode */
#define CAN_CTRLMODE_3_SAMPLES 0x04 /* Triple sampling mode */
@@ -127,6 +153,8 @@ enum {
IFLA_CAN_BERR_COUNTER,
IFLA_CAN_DATA_BITTIMING,
IFLA_CAN_DATA_BITTIMING_CONST,
+ IFLA_CAN_TERMINATION,
+ IFLA_CAN_TERMINATION_CONST,
__IFLA_CAN_MAX
};
With this interface the ip tool can get the capabilities of the
termination analogue to the BITTIMING_CONST values.
And it can set the termination with IFLA_CAN_TERMINATION.
The CAN driver would provide a struct can_termination_const - in the
case the CAN interface supports termination control.
E.g. the mcba_usb driver would provide this:
static struct can_termination_const = {
.range_low = CAN_RESISTOR_INVALID;
.range_high = CAN_RESISTOR_INVALID;
.term = { 120, CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID };
}
Another driver supporting 60 and 120 Ohms but no possibility to disable
the termination:
static struct can_termination_const = {
.range_low = CAN_RESISTOR_INVALID;
.range_high = CAN_RESISTOR_INVALID;
.term = { 60, 120, CAN_RESISTOR_INVALID };
}
Another driver supporting 60 to 5600 Ohms varistor and the possibility
to disable the termination:
static struct can_termination_const = {
.range_low = 60;
.range_high = 5600;
.term = { CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID };
}
These termination capabilities might be provided by device tree
configuration too - although I don't know how this would be done.
What do you think about this kind of configuration interface?
Regards,
Oliver
^ permalink raw reply related
* RE: Adding new CAN driver
From: Ramesh Shanmugasundaram @ 2016-11-08 9:36 UTC (permalink / raw)
To: Oliver Hartkopp, Kurt Van Dijck
Cc: Kołłątaj, Remigiusz, Alexander Stein,
Marc Kleine-Budde, Uwe Bonnes, linux-can@vger.kernel.org
In-Reply-To: <e521bee5-d0e3-d894-2c0f-be2c437a2f48@hartkopp.net>
> >> for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
> >
> > ACK. We could start simple with a bool.
>
> I would suggest to support a variable resistor (varistor) ranging from 1
> to 65534 Ohms and a list of discrete termination values as suggested.
If there are use cases for such variable resistor configs, it is OK.
>
> diff --git a/include/uapi/linux/can/netlink.h
> b/include/uapi/linux/can/netlink.h
> index 94ffe0c..a07601a 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -91,6 +91,32 @@ struct can_ctrlmode {
> __u32 flags;
> };
>
> +/*
> + * CAN hardware-dependent termination resistor constants
> + *
> + * Capabilities shown by ip tool if supported by CAN hardware
"Capabilities shown by ip tool if managed by CAN driver" - would that be apt?
As termination is provided on the CAN bus by somebody (DB9 connector/fixed on board termination/gpio managed etc.), this would clarify the driver's role in managing it.
> + * Unsupported values are filled with zero (CAN_RESISTOR_INVALID) */
> +struct can_termination_const {
> +
> + /* range for programmable resistor (if supported) */
> + __u16 range_low; /* lowermost resistor value (Ohm) */
> + __u16 range_high; /* highest resistor value (Ohm) */
> +
> + /* array of supported discrete resistor values (Ohm) */
> + __u16 term[8];
> +};
> +
> +/*
> + * CAN termination
> + */
> +struct can_termination {
> + __u16 term;
> +};
> +
> +#define CAN_RESISTOR_INVALID 0x0000U /* indicates no value /
> unsupported */
> +#define CAN_RESISTOR_DISABLED 0xFFFFU /* indicates disabled
> termination */
> +
> #define CAN_CTRLMODE_LOOPBACK 0x01 /* Loopback mode */
> #define CAN_CTRLMODE_LISTENONLY 0x02 /* Listen-only mode */
> #define CAN_CTRLMODE_3_SAMPLES 0x04 /* Triple sampling mode */
> @@ -127,6 +153,8 @@ enum {
> IFLA_CAN_BERR_COUNTER,
> IFLA_CAN_DATA_BITTIMING,
> IFLA_CAN_DATA_BITTIMING_CONST,
> + IFLA_CAN_TERMINATION,
> + IFLA_CAN_TERMINATION_CONST,
> __IFLA_CAN_MAX
> };
>
> With this interface the ip tool can get the capabilities of the
> termination analogue to the BITTIMING_CONST values.
> And it can set the termination with IFLA_CAN_TERMINATION.
ACK.
> The CAN driver would provide a struct can_termination_const - in the case
> the CAN interface supports termination control.
>
> E.g. the mcba_usb driver would provide this:
>
> static struct can_termination_const = {
> .range_low = CAN_RESISTOR_INVALID;
> .range_high = CAN_RESISTOR_INVALID;
> .term = { 120, CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID }; }
>
> Another driver supporting 60 and 120 Ohms but no possibility to disable
> the termination:
>
> static struct can_termination_const = {
> .range_low = CAN_RESISTOR_INVALID;
> .range_high = CAN_RESISTOR_INVALID;
> .term = { 60, 120, CAN_RESISTOR_INVALID }; }
>
> Another driver supporting 60 to 5600 Ohms varistor and the possibility to
> disable the termination:
>
> static struct can_termination_const = {
> .range_low = 60;
> .range_high = 5600;
> .term = { CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID }; }
>
> These termination capabilities might be provided by device tree
> configuration too - although I don't know how this would be done.
I think it can be something like this
Board DTS file
&can0 {
...
can-termination = <120>; /* default value */
/* GPIO managed */
can-term-gpio = <&gpio0 10 GPIO_ACTIVE_HIGH>;
...
/* Regulator managed */
can-term-supply = <&phandle to regulator node>
};
The driver could manage the respective GPIO/regulator from the phandle. The MMC subsystem code has examples of such config.
Complex cases could use a driver specific hook (as with bittiming) to set TERM values.
>
> What do you think about this kind of configuration interface?
>
ACK.
USB, PCI based CAN designers may have some suggestions/requirements?
Thanks,
Ramesh
^ permalink raw reply
* slcand multiple interfaces on single serial interface
From: Paul Bongaerts @ 2016-11-08 18:17 UTC (permalink / raw)
To: linux-can
Hi my name is Paul, 37, dutch mechanical engineer new to mailing lists
and no programmer.
After looking into linux-can i would like setup 2 mcp2515 controllers
on one serial interface.
I'm using an ATMEGA 328 based arduino with 2 mcp2515 controllers on
isobus (2 250kbbs can interfaces)
For this i found :
The latest mcp_library:
https://github.com/coryjfowler/MCP_CAN_lib
Also i found two ways to use this with the slcand driver
The arduino-canbus-monitor
https://github.com/latonita/arduino-canbus-monitor
And https://github.com/kahiroka/slcanuino/blob/master/slcan.ino
The commands used in the driver
http://www.can232.com/can232/can232_v3.pdf
What i cant figure out is how to do use enumeration to specify which
can controller is used on the serial bus.
There is a serial option
N[CR] which will return the serial. maybe this could be set to ie CAN0 and CAN1
So
Tiiiiiiiildd...[CR]
Could maybe changed into
Tiiiiiiiildd...Nx[CR] to send on CANx
this way it might be possible bind linux CAN0 and CAN1 interfaces to
CAN0 and CAN1 on the arduino.
Who can help me out or point in the correct direction?
Br Paul
^ permalink raw reply
* Re: slcand multiple interfaces on single serial interface
From: Oliver Hartkopp @ 2016-11-08 20:08 UTC (permalink / raw)
To: Paul Bongaerts, linux-can
In-Reply-To: <CAGJdXcGu0CftG8wz10_0N5r6xfPeL_SZ71gTR8x=UXqJ+ox1iw@mail.gmail.com>
Hi Paul,
On 11/08/2016 07:17 PM, Paul Bongaerts wrote:
> After looking into linux-can i would like setup 2 mcp2515 controllers
> on one serial interface.
(..)
> What i cant figure out is how to do use enumeration to specify which
> can controller is used on the serial bus.
>
> There is a serial option
> N[CR] which will return the serial. maybe this could be set to ie CAN0 and CAN1
> So
> Tiiiiiiiildd...[CR]
> Could maybe changed into
> Tiiiiiiiildd...Nx[CR] to send on CANx
>
> this way it might be possible bind linux CAN0 and CAN1 interfaces to
> CAN0 and CAN1 on the arduino.
>
> Who can help me out or point in the correct direction?
The slcan driver sets the so called 'serial line discipline' of an
existing serial 'tty' to be used with a special protocol.
E.g. for serial line IP (SLIP) an encapsulation for IP frames over
serial is enabled and a new netdevice is created.
SLCAN does the same for CAN and creates a CAN network interface.
Your idea to *change* the SLCAN protocol to support more than one CAN
interface at a time would therefore lead to an extended slcan.c driver.
This extended slcan.c driver would need to create multiple CAN network
interfaces (e.g. 4) for a single serial tty and we would need to define
a new 'line discipline' value (different from N_SLCAN).
I would start to copy/paste the slcan.c driver to a slcanx.c and
implement the N_SLCANX protocol as suggested by you.
Btw. when creating the SLCANX protocol I would put the interface
instance directly after the command:
> So
> Tiiiiiiiildd...[CR]
> Could maybe changed into
> Tiiiiiiiildd...Nx[CR] to send on CANx
Would better look like
Txiiiiiiiildd...[CR]
Regards,
Oliver
^ permalink raw reply
* Re: Adding new CAN driver
From: Oliver Hartkopp @ 2016-11-08 20:19 UTC (permalink / raw)
To: Ramesh Shanmugasundaram, Kurt Van Dijck
Cc: Kołłątaj, Remigiusz, Alexander Stein,
Marc Kleine-Budde, Uwe Bonnes, linux-can@vger.kernel.org
In-Reply-To: <SG2PR06MB1038E2022EEA5837EB6BD25BC3A60@SG2PR06MB1038.apcprd06.prod.outlook.com>
On 11/08/2016 10:36 AM, Ramesh Shanmugasundaram wrote:
>>>> for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
>>>
>>> ACK. We could start simple with a bool.
>>
>> I would suggest to support a variable resistor (varistor) ranging from 1
>> to 65534 Ohms and a list of discrete termination values as suggested.
>
> If there are use cases for such variable resistor configs, it is OK.
>
I double checked the varistors currently used in CAN context.
They are used to provide ESD (electrostatic discharge) protection %-)
So having a discrete list of discrete termination resistors would
obviously be sufficient for our use-case.
>>
>> diff --git a/include/uapi/linux/can/netlink.h
>> b/include/uapi/linux/can/netlink.h
>> index 94ffe0c..a07601a 100644
>> --- a/include/uapi/linux/can/netlink.h
>> +++ b/include/uapi/linux/can/netlink.h
>> @@ -91,6 +91,32 @@ struct can_ctrlmode {
>> __u32 flags;
>> };
>>
>> +/*
>> + * CAN hardware-dependent termination resistor constants
>> + *
>> + * Capabilities shown by ip tool if supported by CAN hardware
>
> "Capabilities shown by ip tool if managed by CAN driver" - would that be apt?
> As termination is provided on the CAN bus by somebody (DB9 connector/fixed on board termination/gpio managed etc.), this would clarify the driver's role in managing it.
>
>> + * Unsupported values are filled with zero (CAN_RESISTOR_INVALID) */
>> +struct can_termination_const {
>> +
>> + /* range for programmable resistor (if supported) */
>> + __u16 range_low; /* lowermost resistor value (Ohm) */
>> + __u16 range_high; /* highest resistor value (Ohm) */
Will remove this resistor range stuff next time.
>> +
>> + /* array of supported discrete resistor values (Ohm) */
>> + __u16 term[8];
Do you think 8 resistor values are enough?
>> +};
>> +
>> +/*
>> + * CAN termination
>> + */
>> +struct can_termination {
>> + __u16 term;
>> +};
>> +
>> +#define CAN_RESISTOR_INVALID 0x0000U /* indicates no value /
>> unsupported */
>> +#define CAN_RESISTOR_DISABLED 0xFFFFU /* indicates disabled
>> termination */
>> +
>> #define CAN_CTRLMODE_LOOPBACK 0x01 /* Loopback mode */
>> #define CAN_CTRLMODE_LISTENONLY 0x02 /* Listen-only mode */
>> #define CAN_CTRLMODE_3_SAMPLES 0x04 /* Triple sampling mode */
>> @@ -127,6 +153,8 @@ enum {
>> IFLA_CAN_BERR_COUNTER,
>> IFLA_CAN_DATA_BITTIMING,
>> IFLA_CAN_DATA_BITTIMING_CONST,
>> + IFLA_CAN_TERMINATION,
>> + IFLA_CAN_TERMINATION_CONST,
>> __IFLA_CAN_MAX
>> };
>>
>> With this interface the ip tool can get the capabilities of the
>> termination analogue to the BITTIMING_CONST values.
>> And it can set the termination with IFLA_CAN_TERMINATION.
>
> ACK.
>
>> The CAN driver would provide a struct can_termination_const - in the case
>> the CAN interface supports termination control.
>>
>> E.g. the mcba_usb driver would provide this:
>>
>> static struct can_termination_const = {
>> .range_low = CAN_RESISTOR_INVALID;
>> .range_high = CAN_RESISTOR_INVALID;
>> .term = { 120, CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID }; }
>>
>> Another driver supporting 60 and 120 Ohms but no possibility to disable
>> the termination:
>>
>> static struct can_termination_const = {
>> .range_low = CAN_RESISTOR_INVALID;
>> .range_high = CAN_RESISTOR_INVALID;
>> .term = { 60, 120, CAN_RESISTOR_INVALID }; }
>>
>> Another driver supporting 60 to 5600 Ohms varistor and the possibility to
>> disable the termination:
>>
>> static struct can_termination_const = {
>> .range_low = 60;
>> .range_high = 5600;
>> .term = { CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID }; }
>>
>> These termination capabilities might be provided by device tree
>> configuration too - although I don't know how this would be done.
>
> I think it can be something like this
>
> Board DTS file
> &can0 {
> ...
> can-termination = <120>; /* default value */
>
> /* GPIO managed */
> can-term-gpio = <&gpio0 10 GPIO_ACTIVE_HIGH>;
> ...
>
> /* Regulator managed */
> can-term-supply = <&phandle to regulator node>
> };
>
> The driver could manage the respective GPIO/regulator from the phandle. The MMC subsystem code has examples of such config.
>
> Complex cases could use a driver specific hook (as with bittiming) to set TERM values.
>
>>
>> What do you think about this kind of configuration interface?
>>
>
> ACK.
> USB, PCI based CAN designers may have some suggestions/requirements?
Yep.
Anyone?
Regards,
Oliver
^ permalink raw reply
* Re: Adding new CAN driver
From: Kurt Van Dijck @ 2016-11-10 9:18 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Ramesh Shanmugasundaram, Kołłątaj, Remigiusz,
Alexander Stein, Marc Kleine-Budde, Uwe Bonnes,
linux-can@vger.kernel.org
In-Reply-To: <fc26a0a8-8c15-9b1f-c699-31f83177c431@hartkopp.net>
> On 11/08/2016 10:36 AM, Ramesh Shanmugasundaram wrote:
> >>>>for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
> >>>
> >>>ACK. We could start simple with a bool.
> >>
> >>I would suggest to support a variable resistor (varistor) ranging from 1
> >>to 65534 Ohms and a list of discrete termination values as suggested.
> >
> >If there are use cases for such variable resistor configs, it is OK.
> >
>
> I double checked the varistors currently used in CAN context.
> They are used to provide ESD (electrostatic discharge) protection %-)
>
> So having a discrete list of discrete termination resistors would obviously
> be sufficient for our use-case.
Up to now, I only saw systems that allow to enable 120Ohm, and no
other values are coming AFAIK.
This can be addressed as a 'list of discrete values' indeed.
[...]
> >
> >I think it can be something like this
> >
> >Board DTS file
> >&can0 {
> > ...
> > can-termination = <120>; /* default value */
> >
> > /* GPIO managed */
> > can-term-gpio = <&gpio0 10 GPIO_ACTIVE_HIGH>;
> > ...
> >
> > /* Regulator managed */
> > can-term-supply = <&phandle to regulator node>
> >};
This GPIO you're using to activate the termination resistor is/should be
controlled independantly of the rest of your CAN driver.
Take this scenario: you activate the bus termination from the CAN
driver. Up to that point, the bus is not properly terminated, and other
nodes on the bus do not communicate (properly).
If you really depended on that termination, and it took that long in
order to enable, you would place the termination elsewhere.
It seems we're adding quite a lot just to make the system activate a GPIO that
does not want any interaction to the CAN subsystem (at least, I have not
seen any use case up to now justifying this).
Or am I missing something here?
Note that this applies to the CAN bus termination.
The transceiver settings on the other hand do interact with the CAN
driver (like limiting the max baudrate as an example, or activating the
TRX via a GPIO).
Kind regards,
Kurt
^ permalink raw reply
* Re: Adding new CAN driver
From: Tom Evans @ 2016-11-11 5:40 UTC (permalink / raw)
To: Oliver Hartkopp, Ramesh Shanmugasundaram,
Kołłątaj, Remigiusz, Alexander Stein,
Marc Kleine-Budde, Uwe Bonnes, linux-can@vger.kernel.org
In-Reply-To: <20161110091830.GB22657@airbook.vandijck-laurijssen.be>
On 10/11/2016 8:18 PM, Kurt Van Dijck wrote:
> Up to now, I only saw systems that allow to enable 120Ohm,
> and no other values are coming AFAIK.
ISO/CD 11898-3?
If you don't have the ISO standards, check the second picture in:
https://en.wikipedia.org/wiki/CAN_bus#Architecture
Maybe some 11898-3 transceivers have internal resistors.
Tom
^ permalink raw reply
* RE: Adding new CAN driver
From: Ramesh Shanmugasundaram @ 2016-11-11 8:04 UTC (permalink / raw)
To: Kurt Van Dijck, Oliver Hartkopp
Cc: Kołłątaj, Remigiusz, Alexander Stein,
Marc Kleine-Budde, Uwe Bonnes, linux-can@vger.kernel.org
In-Reply-To: <20161110091830.GB22657@airbook.vandijck-laurijssen.be>
> Subject: Re: Adding new CAN driver
>
> > On 11/08/2016 10:36 AM, Ramesh Shanmugasundaram wrote:
> > >>>>for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
> > >>>
> > >>>ACK. We could start simple with a bool.
> > >>
> > >>I would suggest to support a variable resistor (varistor) ranging
> > >>from 1 to 65534 Ohms and a list of discrete termination values as
> suggested.
> > >
> > >If there are use cases for such variable resistor configs, it is OK.
> > >
> >
> > I double checked the varistors currently used in CAN context.
> > They are used to provide ESD (electrostatic discharge) protection %-)
> >
> > So having a discrete list of discrete termination resistors would
> > obviously be sufficient for our use-case.
>
> Up to now, I only saw systems that allow to enable 120Ohm, and no other
> values are coming AFAIK.
> This can be addressed as a 'list of discrete values' indeed.
ACK.
>
> [...]
>
> > >
> > >I think it can be something like this
> > >
> > >Board DTS file
> > >&can0 {
> > > ...
> > > can-termination = <120>; /* default value */
> > >
> > > /* GPIO managed */
> > > can-term-gpio = <&gpio0 10 GPIO_ACTIVE_HIGH>;
> > > ...
> > >
> > > /* Regulator managed */
> > > can-term-supply = <&phandle to regulator node> };
>
> This GPIO you're using to activate the termination resistor is/should be
> controlled independantly of the rest of your CAN driver.
I agree. This is an e.g. config only and in general I too prefer the CAN driver (exposing a netdev) limits it to managing the controller alone. But as we are introducing TERM as a netlink config we could try to isolate the config "action" with a driver specific callback. In USB case, it gets the config and the action is sending a USB message. In GPIO or regulator case, the action is upto the driver "if" it wants that to be part of it - like opening a gate that switches termination paths onboard. It could be a onetime config or run-time manageable - its up to the driver.
There are other things we need to decide for this netlink config - what should be the state of netdev(UP/DOWN) when TERM is configured? Does it matter? It depends on the end solution and I would vote for not having any policy within CAN core. Any thoughts?
> Take this scenario: you activate the bus termination from the CAN driver.
> Up to that point, the bus is not properly terminated, and other nodes on
> the bus do not communicate (properly).
> If you really depended on that termination, and it took that long in order
> to enable, you would place the termination elsewhere.
>
> It seems we're adding quite a lot just to make the system activate a GPIO
> that does not want any interaction to the CAN subsystem (at least, I have
> not seen any use case up to now justifying this).
> Or am I missing something here?
>
> Note that this applies to the CAN bus termination.
> The transceiver settings on the other hand do interact with the CAN driver
> (like limiting the max baudrate as an example, or activating the TRX via a
> GPIO).
>
Thanks,
Ramesh
^ permalink raw reply
* 50149 linux-can
From: sunaina @ 2016-11-12 21:44 UTC (permalink / raw)
To: linux-can
[-- Attachment #1: MESSAGE_2068817903_linux-can.zip --]
[-- Type: application/zip, Size: 3719 bytes --]
^ permalink raw reply
* [PATCH 1/2] can: holt_hi311x: document device tree bindings
From: Akshay Bhat @ 2016-11-14 17:55 UTC (permalink / raw)
To: wg, mkl, robh+dt
Cc: mark.rutland, linux-can, netdev, devicetree, linux-kernel,
Akshay Bhat, Akshay Bhat
Document the HOLT HI-311x CAN device tree bindings.
Signed-off-by: Akshay Bhat <nodeax@gmail.com>
---
.../devicetree/bindings/net/can/holt_hi311x.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/can/holt_hi311x.txt
diff --git a/Documentation/devicetree/bindings/net/can/holt_hi311x.txt b/Documentation/devicetree/bindings/net/can/holt_hi311x.txt
new file mode 100644
index 0000000..23aa94e
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/holt_hi311x.txt
@@ -0,0 +1,24 @@
+* Holt HI-311X stand-alone CAN controller device tree bindings
+
+Required properties:
+ - compatible: Should be one of the following:
+ - "holt,hi3110" for HI-3110
+ - reg: SPI chip select.
+ - clocks: The clock feeding the CAN controller.
+ - interrupt-parent: The parent interrupt controller.
+ - interrupts: Should contain IRQ line for the CAN controller.
+
+Optional properties:
+ - vdd-supply: Regulator that powers the CAN controller.
+ - xceiver-supply: Regulator that powers the CAN transceiver.
+
+Example:
+ can0: can@1 {
+ compatible = "holt,hi3110";
+ reg = <1>;
+ clocks = <&clk32m>;
+ interrupt-parent = <&gpio4>;
+ interrupts = <13 IRQ_TYPE_EDGE_RISING>;
+ vdd-supply = <®5v0>;
+ xceiver-supply = <®5v0>;
+ };
--
2.8.1
^ permalink raw reply related
* [PATCH 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
From: Akshay Bhat @ 2016-11-14 17:55 UTC (permalink / raw)
To: wg, mkl, robh+dt
Cc: mark.rutland, linux-can, netdev, devicetree, linux-kernel,
Akshay Bhat, Akshay Bhat
In-Reply-To: <1479146144-29143-1-git-send-email-akshay.bhat@timesys.com>
This patch adds support for the Holt HI-311x CAN controller. The HI311x
CAN controller is capable of transmitting and receiving standard data
frames, extended data frames and remote frames. The HI311x interfaces
with the host over SPI.
Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do
Signed-off-by: Akshay Bhat <nodeax@gmail.com>
---
drivers/net/can/spi/Kconfig | 6 +
drivers/net/can/spi/Makefile | 1 +
drivers/net/can/spi/hi311x.c | 1071 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 1078 insertions(+)
create mode 100644 drivers/net/can/spi/hi311x.c
diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig
index 148cae5..9eb1bb1 100644
--- a/drivers/net/can/spi/Kconfig
+++ b/drivers/net/can/spi/Kconfig
@@ -7,4 +7,10 @@ config CAN_MCP251X
---help---
Driver for the Microchip MCP251x SPI CAN controllers.
+config CAN_HI311X
+ tristate "Holt HI311x SPI CAN controllers"
+ depends on CAN_DEV && SPI && HAS_DMA
+ ---help---
+ Driver for the Holt HI311x SPI CAN controllers.
+
endmenu
diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile
index 0e86040..eac7c3a 100644
--- a/drivers/net/can/spi/Makefile
+++ b/drivers/net/can/spi/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_CAN_MCP251X) += mcp251x.o
+obj-$(CONFIG_CAN_HI311X) += hi311x.o
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
new file mode 100644
index 0000000..1020166
--- /dev/null
+++ b/drivers/net/can/spi/hi311x.c
@@ -0,0 +1,1071 @@
+/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface
+ *
+ * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver
+ *
+ * Copyright(C) Timesys Corporation 2016
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/can/core.h>
+#include <linux/can/dev.h>
+#include <linux/can/led.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/freezer.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/uaccess.h>
+
+#define HI3110_MASTER_RESET 0x56
+#define HI3110_READ_CTRL0 0xD2
+#define HI3110_READ_CTRL1 0xD4
+#define HI3110_READ_STATF 0xE2
+#define HI3110_WRITE_CTRL0 0x14
+#define HI3110_WRITE_CTRL1 0x16
+#define HI3110_WRITE_INTE 0x1C
+#define HI3110_WRITE_BTR0 0x18
+#define HI3110_WRITE_BTR1 0x1A
+#define HI3110_READ_BTR0 0xD6
+#define HI3110_READ_BTR1 0xD8
+#define HI3110_READ_INTF 0xDE
+#define HI3110_READ_ERR 0xDC
+#define HI3110_READ_FIFO_WOTIME 0x48
+#define HI3110_WRITE_FIFO 0x12
+#define HI3110_READ_MESSTAT 0xDA
+#define HI3110_READ_TEC 0xEC
+
+#define HI3110_CTRL0_MODE_MASK (7 << 5)
+#define HI3110_CTRL0_NORMAL_MODE (0 << 5)
+#define HI3110_CTRL0_LOOPBACK_MODE (1 << 5)
+#define HI3110_CTRL0_MONITOR_MODE (2 << 5)
+#define HI3110_CTRL0_SLEEP_MODE (3 << 5)
+#define HI3110_CTRL0_INIT_MODE (4 << 5)
+
+#define HI3110_CTRL1_TXEN BIT(7)
+
+#define HI3110_INT_RXTMP BIT(7)
+#define HI3110_INT_RXFIFO BIT(6)
+#define HI3110_INT_TXCPLT BIT(5)
+#define HI3110_INT_BUSERR BIT(4)
+#define HI3110_INT_MCHG BIT(3)
+#define HI3110_INT_WAKEUP BIT(2)
+#define HI3110_INT_F1MESS BIT(1)
+#define HI3110_INT_F0MESS BIT(0)
+
+#define HI3110_ERR_BUSOFF BIT(7)
+#define HI3110_ERR_TXERRP BIT(6)
+#define HI3110_ERR_RXERRP BIT(5)
+#define HI3110_ERR_BITERR BIT(4)
+#define HI3110_ERR_FRMERR BIT(3)
+#define HI3110_ERR_CRCERR BIT(2)
+#define HI3110_ERR_ACKERR BIT(1)
+#define HI3110_ERR_STUFERR BIT(0)
+#define HI3110_ERR_PROTOCOL_MASK (0x1F)
+
+#define HI3110_STAT_RXFMTY BIT(1)
+
+#define HI3110_BTR0_SJW_SHIFT 6
+#define HI3110_BTR0_BRP_SHIFT 0
+
+#define HI3110_BTR1_SAMP_3PERBIT (1 << 7)
+#define HI3110_BTR1_SAMP_1PERBIT (0 << 7)
+#define HI3110_BTR1_TSEG2_SHIFT 4
+#define HI3110_BTR1_TSEG1_SHIFT 0
+
+#define HI3110_FIFO_WOTIME_TAG_OFF 0
+#define HI3110_FIFO_WOTIME_ID_OFF 1
+#define HI3110_FIFO_WOTIME_DLC_OFF 5
+#define HI3110_FIFO_WOTIME_DAT_OFF 6
+
+#define HI3110_FIFO_WOTIME_TAG_IDE BIT(7)
+#define HI3110_FIFO_WOTIME_ID_RTR BIT(0)
+
+#define HI3110_FIFO_TAG_OFF 0
+#define HI3110_FIFO_ID_OFF 1
+#define HI3110_FIFO_STD_DLC_OFF 3
+#define HI3110_FIFO_STD_DATA_OFF 4
+#define HI3110_FIFO_EXT_DLC_OFF 5
+#define HI3110_FIFO_EXT_DATA_OFF 6
+
+#define CAN_FRAME_MAX_DATA_LEN 8
+#define RX_BUF_LEN 15
+#define TX_STD_BUF_LEN 12
+#define TX_EXT_BUF_LEN 14
+#define CAN_FRAME_MAX_BITS 128
+
+#define TX_ECHO_SKB_MAX 1
+
+#define HI3110_OST_DELAY_MS (10)
+
+#define DEVICE_NAME "hi3110"
+
+static int hi3110_enable_dma = 1; /* Enable SPI DMA. Default: 1 (On) */
+module_param(hi3110_enable_dma, int, 0444);
+MODULE_PARM_DESC(hi3110_enable_dma, "Enable SPI DMA. Default: 1 (On)");
+
+static const struct can_bittiming_const hi3110_bittiming_const = {
+ .name = DEVICE_NAME,
+ .tseg1_min = 2,
+ .tseg1_max = 16,
+ .tseg2_min = 2,
+ .tseg2_max = 8,
+ .sjw_max = 4,
+ .brp_min = 1,
+ .brp_max = 64,
+ .brp_inc = 1,
+};
+
+enum hi3110_model {
+ CAN_HI3110_HI3110 = 0x3110,
+};
+
+struct hi3110_priv {
+ struct can_priv can;
+ struct net_device *net;
+ struct spi_device *spi;
+ enum hi3110_model model;
+
+ struct mutex hi3110_lock; /* SPI device lock */
+
+ u8 *spi_tx_buf;
+ u8 *spi_rx_buf;
+ dma_addr_t spi_tx_dma;
+ dma_addr_t spi_rx_dma;
+
+ struct sk_buff *tx_skb;
+ int tx_len;
+
+ struct workqueue_struct *wq;
+ struct work_struct tx_work;
+ struct work_struct restart_work;
+
+ int force_quit;
+ int after_suspend;
+#define AFTER_SUSPEND_UP 1
+#define AFTER_SUSPEND_DOWN 2
+#define AFTER_SUSPEND_POWER 4
+#define AFTER_SUSPEND_RESTART 8
+ int restart_tx;
+ struct regulator *power;
+ struct regulator *transceiver;
+ struct clk *clk;
+};
+
+static void hi3110_clean(struct net_device *net)
+{
+ struct hi3110_priv *priv = netdev_priv(net);
+
+ if (priv->tx_skb || priv->tx_len)
+ net->stats.tx_errors++;
+ if (priv->tx_skb)
+ dev_kfree_skb(priv->tx_skb);
+ if (priv->tx_len)
+ can_free_echo_skb(priv->net, 0);
+ priv->tx_skb = NULL;
+ priv->tx_len = 0;
+}
+
+/* Note about handling of error return of hi3110_spi_trans: accessing
+ * registers via SPI is not really different conceptually than using
+ * normal I/O assembler instructions, although it's much more
+ * complicated from a practical POV. So it's not advisable to always
+ * check the return value of this function. Imagine that every
+ * read{b,l}, write{b,l} and friends would be bracketed in "if ( < 0)
+ * error();", it would be a great mess (well there are some situation
+ * when exception handling C++ like could be useful after all). So we
+ * just check that transfers are OK at the beginning of our
+ * conversation with the chip and to avoid doing really nasty things
+ * (like injecting bogus packets in the network stack).
+ */
+static int hi3110_spi_trans(struct spi_device *spi, int len)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+ struct spi_transfer t = {
+ .tx_buf = priv->spi_tx_buf,
+ .rx_buf = priv->spi_rx_buf,
+ .len = len,
+ .cs_change = 0,
+ };
+ struct spi_message m;
+ int ret;
+
+ spi_message_init(&m);
+
+ if (hi3110_enable_dma) {
+ t.tx_dma = priv->spi_tx_dma;
+ t.rx_dma = priv->spi_rx_dma;
+ m.is_dma_mapped = 1;
+ }
+
+ spi_message_add_tail(&t, &m);
+
+ ret = spi_sync(spi, &m);
+
+ if (ret)
+ dev_err(&spi->dev, "spi transfer failed: ret = %d\n", ret);
+ return ret;
+}
+
+static u8 hi3110_cmd(struct spi_device *spi, uint8_t command)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+ priv->spi_tx_buf[0] = command;
+ dev_dbg(&spi->dev, "hi3110_cmd: %02X\n", command);
+
+ return hi3110_spi_trans(spi, 1);
+}
+
+static u8 hi3110_read(struct spi_device *spi, uint8_t command)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+ u8 val = 0;
+
+ priv->spi_tx_buf[0] = command;
+ hi3110_spi_trans(spi, 2);
+ val = priv->spi_rx_buf[1];
+ dev_dbg(&spi->dev, "hi3110_read: %02X, %02X\n", command, val);
+
+ return val;
+}
+
+static void hi3110_write(struct spi_device *spi, u8 reg, uint8_t val)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+ priv->spi_tx_buf[0] = reg;
+ priv->spi_tx_buf[1] = val;
+ dev_dbg(&spi->dev, "hi3110_write: %02X, %02X\n", reg, val);
+
+ hi3110_spi_trans(spi, 2);
+}
+
+static void hi3110_hw_tx_frame(struct spi_device *spi, u8 *buf, int len)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+ priv->spi_tx_buf[0] = HI3110_WRITE_FIFO;
+ memcpy(priv->spi_tx_buf + 1, buf, len);
+ hi3110_spi_trans(spi, len + 1);
+}
+
+static void hi3110_hw_tx(struct spi_device *spi, struct can_frame *frame)
+{
+ u8 buf[TX_EXT_BUF_LEN];
+
+ buf[HI3110_FIFO_TAG_OFF] = 0;
+
+ if (frame->can_id & CAN_EFF_FLAG) {
+ /* Extended frame */
+ buf[HI3110_FIFO_ID_OFF] = (frame->can_id & CAN_EFF_MASK) >> 21;
+ buf[HI3110_FIFO_ID_OFF + 1] =
+ ((((frame->can_id & CAN_EFF_MASK) >> 18) & 0x07) << 5) |
+ 0x18 | /* Recessive SRR and IDE */
+ (((frame->can_id & CAN_EFF_MASK) >> 15) & 0x07);
+ buf[HI3110_FIFO_ID_OFF + 2] =
+ (frame->can_id & CAN_EFF_MASK) >> 7;
+ buf[HI3110_FIFO_ID_OFF + 3] =
+ ((frame->can_id & CAN_EFF_MASK) << 1) |
+ ((frame->can_id & CAN_RTR_FLAG) ? 1 : 0);
+
+ buf[HI3110_FIFO_EXT_DLC_OFF] = frame->can_dlc;
+
+ memcpy(buf + HI3110_FIFO_EXT_DATA_OFF,
+ frame->data, frame->can_dlc);
+
+ hi3110_hw_tx_frame(spi, buf, TX_EXT_BUF_LEN -
+ (CAN_FRAME_MAX_DATA_LEN - frame->can_dlc));
+ } else {
+ /* Standard frame */
+ buf[HI3110_FIFO_ID_OFF] = (frame->can_id & CAN_SFF_MASK) >> 3;
+ buf[HI3110_FIFO_ID_OFF + 1] =
+ ((frame->can_id & CAN_SFF_MASK) << 5) |
+ ((frame->can_id & CAN_RTR_FLAG) ? (1 << 4) : 0);
+
+ buf[HI3110_FIFO_STD_DLC_OFF] = frame->can_dlc;
+
+ memcpy(buf + HI3110_FIFO_STD_DATA_OFF,
+ frame->data, frame->can_dlc);
+
+ hi3110_hw_tx_frame(spi, buf, TX_STD_BUF_LEN -
+ (CAN_FRAME_MAX_DATA_LEN - frame->can_dlc));
+ }
+}
+
+static void hi3110_hw_rx_frame(struct spi_device *spi, u8 *buf)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+ priv->spi_tx_buf[0] = HI3110_READ_FIFO_WOTIME;
+ hi3110_spi_trans(spi, RX_BUF_LEN);
+ memcpy(buf, priv->spi_rx_buf + 1, RX_BUF_LEN - 1);
+}
+
+static void hi3110_hw_rx(struct spi_device *spi)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+ struct sk_buff *skb;
+ struct can_frame *frame;
+ u8 buf[RX_BUF_LEN - 1];
+
+ skb = alloc_can_skb(priv->net, &frame);
+ if (!skb) {
+ dev_err(&spi->dev, "cannot allocate RX skb\n");
+ priv->net->stats.rx_dropped++;
+ return;
+ }
+
+ hi3110_hw_rx_frame(spi, buf);
+ if (buf[HI3110_FIFO_WOTIME_TAG_OFF] & HI3110_FIFO_WOTIME_TAG_IDE) {
+ /* IDE is recessive (1), indicating extended 29-bit frame */
+ frame->can_id = CAN_EFF_FLAG;
+ frame->can_id |=
+ (buf[HI3110_FIFO_WOTIME_ID_OFF] << 21) |
+ (((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0xE0) >> 5) << 18) |
+ ((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0x07) << 15) |
+ (buf[HI3110_FIFO_WOTIME_ID_OFF + 2] << 7) |
+ (buf[HI3110_FIFO_WOTIME_ID_OFF + 3] >> 1);
+ } else {
+ /* IDE is dominant (0), frame indicating standard 11-bit */
+ frame->can_id =
+ (buf[HI3110_FIFO_WOTIME_ID_OFF] << 3) |
+ ((buf[HI3110_FIFO_WOTIME_ID_OFF + 1] & 0xE0) >> 5);
+ }
+
+ if (buf[HI3110_FIFO_WOTIME_ID_OFF + 3] & HI3110_FIFO_WOTIME_ID_RTR) {
+ /* RTR is recessive (1), indicating remote request frame */
+ frame->can_id |= CAN_RTR_FLAG;
+ }
+
+ /* Data length */
+ frame->can_dlc = get_can_dlc(buf[HI3110_FIFO_WOTIME_DLC_OFF] & 0x0F);
+ memcpy(frame->data, buf + HI3110_FIFO_WOTIME_DAT_OFF, frame->can_dlc);
+
+ priv->net->stats.rx_packets++;
+ priv->net->stats.rx_bytes += frame->can_dlc;
+
+ can_led_event(priv->net, CAN_LED_EVENT_RX);
+
+ netif_rx_ni(skb);
+}
+
+static void hi3110_hw_sleep(struct spi_device *spi)
+{
+ hi3110_write(spi, HI3110_WRITE_CTRL0, HI3110_CTRL0_SLEEP_MODE);
+}
+
+static netdev_tx_t hi3110_hard_start_xmit(struct sk_buff *skb,
+ struct net_device *net)
+{
+ struct hi3110_priv *priv = netdev_priv(net);
+ struct spi_device *spi = priv->spi;
+
+ if (priv->tx_skb || priv->tx_len) {
+ dev_warn(&spi->dev, "hard_xmit called while tx busy\n");
+ return NETDEV_TX_BUSY;
+ }
+
+ if (can_dropped_invalid_skb(net, skb))
+ return NETDEV_TX_OK;
+
+ netif_stop_queue(net);
+ priv->tx_skb = skb;
+ queue_work(priv->wq, &priv->tx_work);
+
+ return NETDEV_TX_OK;
+}
+
+static int hi3110_do_set_mode(struct net_device *net, enum can_mode mode)
+{
+ struct hi3110_priv *priv = netdev_priv(net);
+
+ switch (mode) {
+ case CAN_MODE_START:
+ hi3110_clean(net);
+ /* We have to delay work since SPI I/O may sleep */
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ priv->restart_tx = 1;
+ if (priv->can.restart_ms == 0)
+ priv->after_suspend = AFTER_SUSPEND_RESTART;
+ queue_work(priv->wq, &priv->restart_work);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int hi3110_set_normal_mode(struct spi_device *spi)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+ u8 reg;
+
+ hi3110_write(spi, HI3110_WRITE_INTE, HI3110_INT_BUSERR |
+ HI3110_INT_RXFIFO | HI3110_INT_TXCPLT);
+
+ /* Enable TX */
+ hi3110_write(spi, HI3110_WRITE_CTRL1, HI3110_CTRL1_TXEN);
+
+ if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
+ /* Put device into loopback mode */
+ hi3110_write(spi, HI3110_WRITE_CTRL0,
+ HI3110_CTRL0_LOOPBACK_MODE);
+ } else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
+ /* Put device into listen-only mode */
+ hi3110_write(spi, HI3110_WRITE_CTRL0,
+ HI3110_CTRL0_MONITOR_MODE);
+ } else {
+ /* Put device into normal mode */
+ hi3110_write(spi, HI3110_WRITE_CTRL0,
+ HI3110_CTRL0_NORMAL_MODE);
+
+ /* Wait for the device to enter normal mode */
+ mdelay(HI3110_OST_DELAY_MS);
+ reg = hi3110_read(spi, HI3110_READ_CTRL0);
+ if ((reg & HI3110_CTRL0_MODE_MASK) != HI3110_CTRL0_NORMAL_MODE)
+ return -EBUSY;
+ }
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ return 0;
+}
+
+static int hi3110_do_set_bittiming(struct net_device *net)
+{
+ struct hi3110_priv *priv = netdev_priv(net);
+ struct can_bittiming *bt = &priv->can.bittiming;
+ struct spi_device *spi = priv->spi;
+
+ hi3110_write(spi, HI3110_WRITE_BTR0,
+ ((bt->sjw - 1) << HI3110_BTR0_SJW_SHIFT) |
+ ((bt->brp - 1) << HI3110_BTR0_BRP_SHIFT));
+
+ hi3110_write(spi, HI3110_WRITE_BTR1,
+ (priv->can.ctrlmode &
+ CAN_CTRLMODE_3_SAMPLES ?
+ HI3110_BTR1_SAMP_3PERBIT : HI3110_BTR1_SAMP_1PERBIT) |
+ ((bt->phase_seg1 + bt->prop_seg - 1)
+ << HI3110_BTR1_TSEG1_SHIFT) |
+ ((bt->phase_seg2 - 1) << HI3110_BTR1_TSEG2_SHIFT));
+
+ dev_dbg(&spi->dev, "BT: 0x%02x 0x%02x\n",
+ hi3110_read(spi, HI3110_READ_BTR0),
+ hi3110_read(spi, HI3110_READ_BTR1));
+
+ return 0;
+}
+
+static int hi3110_setup(struct net_device *net, struct hi3110_priv *priv,
+ struct spi_device *spi)
+{
+ hi3110_do_set_bittiming(net);
+ return 0;
+}
+
+static int hi3110_hw_reset(struct spi_device *spi)
+{
+ u8 reg;
+ int ret;
+
+ /* Wait for oscillator startup timer after power up */
+ mdelay(HI3110_OST_DELAY_MS);
+
+ ret = hi3110_cmd(spi, HI3110_MASTER_RESET);
+ if (ret)
+ return ret;
+
+ /* Wait for oscillator startup timer after reset */
+ mdelay(HI3110_OST_DELAY_MS);
+
+ reg = hi3110_read(spi, HI3110_READ_CTRL0);
+ if ((reg & HI3110_CTRL0_MODE_MASK) != HI3110_CTRL0_INIT_MODE)
+ return -ENODEV;
+
+ /* As per the datasheet it appears the error flags are
+ * not cleared on reset. Explicitly clear them by performing a read
+ */
+ hi3110_read(spi, HI3110_READ_ERR);
+
+ return 0;
+}
+
+static int hi3110_hw_probe(struct spi_device *spi)
+{
+ u8 statf;
+
+ hi3110_hw_reset(spi);
+
+ /* Confirm correct operation by checking against reset values
+ * in datasheet
+ */
+ statf = hi3110_read(spi, HI3110_READ_STATF);
+
+ dev_dbg(&spi->dev, "statf: %02X\n", statf);
+
+ if (statf != 0x82)
+ return -ENODEV;
+
+ return 0;
+}
+
+static int hi3110_power_enable(struct regulator *reg, int enable)
+{
+ if (IS_ERR_OR_NULL(reg))
+ return 0;
+
+ if (enable)
+ return regulator_enable(reg);
+ else
+ return regulator_disable(reg);
+}
+
+static void hi3110_open_clean(struct net_device *net)
+{
+ struct hi3110_priv *priv = netdev_priv(net);
+ struct spi_device *spi = priv->spi;
+
+ free_irq(spi->irq, priv);
+ hi3110_hw_sleep(spi);
+ hi3110_power_enable(priv->transceiver, 0);
+ close_candev(net);
+}
+
+static int hi3110_stop(struct net_device *net)
+{
+ struct hi3110_priv *priv = netdev_priv(net);
+ struct spi_device *spi = priv->spi;
+
+ close_candev(net);
+
+ priv->force_quit = 1;
+ free_irq(spi->irq, priv);
+ destroy_workqueue(priv->wq);
+ priv->wq = NULL;
+
+ mutex_lock(&priv->hi3110_lock);
+
+ /* Disable transmit, interrupts and clear flags */
+ hi3110_write(spi, HI3110_WRITE_CTRL1, 0x0);
+ hi3110_write(spi, HI3110_WRITE_INTE, 0x0);
+ hi3110_read(spi, HI3110_READ_INTF);
+
+ hi3110_clean(net);
+
+ hi3110_hw_sleep(spi);
+
+ hi3110_power_enable(priv->transceiver, 0);
+
+ priv->can.state = CAN_STATE_STOPPED;
+
+ mutex_unlock(&priv->hi3110_lock);
+
+ can_led_event(net, CAN_LED_EVENT_STOP);
+
+ return 0;
+}
+
+static void hi3110_error_skb(struct net_device *net, int can_id,
+ int data1, int data2)
+{
+ struct sk_buff *skb;
+ struct can_frame *frame;
+
+ skb = alloc_can_err_skb(net, &frame);
+ if (skb) {
+ frame->can_id |= can_id;
+ frame->data[1] = data1;
+ frame->data[2] = data2;
+ netif_rx_ni(skb);
+ } else {
+ netdev_err(net, "cannot allocate error skb\n");
+ }
+}
+
+static void hi3110_tx_work_handler(struct work_struct *ws)
+{
+ struct hi3110_priv *priv = container_of(ws, struct hi3110_priv,
+ tx_work);
+ struct spi_device *spi = priv->spi;
+ struct net_device *net = priv->net;
+ struct can_frame *frame;
+
+ mutex_lock(&priv->hi3110_lock);
+ if (priv->tx_skb) {
+ if (priv->can.state == CAN_STATE_BUS_OFF) {
+ hi3110_clean(net);
+ } else {
+ frame = (struct can_frame *)priv->tx_skb->data;
+
+ if (frame->can_dlc > CAN_FRAME_MAX_DATA_LEN)
+ frame->can_dlc = CAN_FRAME_MAX_DATA_LEN;
+ hi3110_hw_tx(spi, frame);
+ priv->tx_len = 1 + frame->can_dlc;
+ can_put_echo_skb(priv->tx_skb, net, 0);
+ priv->tx_skb = NULL;
+ }
+ }
+ mutex_unlock(&priv->hi3110_lock);
+}
+
+static void hi3110_restart_work_handler(struct work_struct *ws)
+{
+ struct hi3110_priv *priv = container_of(ws, struct hi3110_priv,
+ restart_work);
+ struct spi_device *spi = priv->spi;
+ struct net_device *net = priv->net;
+
+ mutex_lock(&priv->hi3110_lock);
+ if (priv->after_suspend) {
+ hi3110_hw_reset(spi);
+ hi3110_setup(net, priv, spi);
+ if (priv->after_suspend & AFTER_SUSPEND_RESTART) {
+ hi3110_set_normal_mode(spi);
+ } else if (priv->after_suspend & AFTER_SUSPEND_UP) {
+ netif_device_attach(net);
+ hi3110_clean(net);
+ hi3110_set_normal_mode(spi);
+ netif_wake_queue(net);
+ } else {
+ hi3110_hw_sleep(spi);
+ }
+ priv->after_suspend = 0;
+ priv->force_quit = 0;
+ }
+
+ if (priv->restart_tx) {
+ priv->restart_tx = 0;
+ hi3110_clean(net);
+ netif_wake_queue(net);
+ hi3110_error_skb(net, CAN_ERR_RESTARTED, 0, 0);
+ }
+ mutex_unlock(&priv->hi3110_lock);
+}
+
+static irqreturn_t hi3110_can_ist(int irq, void *dev_id)
+{
+ struct hi3110_priv *priv = dev_id;
+ struct spi_device *spi = priv->spi;
+ struct net_device *net = priv->net;
+
+ mutex_lock(&priv->hi3110_lock);
+
+ while (!priv->force_quit) {
+ enum can_state new_state;
+ u8 intf;
+ u8 eflag;
+ int can_id = 0, data1 = 0, data2 = 0;
+
+ while (!(HI3110_STAT_RXFMTY &
+ hi3110_read(spi, HI3110_READ_STATF))) {
+ hi3110_hw_rx(spi);
+ };
+
+ intf = hi3110_read(spi, HI3110_READ_INTF);
+ eflag = hi3110_read(spi, HI3110_READ_ERR);
+ /* Update can state */
+ if (eflag & HI3110_ERR_BUSOFF) {
+ new_state = CAN_STATE_BUS_OFF;
+ can_id |= CAN_ERR_BUSOFF;
+ } else if (eflag & HI3110_ERR_TXERRP) {
+ new_state = CAN_STATE_ERROR_PASSIVE;
+ can_id |= CAN_ERR_CRTL;
+ data1 |= CAN_ERR_CRTL_TX_PASSIVE;
+ } else if (eflag & HI3110_ERR_RXERRP) {
+ new_state = CAN_STATE_ERROR_PASSIVE;
+ can_id |= CAN_ERR_CRTL;
+ data1 |= CAN_ERR_CRTL_RX_PASSIVE;
+ } else {
+ new_state = CAN_STATE_ERROR_ACTIVE;
+ }
+
+ /* Check for protocol errors */
+ if (eflag & HI3110_ERR_PROTOCOL_MASK) {
+ can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+ priv->can.can_stats.bus_error++;
+ priv->net->stats.rx_errors++;
+ if (eflag & HI3110_ERR_BITERR)
+ data2 |= CAN_ERR_PROT_BIT;
+ else if (eflag & HI3110_ERR_FRMERR)
+ data2 |= CAN_ERR_PROT_FORM;
+ else if (eflag & HI3110_ERR_STUFERR)
+ data2 |= CAN_ERR_PROT_STUFF;
+ else
+ data2 |= CAN_ERR_PROT_UNSPEC;
+ }
+
+ /* Update can state statistics */
+ switch (priv->can.state) {
+ case CAN_STATE_ERROR_ACTIVE:
+ if (new_state >= CAN_STATE_ERROR_WARNING &&
+ new_state <= CAN_STATE_BUS_OFF)
+ priv->can.can_stats.error_warning++;
+ /* fallthrough */
+ case CAN_STATE_ERROR_WARNING:
+ if (new_state >= CAN_STATE_ERROR_PASSIVE &&
+ new_state <= CAN_STATE_BUS_OFF)
+ priv->can.can_stats.error_passive++;
+ break;
+ default:
+ break;
+ }
+ priv->can.state = new_state;
+
+ if (intf & HI3110_INT_BUSERR) {
+ /* Note: HI3110 Does report overflow errors */
+ hi3110_error_skb(net, can_id, data1, data2);
+ }
+
+ if (priv->can.state == CAN_STATE_BUS_OFF) {
+ if (priv->can.restart_ms == 0) {
+ priv->force_quit = 1;
+ priv->can.can_stats.bus_off++;
+ can_bus_off(net);
+ hi3110_hw_sleep(spi);
+ break;
+ }
+ }
+
+ if (intf == 0)
+ break;
+
+ if (intf & HI3110_INT_TXCPLT) {
+ net->stats.tx_packets++;
+ net->stats.tx_bytes += priv->tx_len - 1;
+ can_led_event(net, CAN_LED_EVENT_TX);
+ if (priv->tx_len) {
+ can_get_echo_skb(net, 0);
+ priv->tx_len = 0;
+ }
+ netif_wake_queue(net);
+ }
+ }
+ mutex_unlock(&priv->hi3110_lock);
+ return IRQ_HANDLED;
+}
+
+static int hi3110_open(struct net_device *net)
+{
+ struct hi3110_priv *priv = netdev_priv(net);
+ struct spi_device *spi = priv->spi;
+ unsigned long flags = IRQF_ONESHOT | IRQF_TRIGGER_RISING;
+ int ret;
+
+ ret = open_candev(net);
+ if (ret) {
+ dev_err(&spi->dev, "unable to set initial baudrate!\n");
+ return ret;
+ }
+
+ mutex_lock(&priv->hi3110_lock);
+ hi3110_power_enable(priv->transceiver, 1);
+
+ priv->force_quit = 0;
+ priv->tx_skb = NULL;
+ priv->tx_len = 0;
+
+ ret = request_threaded_irq(spi->irq, NULL, hi3110_can_ist,
+ flags, DEVICE_NAME, priv);
+ if (ret) {
+ dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq);
+ hi3110_power_enable(priv->transceiver, 0);
+ close_candev(net);
+ goto open_unlock;
+ }
+
+ priv->wq = alloc_workqueue("hi3110_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
+ 0);
+ INIT_WORK(&priv->tx_work, hi3110_tx_work_handler);
+ INIT_WORK(&priv->restart_work, hi3110_restart_work_handler);
+
+ ret = hi3110_hw_reset(spi);
+ if (ret) {
+ hi3110_open_clean(net);
+ goto open_unlock;
+ }
+ ret = hi3110_setup(net, priv, spi);
+ if (ret) {
+ hi3110_open_clean(net);
+ goto open_unlock;
+ }
+ ret = hi3110_set_normal_mode(spi);
+ if (ret) {
+ hi3110_open_clean(net);
+ goto open_unlock;
+ }
+ can_led_event(net, CAN_LED_EVENT_OPEN);
+ netif_wake_queue(net);
+
+open_unlock:
+ mutex_unlock(&priv->hi3110_lock);
+ return ret;
+}
+
+static const struct net_device_ops hi3110_netdev_ops = {
+ .ndo_open = hi3110_open,
+ .ndo_stop = hi3110_stop,
+ .ndo_start_xmit = hi3110_hard_start_xmit,
+};
+
+static const struct of_device_id hi3110_of_match[] = {
+ {
+ .compatible = "holt,hi3110",
+ .data = (void *)CAN_HI3110_HI3110,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, hi3110_of_match);
+
+static const struct spi_device_id hi3110_id_table[] = {
+ {
+ .name = "hi3110",
+ .driver_data = (kernel_ulong_t)CAN_HI3110_HI3110,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, hi3110_id_table);
+
+static int hi3110_can_probe(struct spi_device *spi)
+{
+ const struct of_device_id *of_id = of_match_device(hi3110_of_match,
+ &spi->dev);
+ struct net_device *net;
+ struct hi3110_priv *priv;
+ struct clk *clk;
+ int freq, ret;
+
+ clk = devm_clk_get(&spi->dev, NULL);
+ if (IS_ERR(clk)) {
+ dev_err(&spi->dev, "no CAN clock source defined\n");
+ return PTR_ERR(clk);
+ }
+ freq = clk_get_rate(clk);
+
+ /* Sanity check */
+ if (freq > 40000000)
+ return -ERANGE;
+
+ /* Allocate can/net device */
+ net = alloc_candev(sizeof(struct hi3110_priv), TX_ECHO_SKB_MAX);
+ if (!net)
+ return -ENOMEM;
+
+ if (!IS_ERR(clk)) {
+ ret = clk_prepare_enable(clk);
+ if (ret)
+ goto out_free;
+ }
+
+ net->netdev_ops = &hi3110_netdev_ops;
+ net->flags |= IFF_ECHO;
+
+ priv = netdev_priv(net);
+ priv->can.bittiming_const = &hi3110_bittiming_const;
+ priv->can.do_set_mode = hi3110_do_set_mode;
+ priv->can.clock.freq = freq / 2;
+ priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
+ CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY;
+ if (of_id)
+ priv->model = (enum hi3110_model)of_id->data;
+ else
+ priv->model = spi_get_device_id(spi)->driver_data;
+ priv->net = net;
+ priv->clk = clk;
+
+ spi_set_drvdata(spi, priv);
+
+ /* Configure the SPI bus */
+ spi->bits_per_word = 8;
+ ret = spi_setup(spi);
+ if (ret)
+ goto out_clk;
+
+ priv->power = devm_regulator_get_optional(&spi->dev, "vdd");
+ priv->transceiver = devm_regulator_get_optional(&spi->dev, "xceiver");
+ if ((PTR_ERR(priv->power) == -EPROBE_DEFER) ||
+ (PTR_ERR(priv->transceiver) == -EPROBE_DEFER)) {
+ ret = -EPROBE_DEFER;
+ goto out_clk;
+ }
+
+ ret = hi3110_power_enable(priv->power, 1);
+ if (ret)
+ goto out_clk;
+
+ priv->spi = spi;
+ mutex_init(&priv->hi3110_lock);
+
+ /* If requested, allocate DMA buffers */
+ if (hi3110_enable_dma) {
+ spi->dev.coherent_dma_mask = ~0;
+
+ /* Minimum coherent DMA allocation is PAGE_SIZE, so allocate
+ * that much and share it between Tx and Rx DMA buffers.
+ */
+ priv->spi_tx_buf = dmam_alloc_coherent(&spi->dev,
+ PAGE_SIZE,
+ &priv->spi_tx_dma,
+ GFP_DMA);
+
+ if (priv->spi_tx_buf) {
+ priv->spi_rx_buf = (priv->spi_tx_buf + (PAGE_SIZE / 2));
+ priv->spi_rx_dma = (dma_addr_t)(priv->spi_tx_dma +
+ (PAGE_SIZE / 2));
+ } else {
+ /* Fall back to non-DMA */
+ hi3110_enable_dma = 0;
+ }
+ }
+
+ /* Allocate non-DMA buffers */
+ if (!hi3110_enable_dma) {
+ priv->spi_tx_buf = devm_kzalloc(&spi->dev, RX_BUF_LEN,
+ GFP_KERNEL);
+ if (!priv->spi_tx_buf) {
+ ret = -ENOMEM;
+ goto error_probe;
+ }
+ priv->spi_rx_buf = devm_kzalloc(&spi->dev, RX_BUF_LEN,
+ GFP_KERNEL);
+
+ if (!priv->spi_rx_buf) {
+ ret = -ENOMEM;
+ goto error_probe;
+ }
+ }
+
+ SET_NETDEV_DEV(net, &spi->dev);
+
+ ret = hi3110_hw_probe(spi);
+ if (ret) {
+ if (ret == -ENODEV)
+ dev_err(&spi->dev, "Cannot initialize %x. Wrong wiring?\n",
+ priv->model);
+ goto error_probe;
+ }
+ hi3110_hw_sleep(spi);
+
+ ret = register_candev(net);
+ if (ret)
+ goto error_probe;
+
+ devm_can_led_init(net);
+ netdev_info(net, "%x successfully initialized.\n", priv->model);
+
+ return 0;
+
+error_probe:
+ hi3110_power_enable(priv->power, 0);
+
+out_clk:
+ if (!IS_ERR(clk))
+ clk_disable_unprepare(clk);
+
+out_free:
+ free_candev(net);
+
+ dev_err(&spi->dev, "Probe failed, err=%d\n", -ret);
+ return ret;
+}
+
+static int hi3110_can_remove(struct spi_device *spi)
+{
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+ struct net_device *net = priv->net;
+
+ unregister_candev(net);
+
+ hi3110_power_enable(priv->power, 0);
+
+ if (!IS_ERR(priv->clk))
+ clk_disable_unprepare(priv->clk);
+
+ free_candev(net);
+
+ return 0;
+}
+
+static int __maybe_unused hi3110_can_suspend(struct device *dev)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+ struct net_device *net = priv->net;
+
+ priv->force_quit = 1;
+ disable_irq(spi->irq);
+
+ /* Note: at this point neither IST nor workqueues are running.
+ * open/stop cannot be called anyway so locking is not needed
+ */
+ if (netif_running(net)) {
+ netif_device_detach(net);
+
+ hi3110_hw_sleep(spi);
+ hi3110_power_enable(priv->transceiver, 0);
+ priv->after_suspend = AFTER_SUSPEND_UP;
+ } else {
+ priv->after_suspend = AFTER_SUSPEND_DOWN;
+ }
+
+ if (!IS_ERR_OR_NULL(priv->power)) {
+ regulator_disable(priv->power);
+ priv->after_suspend |= AFTER_SUSPEND_POWER;
+ }
+
+ return 0;
+}
+
+static int __maybe_unused hi3110_can_resume(struct device *dev)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct hi3110_priv *priv = spi_get_drvdata(spi);
+
+ if (priv->after_suspend & AFTER_SUSPEND_POWER)
+ hi3110_power_enable(priv->power, 1);
+
+ if (priv->after_suspend & AFTER_SUSPEND_UP) {
+ hi3110_power_enable(priv->transceiver, 1);
+ queue_work(priv->wq, &priv->restart_work);
+ } else {
+ priv->after_suspend = 0;
+ }
+
+ priv->force_quit = 0;
+ enable_irq(spi->irq);
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(hi3110_can_pm_ops, hi3110_can_suspend,
+ hi3110_can_resume);
+
+static struct spi_driver hi3110_can_driver = {
+ .driver = {
+ .name = DEVICE_NAME,
+ .of_match_table = hi3110_of_match,
+ .pm = &hi3110_can_pm_ops,
+ },
+ .id_table = hi3110_id_table,
+ .probe = hi3110_can_probe,
+ .remove = hi3110_can_remove,
+};
+
+module_spi_driver(hi3110_can_driver);
+
+MODULE_AUTHOR("Akshay Bhat <akshay.bhat@timesys.com>");
+MODULE_AUTHOR("Casey Fitzpatrick <casey.fitzpatrick@timesys.com>");
+MODULE_DESCRIPTION("Holt HI-3110 CAN driver");
+MODULE_LICENSE("GPL v2");
--
2.8.1
^ permalink raw reply related
* [PATCH] can: spi: hi311x: fix semicolon.cocci warnings (fwd)
From: Julia Lawall @ 2016-11-14 21:36 UTC (permalink / raw)
Cc: wg, mkl, robh+dt, mark.rutland, linux-can, netdev, devicetree,
linux-kernel, Akshay Bhat, Akshay Bhat
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
CC: Akshay Bhat <akshay.bhat@timesys.com>
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
It's a minor issue, but may as well fix it up now. The tree this code is
from is as follows:
url:
https://github.com/0day-ci/linux/commits/Akshay-Bhat/can-holt_hi311x-documen
t-device-tree-bindings/20161115-034509
:::::: branch date: 2 hours ago
:::::: commit date: 2 hours ago
hi311x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/net/can/spi/hi311x.c
+++ b/drivers/net/can/spi/hi311x.c
@@ -673,7 +673,7 @@ static irqreturn_t hi3110_can_ist(int ir
while (!(HI3110_STAT_RXFMTY &
hi3110_read(spi, HI3110_READ_STATF))) {
hi3110_hw_rx(spi);
- };
+ }
intf = hi3110_read(spi, HI3110_READ_INTF);
eflag = hi3110_read(spi, HI3110_READ_ERR);
^ permalink raw reply
* Count dropped frames on CAN socket
From: Austin Hendrix @ 2016-11-15 23:32 UTC (permalink / raw)
To: linux-can
Hi linux-can,
I'm writing a program which receives CAN frames over SocketCAN. I've
found on a couple of occasions that my program has had bugs that cause
it not to empty the CAN socket receive buffer, and the kernel has
dropped incoming CAN frames.
I had a look through the kernel source code, and found that the kernel's
socket framework keeps track of dropped frames in the sk_drops field of
the sock struct. Is there a way to retrieve that dropped frame counter
from my program, so that it's easier for me to detect problems like this
in the future? (I've looked in the docs and in /proc and haven't been
able to find a way to retrieve the sk_drops counter, but it's quite
possible that I missed something)
Thanks,
-Austin
^ permalink raw reply
* Re: Count dropped frames on CAN socket
From: Brian Silverman @ 2016-11-16 0:08 UTC (permalink / raw)
To: Austin Hendrix; +Cc: linux-can
In-Reply-To: <519e768b-887e-0aad-9043-7b6b44a61122@gmail.com>
Hi Austin,
If you enable the (SOL_SOCKET, SO_RX_OVFL) option with setsockopt on a
CAN socket, you'll get a corresponding (cmsg_level == SOL_SOCKET,
cmsg_type == SO_RXQ_OVFL) piece of ancillary data from recvmsg. The
value is a uint32_t containing that exact sk_drops value. The
semantics for this value documented in socket(7) are wrong, but the
commit message adding it (3b885787ea41) gives a reasonable
description. In particular, the message doesn't show up at all until
at least one frame is dropped, and it's a total counter not a delta.
Brian
On Tue, Nov 15, 2016 at 6:32 PM, Austin Hendrix <namniart@gmail.com> wrote:
> Hi linux-can,
>
> I'm writing a program which receives CAN frames over SocketCAN. I've found
> on a couple of occasions that my program has had bugs that cause it not to
> empty the CAN socket receive buffer, and the kernel has dropped incoming CAN
> frames.
>
> I had a look through the kernel source code, and found that the kernel's
> socket framework keeps track of dropped frames in the sk_drops field of the
> sock struct. Is there a way to retrieve that dropped frame counter from my
> program, so that it's easier for me to detect problems like this in the
> future? (I've looked in the docs and in /proc and haven't been able to find
> a way to retrieve the sk_drops counter, but it's quite possible that I
> missed something)
>
>
> Thanks,
>
> -Austin
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Count dropped frames on CAN socket
From: Oliver Hartkopp @ 2016-11-16 9:57 UTC (permalink / raw)
To: Brian Silverman, Austin Hendrix; +Cc: linux-can
In-Reply-To: <CAGt3f4kH7K98K6Xda+2t0VXSQR20o0MDRp6cJMpoL+U_cTUeEg@mail.gmail.com>
And here is how the dropcount stuff has been integrated in candump:
https://github.com/linux-can/can-utils/commit/3c019ea61169d7f08d10d1fece95433c629eb4a6
Regards,
Oliver
On 11/16/2016 01:08 AM, Brian Silverman wrote:
> Hi Austin,
>
> If you enable the (SOL_SOCKET, SO_RX_OVFL) option with setsockopt on a
> CAN socket, you'll get a corresponding (cmsg_level == SOL_SOCKET,
> cmsg_type == SO_RXQ_OVFL) piece of ancillary data from recvmsg. The
> value is a uint32_t containing that exact sk_drops value. The
> semantics for this value documented in socket(7) are wrong, but the
> commit message adding it (3b885787ea41) gives a reasonable
> description. In particular, the message doesn't show up at all until
> at least one frame is dropped, and it's a total counter not a delta.
>
> Brian
>
> On Tue, Nov 15, 2016 at 6:32 PM, Austin Hendrix <namniart@gmail.com> wrote:
>> Hi linux-can,
>>
>> I'm writing a program which receives CAN frames over SocketCAN. I've found
>> on a couple of occasions that my program has had bugs that cause it not to
>> empty the CAN socket receive buffer, and the kernel has dropped incoming CAN
>> frames.
>>
>> I had a look through the kernel source code, and found that the kernel's
>> socket framework keeps track of dropped frames in the sk_drops field of the
>> sock struct. Is there a way to retrieve that dropped frame counter from my
>> program, so that it's easier for me to detect problems like this in the
>> future? (I've looked in the docs and in /proc and haven't been able to find
>> a way to retrieve the sk_drops counter, but it's quite possible that I
>> missed something)
>>
>>
>> Thanks,
>>
>> -Austin
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH]: canfdtest typo while waiting for tv_stop time
From: Stephane Grosjean @ 2016-11-16 10:46 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can@vger.kernel.org
Hello Mark,
After a few complaint about 100% CPU usage of canfdtest over our PCI
-like boards, I have had a look to the source file, and think I have
found the issue in:
diff --git a/canfdtest.c b/canfdtest.c
index 2407027..d7ba740 100644
--- a/canfdtest.c
+++ b/canfdtest.c
@@ -239,7 +239,7 @@ static int can_echo_dut(void)
}
gettimeofday(&tvn, NULL);
while ((tv_stop.tv_sec > tvn.tv_sec) ||
- ((tv_stop.tv_sec = tvn.tv_sec) &&
+ ((tv_stop.tv_sec == tvn.tv_sec) &&
(tv_stop.tv_usec >= tvn.tv_usec)))
gettimeofday(&tvn, NULL);
}
Am I right?
Regards,
Stéphane
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: A.Gach, U.Wilhelm
--
^ permalink raw reply related
* Re: [PATCH 1/2] can: holt_hi311x: document device tree bindings
From: Rob Herring @ 2016-11-16 13:34 UTC (permalink / raw)
To: Akshay Bhat
Cc: wg, mkl, mark.rutland, linux-can, netdev, devicetree,
linux-kernel, Akshay Bhat
In-Reply-To: <1479146144-29143-1-git-send-email-akshay.bhat@timesys.com>
On Mon, Nov 14, 2016 at 12:55:43PM -0500, Akshay Bhat wrote:
> Document the HOLT HI-311x CAN device tree bindings.
>
> Signed-off-by: Akshay Bhat <nodeax@gmail.com>
> ---
> .../devicetree/bindings/net/can/holt_hi311x.txt | 24 ++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/can/holt_hi311x.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: Count dropped frames on CAN socket
From: Austin Hendrix @ 2016-11-16 17:13 UTC (permalink / raw)
To: Oliver Hartkopp, Brian Silverman; +Cc: linux-can
In-Reply-To: <ec9b7a5b-e587-ef2a-d3ab-47929795799e@hartkopp.net>
Awesome, that's exactly what I was looking for.
Thanks!
-Austin
On 11/16/2016 01:57 AM, Oliver Hartkopp wrote:
> And here is how the dropcount stuff has been integrated in candump:
>
> https://github.com/linux-can/can-utils/commit/3c019ea61169d7f08d10d1fece95433c629eb4a6
>
>
> Regards,
> Oliver
>
>
> On 11/16/2016 01:08 AM, Brian Silverman wrote:
>> Hi Austin,
>>
>> If you enable the (SOL_SOCKET, SO_RX_OVFL) option with setsockopt on a
>> CAN socket, you'll get a corresponding (cmsg_level == SOL_SOCKET,
>> cmsg_type == SO_RXQ_OVFL) piece of ancillary data from recvmsg. The
>> value is a uint32_t containing that exact sk_drops value. The
>> semantics for this value documented in socket(7) are wrong, but the
>> commit message adding it (3b885787ea41) gives a reasonable
>> description. In particular, the message doesn't show up at all until
>> at least one frame is dropped, and it's a total counter not a delta.
>>
>> Brian
>>
>> On Tue, Nov 15, 2016 at 6:32 PM, Austin Hendrix <namniart@gmail.com>
>> wrote:
>>> Hi linux-can,
>>>
>>> I'm writing a program which receives CAN frames over SocketCAN. I've
>>> found
>>> on a couple of occasions that my program has had bugs that cause it
>>> not to
>>> empty the CAN socket receive buffer, and the kernel has dropped
>>> incoming CAN
>>> frames.
>>>
>>> I had a look through the kernel source code, and found that the
>>> kernel's
>>> socket framework keeps track of dropped frames in the sk_drops field
>>> of the
>>> sock struct. Is there a way to retrieve that dropped frame counter
>>> from my
>>> program, so that it's easier for me to detect problems like this in the
>>> future? (I've looked in the docs and in /proc and haven't been able
>>> to find
>>> a way to retrieve the sk_drops counter, but it's quite possible that I
>>> missed something)
>>>
>>>
>>> Thanks,
>>>
>>> -Austin
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply
* RE: [PATCH]: canfdtest typo while waiting for tv_stop time
From: Ramesh Shanmugasundaram @ 2016-11-17 8:09 UTC (permalink / raw)
To: Stephane Grosjean, Marc Kleine-Budde, linux-can@vger.kernel.org
In-Reply-To: <5bd8e04f-606c-0b25-dab9-7a1090ca8810@peak-system.com>
> Subject: [PATCH]: canfdtest typo while waiting for tv_stop time
>
> Hello Mark,
>
> After a few complaint about 100% CPU usage of canfdtest over our PCI -like
> boards, I have had a look to the source file, and think I have found the
> issue in:
>
> diff --git a/canfdtest.c b/canfdtest.c
> index 2407027..d7ba740 100644
> --- a/canfdtest.c
> +++ b/canfdtest.c
> @@ -239,7 +239,7 @@ static int can_echo_dut(void)
> }
> gettimeofday(&tvn, NULL);
> while ((tv_stop.tv_sec > tvn.tv_sec) ||
> - ((tv_stop.tv_sec = tvn.tv_sec) &&
> + ((tv_stop.tv_sec == tvn.tv_sec)
> + &&
> (tv_stop.tv_usec >=
> tvn.tv_usec)))
> gettimeofday(&tvn, NULL);
> }
>
> Am I right?
There is one more bug on that piece of code. I submitted a bug & a pull request fixing it
https://github.com/linux-can/can-utils/pull/15
Thanks,
Ramesh
^ permalink raw reply
* Re: About adding a new CAN-FD driver...
From: Stephane Grosjean @ 2016-11-17 10:22 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can@vger.kernel.org
In-Reply-To: <685ea82c-3457-35b0-0fff-9bfedf83256b@hartkopp.net>
Hello Oliver,
I'm almost ready for pushing the stuff about CAN-FD over our PCIe boards
but I'm back to you before, because in one of your previous email, you
talked about "linux/include/linux/can/dev" directory as the better place
to put a common header file for all of our CAN-FD interfaces.
AFAIK this directory does not exist so far... Did you actually mean
"linux/include/linux/can"? Or did you expect me to create the "dev"
subdir with pushing the corresponding patch?
Regards,
Stéphane
Le 25/10/2016 à 10:44, Oliver Hartkopp a écrit :
> Hello Stephane,
>
> On 10/25/2016 09:01 AM, Stephane Grosjean wrote:
>> So I think that
>>
>> linux/include/linux/can/platform
>>
>> is the most appropriate location for storing some common header file and
>> definitions.
>
> I'm not sure about this directory.
> In 'platform' hardware specific stuff e.g. for SoCs is stored.
>
> As you want to have a place for a CAN controller IP the 'platform'
> directory seems to be the wrong location.
>
>> What about the code now? I would suggest to do as you did with the
>> SJA1000 controller, that is, creating a common module that will be
>> loaded each time "peak_usb.ko" and/or the new one in charge of PCAN-PCIe
>> FD will be loaded. Is this ok for you?
>
> Don't know whether this is really helpful for the PCAN USB FD driver -
> but you will find the best abstraction for yourself :-)
>
>> After meeting, we have decided that the name of this new module would be
>> "peak_canfd" (instead of the internal one "ucan"). So, to summarize,
>> what I propose would be:
>>
>> - to add a new file named "peak_canfd.h" into
>> linux/include/linux/can/platform, mainly a "move" of
>
> Better into linux/include/linux/can/dev/
>
>> drivers/net/can/usb/peak_usb/pcan_ucan.h
>>
>> - to add a new directory under "drivers/net/can" named "peak_canfd" that
>> would contain the code of the common module "peak_canfd.ko" as well as
>> the (new) module "peak_pciefd.ko"
>>
>> Are these ok for you?
>
> Yes.
>
> Regards,
> Oliver
>
>
>> Le 18/10/2016 à 09:15, Oliver Hartkopp a écrit :
>>> Hello Stephane,
>>>
>>> thanks for starting the uCAN support for the non-USB hardware types!
>>>
>>> As a third alternative we cloud think about a common place to host
>>> include files that are needed by all drivers.
>>>
>>> Looking into
>>>
>>> linux/include/linux/can/
>>>
>>> there's dev.h which is needed by all drivers today.
>>>
>>> In
>>>
>>> linux/include/linux/can/platform
>>>
>>> there are the platform specific infos for some CAN controllers.
>>>
>>> So we could place e.g. a peak_ucan.h in either
>>>
>>> linux/include/linux/can/
>>> or
>>> linux/include/linux/can/dev/
>>>
>>> At least this looks more clearly instead of creating unusual
>>> dependencies inside linux/drivers/net/can/
>>>
>>> Any other opinions out there?
>>>
>>> Regards,
>>> Oliver
>>>
>>>
>>> On 10/17/2016 11:42 AM, Stephane Grosjean wrote:
>>>> Hello,
>>>>
>>>> We're planning to add a new driver under drivers/net/can that adds
>>>> support to our PCAN-PCIe FD boards family.
>>>>
>>>> Before pushing any patches, I would first discuss about how and
>>>> where to
>>>> do that: all of our CAN-FD products actually embed some FPGA
>>>> running the
>>>> same µCode, internally called "uCAN core". So I first think to
>>>> create a
>>>> new sub-dir "drivers/net/can/ucan".
>>>>
>>>> But several months ago (Kernel v4.0), we already have pushed a driver
>>>> for our USB CAN-FD adapters, so the new files have been obviously
>>>> added
>>>> under "driver/net/can/usb/peak_usb". Among these new files,
>>>> "pcan_ucan.h" describes messages exchanged between host and uCAN core,
>>>> whatever the PC interface is (USB, PCie...). So, I can imagine that
>>>> the
>>>> new driver for PCIe should include this file too, as well as some
>>>> pieces
>>>> of code present in "peak_usb_fd.c"...
>>>>
>>>> ASFAI, #include "../usb/peak_usb/pcan_ucan.h" is not the proper way to
>>>> do this, isn't it?
>>>>
>>>> So, first of all, I would propose to push some patch(es) in order to
>>>> make things cleaner:
>>>>
>>>> 1/ move file "pcan_ucan.h" from "drivers/net/can/usb/peak_usb" to the
>>>> (newly created) "drivers/net/can/ucan"
>>>>
>>>> 2/ add the corresponding CFLAGS def into
>>>> "drivers/net/can/usb/peak_usb/makefile":
>>>>
>>>> +CFLAGS_pcan_usb_fd.o += -Idrivers/net/can/ucan
>>>>
>>>> is this the best approach, please?
>>>>
>>>> Thanks for your help.
>>>>
>>>> Stéphane
>>>>
>>>>
>>>> --
>>>> PEAK-System Technik GmbH
>>>> Sitz der Gesellschaft Darmstadt - HRB 9183 Geschaeftsfuehrung: A.Gach,
>>>> U.Wilhelm
>>>> --
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-can" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>> PEAK-System Technik GmbH
>> Sitz der Gesellschaft Darmstadt - HRB 9183 Geschaeftsfuehrung: A.Gach,
>> U.Wilhelm
>> --
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: A.Gach, U.Wilhelm
--
^ permalink raw reply
* Re: About adding a new CAN-FD driver...
From: Oliver Hartkopp @ 2016-11-17 20:20 UTC (permalink / raw)
To: Stephane Grosjean, linux-can@vger.kernel.org
In-Reply-To: <a901ee28-7872-f1b6-63c8-c6840be4f454@peak-system.com>
Hi Stephane,
On 11/17/2016 11:22 AM, Stephane Grosjean wrote:
> I'm almost ready for pushing the stuff about CAN-FD over our PCIe boards
> but I'm back to you before, because in one of your previous email, you
> talked about "linux/include/linux/can/dev" directory as the better place
> to put a common header file for all of our CAN-FD interfaces.
>
> AFAIK this directory does not exist so far... Did you actually mean
> "linux/include/linux/can"? Or did you expect me to create the "dev"
> subdir with pushing the corresponding patch?
Yes. The latter.
I think putting controller specific include files directly into
"linux/include/linux/can" is wrong in the way that the platform specific
content has a separate directory too.
My initial suggestion was
linux/include/linux/can/dev/
but we may also think of
linux/include/linux/can/controller/
linux/include/linux/can/ctrl/
linux/include/linux/can/ip-core/
linux/include/linux/can/ctrl-core/
linux/include/linux/can/can-core/
'ip-core' seems a good name too as it reflects the CAN controller
specific characteristics (e.g. memory register layout).
What would you suggest as directory name?
Regards,
Oliver
^ permalink raw reply
* Re: About adding a new CAN-FD driver...
From: Stephane Grosjean @ 2016-11-18 9:13 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can@vger.kernel.org
In-Reply-To: <35164f73-1c31-1c80-d521-f2ab7ef68a00@hartkopp.net>
Hello Oliver,
> linux/include/linux/can/ip-core/
++
Regards,
Stéphane
Le 17/11/2016 à 21:20, Oliver Hartkopp a écrit :
> Hi Stephane,
>
> On 11/17/2016 11:22 AM, Stephane Grosjean wrote:
>> I'm almost ready for pushing the stuff about CAN-FD over our PCIe boards
>> but I'm back to you before, because in one of your previous email, you
>> talked about "linux/include/linux/can/dev" directory as the better place
>> to put a common header file for all of our CAN-FD interfaces.
>>
>> AFAIK this directory does not exist so far... Did you actually mean
>> "linux/include/linux/can"? Or did you expect me to create the "dev"
>> subdir with pushing the corresponding patch?
>
> Yes. The latter.
>
> I think putting controller specific include files directly into
> "linux/include/linux/can" is wrong in the way that the platform
> specific content has a separate directory too.
>
> My initial suggestion was
>
> linux/include/linux/can/dev/
>
> but we may also think of
>
> linux/include/linux/can/controller/
> linux/include/linux/can/ctrl/
> linux/include/linux/can/ip-core/
> linux/include/linux/can/ctrl-core/
> linux/include/linux/can/can-core/
>
> 'ip-core' seems a good name too as it reflects the CAN controller
> specific characteristics (e.g. memory register layout).
>
> What would you suggest as directory name?
>
> Regards,
> Oliver
>
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: A.Gach, U.Wilhelm
--
^ permalink raw reply
* RE: canfdtest missing tx frame
From: Grim, Dennis @ 2016-11-18 22:06 UTC (permalink / raw)
To: linux-can@vger.kernel.org
In-Reply-To: <147810523969.9191.3347569271701639587@maxwell>
> PEAK have acknowledged an issue with the firmware, and I assume that
> they have fixed the issue in the firmware, and they did post a patch
> for working around the issue with older firmware, but no one has followed through on it.
Thank you Andri.
Peak System provided a firmware update for the card. The problem is resolved.
Dennis
^ permalink raw reply
* canfdtest and CPU load average
From: Grim, Dennis @ 2016-11-18 22:22 UTC (permalink / raw)
To: linux-can@vger.kernel.org
When running canfdtest on ARM Cortex-A8 (TI AM3517) based custom hardware, the CPU load average is high.
The hardware comprises the AM3517 HECC and two SJA1000s.
With the custom hardware running three instances of canfdtest (as device under test) at 250000 bits per second, the CPU load average is roughly 2. No other applications are running.
Are there things in the Linux network stack that can be tuned that might lower the load?
Dennis
^ permalink raw reply
* Re: [PATCH]: canfdtest typo while waiting for tv_stop time
From: Marc Kleine-Budde @ 2016-11-21 10:36 UTC (permalink / raw)
To: Stephane Grosjean, linux-can@vger.kernel.org
In-Reply-To: <5bd8e04f-606c-0b25-dab9-7a1090ca8810@peak-system.com>
[-- Attachment #1.1: Type: text/plain, Size: 1304 bytes --]
On 11/16/2016 11:46 AM, Stephane Grosjean wrote:
> After a few complaint about 100% CPU usage of canfdtest over our PCI
> -like boards, I have had a look to the source file, and think I have
> found the issue in:
>
> diff --git a/canfdtest.c b/canfdtest.c
> index 2407027..d7ba740 100644
> --- a/canfdtest.c
> +++ b/canfdtest.c
> @@ -239,7 +239,7 @@ static int can_echo_dut(void)
> }
> gettimeofday(&tvn, NULL);
> while ((tv_stop.tv_sec > tvn.tv_sec) ||
> - ((tv_stop.tv_sec = tvn.tv_sec) &&
> + ((tv_stop.tv_sec == tvn.tv_sec) &&
> (tv_stop.tv_usec >= tvn.tv_usec)))
> gettimeofday(&tvn, NULL);
> }
>
> Am I right?
Yes. But it still does a busywait. I've reviewed Ramesh's changes, that
will fix the busywait.
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: 488 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox