[PATCH] can: mcba_usb: Fix termination command argument

[PATCH] can: mcba_usb: Fix termination command argument

[Yasushi SHOJI]

Microchip USB Analyzer can be set with termination setting ON or OFF.
As I've observed, both with my oscilloscope and USB packet capture
below, you must send "0" to turn it ON, and "1" to turn it OFF.

Reverse the argument value to fix this.

These are the two commands sequence, ON then OFF.

> No.     Time           Source                Destination           Protocol Length Info
>       1 0.000000       host                  1.3.1                 USB      46     URB_BULK out
>
> Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> USB URB
> Leftover Capture Data: a80000000000000000000000000000000000a8
>
> No.     Time           Source                Destination           Protocol Length Info
>       2 4.372547       host                  1.3.1                 USB      46     URB_BULK out
>
> Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> USB URB
> Leftover Capture Data: a80100000000000000000000000000000000a9

Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
---
 drivers/net/can/usb/mcba_usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index 218b098b261d..67beff1a3876 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
 	};
 
 	if (term == MCBA_TERMINATION_ENABLED)
-		usb_msg.termination = 1;
-	else
 		usb_msg.termination = 0;
+	else
+		usb_msg.termination = 1;
 
 	mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
 
-- 
2.38.1

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Marc Kleine-Budde]

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

Let's take the original driver author into the loop.

On 24.11.2022 04:44:06, Yasushi SHOJI wrote:
> Microchip USB Analyzer can be set with termination setting ON or OFF.
> As I've observed, both with my oscilloscope and USB packet capture
> below, you must send "0" to turn it ON, and "1" to turn it OFF.
> 
> Reverse the argument value to fix this.
> 
> These are the two commands sequence, ON then OFF.
> 
> > No.     Time           Source                Destination           Protocol Length Info
> >       1 0.000000       host                  1.3.1                 USB      46     URB_BULK out
> >
> > Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > USB URB
> > Leftover Capture Data: a80000000000000000000000000000000000a8
> >
> > No.     Time           Source                Destination           Protocol Length Info
> >       2 4.372547       host                  1.3.1                 USB      46     URB_BULK out
> >
> > Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > USB URB
> > Leftover Capture Data: a80100000000000000000000000000000000a9

Is this the USB data after applying the patch?

Can you measure the resistance between CAN-H and CAN-L to verify that
your patch fixes the problem?

> Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
> ---
>  drivers/net/can/usb/mcba_usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> index 218b098b261d..67beff1a3876 100644
> --- a/drivers/net/can/usb/mcba_usb.c
> +++ b/drivers/net/can/usb/mcba_usb.c
> @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
>  	};
>  
>  	if (term == MCBA_TERMINATION_ENABLED)
> -		usb_msg.termination = 1;
> -	else
>  		usb_msg.termination = 0;
> +	else
> +		usb_msg.termination = 1;
>  
>  	mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);

What about the static void mcba_usb_process_ka_usb() function? Do you
need to convert this, too?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Vincent Mailhol]

On Thu. 24 Nov. 2022 at 04:53, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:
> Microchip USB Analyzer can be set with termination setting ON or OFF.
> As I've observed, both with my oscilloscope and USB packet capture
> below, you must send "0" to turn it ON, and "1" to turn it OFF.
>
> Reverse the argument value to fix this.
>
> These are the two commands sequence, ON then OFF.
>
> > No.     Time           Source                Destination           Protocol Length Info
> >       1 0.000000       host                  1.3.1                 USB      46     URB_BULK out
> >
> > Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > USB URB
> > Leftover Capture Data: a80000000000000000000000000000000000a8
> >
> > No.     Time           Source                Destination           Protocol Length Info
> >       2 4.372547       host                  1.3.1                 USB      46     URB_BULK out
> >
> > Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > USB URB
> > Leftover Capture Data: a80100000000000000000000000000000000a9
>
> Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
> ---
>  drivers/net/can/usb/mcba_usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> index 218b098b261d..67beff1a3876 100644
> --- a/drivers/net/can/usb/mcba_usb.c
> +++ b/drivers/net/can/usb/mcba_usb.c
> @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
>         };
>
>         if (term == MCBA_TERMINATION_ENABLED)
> -               usb_msg.termination = 1;
> -       else
>                 usb_msg.termination = 0;
> +       else
> +               usb_msg.termination = 1;
>
>         mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);

