Kevin,

On Wed, Sep 16, 2009 at 21:43:46, Kevin Hilman wrote:
> Chaithrika U S <chaithr...@ti.com> writes:
> 
> > DA850/OMAP-L138 EVM has a RMII ethernet PHY on the UI
> > daughter card. The PHY is enabled by proper programming of the
> > IO Expander (TCA6416) ports. Also for RMII PHY to work, the MDIO
> > clock of MII PHY has to be disabled since both the PHYs have the
> > same address. This is done via the GPIO2[6] pin.
> >
> > This patch adds support for RMII PHY. It also provides a menuconfig
> > option to choose the required PHY interface.
> >
> > Signed-off-by: Chaithrika U S <chaithr...@ti.com>
> 
> Functionally, looks mostly ok but some organizational comments inline below.
> 
> Also, after addressing comments, please refresh against current
> master.  There are some other mux changes now pushed that cause some
> minor conflits with yours.
> 
OK, will send an updated patch.

> > ---
> >  arch/arm/mach-davinci/Kconfig              |   12 +++
> >  arch/arm/mach-davinci/board-da850-evm.c    |  104 
> > ++++++++++++++++++++++++++--
> >  arch/arm/mach-davinci/da850.c              |   17 +++++
> >  arch/arm/mach-davinci/include/mach/da8xx.h |    1 +
> >  arch/arm/mach-davinci/include/mach/mux.h   |    9 +++
> >  5 files changed, 137 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
> > index 40866c6..08bdeb3 100644
> > --- a/arch/arm/mach-davinci/Kconfig
> > +++ b/arch/arm/mach-davinci/Kconfig
> > @@ -145,6 +145,18 @@ config DAVINCI_RESET_CLOCKS
> >       probably do not want this option enabled until your
> >       device drivers work properly.
> >  
> > +config DA850_RMII
> > +   bool "Use RMII Ethernet PHY on DA850/OMAP-L138 EVM"
> > +   depends on MACH_DAVINCI_DA850_EVM
> > +   help
> > +     Say Y if you want to use the RMII PHY on the DA850/OMAP-L138 EVM.
> > +     This PHY is found on the UI daughter card that is supplied with
> > +     the EVM.
> > +     NOTE: Please take care while choosing this option, MII PHY will
> > +     not be functional if RMII mode is selected. This also affects
> > +     the operation of video devices as they are pin multiplexed with
> > +     RMII pins.
> > +
> >  endmenu
> 
> Can you rework the Kconfig options to be similar to the ones done for
> DA830 (See the new DA830_UI Kconfig option)
> 
> Basically, in Kconfig, you can select the UI board and select which
> peripherals on the UI board to enable, and these are sub-options of
> the DA830_EVM Kconfig option.
> 
> On da830, currently the only sub-option is LCD, but on da850, there
> would be LCD and now this PHY.
> 
OK. 

