Hi Marc, > -----Original Message----- > From: Marc Kleine-Budde [mailto:[email protected]] > Sent: Wednesday, September 01, 2010 4:48 PM > To: Bhupesh SHARMA > Cc: [email protected]; [email protected] > Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver > > Bhupesh SHARMA wrote: > > Hi Wolfgang, > > > > Please ignore the earlier mail.. > > > >> -----Original Message----- > >> From: Wolfgang Grandegger [mailto:[email protected]] > >> Sent: Wednesday, September 01, 2010 2:44 PM > >> To: Bhupesh SHARMA > >> Cc: [email protected] > >> Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver > >> > >> Hi Bhupesh, > >> > >> On 09/01/2010 11:01 AM, Bhupesh SHARMA wrote: > >>> Hi Wolfgang, > >>> > >>>> -----Original Message----- > >>>> From: Wolfgang Grandegger [mailto:[email protected]] > >>>> Sent: Wednesday, September 01, 2010 12:56 PM > >>>> To: Bhupesh SHARMA > >>>> Cc: [email protected] > >>>> Subject: Re: [RFC PATCH 1/4] SPEAr320 CCAN driver > >>>> > >>>> Hi Bhupesh, > >>>> > >>>> On 09/01/2010 06:40 AM, Bhupesh SHARMA wrote: > >> ... > >> > >>>>> It is also particularly useful as we now have a new SPEAr1310 SoC > >>>> which also uses the Bosch CCAN controller > >>>>> , but the glue logic has changed from SPEAr320 and hence we can > >>>> handle most of the platform/machine specific details > >>>>> in SPEAr1310 driver and keep Bosch CCAN driver as it is. > >>>> What has changed with the new Soc and what platform specific > >>>> details and initializations do you think about? I personally do > not > >>>> see a lot of platform specific code in your spear320_can.c. Just > >> spear320_can_read() > >>>> and spear320_can_write() and also these functions look pretty > >> generic. > >>>> Also note that platform specific initialization is usually not > done > >> in > >>>> the CAN driver but the platform code. > >>> In a way you are correct: mainly the spear320_can_read() and > >> spear320_can_write() > >>> change from spear1310_can_read() and spear1310_can_write() > >> implementations. > >> > >> Can you show us the new spear1310_can_read/write functions? I want > to > >> understand how generic they really are. > > > > spear1310 differs from spear320 in the way that 16-bit registers are > > aligned at a 16-bit boundary whereas in spear320 these 16-bit > > registers are aligned at a 32-bit boundary. So, while spear1310 > routines should look like this: > > > > static u16 spear1310_can_read_reg(const struct bosch_ccan_priv *priv, > > enum ccan_regs reg) > > { > > u16 val; > > > > /* 16 bit registers are aligned at 16-bit boundary */ > > val = readw(priv->reg_base + reg); > > return val; > > } > > > > static void spear1310_can_write_reg(const struct bosch_ccan_priv > *priv, > > enum ccan_regs reg, u16 val) > > { > > /* 16 bit registers are aligned at 16-bit boundary */ > > writew(val, priv->reg_base + reg); > > } > > > > The one for SPEAr320 will look like: > > static u16 spear320_can_read_reg(const struct bosch_ccan_priv *priv, > > enum ccan_regs reg) > > { > > u16 val; > > > > /* shifting 1 place because 16 bit registers are word aligned */ > > val = readw(priv->reg_base + (reg << 1)); > > return val; > > } > > > > static void spear320_can_write_reg(const struct bosch_ccan_priv > *priv, > > enum ccan_regs reg, u16 val) > > { > > /* shifting 1 place because 16 bit registers are word aligned */ > > writew(val, priv->reg_base + (reg << 1)); } > > [...] > > > Please let me know on basis of the above register definitions. > > Have a look at the sja1000_platform driver that Wolfgang already > mentioned: > http://lxr.linux.no/linux+v2.6.35/drivers/net/can/sja1000/sja1000_platf > orm.c#L39 > > IMHO for normal SOC like your spears it would be sufficient to have a > generic ccan_platform driver, just like the sja1000. > > If you have a broken ccan integration in your SOC, like on the hynix > chip (the one we developed a driver for at pengutronix), you can > provide your own driver with the sepcial read/write functions.
Infact the case for SPEAr is almost similar to the Hynix chip as the glue logic (read, register interface) has and will keep changing for different SoCs- just have a look at the SPEAr320 and SPEAr1310 interfaces in my original mail. This makes it almost impossible to keep a single platform driver file on lines of sja1000_platform.c for CCAN. > Does the generic ccan driver support clock handling and does it have > support for transceiver enable/disable callbacks? Here again the glue logic comes into picture. On both the SoCs that we have now the CAN source clock is APB(83.3 MHz) However this changes if someone uses a SoC's that doesn't have a ARM core yet it still uses the Bosch CCAN controllers internally. In that case the CCAN driver should be kept independent of the clock handling stuff and it should be done by a driver like SoCxxx_can.c that only uses the Bosch CCAN driver services and keeps track of all the platform data disparities. Please let me know if this makes sense to you. Regards, Bhupesh _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
