Another OCP enet patch

Another OCP enet patch

[David Gibson]

Armin, please consider the patch below.  It removes your recently
added ocp-dma.h and instead makes the ocp enet driver uses the DMA
direction constants from pci.h.

I realise that logically the OCP enet driver, and the
consistent_sync() has nothing to do with PCI.  However using the pci.h
constants seems a better approach than defining new constants with the
same values, when the switch in consistent_sync() explicitly checks
against the PCI constants.

In the longer term consistent_sync() itself should be changed not to
reference the PCI constants - in fact the PCI constants should
probably be moved and renamed since they have no inherent connection
with PCI at all.

Oh, I also change the consistent_sync() in the Tx routine to be
PCI_DMA_TODEVICE rather than BIDIRECTIONAL, since there is no need to
invalidate the cache here, a writeback is all that's necessary.

diff -urN /home/dgibson/kernel/linuxppc_2_4_devel/drivers/net/ibm_ocp/ibm_ocp_enet.c linux-grinch/drivers/net/ibm_ocp/ibm_ocp_enet.c
--- /home/dgibson/kernel/linuxppc_2_4_devel/drivers/net/ibm_ocp/ibm_ocp_enet.c	Fri May 24 11:19:23 2002
+++ linux-grinch/drivers/net/ibm_ocp/ibm_ocp_enet.c	Mon May 27 13:57:27 2002
@@ -165,6 +165,7 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/types.h>
+#include <linux/pci.h>

 #include <asm/processor.h>	/* Processor type for cache alignment. */
 #include <asm/bitops.h>
@@ -172,7 +173,6 @@
 #include <asm/dma.h>
 #include <asm/irq.h>
 #include <asm/ocp.h>
-#include <asm/ocp-dma.h>

 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
@@ -476,7 +476,7 @@
 	 * interrupt.
 	 */
 	fep->tx_skb[fep->tx_slot] = skb;
-	consistent_sync((void *) skb->data, skb->len, DMA_BIDIRECTIONAL);
+	consistent_sync((void *) skb->data, skb->len, PCI_DMA_TODEVICE);

 	ctrl = EMAC_TX_CTRL_DFLT;
 	if ((NUM_TX_BUFF - 1) == fep->tx_slot)
@@ -974,8 +974,8 @@
 				skb_reserve(fep->rx_skb[i], skb_res);

 			consistent_sync((void *) fep->rx_skb[i]->
-					    data, DESC_RX_BUF_SIZE,
-					    DMA_BIDIRECTIONAL);
+					data, DESC_RX_BUF_SIZE,
+					PCI_DMA_BIDIRECTIONAL);
 			ptr = (char *) virt_to_phys(fep->rx_skb[i]->data);
 		}
 		fep->rx_desc[i].ctrl = MAL_RX_CTRL_EMPTY | MAL_RX_CTRL_INTR |	/*could be smarter about this to avoid ints at high loads */
diff -urN /home/dgibson/kernel/linuxppc_2_4_devel/include/asm-ppc/ocp-dma.h linux-grinch/include/asm-ppc/ocp-dma.h
--- /home/dgibson/kernel/linuxppc_2_4_devel/include/asm-ppc/ocp-dma.h	Tue May 21 10:27:14 2002
+++ linux-grinch/include/asm-ppc/ocp-dma.h	Thu Jan 01 10:00:00 1970
@@ -1,49 +0,0 @@
-/*
- * ocp-dma.h
- *
- *
- * 	Current Maintainer
- *      Armin Kuster akuster@pacbell.net
- *      May, 2002
- *
- *
- *
- * 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.
- *
- *  THIS  SOFTWARE  IS PROVIDED   ``AS  IS'' AND   ANY  EXPRESS OR   IMPLIED
- *  WARRANTIES,   INCLUDING, BUT NOT  LIMITED  TO, THE IMPLIED WARRANTIES OF
- *  MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN
- *  NO  EVENT  SHALL   THE AUTHOR  BE    LIABLE FOR ANY   DIRECT,  INDIRECT,
- *  INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
- *  NOT LIMITED   TO, PROCUREMENT OF  SUBSTITUTE GOODS  OR SERVICES; LOSS OF
- *  USE, DATA,  OR PROFITS; OR  BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
- *  ANY THEORY OF LIABILITY, WHETHER IN  CONTRACT, STRICT LIABILITY, OR TORT
- *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
- *  THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
- *  You should have received a copy of the  GNU General Public License along
- *  with this program; if not, write  to the Free Software Foundation, Inc.,
- *  675 Mass Ave, Cambridge, MA 02139, USA.
- *
- *	Version 1.0 (05/16/02) - A. Kuster
- *	Initial version	 -
- *
- */
-
-#ifdef __KERNEL__
-#ifndef __OCP_DMA_H__
-#define __OCP_DMA_H__
-
-
-/* This defines the direction arg to the DMA mapping routines. */
-#define DMA_BIDIRECTIONAL	0
-#define DMA_TODEVICE		1
-#define DMA_FROMDEVICE		2
-#define DMA_NONE		3
-
-#endif				/* __OCP_DMA_H__ */
-#endif				/* __KERNEL__ */
-

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Armin Kuster]

