[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.
Ben Warren
biggerbadderben at gmail.com
Mon Jan 18 06:59:19 CET 2010
Hi Richard,
Richard Retanubun wrote:
> Wolfgang Denk wrote:
> <snip>
>> Can you please check the status of this patch:
>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/61758
>>
>> Thanks.
>>
>> Best regards,
>>
>> Wolfgang Denk
>>
>
> Hello,
>
> Wofgang, I believe at the time Ben made the comment that my patch's
> usage of things like these:
>
> -#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM))
> - volatile ioport_t *iop = ioport_addr ((immap_t *)
> CONFIG_SYS_IMMR, MDIO_PORT);
> +#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM)) \
> + && !defined(CONFIG_MPC83XX)
> + volatile gpio_n_t *iop = ioport_addr ((immap_t *)
> CONFIG_SYS_IMMR, MDIO_PORT);
> +#elif defined(CONFIG_MPC83XX)
> + volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR;
> + volatile qepio83xx_t *par_io = (volatile qepio83xx_t *)&im->qepio;
> + volatile gpio_n_t *iop = &(par_io->ioport[MDIO_PORT]);
> #endif
>
> Is propagating bad practice, and I agreed. Since then, Luigi's deft
> implementation MDIO_DECLARE and MDC_DELCARE
> have eliminated this problem. (or at least move it away from Ben's
> vigilant eyes into the board specific header file).
>
Luigi did some good things here. I'm glad you're sync'ing up.
> As a refresher, the declaration for which UEC uses the bitbang in the
> board header file is like this:
>
> #define CONFIG_SYS_BITBANG_PHY_PORT(name) name,
>
> /* Bit-bang SMI ports */
> #define CONFIG_SYS_BITBANG_PHY_PORTS \
> CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC4") \
> CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC5")
>
> similar to how CONFIG_SYS_FIXED_PHY_PORT is used.
>
>
> And so, here is V2 of the patch, with less ugly board specific code
> and rebased to commit 2ff6922280025c1315c53fa2eb4ab33f0c9591de
> (Tue Jan 12 23:47:03 2010 +0100)
>
> - Richard
>
> ====================================================================================================
>
> From 46380bee3b8e63afff9d00729f38cb960e0d2bfb Mon Sep 17 00:00:00 2001
> From: Richard Retanubun <RichardRetanubun at RuggedCom.com>
> Date: Wed, 17 Jun 2009 16:00:41 -0400
> Subject: [PATCH] 83xx: UEC: Added support for bitBang MII driver
> access to PHYs
>
> This patch enabled support for having PHYs on bitBang MII and uec MII
> operating at the same time. Modeled after the MPC8360ADS implementation
> and added the ability to specify which ethernet interfaces have bitbang
> SMI on the board header file.
>
> Signed-off-by: Richard Retanubun <RichardRetanubun at RuggedCom.com>
> ---
> drivers/net/phy/miiphybb.h | 26 +++++++++++++++++++++++
> drivers/qe/uec.c | 6 +---
> drivers/qe/uec_phy.c | 48
> ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 76 insertions(+), 4 deletions(-)
> create mode 100644 drivers/net/phy/miiphybb.h
>
> diff --git a/drivers/net/phy/miiphybb.h b/drivers/net/phy/miiphybb.h
> new file mode 100644
> index 0000000..4fff40d
> --- /dev/null
> +++ b/drivers/net/phy/miiphybb.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright (C) 2006-2009 RuggedCom, Inc.
> + *
> + * Richard Retanubun <richardretanubun at ruggedcom.com>
> + *
> + * 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 program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +int bb_miiphy_read (char *devname, unsigned char addr,
> + unsigned char reg, unsigned short *value);
> +
> +int bb_miiphy_write (char *devname, unsigned char addr,
> + unsigned char reg, unsigned short value);
There's already BB-related stuff in include/miiphy.h. Why not put the
prototypes there?
> diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
> index db95ada..109edd9 100644
> --- a/drivers/qe/uec.c
> +++ b/drivers/qe/uec.c
> @@ -579,8 +579,7 @@ static void phy_change(struct eth_device *dev)
> adjust_link(dev);
> }
>
> -#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
> - && !defined(BITBANGMII)
> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /*&&
> !defined(BITBANGMII)*/
What's the point of this comment?
>
> /*
> * Find a device index from the devlist by name
> @@ -1372,8 +1371,7 @@ int uec_initialize(bd_t *bis, uec_info_t *uec_info)
> return err;
> }
>
> -#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
> - && !defined(BITBANGMII)
> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /* &&
> !defined(CONFIG_BITBANGMII) */
> miiphy_register(dev->name, uec_miiphy_read, uec_miiphy_write);
Same comment-related comment as above.
> #endif
>
> diff --git a/drivers/qe/uec_phy.c b/drivers/qe/uec_phy.c
> index 9715183..6fb3b4d 100644
> --- a/drivers/qe/uec_phy.c
> +++ b/drivers/qe/uec_phy.c
> @@ -25,6 +25,7 @@
> #include "uec.h"
> #include "uec_phy.h"
> #include "miiphy.h"
> +#include "../net/phy/miiphybb.h"
Please don't use relative includes. As I'm sure you've noticed, people
around here are keen on massive file reorganization, and this sort of
thing makes that more work. If you put your prototypes in miiphy.h it
won't be a problem
>
> #define ugphy_printk(format, arg...) \
> printf(format "\n", ## arg)
> @@ -93,6 +94,27 @@ static const struct fixed_phy_port fixed_phy_port[]
> = {
> CONFIG_SYS_FIXED_PHY_PORTS /* defined in board configuration file */
> };
>
> +/*--------------------------------------------------------------------+
> + * BitBang MII support for ethernet ports
> + *
> + * Based from MPC8560ADS implementation
> + *--------------------------------------------------------------------*/
> +/*
> + * Example board header file to define bitbang ethernet ports:
> + *
> + * #define CONFIG_SYS_BITBANG_PHY_PORT(name) name,
> + * #define CONFIG_SYS_BITBANG_PHY_PORTS
> CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC0")
Maybe you could come up with a shorter name?
> +*/
> +#ifndef CONFIG_SYS_BITBANG_PHY_PORTS
> +#define CONFIG_SYS_BITBANG_PHY_PORTS /* default is an empty array */
> +#endif
> +
> +#if defined(CONFIG_BITBANGMII)
> +static const char *bitbang_phy_port[] = {
> + CONFIG_SYS_BITBANG_PHY_PORTS /* defined in board configuration
> file */
> +};
> +#endif /* CONFIG_BITBANGMII */
> +
> static void config_genmii_advert (struct uec_mii_info *mii_info);
> static void genmii_setup_forced (struct uec_mii_info *mii_info);
> static void genmii_restart_aneg (struct uec_mii_info *mii_info);
> @@ -113,6 +135,19 @@ void uec_write_phy_reg (struct eth_device *dev,
> int mii_id, int regnum, int valu
> enet_tbi_mii_reg_e mii_reg = (enet_tbi_mii_reg_e) regnum;
> u32 tmp_reg;
>
> +
> +#if defined(CONFIG_BITBANGMII)
> + u8 i = 0;
Why is this a u8? I don't believe you save any space by making it less
than an int, and probably invite compiler warnings with the comparison
to ARRAY_SIZE()
> +
> + for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
> + if (strncmp(dev->name, bitbang_phy_port[i],
> + strlen(dev->name)) == 0) {
It's probably better to do sizeof(dev->name)
> + (void)bb_miiphy_write(NULL, mii_id, regnum, value);
What's the point of this cast?
> + return;
> + }
> + }
> +#endif /* CONFIG_BITBANGMII */
> +
> ug_regs = ugeth->uec_mii_regs;
>
> /* Stop the MII management read cycle */
> @@ -140,6 +175,19 @@ int uec_read_phy_reg (struct eth_device *dev, int
> mii_id, int regnum)
> u32 tmp_reg;
> u16 value;
>
> +
> +#if defined(CONFIG_BITBANGMII)
> + u8 i = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
> + if (strncmp(dev->name, bitbang_phy_port[i],
> + strlen(dev->name)) == 0) {
> + (void)bb_miiphy_read(NULL, mii_id, regnum, &value);
> + return (value);
> + }
> + }
Same comments as above.
> +#endif /* CONFIG_BITBANGMII */
> +
> ug_regs = ugeth->uec_mii_regs;
>
> /* Setting up the MII Mangement Address Register */
> --
> 1.6.5
>
thanks a lot,
Ben
More information about the U-Boot
mailing list