| Message ID | 1456775971-4946-4-git-send-email-marex@denx.de |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Marek, On 02/29/2016 08:59 PM, Marek Vasut wrote: > The RX and TX ID mask for CAN2.0 is 11 bits wide. This patch fixes > the incorrect mask, which caused the CAN IDs to miss the MSBit both > on receive and transmit. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Marc Kleine-Budde <mkl@pengutronix.de> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Oliver Hartkopp <socketcan@hartkopp.net> > Cc: Wolfgang Grandegger <wg@grandegger.com> > --- > drivers/net/can/ifi_canfd/ifi_canfd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c > index 82a33bd..6704098 100644 > --- a/drivers/net/can/ifi_canfd/ifi_canfd.c > +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c > @@ -135,7 +135,7 @@ > > #define IFI_CANFD_RXFIFO_ID 0x6c > #define IFI_CANFD_RXFIFO_ID_ID_OFFSET 0 > -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x3ff > +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x7ff > #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK 0x1fffffff You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions. You won't have trapped into this problem then :-) > #define IFI_CANFD_RXFIFO_ID_IDE BIT(29) > > @@ -156,7 +156,7 @@ > > #define IFI_CANFD_TXFIFO_ID 0xbc > #define IFI_CANFD_TXFIFO_ID_ID_OFFSET 0 > -#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK 0x3ff > +#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK 0x7ff > #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK 0x1fffffff dito. Regards, Oliver > #define IFI_CANFD_TXFIFO_ID_IDE BIT(29) > >
On 03/01/2016 06:49 PM, Oliver Hartkopp wrote: > Hi Marek, Hi Oliver, > On 02/29/2016 08:59 PM, Marek Vasut wrote: >> The RX and TX ID mask for CAN2.0 is 11 bits wide. This patch fixes >> the incorrect mask, which caused the CAN IDs to miss the MSBit both >> on receive and transmit. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Marc Kleine-Budde <mkl@pengutronix.de> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Oliver Hartkopp <socketcan@hartkopp.net> >> Cc: Wolfgang Grandegger <wg@grandegger.com> >> --- >> drivers/net/can/ifi_canfd/ifi_canfd.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c >> index 82a33bd..6704098 100644 >> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c >> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c >> @@ -135,7 +135,7 @@ >> >> #define IFI_CANFD_RXFIFO_ID 0x6c >> #define IFI_CANFD_RXFIFO_ID_ID_OFFSET 0 >> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x3ff >> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x7ff >> #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK 0x1fffffff > > You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of > defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions. > > You won't have trapped into this problem then :-) These are register bitfield definitions, so should I really ? My OCD kicks in and tells me it'd be odd and inconsistent with the rest of the bitfields, but if you prefer it that way, I'll just send an updated patch. >> #define IFI_CANFD_RXFIFO_ID_IDE BIT(29) >> >> @@ -156,7 +156,7 @@ >> >> #define IFI_CANFD_TXFIFO_ID 0xbc >> #define IFI_CANFD_TXFIFO_ID_ID_OFFSET 0 >> -#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK 0x3ff >> +#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK 0x7ff >> #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK 0x1fffffff > > dito. > > > Regards, > Oliver > >> #define IFI_CANFD_TXFIFO_ID_IDE BIT(29) >> >>
Hi Marek, On 03/01/2016 10:23 PM, Marek Vasut wrote: > On 03/01/2016 06:49 PM, Oliver Hartkopp wrote: >>> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x3ff >>> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x7ff >>> #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK 0x1fffffff >> >> You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of >> defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions. >> >> You won't have trapped into this problem then :-) > > These are register bitfield definitions, so should I really ? > > My OCD kicks in and tells me it'd be odd and inconsistent with the rest > of the bitfields, but if you prefer it that way, I'll just send an > updated patch. > Your bit mask is masking the CAN ID out of a given variable. That's what CAN_SFF_MASK and CAN_EFF_MASK is made for. So at least it should be: #define IFI_CANFD_RXFIFO_ID_ID_STD_MASK CAN_SFF_MASK #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK CAN_EFF_MASK Btw. These defines are _never_ referenced in ifi_canfd.c so they should be removed anyway. Regards, Oliver
On 03/02/2016 07:10 AM, Oliver Hartkopp wrote: > Hi Marek, > > On 03/01/2016 10:23 PM, Marek Vasut wrote: >> On 03/01/2016 06:49 PM, Oliver Hartkopp wrote: > > >>>> -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x3ff >>>> +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x7ff >>>> #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK 0x1fffffff >>> >>> You should use the CAN_SFF_MASK and CAN_EFF_MASK in your code instead of >>> defining you private IFI_CANFD_RXFIFO_ID_ID_?TD_MASK definitions. >>> >>> You won't have trapped into this problem then :-) >> >> These are register bitfield definitions, so should I really ? >> >> My OCD kicks in and tells me it'd be odd and inconsistent with the rest >> of the bitfields, but if you prefer it that way, I'll just send an >> updated patch. >> > > Your bit mask is masking the CAN ID out of a given variable. > That's what CAN_SFF_MASK and CAN_EFF_MASK is made for. > > So at least it should be: > > #define IFI_CANFD_RXFIFO_ID_ID_STD_MASK CAN_SFF_MASK > #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK CAN_EFF_MASK This is good, I will do this. Thanks! > Btw. These defines are _never_ referenced in ifi_canfd.c so they should be > removed anyway. The documentation for this core is not available, so if you don't mind, I'd like to keep those.
diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c index 82a33bd..6704098 100644 --- a/drivers/net/can/ifi_canfd/ifi_canfd.c +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c @@ -135,7 +135,7 @@ #define IFI_CANFD_RXFIFO_ID 0x6c #define IFI_CANFD_RXFIFO_ID_ID_OFFSET 0 -#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x3ff +#define IFI_CANFD_RXFIFO_ID_ID_STD_MASK 0x7ff #define IFI_CANFD_RXFIFO_ID_ID_XTD_MASK 0x1fffffff #define IFI_CANFD_RXFIFO_ID_IDE BIT(29) @@ -156,7 +156,7 @@ #define IFI_CANFD_TXFIFO_ID 0xbc #define IFI_CANFD_TXFIFO_ID_ID_OFFSET 0 -#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK 0x3ff +#define IFI_CANFD_TXFIFO_ID_ID_STD_MASK 0x7ff #define IFI_CANFD_TXFIFO_ID_ID_XTD_MASK 0x1fffffff #define IFI_CANFD_TXFIFO_ID_IDE BIT(29)
The RX and TX ID mask for CAN2.0 is 11 bits wide. This patch fixes the incorrect mask, which caused the CAN IDs to miss the MSBit both on receive and transmit. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Marc Kleine-Budde <mkl@pengutronix.de> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Oliver Hartkopp <socketcan@hartkopp.net> Cc: Wolfgang Grandegger <wg@grandegger.com> --- drivers/net/can/ifi_canfd/ifi_canfd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)