David Gibson wrote:
> Armin, please consider the patch below.  It removes your recently
> added ocp-dma.h and instead makes the ocp enet driver uses the DMA
> direction constants from pci.h.

Go ahead and push it.  FYI - I will update the ocp ide driver since I am
making changes in it any ways.

>
> I realise that logically the OCP enet driver, and the
> consistent_sync() has nothing to do with PCI.  However using the pci.h
> constants seems a better approach than defining new constants with the
> same values, when the switch in consistent_sync() explicitly checks
> against the PCI constants.
>
> In the longer term consistent_sync() itself should be changed not to
> reference the PCI constants - in fact the PCI constants should
> probably be moved and renamed since they have no inherent connection
> with PCI at all.

I have already had to make that break with the ocp usb driver.  If a 4xx
core shows up with PCI and ocp ide or ocp USB then we will need to
address this issue.  thus those dma APIs I sent you are a result of my
concern.
>
> Oh, I also change the consistent_sync() in the Tx routine to be
> PCI_DMA_TODEVICE rather than BIDIRECTIONAL, since there is no need to
> invalidate the cache here, a writeback is all that's necessary.
>
>


Thanks

armin


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Tom Rini]

On Mon, May 27, 2002 at 02:03:30PM +1000, David Gibson wrote:

> I realise that logically the OCP enet driver, and the
> consistent_sync() has nothing to do with PCI.  However using the pci.h
> constants seems a better approach than defining new constants with the
> same values, when the switch in consistent_sync() explicitly checks
> against the PCI constants.
>
> In the longer term consistent_sync() itself should be changed not to
> reference the PCI constants - in fact the PCI constants should
> probably be moved and renamed since they have no inherent connection
> with PCI at all.

At this point I think I should give my 2 cents, so...

