Re: [PATCH 6/7 v2] fs_enet: Be an of_platform device when CONFIG_PPC_CPM_NEW_BINDING is set.
On Tue, 21 Aug 2007 11:47:41 -0500
Scott Wood [EMAIL PROTECTED] wrote:
Vitaly Bordug wrote:
On Fri, 17 Aug 2007 13:17:18 -0500
Scott Wood wrote:
The existing OF glue code was crufty and broken. Rather than fix
it, it will be removed, and the ethernet driver now talks to the
device tree directly.
A bit short description, I'd rather expect some specific
improvements list, that are now up and running using device tree.
Or if it is just move to new infrastucture, let's state that, too.
Some of specific binding changes (there are too many to exhaustively
list) are enumerated in the new CPM binding patch, which I'll send
after Kumar's include/asm-ppc patch goes in (since it modifies one of
those files).
ok
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+static int __devinit find_phy(struct device_node *np,
+ struct fs_platform_info *fpi)
+{
+ struct device_node *phynode, *mdionode;
+ struct resource res;
+ int ret = 0, len;
+
+ const u32 *data = of_get_property(np, phy-handle, len);
+ if (!data || len != 4)
+ return -EINVAL;
+
+ phynode = of_find_node_by_phandle(*data);
+ if (!phynode)
+ return -EINVAL;
+
+ mdionode = of_get_parent(phynode);
+ if (!phynode)
+ goto out_put_phy;
+
+ ret = of_address_to_resource(mdionode, 0, res);
+ if (ret)
+ goto out_put_mdio;
+
+ data = of_get_property(phynode, reg, len);
+ if (!data || len != 4)
+ goto out_put_mdio;
+
+ snprintf(fpi-bus_id, 16, PHY_ID_FMT, res.start, *data);
+
+out_put_mdio:
+ of_node_put(mdionode);
+out_put_phy:
+ of_node_put(phynode);
+ return ret;
+}
And without phy node?
It returns -EINVAL. :-)
+#ifdef CONFIG_FS_ENET_HAS_FEC
+#define IS_FEC(match) ((match)-data == fs_fec_ops)
+#else
+#define IS_FEC(match) 0
+#endif
+
Since we're talking directly with device tree, why bother with
CONFIG_ stuff? We are able to figure it out from dts..
We are figuring it out from the DTS (that's what match-data is). I
just didn't want boards without a FEC to have to build in support for
it and waste memory.
yes, wrong snippet
what about
#ifdef CONFIG_CPM2
+ r = fs_enet_mdio_bb_init();
+ if (r != 0)
+ goto out_mdio_bb;
+#endif
+#ifdef CONFIG_8xx
+ r = fs_enet_mdio_fec_init();
+ if (r != 0)
+ goto out_mdio_fec;
+#endif
We had to pray and hope that 8xx would only have fec, and cpm2 has some
bitbanged stuff. now we can inquire dts and know for sure, at least it seems so.
+ fpi-rx_ring = 32;
+ fpi-tx_ring = 32;
+ fpi-rx_copybreak = 240;
+ fpi-use_napi = 0;
+ fpi-napi_weight = 17;
+
move params over to dts?
No. These aren't attributes of the hardware, they're choices the
driver makes about how much memory to use and how to interact with
the rest of the kernel.
+ ret = find_phy(ofdev-node, fpi);
+ if (ret)
+ goto out_free_fpi;
+
so we're hosed without phy node.
How is that different from the old code, where you're hosed without
fep-fpi-bus_id?
I wasn't defending old code, and consider old code is POS, new one is just
great game meaningless.
I am just stating the problem, that we'll have to address later. On 8xx even
reference boards may be
without phy at all.
+static struct of_device_id fs_enet_match[] = {
+#ifdef CONFIG_FS_ENET_HAS_SCC
same nagging. Are we able to get rid of Kconfig arcane defining
which SoC currently plays the game for fs_enet?
No, it's still needed for mpc885ads to determine pin setup and
conflicting device tree node removal (though that could go away if
the device tree is changed to reflect the jumper settings).
It's also useful for excluding unwanted code. I don't like using
8xx/CPM2 as the decision point for that -- why should I build in
mac-scc.c if I have no intention of using an SCC ethernet (either
because my board doesn't have one, or because it's slow and conflicts
with other devices)?
ok, agreed, size is most serious judge here. we'll definitely have to revisit
pin problem later too
(because custom designs sometimes switch contradictory devices on-the-fly,
disable soc parts for alternative function, etc.) QE-like pin encoding may be
an option for this or not- I'm inclined to look at
most resource-safe approach.
-Scott
--
Sincerely, Vitaly
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7 v2] fs_enet: Be an of_platform device when CONFIG_PPC_CPM_NEW_BINDING is set.
Vitaly Bordug wrote: yes, wrong snippet what about #ifdef CONFIG_CPM2 +r = fs_enet_mdio_bb_init(); + if (r != 0) + goto out_mdio_bb; +#endif +#ifdef CONFIG_8xx + r = fs_enet_mdio_fec_init(); + if (r != 0) + goto out_mdio_fec; +#endif We had to pray and hope that 8xx would only have fec, and cpm2 has some bitbanged stuff. now we can inquire dts and know for sure, at least it seems so. Yeah, that sucks. We should add kconfig options for each mii type, and let them have their own init functions. That only affects the initcalls (and kernel size), though; it still uses the phy-handle to decide what mdio controller to actually talk to. How is that different from the old code, where you're hosed without fep-fpi-bus_id? I wasn't defending old code, and consider old code is POS, new one is just great game meaningless. I am just stating the problem, that we'll have to address later. On 8xx even reference boards may be without phy at all. OK -- would it suffice to just never call any phylib functions and always assume the link is up if the phy-handle property is undefined? ok, agreed, size is most serious judge here. we'll definitely have to revisit pin problem later too (because custom designs sometimes switch contradictory devices on-the-fly, disable soc parts for alternative function, etc.) If it's really on-the-fly (and not jumpered/once-per-boot), then board code should expose some kind of API to switch the functions, acting like a hotplug bus. Associating it with open/close of the device won't work if one of the devices is something like USB which needs to start working without any internal open()-like action. -Scott - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7 v2] fs_enet: Be an of_platform device when CONFIG_PPC_CPM_NEW_BINDING is set.
On Fri, 17 Aug 2007 13:17:18 -0500
Scott Wood wrote:
The existing OF glue code was crufty and broken. Rather than fix it,
it will be removed, and the ethernet driver now talks to the device
tree directly.
A bit short description, I'd rather expect some specific improvements list,
that are now up and running using device tree. Or if it is just move to new
infrastucture, let's state that, too.
Some other notes below.
The old, non-CONFIG_PPC_CPM_NEW_BINDING code can go away once CPM
platforms are dropped from arch/ppc (which will hopefully be soon),
and existing arch/powerpc boards that I wasn't able to test on for
this patchset get converted (which should be even sooner).
mii-bitbang.c now also uses the generic bitbang code.
Signed-off-by: Scott Wood [EMAIL PROTECTED]
---
Sorry, the previous version of this patch had bb_ops in the wrong
place, and wouldn't build when not using the new binding.
drivers/net/fs_enet/Kconfig|1 +
drivers/net/fs_enet/fs_enet-main.c | 286 ++--
drivers/net/fs_enet/fs_enet.h | 55 +-
drivers/net/fs_enet/mac-fcc.c | 89 ++--
drivers/net/fs_enet/mac-fec.c | 23 ++-
drivers/net/fs_enet/mac-scc.c | 55 +++--
drivers/net/fs_enet/mii-bitbang.c | 438
---
drivers/net/fs_enet/mii-fec.c | 140 -
include/linux/fs_enet_pd.h |5 + 9 files changed, 740
insertions(+), 352 deletions(-)
diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig
index e27ee21..2765e49 100644
--- a/drivers/net/fs_enet/Kconfig
+++ b/drivers/net/fs_enet/Kconfig
@@ -11,6 +11,7 @@ config FS_ENET_HAS_SCC
config FS_ENET_HAS_FCC
bool Chip has an FCC usable for ethernet
depends on FS_ENET CPM2
+ select MDIO_BITBANG
default y
config FS_ENET_HAS_FEC
diff --git a/drivers/net/fs_enet/fs_enet-main.c
b/drivers/net/fs_enet/fs_enet-main.c index 0ec30a8..7bf29a2 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -44,12 +44,18 @@
#include asm/irq.h
#include asm/uaccess.h
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+#include linux/of_platform.h
+#endif
+
#include fs_enet.h
/*/
+#ifndef CONFIG_PPC_CPM_NEW_BINDING
static char version[] __devinitdata =
DRV_MODULE_NAME .c:v DRV_MODULE_VERSION (
DRV_MODULE_RELDATE ) \n; +#endif
MODULE_AUTHOR(Pantelis Antoniou [EMAIL PROTECTED]);
MODULE_DESCRIPTION(Freescale Ethernet Driver);
@@ -953,6 +959,7 @@ static int fs_ioctl(struct net_device *dev,
struct ifreq *rq, int cmd) extern int fs_mii_connect(struct
net_device *dev); extern void fs_mii_disconnect(struct net_device
*dev);
+#ifndef CONFIG_PPC_CPM_NEW_BINDING
static struct net_device *fs_init_instance(struct device *dev,
struct fs_platform_info *fpi)
{
@@ -1132,6 +1139,7 @@ static int fs_cleanup_instance(struct
net_device *ndev)
return 0;
}
+#endif
/**/
@@ -1140,35 +1148,279 @@ void *fs_enet_immap = NULL;
static int setup_immap(void)
{
- phys_addr_t paddr = 0;
- unsigned long size = 0;
-
#ifdef CONFIG_CPM1
- paddr = IMAP_ADDR;
- size = 0x1; /* map 64K */
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+ fs_enet_immap = mpc8xx_immr;
+#else
+ fs_enet_immap = ioremap(IMAP_ADDR, 0x4000);
+ WARN_ON(!fs_enet_immap);
#endif
-
-#ifdef CONFIG_CPM2
- paddr = CPM_MAP_ADDR;
- size = 0x4; /* map 256 K */
+#elif defined(CONFIG_CPM2)
+ fs_enet_immap = cpm2_immr;
#endif
- fs_enet_immap = ioremap(paddr, size);
- if (fs_enet_immap == NULL)
- return -EBADF; /* XXX ahem; maybe just
BUG_ON? */
return 0;
}
+#ifndef CONFIG_PPC_CPM_NEW_BINDING
static void cleanup_immap(void)
{
- if (fs_enet_immap != NULL) {
- iounmap(fs_enet_immap);
- fs_enet_immap = NULL;
- }
+#if defined(CONFIG_CPM1)
+ iounmap(fs_enet_immap);
+#endif
}
+#endif
/**/
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+static int __devinit find_phy(struct device_node *np,
+ struct fs_platform_info *fpi)
+{
+ struct device_node *phynode, *mdionode;
+ struct resource res;
+ int ret = 0, len;
+
+ const u32 *data = of_get_property(np, phy-handle, len);
+ if (!data || len != 4)
+ return -EINVAL;
+
+ phynode = of_find_node_by_phandle(*data);
+ if (!phynode)
+ return -EINVAL;
+
+ mdionode = of_get_parent(phynode);
+ if (!phynode)
+ goto out_put_phy;
+
+ ret = of_address_to_resource(mdionode, 0, res);
+ if (ret)
+ goto out_put_mdio;
+
+ data = of_get_property(phynode,
Re: [PATCH 6/7 v2] fs_enet: Be an of_platform device when CONFIG_PPC_CPM_NEW_BINDING is set.
Vitaly Bordug wrote:
On Fri, 17 Aug 2007 13:17:18 -0500
Scott Wood wrote:
The existing OF glue code was crufty and broken. Rather than fix it,
it will be removed, and the ethernet driver now talks to the device
tree directly.
A bit short description, I'd rather expect some specific improvements list,
that are now up and running using device tree. Or if it is just move to new
infrastucture, let's state that, too.
Some of specific binding changes (there are too many to exhaustively
list) are enumerated in the new CPM binding patch, which I'll send
after Kumar's include/asm-ppc patch goes in (since it modifies one of
those files).
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+static int __devinit find_phy(struct device_node *np,
+ struct fs_platform_info *fpi)
+{
+ struct device_node *phynode, *mdionode;
+ struct resource res;
+ int ret = 0, len;
+
+ const u32 *data = of_get_property(np, phy-handle, len);
+ if (!data || len != 4)
+ return -EINVAL;
+
+ phynode = of_find_node_by_phandle(*data);
+ if (!phynode)
+ return -EINVAL;
+
+ mdionode = of_get_parent(phynode);
+ if (!phynode)
+ goto out_put_phy;
+
+ ret = of_address_to_resource(mdionode, 0, res);
+ if (ret)
+ goto out_put_mdio;
+
+ data = of_get_property(phynode, reg, len);
+ if (!data || len != 4)
+ goto out_put_mdio;
+
+ snprintf(fpi-bus_id, 16, PHY_ID_FMT, res.start, *data);
+
+out_put_mdio:
+ of_node_put(mdionode);
+out_put_phy:
+ of_node_put(phynode);
+ return ret;
+}
And without phy node?
It returns -EINVAL. :-)
+#ifdef CONFIG_FS_ENET_HAS_FEC
+#define IS_FEC(match) ((match)-data == fs_fec_ops)
+#else
+#define IS_FEC(match) 0
+#endif
+
Since we're talking directly with device tree, why bother with CONFIG_ stuff?
We are able to figure it out from dts..
We are figuring it out from the DTS (that's what match-data is). I
just didn't want boards without a FEC to have to build in support for it
and waste memory.
+ fpi-rx_ring = 32;
+ fpi-tx_ring = 32;
+ fpi-rx_copybreak = 240;
+ fpi-use_napi = 0;
+ fpi-napi_weight = 17;
+
move params over to dts?
No. These aren't attributes of the hardware, they're choices the driver
makes about how much memory to use and how to interact with the rest of
the kernel.
+ ret = find_phy(ofdev-node, fpi);
+ if (ret)
+ goto out_free_fpi;
+
so we're hosed without phy node.
How is that different from the old code, where you're hosed without
fep-fpi-bus_id?
+static struct of_device_id fs_enet_match[] = {
+#ifdef CONFIG_FS_ENET_HAS_SCC
same nagging. Are we able to get rid of Kconfig arcane defining which SoC
currently plays the game for fs_enet?
No, it's still needed for mpc885ads to determine pin setup and
conflicting device tree node removal (though that could go away if the
device tree is changed to reflect the jumper settings).
It's also useful for excluding unwanted code. I don't like using
8xx/CPM2 as the decision point for that -- why should I build in
mac-scc.c if I have no intention of using an SCC ethernet (either
because my board doesn't have one, or because it's slow and conflicts
with other devices)?
-Scott
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7 v2] fs_enet: Be an of_platform device when CONFIG_PPC_CPM_NEW_BINDING is set.
On Sat, Aug 18, 2007 at 11:36:24AM -0500, Kumar Gala wrote:
This patch seems to mix moving to using the device tree directly w/o
some other modifications. Can it be broken into those two changes as
they'd be easier to review.
The last iteration of these patches, I got complaints that I was
splitting them up too fine-grained. I don't think it's productive to
keep iterating on exactly how much is in any given patch until I hit the
right combination of granularity and whoever happens to be reading e-mail
when I submit.
In the case of this particular patch, most of what isn't directly related
to converting to using the device tree directly is fixing problems that I
encountered in doing so -- what value is there in coming up with
intermediary versions that kind-of-sort-of make sense, on a good day, if
you don't look to closely? The existing codebase is crap, and if every
logical change were its own patch, the patchset would be ten times as
long, and take ten times as long to produce. Note that I did separate
what I thought were the more relevant-to-review and/or highly indpendent
changes.
The major thing I see in this patch that could have been usefully
separated out was the conversion of mii_bitbang.c to use the generic code
introduced by patch 1. However, that would require retrieving and
retesting the intermediate version, and I don't think there's sufficient
damage to reviewability (apart perhaps from diff's stupidity in thinking
that a single { is relevant common ground between completely unrelated
chunks of code) relative to the cost in preparing such a split.
Is there anything of the actual content of the patch that you object to,
or have a question about?
-Scott
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7 v2] fs_enet: Be an of_platform device when CONFIG_PPC_CPM_NEW_BINDING is set.
On Aug 17, 2007, at 1:17 PM, Scott Wood wrote: The existing OF glue code was crufty and broken. Rather than fix it, it will be removed, and the ethernet driver now talks to the device tree directly. The old, non-CONFIG_PPC_CPM_NEW_BINDING code can go away once CPM platforms are dropped from arch/ppc (which will hopefully be soon), and existing arch/powerpc boards that I wasn't able to test on for this patchset get converted (which should be even sooner). mii-bitbang.c now also uses the generic bitbang code. Signed-off-by: Scott Wood [EMAIL PROTECTED] --- Sorry, the previous version of this patch had bb_ops in the wrong place, and wouldn't build when not using the new binding. This patch seems to mix moving to using the device tree directly w/o some other modifications. Can it be broken into those two changes as they'd be easier to review. - k - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7 v2] fs_enet: Be an of_platform device when CONFIG_PPC_CPM_NEW_BINDING is set.
The existing OF glue code was crufty and broken. Rather than fix it, it
will be removed, and the ethernet driver now talks to the device tree
directly.
The old, non-CONFIG_PPC_CPM_NEW_BINDING code can go away once CPM
platforms are dropped from arch/ppc (which will hopefully be soon), and
existing arch/powerpc boards that I wasn't able to test on for this
patchset get converted (which should be even sooner).
mii-bitbang.c now also uses the generic bitbang code.
Signed-off-by: Scott Wood [EMAIL PROTECTED]
---
Sorry, the previous version of this patch had bb_ops in the wrong
place, and wouldn't build when not using the new binding.
drivers/net/fs_enet/Kconfig|1 +
drivers/net/fs_enet/fs_enet-main.c | 286 ++--
drivers/net/fs_enet/fs_enet.h | 55 +-
drivers/net/fs_enet/mac-fcc.c | 89 ++--
drivers/net/fs_enet/mac-fec.c | 23 ++-
drivers/net/fs_enet/mac-scc.c | 55 +++--
drivers/net/fs_enet/mii-bitbang.c | 438 ---
drivers/net/fs_enet/mii-fec.c | 140 -
include/linux/fs_enet_pd.h |5 +
9 files changed, 740 insertions(+), 352 deletions(-)
diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig
index e27ee21..2765e49 100644
--- a/drivers/net/fs_enet/Kconfig
+++ b/drivers/net/fs_enet/Kconfig
@@ -11,6 +11,7 @@ config FS_ENET_HAS_SCC
config FS_ENET_HAS_FCC
bool Chip has an FCC usable for ethernet
depends on FS_ENET CPM2
+ select MDIO_BITBANG
default y
config FS_ENET_HAS_FEC
diff --git a/drivers/net/fs_enet/fs_enet-main.c
b/drivers/net/fs_enet/fs_enet-main.c
index 0ec30a8..7bf29a2 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -44,12 +44,18 @@
#include asm/irq.h
#include asm/uaccess.h
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+#include linux/of_platform.h
+#endif
+
#include fs_enet.h
/*/
+#ifndef CONFIG_PPC_CPM_NEW_BINDING
static char version[] __devinitdata =
DRV_MODULE_NAME .c:v DRV_MODULE_VERSION ( DRV_MODULE_RELDATE ) \n;
+#endif
MODULE_AUTHOR(Pantelis Antoniou [EMAIL PROTECTED]);
MODULE_DESCRIPTION(Freescale Ethernet Driver);
@@ -953,6 +959,7 @@ static int fs_ioctl(struct net_device *dev, struct ifreq
*rq, int cmd)
extern int fs_mii_connect(struct net_device *dev);
extern void fs_mii_disconnect(struct net_device *dev);
+#ifndef CONFIG_PPC_CPM_NEW_BINDING
static struct net_device *fs_init_instance(struct device *dev,
struct fs_platform_info *fpi)
{
@@ -1132,6 +1139,7 @@ static int fs_cleanup_instance(struct net_device *ndev)
return 0;
}
+#endif
/**/
@@ -1140,35 +1148,279 @@ void *fs_enet_immap = NULL;
static int setup_immap(void)
{
- phys_addr_t paddr = 0;
- unsigned long size = 0;
-
#ifdef CONFIG_CPM1
- paddr = IMAP_ADDR;
- size = 0x1; /* map 64K */
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+ fs_enet_immap = mpc8xx_immr;
+#else
+ fs_enet_immap = ioremap(IMAP_ADDR, 0x4000);
+ WARN_ON(!fs_enet_immap);
#endif
-
-#ifdef CONFIG_CPM2
- paddr = CPM_MAP_ADDR;
- size = 0x4; /* map 256 K */
+#elif defined(CONFIG_CPM2)
+ fs_enet_immap = cpm2_immr;
#endif
- fs_enet_immap = ioremap(paddr, size);
- if (fs_enet_immap == NULL)
- return -EBADF; /* XXX ahem; maybe just BUG_ON? */
return 0;
}
+#ifndef CONFIG_PPC_CPM_NEW_BINDING
static void cleanup_immap(void)
{
- if (fs_enet_immap != NULL) {
- iounmap(fs_enet_immap);
- fs_enet_immap = NULL;
- }
+#if defined(CONFIG_CPM1)
+ iounmap(fs_enet_immap);
+#endif
}
+#endif
/**/
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+static int __devinit find_phy(struct device_node *np,
+ struct fs_platform_info *fpi)
+{
+ struct device_node *phynode, *mdionode;
+ struct resource res;
+ int ret = 0, len;
+
+ const u32 *data = of_get_property(np, phy-handle, len);
+ if (!data || len != 4)
+ return -EINVAL;
+
+ phynode = of_find_node_by_phandle(*data);
+ if (!phynode)
+ return -EINVAL;
+
+ mdionode = of_get_parent(phynode);
+ if (!phynode)
+ goto out_put_phy;
+
+ ret = of_address_to_resource(mdionode, 0, res);
+ if (ret)
+ goto out_put_mdio;
+
+ data = of_get_property(phynode, reg, len);
+ if (!data || len != 4)
+ goto out_put_mdio;
+
+ snprintf(fpi-bus_id, 16, PHY_ID_FMT, res.start, *data);
+
+out_put_mdio:
+ of_node_put(mdionode);
+out_put_phy:
+ of_node_put(phynode);
+ return ret;
+}
+
+#ifdef CONFIG_FS_ENET_HAS_FEC
+#define IS_FEC(match) ((match)-data ==
