| Message ID | 20170724230521.1436-3-fcooper@ti.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
> + max-data-speed: a positive non 0 value that determines the max data rate > + that can be used in CAN-FD mode. A value of -1 implies > + CAN-FD is not supported by the transceiver. > + > +Examples: (..) > + fixed-transceiver { > + max-data-speed = <(-1)>; Looks ugly IMHO. Why didn't you stay on '0' for 'not supported'?? Regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/25/2017 11:32 AM, Oliver Hartkopp wrote: > >> + max-data-speed: a positive non 0 value that determines the max >> data rate >> + that can be used in CAN-FD mode. A value of -1 implies >> + CAN-FD is not supported by the transceiver. >> + >> +Examples: > > (..) > >> + fixed-transceiver { >> + max-data-speed = <(-1)>; > > Looks ugly IMHO. > > Why didn't you stay on '0' for 'not supported'?? Unless a driver specifically calls of_can_transceiver_fixed priv->max_trans_data_speed will be by default 0. Therefore, all drivers that support CAN-FD will claim that the transceiver indicates that it isn't supported. So one option was to update every single driver to set this property by default which I started to do but it end up becoming a massive patch and it was risky in case I missed a driver which would of resulted in major regressions. Its also problematic for new drivers that miss this property or the many out of tree CAN drivers. The other option was to create another variable to track to see if of_can_transceiver_fixed was called but I didn't think that was the better solution. So using signed values in DT is a bit ugly due to syntax but was valid and I made sure I documented it so its clear. > > Regards, > Oliver > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 24, 2017 at 06:05:19PM -0500, Franklin S Cooper Jr wrote: > Add documentation to describe usage of the new fixed transceiver binding. > This new binding is applicable for any CAN device therefore it exists as > its own document. > > Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> > --- > Version 2: > Tweak commit message working > Tweak wording for properties > Drop unit addressing > Give example if CAN-FD isn't supported > > .../bindings/net/can/fixed-transceiver.txt | 42 ++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/can/fixed-transceiver.txt > > diff --git a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt > new file mode 100644 > index 0000000..dc7e3eb > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt > @@ -0,0 +1,42 @@ > +Fixed transceiver Device Tree binding > +------------------------------ > + > +CAN transceiver typically limits the max speed in standard CAN and CAN FD > +modes. Typically these limitations are static and the transceivers themselves > +provide no way to detect this limitation at runtime. For this situation, > +the "fixed-transceiver" node can be used. > + > +Properties: > + > +Optional: > + max-arbitration-speed: a positive non 0 value that determines the max > + speed that CAN can run in non CAN-FD mode or during the > + arbitration phase in CAN-FD mode. Hi Franklin Since this and the next property are optional, it is good to document what happens when they are not in the DT blob. Also document what 0 means. > + > + max-data-speed: a positive non 0 value that determines the max data rate > + that can be used in CAN-FD mode. A value of -1 implies > + CAN-FD is not supported by the transceiver. -1 is ugly. I think it would be better to have a missing max-data-speed property indicate that CAN-FD is not supported. And maybe put 'fd' into the property name. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/26/2017 06:41 PM, Andrew Lunn wrote: > On Mon, Jul 24, 2017 at 06:05:19PM -0500, Franklin S Cooper Jr wrote: >> + >> +Optional: >> + max-arbitration-speed: a positive non 0 value that determines the max >> + speed that CAN can run in non CAN-FD mode or during the >> + arbitration phase in CAN-FD mode. > > Hi Franklin > > Since this and the next property are optional, it is good to document > what happens when they are not in the DT blob. Also document what 0 > means. > >> + >> + max-data-speed: a positive non 0 value that determines the max data rate >> + that can be used in CAN-FD mode. A value of -1 implies >> + CAN-FD is not supported by the transceiver. > > -1 is ugly. I think it would be better to have a missing > max-data-speed property indicate that CAN-FD is not supported. Thanks Andrew! I had the same feeling about '-1' :-) > And > maybe put 'fd' into the property name. Good point. In fact the common naming to set bitrates for CAN(FD) controllers are 'bitrate' and 'data bitrate'. 'speed' is not really a good word for that. Finally, @Franklin: A CAN transceiver is limited in bandwidth. But you only have one RX and one TX line between the CAN controller and the CAN transceiver. The transceiver does not know about CAN FD - it has just a physical(!) layer with a limited bandwidth. This is ONE limitation. So I tend to specify only ONE 'max-bitrate' property for the fixed-transceiver binding. The fact whether the CAN controller is CAN FD capable or not is provided by the netlink configuration interface for CAN controllers. Regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/26/2017 12:05 PM, Oliver Hartkopp wrote: > On 07/26/2017 06:41 PM, Andrew Lunn wrote: >> On Mon, Jul 24, 2017 at 06:05:19PM -0500, Franklin S Cooper Jr wrote: > >>> + >>> +Optional: >>> + max-arbitration-speed: a positive non 0 value that determines the max >>> + speed that CAN can run in non CAN-FD mode or during the >>> + arbitration phase in CAN-FD mode. >> >> Hi Franklin >> >> Since this and the next property are optional, it is good to document >> what happens when they are not in the DT blob. Also document what 0 >> means. The driver ignores values less than 0 with the exception being max-data-speed which supports a value of -1. Not sure what I'm documenting when the binding specifically says to use a positive non zero value. Its the same reason I don't document what happens if you give it a negative value. >> >>> + >>> + max-data-speed: a positive non 0 value that determines the max >>> data rate >>> + that can be used in CAN-FD mode. A value of -1 implies >>> + CAN-FD is not supported by the transceiver. >> >> -1 is ugly. I think it would be better to have a missing >> max-data-speed property indicate that CAN-FD is not supported. > Although this leads to your later point I don't think this is the right approach. Its an optional property. Not including the property should not assume it isn't supported. > Thanks Andrew! I had the same feeling about '-1' :-) Ok I'll go back to having 0 = not supported. Which will handle the documentation comment above. > >> And >> maybe put 'fd' into the property name. > > Good point. In fact the common naming to set bitrates for CAN(FD) > controllers are 'bitrate' and 'data bitrate'. > > 'speed' is not really a good word for that. I'm fine with switching to using bitrate instead of speed. Kurk was originally the one that suggested to use the term arbitration and data since thats how the spec refers to it. Which I do agree with. But your right that in the drivers (struct can_priv) we just use bittiming and data_bittiming (CAN-FD timings). I don't think adding "fd" into the property name makes sense unless we are calling it something like "max-canfd-bitrate" which I would agree is the easiest to understand. So what is the preference if we end up sticking with two properties? Option 1 or 2? 1) max-bitrate max-data-bitrate 2) max-bitrate max-canfd-bitrate > > Finally, @Franklin: > > A CAN transceiver is limited in bandwidth. But you only have one RX and > one TX line between the CAN controller and the CAN transceiver. The > transceiver does not know about CAN FD - it has just a physical(!) layer > with a limited bandwidth. This is ONE limitation. > > So I tend to specify only ONE 'max-bitrate' property for the > fixed-transceiver binding. > > The fact whether the CAN controller is CAN FD capable or not is provided > by the netlink configuration interface for CAN controllers. Part of the reasoning to have two properties is to indicate that you don't support CAN FD while limiting the "arbitration" bit rate. With one property you can not determine this and end up having to make some assumptions that can quickly end up biting people. > > Regards, > Oliver > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote: > > I'm fine with switching to using bitrate instead of speed. Kurk was > originally the one that suggested to use the term arbitration and data > since thats how the spec refers to it. Which I do agree with. But your > right that in the drivers (struct can_priv) we just use bittiming and > data_bittiming (CAN-FD timings). I don't think adding "fd" into the > property name makes sense unless we are calling it something like > "max-canfd-bitrate" which I would agree is the easiest to understand. > > So what is the preference if we end up sticking with two properties? > Option 1 or 2? > > 1) > max-bitrate > max-data-bitrate > > 2) > max-bitrate > max-canfd-bitrate > > 1 >> A CAN transceiver is limited in bandwidth. But you only have one RX and >> one TX line between the CAN controller and the CAN transceiver. The >> transceiver does not know about CAN FD - it has just a physical(!) layer >> with a limited bandwidth. This is ONE limitation. >> >> So I tend to specify only ONE 'max-bitrate' property for the >> fixed-transceiver binding. >> >> The fact whether the CAN controller is CAN FD capable or not is provided >> by the netlink configuration interface for CAN controllers. > > Part of the reasoning to have two properties is to indicate that you > don't support CAN FD while limiting the "arbitration" bit rate. ?? It's a physical layer device which only has a bandwidth limitation. The transceiver does not know about CAN FD. > With one > property you can not determine this and end up having to make some > assumptions that can quickly end up biting people. Despite the fact that the transceiver does not know anything about ISO layer 2 (CAN/CAN FD) the properties should look like max-bitrate canfd-capable then. But when the tranceiver is 'canfd-capable' agnostic, why provide a property for it? Maybe I'm wrong but I still can't follow your argumentation ideas. Regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/27/2017 01:47 PM, Oliver Hartkopp wrote: > On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote: >> > >> I'm fine with switching to using bitrate instead of speed. Kurk was >> originally the one that suggested to use the term arbitration and data >> since thats how the spec refers to it. Which I do agree with. But your >> right that in the drivers (struct can_priv) we just use bittiming and >> data_bittiming (CAN-FD timings). I don't think adding "fd" into the >> property name makes sense unless we are calling it something like >> "max-canfd-bitrate" which I would agree is the easiest to understand. >> >> So what is the preference if we end up sticking with two properties? >> Option 1 or 2? >> >> 1) >> max-bitrate >> max-data-bitrate >> >> 2) >> max-bitrate >> max-canfd-bitrate >> >> > > 1 > >>> A CAN transceiver is limited in bandwidth. But you only have one RX and >>> one TX line between the CAN controller and the CAN transceiver. The >>> transceiver does not know about CAN FD - it has just a physical(!) layer >>> with a limited bandwidth. This is ONE limitation. >>> >>> So I tend to specify only ONE 'max-bitrate' property for the >>> fixed-transceiver binding. >>> >>> The fact whether the CAN controller is CAN FD capable or not is provided >>> by the netlink configuration interface for CAN controllers. >> >> Part of the reasoning to have two properties is to indicate that you >> don't support CAN FD while limiting the "arbitration" bit rate. > > ?? > > It's a physical layer device which only has a bandwidth limitation. > The transceiver does not know about CAN FD. > >> With one >> property you can not determine this and end up having to make some >> assumptions that can quickly end up biting people. > > Despite the fact that the transceiver does not know anything about ISO > layer 2 (CAN/CAN FD) the properties should look like > > max-bitrate > canfd-capable > > then. > > But when the tranceiver is 'canfd-capable' agnostic, why provide a > property for it? > > Maybe I'm wrong but I still can't follow your argumentation ideas. Your right. I spoke to our CAN transceiver team and I finally get your points. So yes using "max-bitrate" alone is all we need. Sorry for the confusion and I'll create a new rev using this approach. > > Regards, > Oliver -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > On 07/27/2017 01:47 PM, Oliver Hartkopp wrote: > > On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote: > >> > > > >> I'm fine with switching to using bitrate instead of speed. Kurk was > >> originally the one that suggested to use the term arbitration and data > >> since thats how the spec refers to it. Which I do agree with. But your > >> right that in the drivers (struct can_priv) we just use bittiming and > >> data_bittiming (CAN-FD timings). I don't think adding "fd" into the > >> property name makes sense unless we are calling it something like > >> "max-canfd-bitrate" which I would agree is the easiest to understand. > >> > >> So what is the preference if we end up sticking with two properties? > >> Option 1 or 2? > >> > >> 1) > >> max-bitrate > >> max-data-bitrate > >> > >> 2) > >> max-bitrate > >> max-canfd-bitrate > >> > >> > > > > 1 > > > >>> A CAN transceiver is limited in bandwidth. But you only have one RX and > >>> one TX line between the CAN controller and the CAN transceiver. The > >>> transceiver does not know about CAN FD - it has just a physical(!) layer > >>> with a limited bandwidth. This is ONE limitation. > >>> > >>> So I tend to specify only ONE 'max-bitrate' property for the > >>> fixed-transceiver binding. > >>> > >>> The fact whether the CAN controller is CAN FD capable or not is provided > >>> by the netlink configuration interface for CAN controllers. > >> > >> Part of the reasoning to have two properties is to indicate that you > >> don't support CAN FD while limiting the "arbitration" bit rate. > > > > ?? > > > > It's a physical layer device which only has a bandwidth limitation. > > The transceiver does not know about CAN FD. > > > >> With one > >> property you can not determine this and end up having to make some > >> assumptions that can quickly end up biting people. > > > > Despite the fact that the transceiver does not know anything about ISO > > layer 2 (CAN/CAN FD) the properties should look like > > > > max-bitrate > > canfd-capable > > > > then. > > > > But when the tranceiver is 'canfd-capable' agnostic, why provide a > > property for it? > > > > Maybe I'm wrong but I still can't follow your argumentation ideas. > The transceiver does not know about CAN FD, but CAN FD uses the different restrictions of the arbitration & data phase in the CAN frame, i.e. during arbitration, the RX must indicate the wire (dominant/recessive) within 1 bit time, during data in CAN FD, this is not necessary. So while _a_ transceiver may be spec'd to 1MBit during arbitration, CAN FD packets may IMHO exceed that speed during data phase. That was the whole point of CAN FD: exceed the limits required for correct arbitration on transceiver & wire. So I do not agree on the single bandwidth limitation. The word 'max-arbitration-bitrate' makes the difference very clear. > Your right. I spoke to our CAN transceiver team and I finally get your > points. > > So yes using "max-bitrate" alone is all we need. Sorry for the confusion > and I'll create a new rev using this approach. > > > > Regards, > > Oliver Kind regards, Kurt -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/28/2017 06:57 AM, Kurt Van Dijck wrote: > So while _a_ transceiver may be spec'd to 1MBit during arbitration, > CAN FD packets may IMHO exceed that speed during data phase. When the bitrate is limited to 1Mbit/s you are ONLY allowed to use 1Mbit/s in the data section too (either with CAN or CAN FD). > That was the whole point of CAN FD: exceed the limits required for > correct arbitration on transceiver & wire. No. CAN FD is about a different frame format with up to 64 bytes AND the possibility to increase the bitrate in the data section of the frame. > So I do not agree on the single bandwidth limitation. The transceiver provides a single maximum bandwidth. It's an ISO Layer 1 device. > The word 'max-arbitration-bitrate' makes the difference very clear. I think you are mixing up ISO layer 1 and ISO layer 2. Regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt new file mode 100644 index 0000000..dc7e3eb --- /dev/null +++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt @@ -0,0 +1,42 @@ +Fixed transceiver Device Tree binding +------------------------------ + +CAN transceiver typically limits the max speed in standard CAN and CAN FD +modes. Typically these limitations are static and the transceivers themselves +provide no way to detect this limitation at runtime. For this situation, +the "fixed-transceiver" node can be used. + +Properties: + +Optional: + max-arbitration-speed: a positive non 0 value that determines the max + speed that CAN can run in non CAN-FD mode or during the + arbitration phase in CAN-FD mode. + + max-data-speed: a positive non 0 value that determines the max data rate + that can be used in CAN-FD mode. A value of -1 implies + CAN-FD is not supported by the transceiver. + +Examples: + +Based on Texas Instrument's TCAN1042HGV CAN Transceiver + +m_can0 { + .... + fixed-transceiver { + max-arbitration-speed = <1000000>; + max-data-speed = <5000000>; + }; + ... +}; + +Based on a CAN Transceiver that doesn't support CAN-FD. Only needed if the +CAN IP supports CAN-FD but is using a transceiver that doesn't. + +m_can0 { + .... + fixed-transceiver { + max-data-speed = <(-1)>; + }; + ... +};
Add documentation to describe usage of the new fixed transceiver binding. This new binding is applicable for any CAN device therefore it exists as its own document. Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> --- Version 2: Tweak commit message working Tweak wording for properties Drop unit addressing Give example if CAN-FD isn't supported .../bindings/net/can/fixed-transceiver.txt | 42 ++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/can/fixed-transceiver.txt