I think we should keep ocp-dma.h around for now (2.4.x timeframe) and
change consistent_sync() to not check for the PCI versions (which I
believe happened just because it's a cut&paste&fix of the ARM code) and
for 2.5 hope that a more general fix comes out..

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[David Gibson]

On Mon, May 27, 2002 at 09:23:23AM -0700, Tom Rini wrote:
>
> On Mon, May 27, 2002 at 02:03:30PM +1000, David Gibson wrote:
>
> > I realise that logically the OCP enet driver, and the
> > consistent_sync() has nothing to do with PCI.  However using the pci.h
> > constants seems a better approach than defining new constants with the
> > same values, when the switch in consistent_sync() explicitly checks
> > against the PCI constants.
> >
> > In the longer term consistent_sync() itself should be changed not to
> > reference the PCI constants - in fact the PCI constants should
> > probably be moved and renamed since they have no inherent connection
> > with PCI at all.
>
> At this point I think I should give my 2 cents, so...
>
> I think we should keep ocp-dma.h around for now (2.4.x timeframe) and
> change consistent_sync() to not check for the PCI versions (which I
> believe happened just because it's a cut&paste&fix of the ARM code) and
> for 2.5 hope that a more general fix comes out..

I disagree.  Using the constants from pci.h is the least-wrong simple
fix for now - yes it's conceptually wrong, but they're just arbitrary
constants, it doesn't do any real damage.

Changing consistent_sync() to use its own constants isn't completely
trivial, because asm/pci.h calls consistent_sync() with the constants
passed as arguments to pci_map_single() et al, assuming they have the
correct values.  So, to decouple the consistent_sync() constants from
the PCI direction constants we would have to put a switch in every
time consistent_sync() is used in asm/pci.h.  The PCI constants could
be defined in terms of the non-PCI constants, but that means changing
generic code (linux/pci.h).

If/when we do add non-PCI constants for consistent_sync() they
shouldn't go in ocp-dma.h, since they have no more to do with OCP than
they do with PCI.  In io.h along with the prototype for
consistent_sync() would be a better idea.

For 2.5, I think the proper fix is to remove the PCI_DMA_* constants
entirely and replace them with some arbitrary constants to define DMA
direction.  They can then be used for both PCI and OCP (and sbus,
which also uses the PCI constants despite not being PCI).  But that
means convincing Linus, of course.

PS.  In reinstating ocp-dma.h you seem to have deleted ocp.h.  I'm
guessing that's not what you meant to do.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Tom Rini]

On Tue, May 28, 2002 at 10:57:28AM +1000, David Gibson wrote:
> On Mon, May 27, 2002 at 09:23:23AM -0700, Tom Rini wrote:
> >
> > On Mon, May 27, 2002 at 02:03:30PM +1000, David Gibson wrote:
> >
> > > I realise that logically the OCP enet driver, and the
> > > consistent_sync() has nothing to do with PCI.  However using the pci.h
> > > constants seems a better approach than defining new constants with the
> > > same values, when the switch in consistent_sync() explicitly checks
> > > against the PCI constants.
> > >
> > > In the longer term consistent_sync() itself should be changed not to
> > > reference the PCI constants - in fact the PCI constants should
> > > probably be moved and renamed since they have no inherent connection
> > > with PCI at all.
> >
> > At this point I think I should give my 2 cents, so...
> >
> > I think we should keep ocp-dma.h around for now (2.4.x timeframe) and
> > change consistent_sync() to not check for the PCI versions (which I
> > believe happened just because it's a cut&paste&fix of the ARM code) and
> > for 2.5 hope that a more general fix comes out..
>
> I disagree.  Using the constants from pci.h is the least-wrong simple
> fix for now - yes it's conceptually wrong, but they're just arbitrary
> constants, it doesn't do any real damage.

Well, for the moment I think they're less-arbitrary constants than they
should be, but yes..

> Changing consistent_sync() to use its own constants isn't completely
> trivial, because asm/pci.h calls consistent_sync() with the constants
> passed as arguments to pci_map_single() et al, assuming they have the
> correct values.  So, to decouple the consistent_sync() constants from
> the PCI direction constants we would have to put a switch in every
> time consistent_sync() is used in asm/pci.h.  The PCI constants could
> be defined in terms of the non-PCI constants, but that means changing
> generic code (linux/pci.h).

Well, if DMA_* == PCI_DMA_*, we don't have to do anything.  We put a
comment saying something like:
/* The PCI_DMA_* constants have nothing to do with PCI.  Someone should
 * rename this in 2.5... */
And while someone could re-number the arbitrary constants, I'd think
something like that would be questioned before being accepted.

> If/when we do add non-PCI constants for consistent_sync() they
> shouldn't go in ocp-dma.h, since they have no more to do with OCP than
> they do with PCI.  In io.h along with the prototype for
> consistent_sync() would be a better idea.

I agree with them not really belonging in ocp-dma.h either, but it's
either a PCI device or an 'Oh Chip Peripheral' of some sort.

> For 2.5, I think the proper fix is to remove the PCI_DMA_* constants
> entirely and replace them with some arbitrary constants to define DMA
> direction.  They can then be used for both PCI and OCP (and sbus,
> which also uses the PCI constants despite not being PCI).  But that
> means convincing Linus, of course.

2.5 still seems to be in break everything mode, so why not send Linus a
patch which creates <linux/dma.h>, for example, and then do a quick
search/replace of PCI_DMA_* -> DMA_* and keep the numbers the same even.

> PS.  In reinstating ocp-dma.h you seem to have deleted ocp.h.  I'm
> guessing that's not what you meant to do.

I really should email bitmover and see if this is a known bug (cset -x
not re-renaming files).

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[David Gibson]

On Mon, May 27, 2002 at 06:25:16PM -0700, Tom Rini wrote:
> On Tue, May 28, 2002 at 10:57:28AM +1000, David Gibson wrote:
> > On Mon, May 27, 2002 at 09:23:23AM -0700, Tom Rini wrote:
> > > On Mon, May 27, 2002 at 02:03:30PM +1000, David Gibson wrote:
> > >
> > > > I realise that logically the OCP enet driver, and the
> > > > consistent_sync() has nothing to do with PCI.  However using the pci.h
> > > > constants seems a better approach than defining new constants with the
> > > > same values, when the switch in consistent_sync() explicitly checks
> > > > against the PCI constants.
> > > >
> > > > In the longer term consistent_sync() itself should be changed not to
> > > > reference the PCI constants - in fact the PCI constants should
> > > > probably be moved and renamed since they have no inherent connection
> > > > with PCI at all.
> > >
> > > At this point I think I should give my 2 cents, so...
> > >
> > > I think we should keep ocp-dma.h around for now (2.4.x timeframe) and
> > > change consistent_sync() to not check for the PCI versions (which I
> > > believe happened just because it's a cut&paste&fix of the ARM code) and
> > > for 2.5 hope that a more general fix comes out..
> >
> > I disagree.  Using the constants from pci.h is the least-wrong simple
> > fix for now - yes it's conceptually wrong, but they're just arbitrary
> > constants, it doesn't do any real damage.
>
> Well, for the moment I think they're less-arbitrary constants than they
> should be, but yes..

Um, I don't see what you mean by "less arbitrary than they should be".

> > Changing consistent_sync() to use its own constants isn't completely
> > trivial, because asm/pci.h calls consistent_sync() with the constants
> > passed as arguments to pci_map_single() et al, assuming they have the
> > correct values.  So, to decouple the consistent_sync() constants from
> > the PCI direction constants we would have to put a switch in every
> > time consistent_sync() is used in asm/pci.h.  The PCI constants could
> > be defined in terms of the non-PCI constants, but that means changing
> > generic code (linux/pci.h).
>
> Well, if DMA_* == PCI_DMA_*, we don't have to do anything.  We put a
> comment saying something like:
> /* The PCI_DMA_* constants have nothing to do with PCI.  Someone should
>  * rename this in 2.5... */
> And while someone could re-number the arbitrary constants, I'd think
> something like that would be questioned before being accepted.

Well, yes, but calling the function with once constant then testing
against another within the function gives me the willies.  If the
constants are changed it will break horribly, and it will give
misleading results if people try to grep for one constant or the
other.

> > If/when we do add non-PCI constants for consistent_sync() they
> > shouldn't go in ocp-dma.h, since they have no more to do with OCP than
> > they do with PCI.  In io.h along with the prototype for
> > consistent_sync() would be a better idea.
>
> I agree with them not really belonging in ocp-dma.h either, but it's
> either a PCI device or an 'Oh Chip Peripheral' of some sort.

Well, for PPC at the moment that's true, but they could be used for
anything.  Constants like this could just as well (but aren't) used
for Sparc SBUS. The constants belong to consistent_sync(), so there is
an obvious place to define them - with its prototype.

> > For 2.5, I think the proper fix is to remove the PCI_DMA_* constants
> > entirely and replace them with some arbitrary constants to define DMA
> > direction.  They can then be used for both PCI and OCP (and sbus,
> > which also uses the PCI constants despite not being PCI).  But that
> > means convincing Linus, of course.
>
> 2.5 still seems to be in break everything mode, so why not send Linus a
> patch which creates <linux/dma.h>, for example, and then do a quick
> search/replace of PCI_DMA_* -> DMA_* and keep the numbers the same even.

Already planning to, sometime soon.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[David Gibson]

On Tue, May 28, 2002 at 12:02:31AM -0700, Armin wrote:
> >I agree with them not really belonging in ocp-dma.h either, but it's
> >either a PCI device or an 'Oh Chip Peripheral' of some sort.
> >
> >
> >>For 2.5, I think the proper fix is to remove the PCI_DMA_* constants
> >>entirely and replace them with some arbitrary constants to define DMA
> >>direction.  They can then be used for both PCI and OCP (and sbus,
> >>which also uses the PCI constants despite not being PCI).  But that
> >>means convincing Linus, of course.
> >>
> >
> >2.5 still seems to be in break everything mode, so why not send Linus a
> >patch which creates <linux/dma.h>, for example, and then do a quick
> >search/replace of PCI_DMA_* -> DMA_* and keep the numbers the same even.
>
> Well what I did for the USB ocp drive was to call dma_pool_*,
> dma_*_consistent etc... It would be fairly easy to leave the pci_* in
> place , remove as much common code and place them in to generic dma
> fuctions.  I would leave the need or use of the struct pci_dev in the
> pci_* fuctions to do their business and buffer the new dma API's.  This
> will keep us from changing _all_ pci drivers, give a central location
> for common code and give those arch/drivers who want to use dma
> functions and a way to break away from either having to enable "pci" or
> include pci headers to make things work. the ocp drivers could use these
> generic dma api directly or all use some sort of wrapper.

Um, I think you're overestimating how generic this dma_pool API is or
can be.  In general a peripheral might be hanging of one or several
buses from the CPU with IOMMU(s) or TCE(s) or equivalent between it
and main memory.  That's why pci_alloc_consistent() and friends
require the pci_dev structure and why they don't have an interface as
simple as consistent_*().  This is exactly why dma_cache_wback_inv()
and so on are kinda deprecated - they're not well defined on a machine
whose buses have different address spaces.

The direction constants can be made generic trivially.  Generalising
more of the DMA infrastructure is much harder.

In any case we're talking about consistent_sync() here, not
consistent_alloc() which specifically *isn't* called on specially
allocated blocks of memory - the OCP enet driver calls it on random
SKBs.  In some ways, since we're PPC specific anyway, I think it would
make as much sense as anything just to directly call
dcache_flush_range() and so forth, rather than consistent_sync() or
dma_cache_*().

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Armin]

Tom Rini wrote:
> On Tue, May 28, 2002 at 10:57:28AM +1000, David Gibson wrote:
>
>>On Mon, May 27, 2002 at 09:23:23AM -0700, Tom Rini wrote:
>>
>>>On Mon, May 27, 2002 at 02:03:30PM +1000, David Gibson wrote:
>>>
>>>
>>>>I realise that logically the OCP enet driver, and the
>>>>consistent_sync() has nothing to do with PCI.  However using the pci.h
>>>>constants seems a better approach than defining new constants with the
>>>>same values, when the switch in consistent_sync() explicitly checks
>>>>against the PCI constants.
>>>>
>>>>In the longer term consistent_sync() itself should be changed not to
>>>>reference the PCI constants - in fact the PCI constants should
>>>>probably be moved and renamed since they have no inherent connection
>>>>with PCI at all.
>>>>
>>>At this point I think I should give my 2 cents, so...
>>>
>>>I think we should keep ocp-dma.h around for now (2.4.x timeframe) and
>>>change consistent_sync() to not check for the PCI versions (which I
>>>believe happened just because it's a cut&paste&fix of the ARM code) and
>>>for 2.5 hope that a more general fix comes out..
>>>
>>I disagree.  Using the constants from pci.h is the least-wrong simple
>>fix for now - yes it's conceptually wrong, but they're just arbitrary
>>constants, it doesn't do any real damage.
>>
>
> Well, for the moment I think they're less-arbitrary constants than they
> should be, but yes..
>
>
>>Changing consistent_sync() to use its own constants isn't completely
>>trivial, because asm/pci.h calls consistent_sync() with the constants
>>passed as arguments to pci_map_single() et al, assuming they have the
>>correct values.  So, to decouple the consistent_sync() constants from
>>the PCI direction constants we would have to put a switch in every
>>time consistent_sync() is used in asm/pci.h.  The PCI constants could
>>be defined in terms of the non-PCI constants, but that means changing
>>generic code (linux/pci.h).
>>
>
> Well, if DMA_* == PCI_DMA_*, we don't have to do anything.  We put a
> comment saying something like:
> /* The PCI_DMA_* constants have nothing to do with PCI.  Someone should
>  * rename this in 2.5... */
> And while someone could re-number the arbitrary constants, I'd think
> something like that would be questioned before being accepted.
>
>
>>If/when we do add non-PCI constants for consistent_sync() they
>>shouldn't go in ocp-dma.h, since they have no more to do with OCP than
>>they do with PCI.  In io.h along with the prototype for
>>consistent_sync() would be a better idea.

It was put there not to solve everyone's problems.  I wanted to start
off small then slowly move the changes outward.

>>
>
> I agree with them not really belonging in ocp-dma.h either, but it's
> either a PCI device or an 'Oh Chip Peripheral' of some sort.
>
>
>>For 2.5, I think the proper fix is to remove the PCI_DMA_* constants
>>entirely and replace them with some arbitrary constants to define DMA
>>direction.  They can then be used for both PCI and OCP (and sbus,
>>which also uses the PCI constants despite not being PCI).  But that
>>means convincing Linus, of course.
>>
>
> 2.5 still seems to be in break everything mode, so why not send Linus a
> patch which creates <linux/dma.h>, for example, and then do a quick
> search/replace of PCI_DMA_* -> DMA_* and keep the numbers the same even.
>

Well what I did for the USB ocp drive was to call dma_pool_*,
dma_*_consistent etc... It would be fairly easy to leave the pci_* in
place , remove as much common code and place them in to generic dma
fuctions.  I would leave the need or use of the struct pci_dev in the
pci_* fuctions to do their business and buffer the new dma API's.  This
will keep us from changing _all_ pci drivers, give a central location
for common code and give those arch/drivers who want to use dma
functions and a way to break away from either having to enable "pci" or
include pci headers to make things work. the ocp drivers could use these
generic dma api directly or all use some sort of wrapper.

Creating the patch to achive this should be to bad to do, it more on how
to struct the changes, cinfig options etc..:)

armin


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Dan Malek]

David Gibson wrote:



> Changing consistent_sync() to use its own constants isn't completely
> trivial, because asm/pci.h calls consistent_sync() with the constants
> passed as arguments to pci_map_single() et al, ....

The constent_sync() is not unique to 4xx and it absolutely must not
be required to use pci headers/functions for it to work properly.
These lower level functions are common to other platforms, other drivers
will call consistent_* functions directly, and in some cases the platforms
or drivers that use these functions will not require (nor compile correctly)
if PCI is enabled.  Unfortunately, making some of these drivers work without
PCI defined was the great challenge, so changing these constant the time I
put these functions into PowerPC was the least of the problem :-)


On a slightly different topic, are we getting a little carried away with
our "inline" functions?  What do we gain by having these complex functions
in asm/pci.h where clearly the overhead of a function call is insignficant
compared to the work done in the function?  I just noticed that other arches
have collected these functions into C files.


> .....  So, to decouple the consistent_sync() constants from
> the PCI direction constants we would have to put a switch in every
> time consistent_sync() is used in asm/pci.h.

Just define them so they have the same value as their PCI counterparts.

> ... In io.h along with the prototype for
> consistent_sync() would be a better idea.

Yes, thank you :-)


> ..... But that
> means convincing Linus, of course.

...and convincing the world there is something other than x86/PCI/consistent
systems that run Linux.

Thanks.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Dan Malek]

