From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ch1ehsobe001.messaging.microsoft.com ([216.32.181.181]:12139 "EHLO ch1outboundpool.messaging.microsoft.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756223Ab3HLPLs (ORCPT ); Mon, 12 Aug 2013 11:11:48 -0400 Message-ID: <1376320338.28628.3.camel@linux-builds1> Subject: Re: [RESEND PATCH 2/4] arm: socfpga: Add platform initialization for ethernet From: Dinh Nguyen Date: Mon, 12 Aug 2013 10:12:18 -0500 In-Reply-To: <20130811235505.GI26757@quad.lixom.net> References: <1375739925-9110-1-git-send-email-dinguyen@altera.com> <1375739925-9110-3-git-send-email-dinguyen@altera.com> <20130811235505.GI26757@quad.lixom.net> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Sender: devicetree-owner@vger.kernel.org To: Olof Johansson Cc: dinh.linux@gmail.com, Arnd Bergmann , Rob Herring , Pawel Moll , Mark Rutland , Stephen Warren , Ian Campbell , devicetree@vger.kernel.org, Thomas Petazzoni , Pavel Machek , Jack Mitchell List-ID: On Sun, 2013-08-11 at 16:55 -0700, Olof Johansson wrote: > Hi, > > On Mon, Aug 05, 2013 at 04:58:43PM -0500, dinguyen@altera.com wrote: > > From: Dinh Nguyen > > > > Use the PHYLIB to set the correct clock and skew values to the Micrel PHY. Add > > platform specific intilization to put the STMMAC ethernet controller into the > > correct PHY mode. > > > > Signed-off-by: Dinh Nguyen > > Tested-by: Jack Mitchell > > CC: Arnd Bergmann > > CC: Olof Johansson > > Cc: Rob Herring > > Cc: Pawel Moll > > Cc: Mark Rutland > > Cc: Stephen Warren > > Cc: Ian Campbell > > Cc: devicetree@vger.kernel.org > > Cc: Thomas Petazzoni > > Cc: Pavel Machek > > Cc: Jack Mitchell > > --- > > arch/arm/mach-socfpga/core.h | 8 ++++ > > arch/arm/mach-socfpga/socfpga.c | 85 ++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 92 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h > > index 572b8f7..505b8fe5 100644 > > --- a/arch/arm/mach-socfpga/core.h > > +++ b/arch/arm/mach-socfpga/core.h > > @@ -28,6 +28,14 @@ > > #define RSTMGR_CTRL_SWCOLDRSTREQ 0x1 /* Cold Reset */ > > #define RSTMGR_CTRL_SWWARMRSTREQ 0x2 /* Warm Reset */ > > > > +#define SYSMGR_EMACGRP_CTRL_OFFSET 0x60 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_WIDTH 2 > > + > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK 0x00000003 > > + > > extern void socfpga_secondary_startup(void); > > extern void __iomem *socfpga_scu_base_addr; > > > > diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c > > index bfce964..abbde76 100644 > > --- a/arch/arm/mach-socfpga/socfpga.c > > +++ b/arch/arm/mach-socfpga/socfpga.c > > @@ -16,10 +16,14 @@ > > */ > > #include > > #include > > +#include > > #include > > #include > > +#include > > #include > > #include > > +#include > > +#include > > > > #include > > #include > > @@ -33,6 +37,22 @@ void __iomem *rst_manager_base_addr; > > void __iomem *clk_mgr_base_addr; > > unsigned long cpu1start_addr; > > > > +static int stmmac_plat_init(struct platform_device *pdev); > > + > > +static struct plat_stmmacenet_data stmmacenet0_data = { > > + .init = &stmmac_plat_init, > > +}; > > + > > +static struct plat_stmmacenet_data stmmacenet1_data = { > > + .init = &stmmac_plat_init, > > +}; > > I would prefer to see refactoring of the driver such that it gets > the phy configuration data out of the devicetree instead of needing > per-board callbacks. > > > + > > +static const struct of_dev_auxdata socfpga_auxdata_lookup[] __initconst = { > > + OF_DEV_AUXDATA("snps,dwmac-3.70a", 0xff700000, NULL, &stmmacenet0_data), > > + OF_DEV_AUXDATA("snps,dwmac-3.70a", 0xff702000, NULL, &stmmacenet1_data), > > + {/* sentinel */} > > +}; > > + > > static struct map_desc scu_io_desc __initdata = { > > .virtual = SOCFPGA_SCU_VIRT_BASE, > > .pfn = 0, /* run-time */ > > @@ -58,6 +78,65 @@ static void __init socfpga_scu_map_io(void) > > iotable_init(&scu_io_desc, 1); > > } > > > > +static int ksz9021rlrn_phy_fixup(struct phy_device *phydev) > > +{ > > + if (IS_BUILTIN(CONFIG_PHYLIB)) { > > So what happens if PHYLIB is a module? > > > + /* min rx data delay */ > > + phy_write(phydev, MICREL_KSZ9021_EXTREG_CTRL, > > + MICREL_KSZ9021_RGMII_RX_DATA_PAD_SCEW | 0x8000); > > + phy_write(phydev, MICREL_KSZ9021_EXTREG_DATA_WRITE, 0x0000); > > + > > + /* max rx/tx clock delay, min rx/tx control delay */ > > + phy_write(phydev, MICREL_KSZ9021_EXTREG_CTRL, > > + MICREL_KSZ9021_RGMII_CLK_CTRL_PAD_SCEW | 0x8000); > > + phy_write(phydev, MICREL_KSZ9021_EXTREG_DATA_WRITE, 0xa0d0); > > + phy_write(phydev, MICREL_KSZ9021_EXTREG_CTRL, 0x104); > > + } > > + > > + return 0; > > +} > > + > > +static int stmmac_plat_init(struct platform_device *pdev) > > +{ > > + u32 ctrl, val, shift; > > + int phymode; > > + > > + if (of_machine_is_compatible("altr,socfpga-vt")) > > + return 0; > > + > > + phymode = of_get_phy_mode(pdev->dev.of_node); > > + > > + switch (phymode) { > > + case PHY_INTERFACE_MODE_RGMII: > > + val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII; > > + break; > > + case PHY_INTERFACE_MODE_MII: > > + case PHY_INTERFACE_MODE_GMII: > > + val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII; > > + break; > > + default: > > + pr_err("%s bad phy mode %d", __func__, phymode); > > + return -EINVAL; > > + } > > + > > + if (&stmmacenet1_data == pdev->dev.platform_data) > > + shift = SYSMGR_EMACGRP_CTRL_PHYSEL_WIDTH; > > + else if (&stmmacenet0_data == pdev->dev.platform_data) > > + shift = 0; > > This is awkward too, comparing back to the platform data to figure out which of > the two devices you're on. > > > + else { > > + pr_err("%s unexpected platform data pointer\n", __func__); > > + return -EINVAL; > > + } > > + > > + ctrl = readl(sys_manager_base_addr + SYSMGR_EMACGRP_CTRL_OFFSET); > > + ctrl &= ~(SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << shift); > > + ctrl |= (val << shift); > > + > > + writel(ctrl, (sys_manager_base_addr + SYSMGR_EMACGRP_CTRL_OFFSET)); > > + > > + return 0; > > +} > > + > > static void __init socfpga_map_io(void) > > { > > socfpga_scu_map_io(); > > @@ -106,9 +185,13 @@ static void socfpga_cyclone5_restart(enum reboot_mode mode, const char *cmd) > > static void __init socfpga_cyclone5_init(void) > > { > > l2x0_of_init(0, ~0UL); > > - of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > + of_platform_populate(NULL, of_default_bus_match_table, > > + socfpga_auxdata_lookup, NULL); > > of_clk_init(NULL); > > socfpga_init_clocks(); > > + if (IS_BUILTIN(CONFIG_PHYLIB)) > > + phy_register_fixup_for_uid(PHY_ID_KSZ9021RLRN, > > + MICREL_PHY_ID_MASK, ksz9021rlrn_phy_fixup); > Thanks for the review Olof. Will rework. Dinh > > -Olof >