Nitpick: does it make sense to rename the field to something like
usb_msg.termination_disable or usb_msg.termination_off? This would
make it more explicit that this is a "reverse" boolean.

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Yasushi SHOJI]

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

Hi,

On Thu, Nov 24, 2022 at 7:34 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> Let's take the original driver author into the loop.
>
> On 24.11.2022 04:44:06, Yasushi SHOJI wrote:
> > Microchip USB Analyzer can be set with termination setting ON or OFF.
> > As I've observed, both with my oscilloscope and USB packet capture
> > below, you must send "0" to turn it ON, and "1" to turn it OFF.
> >
> > Reverse the argument value to fix this.
> >
> > These are the two commands sequence, ON then OFF.
> >
> > > No.     Time           Source                Destination           Protocol Length Info
> > >       1 0.000000       host                  1.3.1                 USB      46     URB_BULK out
> > >
> > > Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > > USB URB
> > > Leftover Capture Data: a80000000000000000000000000000000000a8
> > >
> > > No.     Time           Source                Destination           Protocol Length Info
> > >       2 4.372547       host                  1.3.1                 USB      46     URB_BULK out
> > >
> > > Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > > USB URB
> > > Leftover Capture Data: a80100000000000000000000000000000000a9
>
> Is this the USB data after applying the patch?

That's not from Linux.

> Can you measure the resistance between CAN-H and CAN-L to verify that
> your patch fixes the problem?

Sure.  The command I'm using on my Linux is:

    sudo ip link set can0 up type can bitrate 100000 termination X

where X is either 0 or 120.

With Debian Sid stock kernel: linux-image-6.0.0-4-amd64
  - termination 0: 135.4 Ohms
  - termination 120: 17.82 Ohms

With my patch on v6.1-rc6
  - termination 0: 22.20 Ohms
  - termination 120: 134.2 Ohms

> > Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
> > ---
> >  drivers/net/can/usb/mcba_usb.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > index 218b098b261d..67beff1a3876 100644
> > --- a/drivers/net/can/usb/mcba_usb.c
> > +++ b/drivers/net/can/usb/mcba_usb.c
> > @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> >       };
> >
> >       if (term == MCBA_TERMINATION_ENABLED)
> > -             usb_msg.termination = 1;
> > -     else
> >               usb_msg.termination = 0;
> > +     else
> > +             usb_msg.termination = 1;
> >
> >       mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
>
> What about the static void mcba_usb_process_ka_usb() function? Do you
> need to convert this, too?

Ah, yes. Thanks.
Attaching a compressed patch.

Let me know if I need to resend it as an email.

Best,
--
          yashi

[-- Attachment #2: 0001-can-mcba_usb-Fix-termination-command-argument.patch.gz --]
[-- Type: application/gzip, Size: 995 bytes --]

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Yasushi SHOJI]

On Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
<vincent.mailhol@gmail.com> wrote:
> > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > index 218b098b261d..67beff1a3876 100644
> > --- a/drivers/net/can/usb/mcba_usb.c
> > +++ b/drivers/net/can/usb/mcba_usb.c
> > @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> >         };
> >
> >         if (term == MCBA_TERMINATION_ENABLED)
> > -               usb_msg.termination = 1;
> > -       else
> >                 usb_msg.termination = 0;
> > +       else
> > +               usb_msg.termination = 1;
> >
> >         mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
>
> Nitpick: does it make sense to rename the field to something like
> usb_msg.termination_disable or usb_msg.termination_off? This would
> make it more explicit that this is a "reverse" boolean.

I'd rather define the values like

#define TERMINATION_ON (0)
#define TERMINATION_OFF (1)

So the block becomes

if (term == MCBA_TERMINATION_ENABLED)
    usb_msg.termination = TERMINATION_ON;
else
    usb_msg.termination = TERMINATION_OFF;