David Gibson wrote:

> ...  In some ways, since we're PPC specific anyway, I think it would
> make as much sense as anything just to directly call
> dcache_flush_range() and so forth, rather than consistent_sync() or
> dma_cache_*().

Yes, that's true.  These consistent_* functions were added when we
started using non-PCI drivers that are common across multiple platforms.
It seems none of the platforms had common names for data cache management
functions, so people started using the consistent_sync() in it's place.
It also made sense because they were using the other consistent_* functions
as well.  No one probably noticed, but at the same time we also changed
the cache management function names to be similar to other architectures
as well.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Tom Rini]

On Tue, May 28, 2002 at 04:36:38PM +1000, David Gibson wrote:
> On Mon, May 27, 2002 at 06:25:16PM -0700, Tom Rini wrote:
> > On Tue, May 28, 2002 at 10:57:28AM +1000, David Gibson wrote:
> > > On Mon, May 27, 2002 at 09:23:23AM -0700, Tom Rini wrote:
> > > > On Mon, May 27, 2002 at 02:03:30PM +1000, David Gibson wrote:
> > > >
> > > > > I realise that logically the OCP enet driver, and the
> > > > > consistent_sync() has nothing to do with PCI.  However using the pci.h
> > > > > constants seems a better approach than defining new constants with the
> > > > > same values, when the switch in consistent_sync() explicitly checks
> > > > > against the PCI constants.
> > > > >
> > > > > In the longer term consistent_sync() itself should be changed not to
> > > > > reference the PCI constants - in fact the PCI constants should
> > > > > probably be moved and renamed since they have no inherent connection
> > > > > with PCI at all.
> > > >
> > > > At this point I think I should give my 2 cents, so...
> > > >
> > > > I think we should keep ocp-dma.h around for now (2.4.x timeframe) and
> > > > change consistent_sync() to not check for the PCI versions (which I
> > > > believe happened just because it's a cut&paste&fix of the ARM code) and
> > > > for 2.5 hope that a more general fix comes out..
> > >
> > > I disagree.  Using the constants from pci.h is the least-wrong simple
> > > fix for now - yes it's conceptually wrong, but they're just arbitrary
> > > constants, it doesn't do any real damage.
> >
> > Well, for the moment I think they're less-arbitrary constants than they
> > should be, but yes..
>
> Um, I don't see what you mean by "less arbitrary than they should be".

Well, you *could* pick any number you like for the DMA_* ones, but it'd
be much nicer (and in a sense safer) to pick the same constants
PCI_DMA_* uses.

> > > Changing consistent_sync() to use its own constants isn't completely
> > > trivial, because asm/pci.h calls consistent_sync() with the constants
> > > passed as arguments to pci_map_single() et al, assuming they have the
> > > correct values.  So, to decouple the consistent_sync() constants from
> > > the PCI direction constants we would have to put a switch in every
> > > time consistent_sync() is used in asm/pci.h.  The PCI constants could
> > > be defined in terms of the non-PCI constants, but that means changing
> > > generic code (linux/pci.h).
> >
> > Well, if DMA_* == PCI_DMA_*, we don't have to do anything.  We put a
> > comment saying something like:
> > /* The PCI_DMA_* constants have nothing to do with PCI.  Someone should
> >  * rename this in 2.5... */
> > And while someone could re-number the arbitrary constants, I'd think
> > something like that would be questioned before being accepted.
>
> Well, yes, but calling the function with once constant then testing
> against another within the function gives me the willies.  If the
> constants are changed it will break horribly, and it will give
> misleading results if people try to grep for one constant or the
> other.

Which is why you put up a comment, which people grep'ing for will see.
And again, while someone *could* go and change all of the constants for
fun, it's unlikley in 2.4.

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[David Gibson]

On Tue, May 28, 2002 at 06:51:23AM -0400, Dan Malek wrote:
>
> David Gibson wrote:
>
> >...  In some ways, since we're PPC specific anyway, I think it would
> >make as much sense as anything just to directly call
> >dcache_flush_range() and so forth, rather than consistent_sync() or
> >dma_cache_*().
>
> Yes, that's true.  These consistent_* functions were added when we
> started using non-PCI drivers that are common across multiple platforms.
> It seems none of the platforms had common names for data cache management
> functions, so people started using the consistent_sync() in it's place.
> It also made sense because they were using the other consistent_* functions
> as well.  No one probably noticed, but at the same time we also changed
> the cache management function names to be similar to other architectures
> as well.

Well, actually, dma_cache_wback() and friends still appear to be more
widely used than consistent_sync().  AFAICT only PPC and ARM use
consistent_sync().

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[David Gibson]

On Tue, May 28, 2002 at 06:39:05AM -0400, Dan Malek wrote:
>
> David Gibson wrote:
>
>
>
> >Changing consistent_sync() to use its own constants isn't completely
> >trivial, because asm/pci.h calls consistent_sync() with the constants
> >passed as arguments to pci_map_single() et al, ....
>
> The constent_sync() is not unique to 4xx and it absolutely must not
> be required to use pci headers/functions for it to work properly.
> These lower level functions are common to other platforms, other drivers
> will call consistent_* functions directly, and in some cases the platforms
> or drivers that use these functions will not require (nor compile correctly)
> if PCI is enabled.  Unfortunately, making some of these drivers work without
> PCI defined was the great challenge, so changing these constant the time I
> put these functions into PowerPC was the least of the problem :-)

