spi_mpc8xxx: Setting cs_change at the last transfer of a message
As stated in spi.h (at struct spi_transfer) the driver can keep the chip select of the last device active if cs_change is set at the last transfer of a message. The spi_mpc8xxx driver always deactivates the chip select after completing the message regardless of cs_change is set or not. I'd like to patch this. But before doing that I'd like to learn if there are any known use cases where the above described behavior of the driver is needed. Thanks Torsten ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: spi_mpc8xxx.c: chip select polarity problem
On Wed, Dec 9, 2009 at 18:46:51 Grant Likely wrote:
to-fleisc...@t-online.de wrote:
[...]
The following patch adds a function to get the active state of the chip
select of a SPI device by looking for the 'spi-cs-high' property in the
associated device tree node.
This function is used by the spi_mpc8xxx driver to set a proper initial
value to the associated GPIOs.
Signed-off-by: Torsten Fleischer to-fleisc...@t-online.de
I like this better. See comments below.
[...]
Hey Grant,
below there is a new version of the patch containing the modifications
according to your comments.
Thanks for the hints,
Torsten
Signed-off-by: Torsten Fleischer to-fleisc...@t-online.de
---
diff -ruN linux-2.6.32_orig//drivers/of/of_spi.c
linux-2.6.32/drivers/of/of_spi.c
--- linux-2.6.32_orig//drivers/of/of_spi.c 2009-12-03 04:51:21.0
+0100
+++ linux-2.6.32/drivers/of/of_spi.c2009-12-11 17:57:22.016787665 +0100
@@ -12,6 +12,43 @@
#include linux/of_spi.h
/**
+ * of_get_spi_cs_active_state - Gets the chip select's active state of a SPI
+ * child devices.
+ * @np:parent node of the SPI device nodes
+ * @index: index/address of the SPI device (refers to the 'reg' property)
+ *
+ * Returns 0 for 'active low' or if the child's node can't be found.
+ * Otherwise it returns 1 ('active high').
+ */
+bool of_get_spi_cs_active_state(struct device_node *np, int index)
+{
+ struct device_node *child;
+ const int *prop;
+ int len;
+ bool active = 0;
+
+ /* search for the matching SPI device */
+ for_each_child_of_node(np, child) {
+ prop = of_get_property(child, reg, len);
+ if (!prop || len sizeof(*prop)) {
+ /* property 'reg' not available (not an error) */
+ continue;
+ }
+
+ if ( *prop == index ) {
+ /* matching device found */
+ if (of_find_property(child, spi-cs-high, NULL))
+ active = 1;
+ break;
+ }
+ }
+
+ return active;
+}
+EXPORT_SYMBOL(of_get_spi_cs_active_state);
+
+
+/**
* of_register_spi_devices - Register child devices onto the SPI bus
* @master:Pointer to spi_master device
* @np:parent node of SPI device nodes
diff -ruN linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c
linux-2.6.32/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c2009-12-03
04:51:21.0 +0100
+++ linux-2.6.32/drivers/spi/spi_mpc8xxx.c 2009-12-11 17:15:47.957379333
+0100
@@ -705,6 +705,7 @@
for (; i ngpios; i++) {
int gpio;
enum of_gpio_flags flags;
+ bool astate;
gpio = of_get_gpio_flags(np, i, flags);
if (!gpio_is_valid(gpio)) {
@@ -721,8 +722,18 @@
pinfo-gpios[i] = gpio;
pinfo-alow_flags[i] = flags OF_GPIO_ACTIVE_LOW;
+ /* get the active state of the SPI device */
+ astate = of_get_spi_cs_active_state(np, i);
+
+ /*
+* alow_flags describe the wiring of the chip select
+* (0 = non-inverted, 1 = inverted); this must be taken into
+* account when setting the GPIO's initial value
+* (see also mpc8xxx_spi_cs_control()); note that astate will
+* be 0 for active low and 1 for active high respectively
+*/
ret = gpio_direction_output(pinfo-gpios[i],
- pinfo-alow_flags[i]);
+ pinfo-alow_flags[i] ^ !astate);
if (ret) {
dev_err(dev, can't set output direction for gpio
#%d: %d\n, i, ret);
diff -ruN linux-2.6.32_orig//include/linux/of_spi.h
linux-2.6.32/include/linux/of_spi.h
--- linux-2.6.32_orig//include/linux/of_spi.h 2009-12-03 04:51:21.0
+0100
+++ linux-2.6.32/include/linux/of_spi.h 2009-12-11 17:10:21.681380685 +0100
@@ -15,4 +15,7 @@
extern void of_register_spi_devices(struct spi_master *master,
struct device_node *np);
+extern bool of_get_spi_cs_active_state(struct device_node *np,
+ int index);
+
#endif /* __LINUX_OF_SPI */
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: spi_mpc8xxx.c: chip select polarity problem
On Thu, Nov 26, 2009 at 20:17:35, Grant Likely wrote:
[...]
spi-cs-high is definitely not a complete solution, but it isn't
actively evil either. Plus it is documented and (presumably) in
active use. so support for it should not be dropped.
Regardless, there needs to be a library function for parsing all the
SPI child nodes and returning the active state for each GPIO chip
select. All the code for parsing the old spi-cs-high properties can
be contained in the same place as a new yet-to-be-defined bus node cs
polarity property. The rework to the driver itself is not ugly.
The following patch adds a function to get the active state of the chip select
of a SPI device by looking for the 'spi-cs-high' property in the associated
device
tree node.
This function is used by the spi_mpc8xxx driver to set a proper initial value
to the associated GPIOs.
Signed-off-by: Torsten Fleischer to-fleisc...@t-online.de
---
diff -ruN linux-2.6.32_orig//drivers/of/of_spi.c
linux-2.6.32/drivers/of/of_spi.c
--- linux-2.6.32_orig//drivers/of/of_spi.c 2009-12-03 04:51:21.0
+0100
+++ linux-2.6.32/drivers/of/of_spi.c2009-12-09 12:37:01.0 +0100
@@ -10,6 +10,49 @@
#include linux/device.h
#include linux/spi/spi.h
#include linux/of_spi.h
+#include linux/errno.h
+
+/**
+ * of_get_spi_cs_active_state - Gets the chip select's active state of a SPI
+ * child devices.
+ * @np:parent node of the SPI device nodes
+ * @index: index/address of the SPI device (refers to the 'reg' property)
+ * @cs_active: pointer to return the chip select's active state
+ * (true = active high; false = active low)
+ *
+ * Returns 0 on success; negative errno on failure
+ */
+int of_get_spi_cs_active_state(struct device_node *np, int index, bool
*cs_active)
+{
+ struct device_node *child;
+ const int *prop;
+ int len;
+ bool active = 0;
+
+ /* search for the matching SPI device */
+ for_each_child_of_node(np, child) {
+ prop = of_get_property(child, reg, len);
+ if (!prop || len sizeof(*prop)) {
+ /* property 'reg' not available (not an error) */
+ continue;
+ }
+
+ if ( *prop == index ) {
+ /* matching device found */
+ if (of_find_property(child, spi-cs-high, NULL))
+ active = 1;
+
+ if (cs_active)
+ *cs_active = active;
+
+ return 0;
+ }
+ }
+
+ return -ENODEV;
+}
+EXPORT_SYMBOL(of_get_spi_cs_active_state);
+
/**
* of_register_spi_devices - Register child devices onto the SPI bus
diff -ruN linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c
linux-2.6.32/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c2009-12-03
04:51:21.0 +0100
+++ linux-2.6.32/drivers/spi/spi_mpc8xxx.c 2009-12-09 12:50:36.0
+0100
@@ -705,6 +705,7 @@
for (; i ngpios; i++) {
int gpio;
enum of_gpio_flags flags;
+ bool astate;
gpio = of_get_gpio_flags(np, i, flags);
if (!gpio_is_valid(gpio)) {
@@ -721,8 +722,15 @@
pinfo-gpios[i] = gpio;
pinfo-alow_flags[i] = flags OF_GPIO_ACTIVE_LOW;
+ ret = of_get_spi_cs_active_state(np, i, astate);
+ if (ret) {
+ dev_err(dev, can't get cs active state of device
+ #%d: %d\n, i, ret);
+ goto err_loop;
+ }
+
ret = gpio_direction_output(pinfo-gpios[i],
- pinfo-alow_flags[i]);
+ pinfo-alow_flags[i] ^ !astate);
if (ret) {
dev_err(dev, can't set output direction for gpio
#%d: %d\n, i, ret);
diff -ruN linux-2.6.32_orig//include/linux/of_spi.h
linux-2.6.32/include/linux/of_spi.h
--- linux-2.6.32_orig//include/linux/of_spi.h 2009-12-03 04:51:21.0
+0100
+++ linux-2.6.32/include/linux/of_spi.h 2009-12-09 12:36:48.0 +0100
@@ -15,4 +15,7 @@
extern void of_register_spi_devices(struct spi_master *master,
struct device_node *np);
+extern int of_get_spi_cs_active_state(struct device_node *np,
+ int index, bool *cs_active);
+
#endif /* __LINUX_OF_SPI */
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: spi_mpc8xxx.c: chip select polarity problem
On Wed, Dec 9, 2009 at 18:46:51 Grant Likely wrote:
[...]
+ ret = of_get_spi_cs_active_state(np, i, astate);
+ if (ret) {
+ dev_err(dev, can't get cs active state of device
+ #%d: %d\n, i, ret);
+ goto err_loop;
+ }
This is a bit heavy handed in that it expects the device tree to be
fully populated with all SPI devices which isn't always a given. For
example a board that has some unpopulated SPI devices could have some
gaps in the GPIO CS layout. If a node can't be found, then just
ignore it silently and move on to the next. I'd do something like
this:
+ astate = of_get_spi_cs_active_state(np, i);
What should be returned if the node can't be found, 'true' or 'false?
Maybe its better to do the following:
+ ret = of_get_spi_cs_active_state(np, i, astate);
+ if (ret) {
+ /* Device node not found */
+ continue;
+ }
ret = gpio_direction_output(pinfo-gpios[i],
- pinfo-alow_flags[i]);
+ pinfo-alow_flags[i] ^ !astate);
BTW, why the xor? The usage is non-obvious enough that I'd like to
see a comment describing the use case.
If I understand it right, the alow_flags describe the wiring. If set to 0 the
wiring is non-inverted, if set to 1 its inverted respectively. To take this
into account the active state has to be xor'd with the appropriate alow_flag.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: spi_mpc8xxx.c: chip select polarity problem
On Thu, Nov 26, 2009 at 13:12:04 Anton Vorontsov wrote: [...] Ah. I understand what you're doing now. Hmmm. This approach concerns me because it relies on firmware or platform code to get CS gpios set up properly before the driver is probed. Yes, that was said at the very beginning of this thread. Firmware doesn't always get it right, and I prefer to avoid platform specific setups as much as possible. Why can't the CS polarity be encoded into the device tree so the driver *does* have the polarity data at probe time? We have the spi-cs-high property, but it duplicates compatible property. 'compatible' is enough to tell whether some device has cs-high or cs-low (device's driver knows that already). The problem is that SPI bus master doesn't know all the devices, so it can't extract that information from the compatible string. To workaround that we can use 'spi-cs-high', but that's ugly workaround. SPI modes (0,1,2,3) is another matter, some devices can work in several modes, so 'spi-cpol' and 'spi-cpha' are actually useful. To get a sane initial state the needed GPIOs can be set to be inputs during the driver's initialization. This requires pull-up/pull-down resistors connected to the chip select lines. I think we can assume that they exist, because the GPIOs are all inputs after the controller's hardware reset and the resistors are needed to have a well-defined voltage level on the chip select lines. Normally the level is set so that the devices are disabled. Therefore, it doesn't matter if the firmware sets the GPIOs wrong. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: spi_mpc8xxx.c: chip select polarity problem
On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
Thanks. However, there needs to be a proper description of what this
patch does to go in the commit header. Can you please write one?
Thanks,
g.
[...]
The initialization of the chip selects is removed from the probe() function of
the spi_mpc8xxx driver, because the driver doesn't know the polarity of the
chip selects of the SPI devices at the time of its initialization.
For this reason the initialization of the several chip selects is postponed
to the point of time when the very first SPI transfer to the associated device
occurs.
Signed-off-by: Torsten Fleischer to-fleisc...@t-online.de
---
diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c
linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c 2009-11-10
01:32:31.0 +0100
+++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c2009-11-19 08:15:33.0
+0100
@@ -114,6 +114,7 @@
u32 rx_shift; /* RX data reg shift when in qe mode */
u32 tx_shift; /* TX data reg shift when in qe mode */
u32 hw_mode;/* Holds HW mode register settings */
+ int initialized;
};
static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
@@ -503,15 +504,52 @@
return ret;
}
+
+
+struct mpc8xxx_spi_probe_info {
+ struct fsl_spi_platform_data pdata;
+ int *gpios;
+ bool *alow_flags;
+};
+
+static struct mpc8xxx_spi_probe_info *
+to_of_pinfo(struct fsl_spi_platform_data *pdata)
+{
+ return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
+}
+
+static int mpc8xxx_spi_cs_init(struct spi_device *spi)
+{
+ struct device *dev = spi-dev.parent;
+ struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev-platform_data);
+ u16 cs = spi-chip_select;
+ int gpio = pinfo-gpios[cs];
+ bool on = pinfo-alow_flags[cs] ^ !(spi-mode SPI_CS_HIGH);
+
+ return gpio_direction_output(gpio, on);
+}
+
static int mpc8xxx_spi_transfer(struct spi_device *spi,
struct spi_message *m)
{
struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi-master);
+ struct spi_mpc8xxx_cs *cs = spi-controller_state;
unsigned long flags;
m-actual_length = 0;
m-status = -EINPROGRESS;
+ if (cs !cs-initialized) {
+ int ret;
+
+ ret = mpc8xxx_spi_cs_init(spi);
+ if (ret) {
+ dev_dbg(spi-dev, cs_init failed: %d\n, ret);
+ return ret;
+ }
+ cs-initialized = 1;
+ }
+
spin_lock_irqsave(mpc8xxx_spi-lock, flags);
list_add_tail(m-queue, mpc8xxx_spi-queue);
queue_work(mpc8xxx_spi-workqueue, mpc8xxx_spi-work);
@@ -648,18 +686,6 @@
return 0;
}
-struct mpc8xxx_spi_probe_info {
- struct fsl_spi_platform_data pdata;
- int *gpios;
- bool *alow_flags;
-};
-
-static struct mpc8xxx_spi_probe_info *
-to_of_pinfo(struct fsl_spi_platform_data *pdata)
-{
- return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
-}
-
static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
{
struct device *dev = spi-dev.parent;
@@ -720,14 +746,6 @@
pinfo-gpios[i] = gpio;
pinfo-alow_flags[i] = flags OF_GPIO_ACTIVE_LOW;
-
- ret = gpio_direction_output(pinfo-gpios[i],
- pinfo-alow_flags[i]);
- if (ret) {
- dev_err(dev, can't set output direction for gpio
- #%d: %d\n, i, ret);
- goto err_loop;
- }
}
pdata-max_chipselect = ngpios;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: spi_mpc8xxx.c: chip select polarity problem
On Sat, Nov 21, 2009 at 09:45:50 Grant Likely wrote:
[...]
Hey Torsten, do you have an updated version of this change to address
the comments? I'm collecting the last few things for some linux-next
exposure now.
Hey Grant,
here is the updated version of the patch containing the recommended
changes.
Best Regards
Torsten
Signed-off-by: Torsten Fleischer to-fleisc...@t-online.de
---
diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c
linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c 2009-11-10
01:32:31.0 +0100
+++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c2009-11-19 08:15:33.0
+0100
@@ -114,6 +114,7 @@
u32 rx_shift; /* RX data reg shift when in qe mode */
u32 tx_shift; /* TX data reg shift when in qe mode */
u32 hw_mode;/* Holds HW mode register settings */
+ int initialized;
};
static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
@@ -503,15 +504,52 @@
return ret;
}
+
+
+struct mpc8xxx_spi_probe_info {
+ struct fsl_spi_platform_data pdata;
+ int *gpios;
+ bool *alow_flags;
+};
+
+static struct mpc8xxx_spi_probe_info *
+to_of_pinfo(struct fsl_spi_platform_data *pdata)
+{
+ return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
+}
+
+static int mpc8xxx_spi_cs_init(struct spi_device *spi)
+{
+ struct device *dev = spi-dev.parent;
+ struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev-platform_data);
+ u16 cs = spi-chip_select;
+ int gpio = pinfo-gpios[cs];
+ bool on = pinfo-alow_flags[cs] ^ !(spi-mode SPI_CS_HIGH);
+
+ return gpio_direction_output(gpio, on);
+}
+
static int mpc8xxx_spi_transfer(struct spi_device *spi,
struct spi_message *m)
{
struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi-master);
+ struct spi_mpc8xxx_cs *cs = spi-controller_state;
unsigned long flags;
m-actual_length = 0;
m-status = -EINPROGRESS;
+ if (cs !cs-initialized) {
+ int ret;
+
+ ret = mpc8xxx_spi_cs_init(spi);
+ if (ret) {
+ dev_dbg(spi-dev, cs_init failed: %d\n, ret);
+ return ret;
+ }
+ cs-initialized = 1;
+ }
+
spin_lock_irqsave(mpc8xxx_spi-lock, flags);
list_add_tail(m-queue, mpc8xxx_spi-queue);
queue_work(mpc8xxx_spi-workqueue, mpc8xxx_spi-work);
@@ -648,18 +686,6 @@
return 0;
}
-struct mpc8xxx_spi_probe_info {
- struct fsl_spi_platform_data pdata;
- int *gpios;
- bool *alow_flags;
-};
-
-static struct mpc8xxx_spi_probe_info *
-to_of_pinfo(struct fsl_spi_platform_data *pdata)
-{
- return container_of(pdata, struct mpc8xxx_spi_probe_info, pdata);
-}
-
static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
{
struct device *dev = spi-dev.parent;
@@ -720,14 +746,6 @@
pinfo-gpios[i] = gpio;
pinfo-alow_flags[i] = flags OF_GPIO_ACTIVE_LOW;
-
- ret = gpio_direction_output(pinfo-gpios[i],
- pinfo-alow_flags[i]);
- if (ret) {
- dev_err(dev, can't set output direction for gpio
- #%d: %d\n, i, ret);
- goto err_loop;
- }
}
pdata-max_chipselect = ngpios;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: spi_mpc8xxx.c: chip select polarity problem
On Wen, Nov 18, 2009 00:28:23 Anton Vorontsov wrote:
[...]
So it might be better to fix up initial value in the platform code?
Oh, we actually cannot, because the driver calls
gpio_direction_output().
And since we don't know the mode prior to SPI device's driver
probe() finished, we'll have to set up an initial state in the
first SPI transfer. I.e. something like this:
In most cases the device drivers perform SPI transfers already in their
probe() function. How can it be ensured that the CS of all other
devices are inactive even if they are not initialized at that time?
Good question. Oh, well... then we have to use spi-cs-high,
no matter that it is a duplication of the 'compatible' property.
SPI bus drivers don't know all the devices and their CS level,
and so spi-cs-high is the only way to tell that information. :-(
Oh. On the other hand, we can postpone the gpio_direction_output()
call, and still require that the platform code (or firmware)
should be responsible for setting a sane default values on the
chip selects.
How about that?
diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c
linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c 2009-11-10
01:32:31.0 +0100
+++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c2009-11-18 10:47:06.0
+0100
@@ -114,6 +114,7 @@
u32 rx_shift; /* RX data reg shift when in qe mode */
u32 tx_shift; /* TX data reg shift when in qe mode */
u32 hw_mode;/* Holds HW mode register settings */
+ int initialized;
};
static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)
@@ -437,6 +438,7 @@
cs = kzalloc(sizeof *cs, GFP_KERNEL);
if (!cs)
return -ENOMEM;
+ cs-initialized = 0;
spi-controller_state = cs;
}
mpc8xxx_spi = spi_master_get_devdata(spi-master);
@@ -503,15 +505,25 @@
return ret;
}
+
+static void mpc8xxx_spi_cs_init(struct spi_device *spi);
+
+
static int mpc8xxx_spi_transfer(struct spi_device *spi,
struct spi_message *m)
{
struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi-master);
+ struct spi_mpc8xxx_cs *cs = spi-controller_state;
unsigned long flags;
m-actual_length = 0;
m-status = -EINPROGRESS;
+ if (cs !cs-initialized) {
+ mpc8xxx_spi_cs_init(spi);
+ cs-initialized = 1;
+ }
+
spin_lock_irqsave(mpc8xxx_spi-lock, flags);
list_add_tail(m-queue, mpc8xxx_spi-queue);
queue_work(mpc8xxx_spi-workqueue, mpc8xxx_spi-work);
@@ -671,6 +683,17 @@
gpio_set_value(gpio, on ^ alow);
}
+static void mpc8xxx_spi_cs_init(struct spi_device *spi)
+{
+ struct device *dev = spi-dev.parent;
+ struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev-platform_data);
+ u16 cs = spi-chip_select;
+ int gpio = pinfo-gpios[cs];
+ bool on = (pinfo-alow_flags[cs] ^ !(spi-mode SPI_CS_HIGH));
+
+ gpio_direction_output(gpio, on);
+}
+
static int of_mpc8xxx_spi_get_chipselects(struct device *dev)
{
struct device_node *np = dev_archdata_get_node(dev-archdata);
@@ -720,14 +743,6 @@
pinfo-gpios[i] = gpio;
pinfo-alow_flags[i] = flags OF_GPIO_ACTIVE_LOW;
-
- ret = gpio_direction_output(pinfo-gpios[i],
- pinfo-alow_flags[i]);
- if (ret) {
- dev_err(dev, can't set output direction for gpio
- #%d: %d\n, i, ret);
- goto err_loop;
- }
}
pdata-max_chipselect = ngpios;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: spi_mpc8xxx.c: chip select polarity problem
On Mon, Nov 16, 2009 at 19:00PM, Anton Vorontsov wrote: [...] So it might be better to fix up initial value in the platform code? Oh, we actually cannot, because the driver calls gpio_direction_output(). And since we don't know the mode prior to SPI device's driver probe() finished, we'll have to set up an initial state in the first SPI transfer. I.e. something like this: In most cases the device drivers perform SPI transfers already in their probe() function. How can it be ensured that the CS of all other devices are inactive even if they are not initialized at that time? Regards Torsten Fleischer ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
spi_mpc8xxx.c: chip select polarity problem
Hi all, I have 3 devices connected to the SPI bus of the MPC8313. For the Chip Select (CS) signals 3 GPIOs of the controller are used. But the driver uses the inverse polarity of the CS either during the initialization or at the transfer - depending on the setup of the flattened device tree. Here is what I discovered: The driver uses a polarity flag for each CS signal (the alow_flags array). These flags are set according to the 'gpios' property of the SPI node of the flattened device tree. Is it correct that alow_flags[x] = 1 means CSx is active low? During the initialization the driver sets the CS to the value of alow_flags[x]. I.e. CSx is High if alow_flags[x] = 1 and otherwise Low. The flags are used in the function mpc8xxx_spi_cs_control() to take care about the polarity when setting the appropriate GPIO pin. But the function mpc8xxx_spi_chipselect() that calls the mpc8xxx_spi_cs_control() takes also care about the polarity of the CS (bool pol = spi-mode SPI_CS_HIGH). Lets assume alow_flags[x] = 1 and the property 'spi-cs-high' is not set for the SPI device. During initialization the driver sets the chip select signal 'x' to High (see of_mpc8xxx_spi_get_chipselects()). This is OK if the chip select is active low, because this disables the device on start-up. But during the transfer the chip select signal is High and after the transfer is completed the signal is set to Low. This is not the intended behavior for an active low chip select. I also tried to set alow_flags[x] = 0 for active low. In this case the transfer works, but the initial value for the CS is wrong (Low instead of High). The problem seems to be that the polarity is taken into account twice (as described above). So what would be the better solution: removing the usage of the alow_flags in mpc8xxx_spi_cs_control() or the variable 'pol' in mpc8xxx_spi_chipselect()? Best Regards Torsten Fleischer ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: gianfar.c: Unwanted VLAN tagging on TX frames
On Tuesday 25 August 2009 at 01:32:18, Andy Fleming wrote: Hmmmhow have you tested this? This looks like it has a bad race condition. The TCTRL register applies to all packets, which means if you send a packet with VLAN tags, followed by one without, or visa versa, there's a reasonable chance that the second packet's VLAN tags (or lack thereof) will take precedence. Without speaking for the company, I suspect that this is just how the eTSEC works with VLAN -- all, or nothing. Andy Hi Andy, I have tested it by sending a single ping to a station within the VLAN followed by a ping to a station thats not in a VLAN. OK, thats not really a significant test, because I did not send a VLAN tagged frame immediately followed by one without a tag. You are right, this code can enable/disable VLAN tagging before the previous packet is processed. Torsten ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
gianfar.c: Unwanted VLAN tagging on TX frames
Hello everyone,
I have the Freescale's MPC8313erdb eval board and run the latest stable linux
kernel version (linux-2.6.30.5).
After creating a VLAN device (e.g. eth0.2) a VLAN tag is also inserted into
frames that don't relate to a VLAN device. This is the case for frames that
are directly sent through a standard ethernet interface (e.g. eth0).
When creating a VLAN device the gianfar driver enables the hardware supported
VLAN tagging on TX frames. This is done by setting the VLINS bit of the TCTRL
register inside the function gianfar_vlan_rx_register().
The problem is that every outgoing frame will be tagged. For frames from an
interface like eth0 the VLN bit of the FCB isn't set. Therefore the eTSEC uses
the content of the Default VLAN control word register (DFVLAN) to tag the
frame. As long as this register will not be modified after a reset of the
controller the outgoing frames will be tagged with VID = 0 (priority tagged
frames).
The following patch solves this problem.
diff -uprN linux-2.6.30.5_orig//drivers/net/gianfar.c
linux-2.6.30.5/drivers/net/gianfar.c
--- linux-2.6.30.5_orig//drivers/net/gianfar.c 2009-08-16 23:19:38.0
+0200
+++ linux-2.6.30.5/drivers/net/gianfar.c2009-08-22 10:38:28.0
+0200
@@ -1309,6 +1309,7 @@ static int gfar_start_xmit(struct sk_buf
u32 bufaddr;
unsigned long flags;
unsigned int nr_frags, length;
+ u32 tempval;
base = priv-tx_bd_base;
@@ -1385,13 +1386,30 @@ static int gfar_start_xmit(struct sk_buf
gfar_tx_checksum(skb, fcb);
}
- if (priv-vlgrp vlan_tx_tag_present(skb)) {
- if (unlikely(NULL == fcb)) {
- fcb = gfar_add_fcb(skb);
- lstatus |= BD_LFLAG(TXBD_TOE);
- }
+ if (priv-vlgrp) {
+ if (vlan_tx_tag_present(skb)) {
+ if (unlikely(NULL == fcb)) {
+ fcb = gfar_add_fcb(skb);
+ lstatus |= BD_LFLAG(TXBD_TOE);
+ }
+
+ /* Enable VLAN tag insertion for frames from VLAN
devices */
+ tempval = gfar_read(priv-regs-tctrl);
+ if ( !(tempval TCTRL_VLINS) ) {
+ tempval |= TCTRL_VLINS;
+ gfar_write(priv-regs-tctrl, tempval);
+ }
- gfar_tx_vlan(skb, fcb);
+ gfar_tx_vlan(skb, fcb);
+ }
+ else {
+ /* Disable VLAN tag insertion for frames that are not
from a VLAN device */
+ tempval = gfar_read(priv-regs-tctrl);
+ if ( tempval TCTRL_VLINS ) {
+ tempval = ~TCTRL_VLINS;
+ gfar_write(priv-regs-tctrl, tempval);
+ }
+ }
}
/* setup the TxBD length and buffer pointer for the first BD */
@@ -1484,23 +1502,11 @@ static void gfar_vlan_rx_register(struct
priv-vlgrp = grp;
if (grp) {
- /* Enable VLAN tag insertion */
- tempval = gfar_read(priv-regs-tctrl);
- tempval |= TCTRL_VLINS;
-
- gfar_write(priv-regs-tctrl, tempval);
-
/* Enable VLAN tag extraction */
tempval = gfar_read(priv-regs-rctrl);
- tempval |= RCTRL_VLEX;
tempval |= (RCTRL_VLEX | RCTRL_PRSDEP_INIT);
gfar_write(priv-regs-rctrl, tempval);
} else {
- /* Disable VLAN tag insertion */
- tempval = gfar_read(priv-regs-tctrl);
- tempval = ~TCTRL_VLINS;
- gfar_write(priv-regs-tctrl, tempval);
-
/* Disable VLAN tag extraction */
tempval = gfar_read(priv-regs-rctrl);
tempval = ~RCTRL_VLEX;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
