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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/