Re: [PATCH] spi: Added spi master driver for Freescale MPC83xx SPI controller

[Kumar Gala <galak@kernel.crashing.org>]

On Apr 7, 2006, at 12:22 AM, David Brownell wrote:

>> This driver supports the SPI controller on the MPC83xx SoC devices  
>> from Freescale.
>> Note, this driver supports only the simple shift register SPI  
>> controller and not
>> the descriptor based CPM or QUICCEngine SPI controller.
>
> Or the QSPI on Coldfire; there's a driver for that floating around,  
> but for an
> older version of this framework (sans lists).

I'll leave that for others to figure out :)

>
>
>> --- /dev/null
>> +++ b/drivers/spi/spi_mpc83xx.c
>> @@ -0,0 +1,349 @@
>> +/*
>> + * MPC83xx SPI controller driver.
>> + *
>> + * Maintainer: Kumar Gala
>
> Needs a "Copyright (C) 2006 <NAME>" for the GPL to be valid; it's
> the copyright holder who licences the code.

Will fix.

>> + *
>> + * 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;  either version 2 of the  License,  
>> or (at your
>> + * option) any later version.
>> + */
>> ...
>> +
>> +/* Default for SPI Mode, slowest speed, MSB, inactive high, 8-bit  
>> char */
>> +#define	SPMODE_INIT_VAL (SPMODE_CI_INACTIVEHIGH |  
>> SPMODE_CP_RISE_EDGECLK | \
>> +			 SPMODE_DIV16 | SPMODE_REV | SPMODE_MS | \
>> +			 SPMODE_LEN(7) | SPMODE_PM(0xf))
>
> Hmm, it will of course be overridden as soon as needed, but shouldn't
> that default be "inactive low" clock?  SPI mode 0 that is.  That  
> stands
> out mostly because you were interpreting CPOL=0 as inactive high, and
> that's not my understanding of how that signal works...

I'll change it, not sure what I was thinking but SPI mode 0 being the  
default makes
sense.

>> +struct mpc83xx_spi {
>> +	/* bitbang has to be first */
>> +	struct spi_bitbang bitbang;
>> +	struct completion tx_done, rx_ready;
>> +
>> +	u32 __iomem *base;
>
> Erm, OK, but fwiw my preference is to have pointer-to-struct and let
> the compiler calculate the offsets (and tell you when you pass the  
> wrong
> kind of pointer).  Otherwise such pointers should use "void __iomem *"
> (or maybe in your case "__be32 *"?) for explicit {base,offset}  
> addressing.

I'll make that change.

>> +static inline void mpc83xx_spi_write_reg(__be32 * base, u32 reg,  
>> u32 val)
>> +{
>> +	out_be32(base + (reg >> 2), val);
>> +}
>> +
>> +static inline u32 mpc83xx_spi_read_reg(__be32 * base, u32 reg)
>> +{
>> +	return in_be32(base + (reg >> 2));
>> +}
>
> ... here you use "__be32" not "u32", and no "__iomem" annotation.  So
> this is inconsistent with the declaration above.  Note that if you
> just made this "&bank->regname" you'd be having the compiler do any
> offset calculation magic, and the source code will be more obvious.

Yep, I know what you mean.

>> +static
>> +int mpc83xx_spi_setup_transfer(struct spi_device *spi, struct  
>> spi_transfer *t)
>> +{
>> +	struct mpc83xx_spi *mpc83xx_spi;
>> +	u32 regval;
>> +	u32 len = t->bits_per_word - 1;
>> +
>> +	if (len == 32)
>> +		len = 0;
>
> So the hardware handles 1-33 bit words?  It'd be good to filter
> the spi_setup() path directly then, returning EINVAL for illegal
> word lengths (and clock speeds).

Uhh, no.  The HW supports 4-bit to 32-bit words.  However the  
encoding of 32-bit is 0 in the register field, and 8-bit is a value  
of 7, etc.. (bit encodings 1 & 2 are invalid).

I'm not following you on spi_setup(), but I think you mean to error  
change bits_per_word there and return EINVAL if its not one we support.

>> +static u32
>> +mpc83xx_spi_txrx(struct spi_device *spi, unsigned nsecs, u32  
>> word, u8 bits)
>> +{
>> +	struct mpc83xx_spi *mpc83xx_spi;
>> +	mpc83xx_spi = spi_master_get_devdata(spi->master);
>> +
>> +	INIT_COMPLETION(mpc83xx_spi->tx_done);
>> +	INIT_COMPLETION(mpc83xx_spi->rx_ready);
>> +
>> +	/* enable tx/rx ints */
>> +	mpc83xx_spi_write_reg(mpc83xx_spi->base, SPIM_REG, SPIM_NF |  
>> SPIM_NE);
>> +
>> +	/* transmit word */
>> +	mpc83xx_spi_write_reg(mpc83xx_spi->base, SPITD_REG, word);
>> +
>> +	/* wait for both a tx & rx interrupt */
>> +	wait_for_completion(&mpc83xx_spi->tx_done);
>> +	wait_for_completion(&mpc83xx_spi->rx_ready);
>
> I guess I'm surprised you're not using txrx_buffers() and having
> that whole thing be IRQ driven, so the per-word cost eliminates
> the task scheduling.  You already paid for IRQ handling ... why
> not have it store the rx byte into the buffer, and write the tx
> byte froom the other buffer?  That'd be cheaper than what you're
> doing now ... in both time and code.  Only wake up a task at
> the end of a given spi_transfer().

I dont follow you at all here.  What are you suggesting I do?

>
> Plus, your IRQ handler should _not_ always return IRQ_HANDLED.
> Only return it if you actually do enter one of those branches...
>
>
>> +	mpc83xx_spi->sysclk = pdata->sysclk;
>
> When MPC/PPC starts to support <linux/clk.h> would seem to be
> the right sort of time to
>
> 	mpc83xx-spi->clk = clk_get(&pdev->dev, "spi_clk");
>
> or whatever.

Will do that once we support <linux/clk.h>

>> +MODULE_DESCRIPTION("Simple Platform SPI Driver");
>
> How about "Simple MPC83xx SPI driver"?

Will change.

- k