I agree it must not depend on PCI functions, but I don't see the
problem with PCI headers (well, I do, but it seems less important than
other considerations).  consistent_sync() relies on the PCI direction
constants defined in linux/pci.h *now* - it checks them explicitly in
its switch statement (so does the ARM version).  That works fine,
because pci.h defines the direction constants even if CONFIG_PCI=n.
The constants don't really have anything to do with PCI, and are
already used for SBUS DMA directions on Sparc and IIRC for ISA DMA in
some places.  We're talking about whether the *callers* of
consistent_sync() should need pci.h.

> >.....  So, to decouple the consistent_sync() constants from
> >the PCI direction constants we would have to put a switch in every
> >time consistent_sync() is used in asm/pci.h.
>
> Just define them so they have the same value as their PCI counterparts.

Well, it looks like I'm not going to win this argument, but calling
with one name for a constant and checking for another in the
implementation, relying on them to have the same value bothers me
much, much more that a few irrelevant PCI_ on the front of constant
names (which is not to say that the latter doesn't bother me at all).

> >... In io.h along with the prototype for
> >consistent_sync() would be a better idea.
>
> Yes, thank you :-)

Ok, well if we have to have the two sets of constants, please lets put
them in io.h and not in ocp-dma.h.  They have nothing to do with OCP
(just as they have nothing to do with PCI).

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Dan Malek]