--
             yashi

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Vincent Mailhol]

On Thu. 24 Nov. 2022 at 18:52, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:
> On Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
> <vincent.mailhol@gmail.com> wrote:
> > > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > > index 218b098b261d..67beff1a3876 100644
> > > --- a/drivers/net/can/usb/mcba_usb.c
> > > +++ b/drivers/net/can/usb/mcba_usb.c
> > > @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> > >         };
> > >
> > >         if (term == MCBA_TERMINATION_ENABLED)
> > > -               usb_msg.termination = 1;
> > > -       else
> > >                 usb_msg.termination = 0;
> > > +       else
> > > +               usb_msg.termination = 1;
> > >
> > >         mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
> >
> > Nitpick: does it make sense to rename the field to something like
> > usb_msg.termination_disable or usb_msg.termination_off? This would
> > make it more explicit that this is a "reverse" boolean.
>
> I'd rather define the values like
>
> #define TERMINATION_ON (0)
> #define TERMINATION_OFF (1)
>
> So the block becomes
>
> if (term == MCBA_TERMINATION_ENABLED)
>     usb_msg.termination = TERMINATION_ON;
> else
>     usb_msg.termination = TERMINATION_OFF;

That also works! Thank you.

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Oliver Hartkopp]

On 24.11.22 10:50, Yasushi SHOJI wrote:
> Hi,
> 
> On Thu, Nov 24, 2022 at 7:34 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>
>> Let's take the original driver author into the loop.
>>
>> On 24.11.2022 04:44:06, Yasushi SHOJI wrote:
>>> Microchip USB Analyzer can be set with termination setting ON or OFF.
>>> As I've observed, both with my oscilloscope and USB packet capture
>>> below, you must send "0" to turn it ON, and "1" to turn it OFF.
>>>
>>> Reverse the argument value to fix this.
>>>
>>> These are the two commands sequence, ON then OFF.
>>>
>>>> No.     Time           Source                Destination           Protocol Length Info
>>>>        1 0.000000       host                  1.3.1                 USB      46     URB_BULK out
>>>>
>>>> Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
>>>> USB URB
>>>> Leftover Capture Data: a80000000000000000000000000000000000a8
>>>>
>>>> No.     Time           Source                Destination           Protocol Length Info
>>>>        2 4.372547       host                  1.3.1                 USB      46     URB_BULK out
>>>>
>>>> Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
>>>> USB URB
>>>> Leftover Capture Data: a80100000000000000000000000000000000a9
>>
>> Is this the USB data after applying the patch?
> 
> That's not from Linux.
> 
>> Can you measure the resistance between CAN-H and CAN-L to verify that
>> your patch fixes the problem?
> 
> Sure.  The command I'm using on my Linux is:
> 
>      sudo ip link set can0 up type can bitrate 100000 termination X
> 
> where X is either 0 or 120.
> 
> With Debian Sid stock kernel: linux-image-6.0.0-4-amd64
>    - termination 0: 135.4 Ohms
>    - termination 120: 17.82 Ohms
> 
> With my patch on v6.1-rc6
>    - termination 0: 22.20 Ohms
>    - termination 120: 134.2 Ohms
> 

Hugh!

What does "termination 0" mean here?

I assumed "termination 0" results in "termination off" (=> no 
termination => very high resistor value) but in fact it gets around 20 
Ohms, which is a pretty heavy termination for a CAN bus.

Best regards,
Oliver


>>> Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
>>> ---
>>>   drivers/net/can/usb/mcba_usb.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
>>> index 218b098b261d..67beff1a3876 100644
>>> --- a/drivers/net/can/usb/mcba_usb.c
>>> +++ b/drivers/net/can/usb/mcba_usb.c
>>> @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
>>>        };
>>>
>>>        if (term == MCBA_TERMINATION_ENABLED)
>>> -             usb_msg.termination = 1;
>>> -     else
>>>                usb_msg.termination = 0;
>>> +     else
>>> +             usb_msg.termination = 1;
>>>
>>>        mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
>>
>> What about the static void mcba_usb_process_ka_usb() function? Do you
>> need to convert this, too?
> 
> Ah, yes. Thanks.
> Attaching a compressed patch.
> 
> Let me know if I need to resend it as an email.
> 
> Best,
> --
>            yashi

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Yasushi SHOJI]

