Re: [PATCH net-next 2/3] can: mscan-mpc5xxx: add support for the MPC521x processor

[Wolfgang Grandegger <wg@grandegger.com>]

Wolfram Sang wrote:
> On Sat, Jan 02, 2010 at 09:17:53AM +0100, Wolfgang Grandegger wrote:
>> From: Wolfgang Grandegger <wg@denx.de>
>>
>> The main differences compared to the MSCAN on the MPC5200 are:
>>
>> - More flexibility in choosing the CAN source clock and frequency:
>>
>>   Three different clock sources can be selected: "ip", "ref" or "sys".
>>   For the latter two, a clock divider can be defined as well. If the
>>   clock source is not specified by the device tree, we first try to
>>   find an optimal CAN source clock based on the system clock. If that
>>   is not possible, the reference clock will be used.
>>
>> - The behavior of bus-off recovery is configurable:
>>
>>   To comply with the usual handling of Socket-CAN bus-off recovery,
>>   "recovery on request" is selected (instead of automatic recovery).
>>
>> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
>> ---
>>  drivers/net/can/mscan/Kconfig       |    2 +-
>>  drivers/net/can/mscan/mpc5xxx_can.c |  234 +++++++++++++++++++++++++++++------
>>  drivers/net/can/mscan/mscan.c       |   41 +++++--
>>  drivers/net/can/mscan/mscan.h       |   81 ++++++------
>>  4 files changed, 271 insertions(+), 87 deletions(-)
>>
[snip]

>> +#else /* !CONFIG_PPC_MPC5200 */
>> +static u32 __devinit mpc52xx_can_get_clock(struct of_device *ofdev,
>> +					   const char *clock_name,
>> +					   int *mscan_clksrc)
>> +{
>> +	return 0;
>> +}
>> +#endif /* CONFIG_PPC_MPC5200 */
> 
> Hmmm, I don't really like those empty functions. I once used the data-field of
> struct of_device_id, which carried a function pointer to a specific
> init-function for the matched device. What do you think about such an approach?

Often the problem is that the function will not compile on the other MPC
arch. This is not true here. So, the main reason for the #ifdefs is
space saving. Your approach will not help in both cases.

>> +
>> +#ifdef CONFIG_PPC_MPC512x
>> +struct mpc512x_clockctl {
>> +	u32 spmr;		/* System PLL Mode Reg */
>> +	u32 sccr[2];		/* System Clk Ctrl Reg 1 & 2 */
>> +	u32 scfr1;		/* System Clk Freq Reg 1 */
>> +	u32 scfr2;		/* System Clk Freq Reg 2 */
>> +	u32 reserved;
>> +	u32 bcr;		/* Bread Crumb Reg */
>> +	u32 pccr[12];		/* PSC Clk Ctrl Reg 0-11 */
>> +	u32 spccr;		/* SPDIF Clk Ctrl Reg */
>> +	u32 cccr;		/* CFM Clk Ctrl Reg */
>> +	u32 dccr;		/* DIU Clk Cnfg Reg */
>> +	u32 mccr[4];		/* MSCAN Clk Ctrl Reg 1-3 */
>> +};
>> +
>> +static struct of_device_id mpc512x_clock_ids[] __devinitdata = {
>> +	{ .compatible = "fsl,mpc5121-clock", },
>> +	{}
>> +};
>> +
>> +static u32  __devinit mpc512x_can_get_clock(struct of_device *ofdev,
>> +					    const char *clock_name,
>> +					    int *mscan_clksrc,
>> +					    ssize_t mscan_addr)
>> +{
>> +	struct mpc512x_clockctl __iomem *clockctl;
>> +	struct device_node *np_clock;
>> +	struct clk *sys_clk, *ref_clk;
>> +	int plen, clockidx, clocksrc = -1;
>> +	u32 sys_freq, val, clockdiv = 1, freq = 0;
>> +	const u32 *pval;
>> +
>> +	np_clock = of_find_matching_node(NULL, mpc512x_clock_ids);
>> +	if (!np_clock) {
>> +		dev_err(&ofdev->dev, "couldn't find clock node\n");
>> +		return -ENODEV;
>> +	}
>> +	clockctl = of_iomap(np_clock, 0);
>> +	if (!clockctl) {
>> +		dev_err(&ofdev->dev, "couldn't map clock registers\n");
>> +		return 0;
>> +	}
>> +
>> +	/* Determine the MSCAN device index from the physical address */
>> +	clockidx = (mscan_addr & 0x80) ? 1 : 0;
>> +	if (mscan_addr & 0x2000)
>> +		clockidx += 2;
> 
> The PSCs use 'cell-index', here we use mscan_addr to derive the index. This is
> not consistent, but should be IMHO. Now, which is the preferred way? I think
> I'd go for 'cell-index', as other processors might have mscan_addr shuffled.
> Also, we could use 'of_iomap' again in the probe_routine.

I understood that "cell-index" is deprecated and it has been removed
from many nodes. That's why I used the address to derive the index.

I will fix all other issues.

Wolfgang.