David Gibson wrote:

> Well, actually, dma_cache_wback() and friends still appear to be more
> widely used than consistent_sync().  AFAICT only PPC and ARM use
> consistent_sync().

Just for a historical note.....these functions were implemented about
the same time to support non-PCI USB controllers.  They have found other
uses since then.  Other architectures seem to assume PCI is always present,
and will call the dma_cache_* functions within the pci functions to get
the same effect.  For non-PCI devices, they still call the pci functions
with a null pointer for the pci_dev.  On PPC and ARM we had systems that
wouldn't compile properly with PCI enabled, since there wasn't any PCI
bridge support (even fake ones :-).  We just added the consistent_sync()
to be orthogonal with the other archtecture independent consistent_* functions.
The other architectures are moving this way as they are finding it more
difficult to continue to fake a PCI on systems that really don't have it.


	-- Dan

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Dan Malek]

David Gibson wrote:


> I agree it must not depend on PCI functions, but I don't see the
> problem with PCI headers (well, I do, but it seems less important than
> other considerations).

OK.

> ...  consistent_sync() relies on the PCI direction
> constants defined in linux/pci.h *now* - it checks them explicitly in
> its switch statement (so does the ARM version).  That works fine,
> because pci.h defines the direction constants even if CONFIG_PCI=n.

