RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
Hi All, Subject line of this patch should be "ARM: CAN: d_can: Add support for Bosch D_CAN controller" Regards AnilKumar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
On 04/03/2012 02:44 PM, Wolfgang Grandegger wrote: > On 04/03/2012 02:32 PM, AnilKumar Ch wrote: >> This patch adds the support for Bosch D_CAN controller. >> >> Bosch D_CAN controller is a full-CAN implementation compliant to >> CAN protocol version 2.0 part A and B. Bosch D_CAN user manual >> can be obtained from: http://www.semiconductors.bosch.de/media/ >> en/pdf/ipmodules_1/can/d_can_users_manual_111.pdf >> >> D_CAN device is used on many SoCs like AM335x, DM8148 and DM813x >> EVMs from TI, D_CAN details on AM335x can be accessed from: >> http://www.ti.com/lit/ug/spruh73c/spruh73c.pdf >> >> D_CAN can be configurable for 16, 32, 64 and 128 message objects. >> The driver implementation is based on 64 message objects. >> >> Following are the design choices made while writing the controller >> driver: >> 1. Interface Register set IF0 has be used for receive and IF1 is >>used for transmit message objects. >> 2. Out of the total Message objects available, half of it are kept >>aside for RX purposes and the rest for TX purposes. >> 3. NAPI implementation is such that both the TX and RX paths >>functions in polling mode. >> >> Signed-off-by: AnilKumar Ch > > Please explain why this CAN controller cannot be handled by the existing > C_CAN driver, eventually with some extensions. The register layout seems > almost identical, at least. ACK. Have a look at the recent at91_can driver. It has been improved to work with a flexible number of message objects. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
On 04/03/2012 02:32 PM, AnilKumar Ch wrote: > This patch adds the support for Bosch D_CAN controller. > > Bosch D_CAN controller is a full-CAN implementation compliant to > CAN protocol version 2.0 part A and B. Bosch D_CAN user manual > can be obtained from: http://www.semiconductors.bosch.de/media/ > en/pdf/ipmodules_1/can/d_can_users_manual_111.pdf > > D_CAN device is used on many SoCs like AM335x, DM8148 and DM813x > EVMs from TI, D_CAN details on AM335x can be accessed from: > http://www.ti.com/lit/ug/spruh73c/spruh73c.pdf > > D_CAN can be configurable for 16, 32, 64 and 128 message objects. > The driver implementation is based on 64 message objects. > > Following are the design choices made while writing the controller > driver: > 1. Interface Register set IF0 has be used for receive and IF1 is >used for transmit message objects. > 2. Out of the total Message objects available, half of it are kept >aside for RX purposes and the rest for TX purposes. > 3. NAPI implementation is such that both the TX and RX paths >functions in polling mode. > > Signed-off-by: AnilKumar Ch Please explain why this CAN controller cannot be handled by the existing C_CAN driver, eventually with some extensions. The register layout seems almost identical, at least. Wolfgang. > --- > drivers/net/can/Kconfig| 14 + > drivers/net/can/Makefile |1 + > drivers/net/can/d_can.c| 1487 > > include/linux/can/platform/d_can.h | 40 + > 4 files changed, 1542 insertions(+), 0 deletions(-) > create mode 100644 drivers/net/can/d_can.c > create mode 100644 include/linux/can/platform/d_can.h > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index bb709fd..2529cba 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -68,6 +68,20 @@ config CAN_TI_HECC > Driver for TI HECC (High End CAN Controller) module found on many > TI devices. The device specifications are available from www.ti.com > > +config CAN_D_CAN > + tristate "Bosch D_CAN Controller" > + depends on CAN_DEV > + ---help--- > + This driver adds support for the D_CAN device found in > + many SoCs like am335x, dm814x and dm813x boards from TI. > + > + The device user guide can be accessed from > + http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/ > + can/d_can_users_manual_111.pdf > + > + To compile this driver as a module, choose M here: the > + module will be called d_can. > + > config CAN_MCP251X > tristate "Microchip MCP251x SPI CAN controllers" > depends on CAN_DEV && SPI && HAS_DMA > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile > index 938be37..4bd3a87 100644 > --- a/drivers/net/can/Makefile > +++ b/drivers/net/can/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_CAN_C_CAN) += c_can/ > obj-$(CONFIG_CAN_CC770) += cc770/ > obj-$(CONFIG_CAN_AT91) += at91_can.o > obj-$(CONFIG_CAN_TI_HECC)+= ti_hecc.o > +obj-$(CONFIG_CAN_D_CAN) += d_can.o > obj-$(CONFIG_CAN_MCP251X)+= mcp251x.o > obj-$(CONFIG_CAN_BFIN) += bfin_can.o > obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o > diff --git a/drivers/net/can/d_can.c b/drivers/net/can/d_can.c > new file mode 100644 > index 000..51e2986 > --- /dev/null > +++ b/drivers/net/can/d_can.c > @@ -0,0 +1,1487 @@ > +/* > + * CAN bus driver for Bosch D_CAN controller > + * > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ > + * > + * Borrowed from C_CAN driver > + * Copyright (C) 2010 ST Microelectronics > + * - Bhupesh Sharma > + * > + * Borrowed heavily from the C_CAN driver originally written by: > + * Copyright (C) 2007 > + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix > + * - Simon Kallweit, intefo AG > + * > + * Bosch D_CAN controller is compliant to CAN protocol version 2.0 part A > and B. > + * Bosch D_CAN user manual can be obtained from: > + * http://www.semiconductors.bosch.de/media/en/pdf/ipmodules_1/can/ > + * d_can_users_manual_111.pdf > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#define D_CAN_DRV_NAME "d_can" > +#define D_CAN_VERSION"1.0" > +#define D_CAN_DRV_DESC "CAN bus driver for Bosch D_CAN controller " \ > + D_C
RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
Hi Wolfgang, Thanks for reviewing the patch On Tue, Apr 03, 2012 at 18:14:55, Wolfgang Grandegger wrote: > On 04/03/2012 02:32 PM, AnilKumar Ch wrote: > > This patch adds the support for Bosch D_CAN controller. > > > > Bosch D_CAN controller is a full-CAN implementation compliant to > > CAN protocol version 2.0 part A and B. Bosch D_CAN user manual > > can be obtained from: http://www.semiconductors.bosch.de/media/ > > en/pdf/ipmodules_1/can/d_can_users_manual_111.pdf > > > > D_CAN device is used on many SoCs like AM335x, DM8148 and DM813x > > EVMs from TI, D_CAN details on AM335x can be accessed from: > > http://www.ti.com/lit/ug/spruh73c/spruh73c.pdf > > > > D_CAN can be configurable for 16, 32, 64 and 128 message objects. > > The driver implementation is based on 64 message objects. > > > > Following are the design choices made while writing the controller > > driver: > > 1. Interface Register set IF0 has be used for receive and IF1 is > >used for transmit message objects. > > 2. Out of the total Message objects available, half of it are kept > >aside for RX purposes and the rest for TX purposes. > > 3. NAPI implementation is such that both the TX and RX paths > >functions in polling mode. > > > > Signed-off-by: AnilKumar Ch > > Please explain why this CAN controller cannot be handled by the existing > C_CAN driver, eventually with some extensions. The register layout seems > almost identical, at least. > > Wolfgang. > These are the some of the pointers I can say, why I had gone for separate file instead of existing driver: * In case of D_CAN driver we can see all the registers are 32bit length but in case of C_CAN registers are in 16bit length. * Some of the registers, bit masks are different, so we have to add checks on every API for differentiating the kind of device * In case of D_CAN we have some extra features like direct message RAM access, DMA support, TX/RX pins can be used as GPIO lines (if applicable), more interrupt lines and three sets of interface registers. * Wait timings while init bit set/reset during bit-timing initialization are different in both the cases * bittiming configurations are different. Thanks AnilKumar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
On 04/03/2012 03:41 PM, AnilKumar, Chimata wrote: > Hi Wolfgang, > > Thanks for reviewing the patch > > On Tue, Apr 03, 2012 at 18:14:55, Wolfgang Grandegger wrote: >> On 04/03/2012 02:32 PM, AnilKumar Ch wrote: >>> This patch adds the support for Bosch D_CAN controller. >>> >>> Bosch D_CAN controller is a full-CAN implementation compliant to >>> CAN protocol version 2.0 part A and B. Bosch D_CAN user manual >>> can be obtained from: http://www.semiconductors.bosch.de/media/ >>> en/pdf/ipmodules_1/can/d_can_users_manual_111.pdf >>> >>> D_CAN device is used on many SoCs like AM335x, DM8148 and DM813x >>> EVMs from TI, D_CAN details on AM335x can be accessed from: >>> http://www.ti.com/lit/ug/spruh73c/spruh73c.pdf >>> >>> D_CAN can be configurable for 16, 32, 64 and 128 message objects. >>> The driver implementation is based on 64 message objects. >>> >>> Following are the design choices made while writing the controller >>> driver: >>> 1. Interface Register set IF0 has be used for receive and IF1 is >>>used for transmit message objects. >>> 2. Out of the total Message objects available, half of it are kept >>>aside for RX purposes and the rest for TX purposes. >>> 3. NAPI implementation is such that both the TX and RX paths >>>functions in polling mode. >>> >>> Signed-off-by: AnilKumar Ch >> >> Please explain why this CAN controller cannot be handled by the existing >> C_CAN driver, eventually with some extensions. The register layout seems >> almost identical, at least. >> >> Wolfgang. >> > > These are the some of the pointers I can say, why I had gone for separate > file instead of existing driver: > * In case of D_CAN driver we can see all the registers are 32bit length > but in case of C_CAN registers are in 16bit length. How many bits in these 32 bit registers are used? > * Some of the registers, bit masks are different, so we have to add > checks on every API for differentiating the kind of device Which registers are this? Can you give us an example? > * In case of D_CAN we have some extra features like direct message RAM > access, DMA support, TX/RX pins can be used as GPIO lines (if applicable), > more interrupt lines and three sets of interface registers. Which of these features are used in you driver? > * Wait timings while init bit set/reset during bit-timing initialization > are different in both the cases That's not the hot code path, so some ifs shouldn't hurt. > * bittiming configurations are different. see above. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
On Tue, Apr 03, 2012 at 19:19:32, Marc Kleine-Budde wrote: > On 04/03/2012 03:41 PM, AnilKumar, Chimata wrote: > > Hi Wolfgang, > > > > Thanks for reviewing the patch > > > > On Tue, Apr 03, 2012 at 18:14:55, Wolfgang Grandegger wrote: > >> On 04/03/2012 02:32 PM, AnilKumar Ch wrote: > >>> This patch adds the support for Bosch D_CAN controller. > >>> > >>> Bosch D_CAN controller is a full-CAN implementation compliant to > >>> CAN protocol version 2.0 part A and B. Bosch D_CAN user manual > >>> can be obtained from: http://www.semiconductors.bosch.de/media/ > >>> en/pdf/ipmodules_1/can/d_can_users_manual_111.pdf > >>> > >>> D_CAN device is used on many SoCs like AM335x, DM8148 and DM813x > >>> EVMs from TI, D_CAN details on AM335x can be accessed from: > >>> http://www.ti.com/lit/ug/spruh73c/spruh73c.pdf > >>> > >>> D_CAN can be configurable for 16, 32, 64 and 128 message objects. > >>> The driver implementation is based on 64 message objects. > >>> > >>> Following are the design choices made while writing the controller > >>> driver: > >>> 1. Interface Register set IF0 has be used for receive and IF1 is > >>>used for transmit message objects. > >>> 2. Out of the total Message objects available, half of it are kept > >>>aside for RX purposes and the rest for TX purposes. > >>> 3. NAPI implementation is such that both the TX and RX paths > >>>functions in polling mode. > >>> > >>> Signed-off-by: AnilKumar Ch > >> > >> Please explain why this CAN controller cannot be handled by the existing > >> C_CAN driver, eventually with some extensions. The register layout seems > >> almost identical, at least. > >> > >> Wolfgang. > >> > > > > These are the some of the pointers I can say, why I had gone for separate > > file instead of existing driver: > > * In case of D_CAN driver we can see all the registers are 32bit length > > but in case of C_CAN registers are in 16bit length. > > How many bits in these 32 bit registers are used? In some cases (D_CAN_TXRQ, D_CAN_INTPND, D_CAN_MSGVAL) I have used all the bits, in some cases used few bits. Roughly I can say that its (higher 16bits) % of usages is similar as compare to 16bits While checking the status of TXRequest registers and INT pending register, which is a hot code path, we have to put if checks for register access. > > > * Some of the registers, bit masks are different, so we have to add > > checks on every API for differentiating the kind of device > > Which registers are this? Can you give us an example? I am pointing out some of the resisters * Single registers in case of D_CAN but multiple register in case of C_CAN So masks will change MASK, ARB, INTPND * D_CAN_IFCMD is the combination of COMM request and COMM mask registers > > > * In case of D_CAN we have some extra features like direct message RAM > > access, DMA support, TX/RX pins can be used as GPIO lines (if applicable), > > more interrupt lines and three sets of interface registers. > > Which of these features are used in you driver? Right now I added only CAN functionality, in that I am using two set of interface registers (0 and 1). Based on other features status register are extended, we have to handle those as well. > > > * Wait timings while init bit set/reset during bit-timing initialization > > are different in both the cases > > That's not the hot code path, so some ifs shouldn't hurt. > > > * bittiming configurations are different. > > see above. > > Marc > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions| Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917- | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
On 04/03/2012 04:29 PM, AnilKumar, Chimata wrote: Please explain why this CAN controller cannot be handled by the existing C_CAN driver, eventually with some extensions. The register layout seems almost identical, at least. Wolfgang. >>> >>> These are the some of the pointers I can say, why I had gone for separate >>> file instead of existing driver: >>> * In case of D_CAN driver we can see all the registers are 32bit length >>> but in case of C_CAN registers are in 16bit length. >> >> How many bits in these 32 bit registers are used? > > In some cases (D_CAN_TXRQ, D_CAN_INTPND, D_CAN_MSGVAL) I have used all the > bits, in some cases used few bits. > > Roughly I can say that its (higher 16bits) % of usages is similar as compare > to 16bits > > While checking the status of TXRequest registers and INT pending register, > which is a hot code path, we have to put if checks for register access. The c_can already has a c_can_read_reg32() function. It's for example used in the rx_poll function. You can make it a function pointer (i.e. pric->read_reg32()) for easy abstraction. >>> * Some of the registers, bit masks are different, so we have to add >>> checks on every API for differentiating the kind of device >> >> Which registers are this? Can you give us an example? > > I am pointing out some of the resisters > * Single registers in case of D_CAN but multiple register in case of C_CAN > So masks will change MASK, ARB, INTPND > * D_CAN_IFCMD is the combination of COMM request and COMM mask registers Maybe you can use the read_reg32 function on both c_can and d_can. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
On 03.04.2012 14:39, AnilKumar, Chimata wrote: > Hi All, > > Subject line of this patch should be > "ARM: CAN: d_can: Add support for Bosch D_CAN controller" No, the d_can IP core is generally not limited to ARM. "can: Add support for Bosch D_CAN controller" would be perfectly fine. The correct way is: 1. post your new CAN driver to linux-can ML 2. When the major reviews & style fixes are done and the driver is fit for mainline you can post it on ti-omap ML too But an initial cross-posting of this CAN driver to netdev ML and linux-kernel ML does not help anyone - it only increases traffic & confusion. See MAINTAINERS: http://lxr.linux.no/#linux+v3.3.1/MAINTAINERS#L1710 Thanks for following these hints, Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
On Tue, Apr 03, 2012 at 21:03:40, Marc Kleine-Budde wrote: > On 04/03/2012 04:29 PM, AnilKumar, Chimata wrote: > Please explain why this CAN controller cannot be handled by the existing > C_CAN driver, eventually with some extensions. The register layout seems > almost identical, at least. > > Wolfgang. > > >>> > >>> These are the some of the pointers I can say, why I had gone for separate > >>> file instead of existing driver: > >>> * In case of D_CAN driver we can see all the registers are 32bit length > >>> but in case of C_CAN registers are in 16bit length. > >> > >> How many bits in these 32 bit registers are used? > > > > In some cases (D_CAN_TXRQ, D_CAN_INTPND, D_CAN_MSGVAL) I have used all the > > bits, in some cases used few bits. > > > > Roughly I can say that its (higher 16bits) % of usages is similar as compare > > to 16bits > > > > While checking the status of TXRequest registers and INT pending register, > > which is a hot code path, we have to put if checks for register access. > > The c_can already has a c_can_read_reg32() function. It's for example > used in the rx_poll function. You can make it a function pointer (i.e. > pric->read_reg32()) for easy abstraction. This won't fit for D_CAN case because offsets are different in c_can compared to d_can. For example if I read CONTROL_REG register (0x0) in case of d_can, which will read only control register. In case of c_can it will read CONTROL_REG + STATUS register values in single read > > >>> * Some of the registers, bit masks are different, so we have to add > >>> checks on every API for differentiating the kind of device > >> > >> Which registers are this? Can you give us an example? > > > > I am pointing out some of the resisters > > * Single registers in case of D_CAN but multiple register in case of C_CAN > > So masks will change MASK, ARB, INTPND > > * D_CAN_IFCMD is the combination of COMM request and COMM mask registers > > Maybe you can use the read_reg32 function on both c_can and d_can. Above comment applies here as well > > regards, Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions| Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917- | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
Hi Oliver, Thanks for the comments, On Tue, Apr 03, 2012 at 21:24:23, Oliver Hartkopp wrote: > On 03.04.2012 14:39, AnilKumar, Chimata wrote: > > > Hi All, > > > > Subject line of this patch should be > > "ARM: CAN: d_can: Add support for Bosch D_CAN controller" > > > No, the d_can IP core is generally not limited to ARM. > > "can: Add support for Bosch D_CAN controller" > > would be perfectly fine. Agree, I will change the subject line in my next version of patch > > The correct way is: > > 1. post your new CAN driver to linux-can ML > 2. When the major reviews & style fixes are done and the driver is fit for > mainline you can post it on ti-omap ML too > > But an initial cross-posting of this CAN driver to netdev ML and linux-kernel > ML does not help anyone - it only increases traffic & confusion > It's true, I will take care from next time onwards Thanks AnilKumar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
> -Original Message- > From: linux-can-ow...@vger.kernel.org [mailto:linux-can- > ow...@vger.kernel.org] On Behalf Of AnilKumar, Chimata > Sent: Tuesday, April 03, 2012 9:28 PM > To: Marc Kleine-Budde > Cc: Wolfgang Grandegger; socket...@hartkopp.net; m.kleine- > bu...@pengutronix.de; linux-...@vger.kernel.org; > net...@vger.kernel.org; linux-omap@vger.kernel.org; linux- > ker...@vger.kernel.org; Gole, Anant; Nori, Sekhar > Subject: RE: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for > Bosch D_CAN controller > > On Tue, Apr 03, 2012 at 21:03:40, Marc Kleine-Budde wrote: > > On 04/03/2012 04:29 PM, AnilKumar, Chimata wrote: > > >>>> Please explain why this CAN controller cannot be handled by the > existing > > >>>> C_CAN driver, eventually with some extensions. The register > layout seems > > >>>> almost identical, at least. > > >>>> > > >>>> Wolfgang. > > >>>> > > >>> > > >>> These are the some of the pointers I can say, why I had gone for > separate > > >>> file instead of existing driver: > > >>> * In case of D_CAN driver we can see all the registers are 32bit > length > > >>> but in case of C_CAN registers are in 16bit length. > > >> > > >> How many bits in these 32 bit registers are used? > > > > > > In some cases (D_CAN_TXRQ, D_CAN_INTPND, D_CAN_MSGVAL) I have used > all the > > > bits, in some cases used few bits. > > > > > > Roughly I can say that its (higher 16bits) % of usages is similar > as compare > > > to 16bits > > > > > > While checking the status of TXRequest registers and INT pending > register, > > > which is a hot code path, we have to put if checks for register > access. > > > > The c_can already has a c_can_read_reg32() function. It's for example > > used in the rx_poll function. You can make it a function pointer > (i.e. > > pric->read_reg32()) for easy abstraction. > > This won't fit for D_CAN case because offsets are different in c_can > compared > to d_can. For example if I read CONTROL_REG register (0x0) in case of > d_can, > which will read only control register. In case of c_can it will read > CONTROL_REG + STATUS register values in single read C_CAN core has both 16-bit as well as 32-bit registers. While the registers like NEWDATA and INTPND are 32-bit the others like Control and Status are 16-bit. These cases are handled by the present C_CAN driver already. If you will see the C_CAN driver then you will see that the normal read/write operations are 16-bit whereas a special variant is provided for 32-bit access. Are you sure you cannot use them as it is? > > > > >>> * Some of the registers, bit masks are different, so we have to > add > > >>> checks on every API for differentiating the kind of device > > >> > > >> Which registers are this? Can you give us an example? > > > > > > I am pointing out some of the resisters > > > * Single registers in case of D_CAN but multiple register in case > of C_CAN > > > So masks will change MASK, ARB, INTPND > > > * D_CAN_IFCMD is the combination of COMM request and COMM mask > registers > > > > Maybe you can use the read_reg32 function on both c_can and d_can. > > Above comment applies here as well > This can be easily handled. I have worked on several drivers that do that. See for example the STMPE multi-function device driver (MFD), here [1] There are various variants of STMPE devices (like STMPE801 etc..) and they can be handled by using a common driver and passing different platform data intelligently that provides specific handling for a specific STMPE device. Regards, Bhupesh [1] http://lxr.linux.no/linux+v3.3.1/drivers/mfd/stmpe.c -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: AM33XX: CAN: d_can: Add support for Bosch D_CAN controller
On 04/03/2012 05:58 PM, AnilKumar, Chimata wrote: > On Tue, Apr 03, 2012 at 21:03:40, Marc Kleine-Budde wrote: >> On 04/03/2012 04:29 PM, AnilKumar, Chimata wrote: >> Please explain why this CAN controller cannot be handled by the existing >> C_CAN driver, eventually with some extensions. The register layout seems >> almost identical, at least. >> >> Wolfgang. >> > > These are the some of the pointers I can say, why I had gone for separate > file instead of existing driver: > * In case of D_CAN driver we can see all the registers are 32bit length > but in case of C_CAN registers are in 16bit length. How many bits in these 32 bit registers are used? >>> >>> In some cases (D_CAN_TXRQ, D_CAN_INTPND, D_CAN_MSGVAL) I have used all the >>> bits, in some cases used few bits. >>> >>> Roughly I can say that its (higher 16bits) % of usages is similar as compare >>> to 16bits >>> >>> While checking the status of TXRequest registers and INT pending register, >>> which is a hot code path, we have to put if checks for register access. >> >> The c_can already has a c_can_read_reg32() function. It's for example >> used in the rx_poll function. You can make it a function pointer (i.e. >> pric->read_reg32()) for easy abstraction. > > This won't fit for D_CAN case because offsets are different in c_can compared > to d_can. For example if I read CONTROL_REG register (0x0) in case of d_can, > which will read only control register. In case of c_can it will read > CONTROL_REG + STATUS register values in single read > >> > * Some of the registers, bit masks are different, so we have to add > checks on every API for differentiating the kind of device Which registers are this? Can you give us an example? >>> >>> I am pointing out some of the resisters >>> * Single registers in case of D_CAN but multiple register in case of C_CAN >>> So masks will change MASK, ARB, INTPND >>> * D_CAN_IFCMD is the combination of COMM request and COMM mask registers >> >> Maybe you can use the read_reg32 function on both c_can and d_can. > > Above comment applies here as well I did look into the manual. Unfortunately, a direct mapping of the C_CAN to the D_CAN registers seems not possible. It's not just a different alignment but sometimes two 16-bit C_CAN registers are folded into *one* 32-bit D_CAN register. Therefore we need something more clever, e.g. using a separate struct or union or handling those register separately. I still think, if feasible, we should avoid an extra driver for the D_CAN controller, also because we sooner than later need the same infrastructure (register_c_can_dev etc.). Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