> >  endif
> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c 
> > b/arch/arm/mach-davinci/board-da850-evm.c
> > index fbc7aae..f7845bb 100644
> > --- a/arch/arm/mach-davinci/board-da850-evm.c
> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/console.h>
> >  #include <linux/i2c.h>
> >  #include <linux/i2c/at24.h>
> > +#include <linux/i2c/pca953x.h>
> >  #include <linux/gpio.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/mtd/mtd.h>
> > @@ -32,6 +33,7 @@
> >  #include <mach/cp_intc.h>
> >  #include <mach/da8xx.h>
> >  #include <mach/nand.h>
> > +#include <mach/mux.h>
> >  
> >  #define DA850_EVM_PHY_MASK         0x1
> >  #define DA850_EVM_MDIO_FREQUENCY   2200000 /* PHY bus frequency */
> > @@ -42,6 +44,8 @@
> >  #define DA850_MMCSD_CD_PIN         GPIO_TO_PIN(4, 0)
> >  #define DA850_MMCSD_WP_PIN         GPIO_TO_PIN(4, 1)
> >  
> > +#define DA850_MII_MDIO_CLKEN_PIN   GPIO_TO_PIN(2, 6)
> > +
> >  static struct mtd_partition da850_evm_norflash_partition[] = {
> >     {
> >             .name           = "NOR filesystem",
> > @@ -264,11 +268,103 @@ static void __init da850_evm_init_nor(void)
> >  #define HAS_MMC 0
> >  #endif
> >  
> > +static int gpio_exp_setup(struct i2c_client *client, unsigned gpio,
> > +                                           unsigned ngpio, void *c)
> > +{
> > +   struct davinci_soc_info *soc_info = &davinci_soc_info;
> > +   int sel_a, sel_b, sel_c;
> > +
> > +   sel_a = gpio + 7;
> > +   sel_b = gpio + 6;
> > +   sel_c = gpio + 5;
> > +
> > +   /* deselect all fucntionalities */
> > +   gpio_request(sel_a, "sel_a");
> > +   gpio_direction_output(sel_a, 1);
> > +
> > +   gpio_request(sel_b, "sel_b");
> > +   gpio_direction_output(sel_b, 1);
> > +
> > +   gpio_request(sel_c, "sel_c");
> > +   gpio_direction_output(sel_c, 1);
> > +
> > +   if (soc_info->emac_pdata->rmii_en) {
> > +           /* enable RMII */
> > +           gpio_direction_output(sel_a, 0);
> > +           gpio_direction_output(sel_b, 1);
> > +           gpio_direction_output(sel_c, 1);
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static int gpio_exp_teardown(struct i2c_client *client, unsigned gpio,
> > +                                           unsigned ngpio, void *c)
> > +{
> > +   gpio_free(gpio + 5);
> > +   gpio_free(gpio + 6);
> > +   gpio_free(gpio + 7);
> 
> Do you want/need to disable all functionality here as well?
> 
It is not needed but can be done. 

> > +   return 0;
> > +}
> > +
> > +static struct pca953x_platform_data gpio_exp = {
> > +   .gpio_base      = DAVINCI_N_GPIO,
> > +   .setup          = gpio_exp_setup,
> > +   .teardown       = gpio_exp_teardown,
> 
> As in da830, you rename thise da850_evm_ui_expander_* since this
> expander is on the UI board.
> 
OK.

> > +};
> > +
> > +static struct i2c_board_info __initdata i2c_info[] =  {
> > +   {
> > +           I2C_BOARD_INFO("tca6416", 0x20),
> > +           .platform_data = &gpio_exp,
> > +   },
> > +};
> 
> Is there a way on da830 or da850 to detect the presence of the UI
> board in software?  It would be nice to detect it and only register
> the IO expander if present.
> 
There is no way to detect presence of UI card. Is it OK to decide 
based in the menuconfig option, something like:

+static struct i2c_board_info __initdata i2c_info[] =  {
+#ifdef CONFIG_DA850_UI
+       {
+               I2C_BOARD_INFO("tca6416", 0x20),
+               .platform_data = &gpio_exp,
+       },
+#endif
+};

The expander setup and teardown callbacks can also be guarded by
the same config option.

> > +
> > +static void __init da850_evm_config_emac(u8 rmii_en)
> > +{
> > +   void __iomem *cfg_chip3_base;
> > +   int ret;
> > +   u32 val;
> > +
> > +   cfg_chip3_base = DA8XX_SYSCFG_VIRT(DA8XX_CFGCHIP3_REG);
> > +
> > +   /* configure the CFGCHIP3 register for RMII or MII */
> > +   val = readl(cfg_chip3_base);
> > +   if (rmii_en)
> > +           val |= BIT(8);
> > +   else
> > +           val &= ~BIT(8);
> > +
> > +   writel(val, cfg_chip3_base);
> > +
> > +   if (!rmii_en)
> > +           ret = da8xx_pinmux_setup(da850_cpgmac_pins);
> > +   else
> > +           ret = da8xx_pinmux_setup(da850_rmii_pins);
> > +   if (ret)
> > +           pr_warning("da850_evm_init: cpgmac/rmii mux setup failed: %d\n",
> > +                           ret);
> > +
> > +   if (rmii_en) {
> > +           /* Disable MII MDIO clock */
> > +           davinci_cfg_reg(DA850_GPIO2_6);
> > +           gpio_request(DA850_MII_MDIO_CLKEN_PIN, "mdio_clk_en");
> 
> Check return code.
OK.

> 
> > +           gpio_direction_output(DA850_MII_MDIO_CLKEN_PIN, 1);
> 
> gpio_free() ?

This pin has to be held high for RMII phy to work. If it is freed it will allow
other users to change the output value of this pin. If MII phy is also enabled 
it
will lead to ambiguity as to which PHY responds to the MDIO commands.

> 
> Kevin
> 


Regards, 
Chaithrika


_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Reply via email to