OK.

> The constants don't really have anything to do with PCI, and are
> already used for SBUS DMA directions on Sparc and IIRC for ISA DMA in
> some places.  We're talking about whether the *callers* of
> consistent_sync() should need pci.h.

I thought so, too.


> Well, it looks like I'm not going to win this argument,

What argument are you trying to win?  I thought you were arguing
for changing the flags to consistent_sync(), but didn't like any
of the solutions?

> .... but calling
> with one name for a constant and checking for another in the
> implementation, relying on them to have the same value bothers me
> much, much more that a few irrelevant PCI_ on the front of constant
> names (which is not to say that the latter doesn't bother me at all).

I was just proposing a solution that addressed one of your other
concerns.  Requiring some flag values to be the same, but with
different names, isn't something unique here :-)


> Ok, well if we have to have the two sets of constants, please lets put
> them in io.h and not in ocp-dma.h.  They have nothing to do with OCP
> (just as they have nothing to do with PCI).

We can't change the PCI names, so I just suggested we change the names to
consistent_sync(), define those in io.h (so non-PCI systems don't have to
include pci header files just to get a flag name), and put a big comment
around the flag definitions that they have to match the PCI_ names.

I'm constantly reminded that Linux is full of hacks to meet its major design
goal of high performance.  This seems to fit nicely :-)

Or, as you said, just not use consistent_sync() in our PowerPC specific
software and call the dma_cache_* functions directly.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Armin Kuster]

Dan Malek wrote:

> We can't change the PCI names, so I just suggested we change the names to
> consistent_sync(), define those in io.h (so non-PCI systems don't have to
> include pci header files just to get a flag name), and put a big comment
> around the flag definitions that they have to match the PCI_ names.

Why not put the flag definition in a /linux/dma.h and include that in
/linux/pci.h.  Have the pci flags be defined as the ones in dma.h.  That
way it's obvious where the orginal defines are and easier to maintain. :)


dma.h
/* This defines the direction arg to the DMA mapping routines. */
#define DMA_BIDIRECTIONAL	0
#define DMA_TODEVICE		1
#define DMA_FROMDEVICE		2
#define DMA_NONE		3

pci.h
/* This defines are defined in linux/dma.h */
#define PCI_DMA_BIDIRECTIONAL	DMA_BIDIRECTIONAL
#define PCI_DMA_TODEVICE	DMA_TODEVICE
#define PCI_DMA_FROMDEVICE	DMA_FROMDEVICE
#define PCI_DMA_NONE		DMA_NONE


>
>
>     -- Dan
>
>
>
>
Armin


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[David Gibson]

On Wed, May 29, 2002 at 11:02:09AM -0400, Dan Malek wrote:
>
> David Gibson wrote:
>
>
> >I agree it must not depend on PCI functions, but I don't see the
> >problem with PCI headers (well, I do, but it seems less important than
> >other considerations).
>
> OK.
>
> >...  consistent_sync() relies on the PCI direction
> >constants defined in linux/pci.h *now* - it checks them explicitly in
> >its switch statement (so does the ARM version).  That works fine,
> >because pci.h defines the direction constants even if CONFIG_PCI=n.
>
> OK.
>
> >The constants don't really have anything to do with PCI, and are
> >already used for SBUS DMA directions on Sparc and IIRC for ISA DMA in
> >some places.  We're talking about whether the *callers* of
> >consistent_sync() should need pci.h.
>
> I thought so, too.
>
> >Well, it looks like I'm not going to win this argument,
>
> What argument are you trying to win?  I thought you were arguing
> for changing the flags to consistent_sync(), but didn't like any
> of the solutions?