Hi

On Thu, Nov 24, 2022 at 7:09 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> What does "termination 0" mean here?
>
> I assumed "termination 0" results in "termination off" (=> no
> termination => very high resistor value) but in fact it gets around 20
> Ohms, which is a pretty heavy termination for a CAN bus.

[ termination { 0..65535 } ]  is what we get when you run
ip link help can

And include/uapi/linux/can/netlink.h states that

/* u16 termination range: 1..65535 Ohms */
#define CAN_TERMINATION_DISABLED 0
-- 
            yashi

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Yasushi SHOJI]

Hi again,

On Thu, Nov 24, 2022 at 7:09 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> I assumed "termination 0" results in "termination off" (=> no
> termination => very high resistor value) but in fact it gets around 20
> Ohms, which is a pretty heavy termination for a CAN bus.

I've checked with one of my colleagues and he told me I measured it wrong.

Here is a correction.

You can find the schematic of the analyzer in its user's guide
https://www.microchip.com/en-us/development-tool/APGDT002
or directly at https://ww1.microchip.com/downloads/aemDocuments/documents/APG/ProductDocuments/UserGuides/CAN-Bus-Analyzer-Users-Guide-DS50001848C.pdf

The schematics is at page 11.

Basically the "termination" controls the CAN_RES signal.  When it's
low, R24 and R25 are connected to the bus.
They are 56 Ohms.

With my patch, "termination 120" drives CAN_RES low and the resistors
are active, and "termination 0" drives it high.
-- 
             yashi

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Marc Kleine-Budde]

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

On 24.11.2022 18:52:14, Yasushi SHOJI wrote:
> On Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
> <vincent.mailhol@gmail.com> wrote:
> > > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > > index 218b098b261d..67beff1a3876 100644
> > > --- a/drivers/net/can/usb/mcba_usb.c
> > > +++ b/drivers/net/can/usb/mcba_usb.c
> > > @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> > >         };
> > >
> > >         if (term == MCBA_TERMINATION_ENABLED)
> > > -               usb_msg.termination = 1;
> > > -       else
> > >                 usb_msg.termination = 0;
> > > +       else
> > > +               usb_msg.termination = 1;
> > >
> > >         mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
> >
> > Nitpick: does it make sense to rename the field to something like
> > usb_msg.termination_disable or usb_msg.termination_off? This would
> > make it more explicit that this is a "reverse" boolean.
> 
> I'd rather define the values like
> 
> #define TERMINATION_ON (0)
> #define TERMINATION_OFF (1)
> 
> So the block becomes
> 
> if (term == MCBA_TERMINATION_ENABLED)
>     usb_msg.termination = TERMINATION_ON;
> else
>     usb_msg.termination = TERMINATION_OFF;

Please send a v2 patch, using git send-email, as you did with the first
version. (No compressed attached patches please.)

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: mcba_usb: Fix termination command argument

[Oliver Hartkopp]

Hi Yashi,

On 24.11.22 14:13, Yasushi SHOJI wrote:

> I've checked with one of my colleagues and he told me I measured it wrong.

Ok.

> Here is a correction.
> 
> You can find the schematic of the analyzer in its user's guide
> https://www.microchip.com/en-us/development-tool/APGDT002
> or directly at https://ww1.microchip.com/downloads/aemDocuments/documents/APG/ProductDocuments/UserGuides/CAN-Bus-Analyzer-Users-Guide-DS50001848C.pdf
> 
> The schematics is at page 11.
> 
> Basically the "termination" controls the CAN_RES signal.  When it's
> low, R24 and R25 are connected to the bus.
> They are 56 Ohms.

Ah, that means 2 x 56 Ohms => 112 Ohms (nearly 120)

> With my patch, "termination 120" drives CAN_RES low and the resistors
> are active, and "termination 0" drives it high.

Thanks for the clarification!

Best regards,
Oliver