I'm attempting to argue that changing the callers of consistent_sync()
to use the PCI_DMA_* constants (even if they're not PCI drivers) is a
lesser evil than ever calling consisent_sync() with constants of
different names to the ones checked inside, even if they have the same
values.

I do want to change the flags to consistent_sync() but only if we
change it everywhere, including in all the PCI stuff: that requires
convincing Linus, and probably won't happen at all in 2.4.

> >.... but calling
> >with one name for a constant and checking for another in the
> >implementation, relying on them to have the same value bothers me
> >much, much more that a few irrelevant PCI_ on the front of constant
> >names (which is not to say that the latter doesn't bother me at all).
>
> I was just proposing a solution that addressed one of your other
> concerns.  Requiring some flag values to be the same, but with
> different names, isn't something unique here :-)

No, but it's the only instance I've come across recently in the stuff
I've been doing.  If I knew of other cases I'd think they were a Bad
Thing and want to change them too.

> >Ok, well if we have to have the two sets of constants, please lets put
> >them in io.h and not in ocp-dma.h.  They have nothing to do with OCP
> >(just as they have nothing to do with PCI).
>
> We can't change the PCI names, so I just suggested we change the names to
> consistent_sync(), define those in io.h (so non-PCI systems don't have to
> include pci header files just to get a flag name), and put a big comment
> around the flag definitions that they have to match the PCI_ names.

Well, I hope that we can change the PCI names in time.  When I get a
chance, I'll send a patch (against 2.5) to lkml.

> I'm constantly reminded that Linux is full of hacks to meet its major design
> goal of high performance.  This seems to fit nicely :-)
>
> Or, as you said, just not use consistent_sync() in our PowerPC specific
> software and call the dma_cache_* functions directly.

Or indeed the even more direct flush_dcache_range() etc.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[David Gibson]

On Wed, May 29, 2002 at 09:01:59AM -0700, Armin Kuster wrote:
>
> Dan Malek wrote:
>
> >We can't change the PCI names, so I just suggested we change the names to
> >consistent_sync(), define those in io.h (so non-PCI systems don't have to
> >include pci header files just to get a flag name), and put a big comment
> >around the flag definitions that they have to match the PCI_ names.
>
> Why not put the flag definition in a /linux/dma.h and include that in
> /linux/pci.h.  Have the pci flags be defined as the ones in dma.h.  That
> way it's obvious where the orginal defines are and easier to
> maintain. :)

That's what I'd like to do, but it means changes to generic code, so
Linus needs to be convinced.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[Dan Malek]

David Gibson wrote:

> I'm attempting to argue that changing the callers of consistent_sync()
> to use the PCI_DMA_* constants ....

I see.  I think that would be fine as long as we can compile and get
these constants defined without having to enable CONFIG_PCI.  It is
a little weird to include pci header files in drivers that don't use it :-)

> I do want to change the flags to consistent_sync() but only if we
> change it everywhere, including in all the PCI stuff: that requires
> convincing Linus, and probably won't happen at all in 2.4.

But, doesn't that involve adding code to test PCI flags and convert
them to consistent_sync() flags that you didn't want to do?

How about this, we just add another set of flags that are disjoint
from the PCI flags, and just test for both in consistent_sync()?

Thanks.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Another OCP enet patch

[David Gibson]

On Thu, May 30, 2002 at 12:16:24AM -0400, Dan Malek wrote:
> David Gibson wrote:
>
> >I'm attempting to argue that changing the callers of consistent_sync()
> >to use the PCI_DMA_* constants ....
>
> I see.  I think that would be fine as long as we can compile and get
> these constants defined without having to enable CONFIG_PCI.  It is
> a little weird to include pci header files in drivers that don't use it :-)

That's what the tree does right now (since Tom backed out his changes
in this area, anyway).  ibp_ocp_enet.c includes linux/pci.h, but in no
way requires CONFIG_PCI.

> >I do want to change the flags to consistent_sync() but only if we
> >change it everywhere, including in all the PCI stuff: that requires
> >convincing Linus, and probably won't happen at all in 2.4.
>
> But, doesn't that involve adding code to test PCI flags and convert
> them to consistent_sync() flags that you didn't want to do?

No, I mean to essentially abolish PCI_DMA_* and replace them with
DMA_*, say in linux/dma.h and use them everywhere that currently uses
the PCI_DMA_* constants - in PCI drivers, in OCP drivers in SBUS
drivers.  Or as an intermediate step we could define the new constants
then define the PCI_DMA_ constants in terms of them.

> How about this, we just add another set of flags that are disjoint
> from the PCI flags, and just test for both in consistent_sync()?

Hmm... IMO marginally better than different names for things which
must be the same, but still worse than using the pci.h constants.
Changing the constants so they weren't disjoint is perhaps less likely
than (in the other case) changing them so they weren't equal, but
would have more confusing consequences.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/