| Message ID | 214127499f10b0099e6f3542e1e879fdd8c1bdcf.1342171151.git.vipulkumar.samar@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 13, 2012 at 10:23 AM, Vipul Kumar Samar <vipulkumar.samar@st.com> wrote: > Use AUXDATA to pass platform data for Ethernet controller. > diff --git a/arch/arm/mach-spear13xx/spear1340.c b/arch/arm/mach-spear13xx/spear1340.c > +static struct plat_stmmacenet_data eth_data = { > + .bus_id = 0, > + .phy_addr = -1, > + .interface = PHY_INTERFACE_MODE_RGMII, > + .has_gmac = 1, > + .enh_desc = 1, > + .tx_coe = 1, > + .dma_cfg = &dma0_private_data, > + .rx_coe = STMMAC_RX_COE_TYPE2, > + .bugged_jumbo = 1, > + .pmt = 1, > + .mdio_bus_data = &mdio0_private_data, > + .init = spear13xx_eth_phy_clk_cfg, > + .clk_csr = STMMAC_CSR_150_250M, > +}; > + > /* SATA device registration */ > static int sata_miphy_init(struct device *dev, void __iomem *addr) > { > @@ -166,6 +197,7 @@ static struct of_dev_auxdata spear1340_auxdata_lookup[] __initdata = { > OF_DEV_AUXDATA("snps,spear-ahci", SPEAR1340_SATA_BASE, NULL, > &sata_pdata), > OF_DEV_AUXDATA("arm,pl011", SPEAR1340_UART1_BASE, NULL, &uart1_data), > + OF_DEV_AUXDATA("st,spear600-gmac", SPEAR13XX_GETH_BASE, NULL, ð_data), > {} > }; Adding Stefan and Peppe. I understand why you can't send all platform data from DT. Let me elaborate the problem statement stmmac is used by platforms with and without DT. - Without DT will pass platform data directly, without any issues. - With DT have to pass all data, some of that via DT and other without DT, like routines (atleast for now) For now what I suggest is, update DT support for whatever we can.. i.e. support Maximum properties there. As finally we will support everything via DT, no platform data. Whatever is left, that can't be passed via DT, like routine, pass it from platform data and merge both these versions of platform data in driver, keeping DT ones in priority. i.e. Whatever is defined in DT properties must come from there and left outs from platform data. -- viresh
On Friday 13 July 2012, viresh kumar wrote: > Adding Stefan and Peppe. > > I understand why you can't send all platform data from DT. > Let me elaborate the problem statement > > stmmac is used by platforms with and without DT. > - Without DT will pass platform data directly, without any issues. > - With DT have to pass all data, some of that via DT and other without > DT, like routines > (atleast for now) > > For now what I suggest is, update DT support for whatever we can.. > i.e. support Maximum > properties there. As finally we will support everything via DT, no > platform data. > > Whatever is left, that can't be passed via DT, like routine, pass it > from platform data > and merge both these versions of platform data in driver, keeping DT > ones in priority. > > i.e. Whatever is defined in DT properties must come from there and > left outs from > platform data. Absolutely agreed. The one part I think you both have wrong is the idea that the spear13xx_eth_phy_clk_cfg function is needed. I believe the correct answer for this is to introduce a driver for the phy in drivers/net/phy/, and have the phy be described correctly in the device tree and referenced found using the of_phy_find_device() function. Arnd
On 14:53 Fri 13 Jul , Vipul Kumar Samar wrote: > Use AUXDATA to pass platform data for Ethernet controller. > > Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> > --- > arch/arm/mach-spear13xx/include/mach/generic.h | 2 + > arch/arm/mach-spear13xx/include/mach/spear.h | 1 + > arch/arm/mach-spear13xx/spear1340.c | 32 +++++++ > arch/arm/mach-spear13xx/spear13xx.c | 104 ++++++++++++++++++++++++ > 4 files changed, 139 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-spear13xx/include/mach/generic.h b/arch/arm/mach-spear13xx/include/mach/generic.h > index dac57fd..8c8fbaa 100644 > --- a/arch/arm/mach-spear13xx/include/mach/generic.h > +++ b/arch/arm/mach-spear13xx/include/mach/generic.h > @@ -15,6 +15,7 @@ > #define __MACH_GENERIC_H > > #include <linux/dmaengine.h> > +#include <linux/platform_device.h> > #include <asm/mach/time.h> > > /* Add spear13xx structure declarations here */ > @@ -31,6 +32,7 @@ void __init spear13xx_map_io(void); > void __init spear13xx_dt_init_irq(void); > void __init spear13xx_l2x0_init(void); > bool dw_dma_filter(struct dma_chan *chan, void *slave); > +int spear13xx_eth_phy_clk_cfg(struct platform_device *pdev); > void spear_restart(char, const char *); > void spear13xx_secondary_startup(void); > > diff --git a/arch/arm/mach-spear13xx/include/mach/spear.h b/arch/arm/mach-spear13xx/include/mach/spear.h > index 65f27de..b0b6f91 100644 > --- a/arch/arm/mach-spear13xx/include/mach/spear.h > +++ b/arch/arm/mach-spear13xx/include/mach/spear.h > @@ -46,6 +46,7 @@ > #define DMAC0_BASE UL(0xEA800000) > #define DMAC1_BASE UL(0xEB000000) > #define MCIF_CF_BASE UL(0xB2800000) > +#define SPEAR13XX_GETH_BASE UL(0xE2000000) > > /* Devices present in SPEAr1310 */ > #ifdef CONFIG_MACH_SPEAR1310 > diff --git a/arch/arm/mach-spear13xx/spear1340.c b/arch/arm/mach-spear13xx/spear1340.c > index 81e4ed7..ab282ed 100644 > --- a/arch/arm/mach-spear13xx/spear1340.c > +++ b/arch/arm/mach-spear13xx/spear1340.c > @@ -18,6 +18,9 @@ > #include <linux/delay.h> > #include <linux/dw_dmac.h> > #include <linux/of_platform.h> > +#include <linux/phy.h> > +#include <linux/platform_device.h> > +#include <linux/stmmac.h> > #include <asm/hardware/gic.h> > #include <asm/mach/arch.h> > #include <mach/dma.h> > @@ -100,6 +103,34 @@ static struct amba_pl011_data uart1_data = { > .dma_rx_param = &uart1_dma_param[1], > }; > > +/* Ethernet platform data */ > +static struct stmmac_mdio_bus_data mdio0_private_data = { > + .bus_id = 0, > + .phy_mask = 0, > +}; > + > +static struct stmmac_dma_cfg dma0_private_data = { > + .pbl = 16, > + .fixed_burst = 1, > + .burst_len = DMA_AXI_BLEN_ALL, > +}; > + > +static struct plat_stmmacenet_data eth_data = { > + .bus_id = 0, > + .phy_addr = -1, > + .interface = PHY_INTERFACE_MODE_RGMII, > + .has_gmac = 1, > + .enh_desc = 1, > + .tx_coe = 1, > + .dma_cfg = &dma0_private_data, > + .rx_coe = STMMAC_RX_COE_TYPE2, > + .bugged_jumbo = 1, > + .pmt = 1, > + .mdio_bus_data = &mdio0_private_data, > + .init = spear13xx_eth_phy_clk_cfg, > + .clk_csr = STMMAC_CSR_150_250M, > +}; sorry I see no reason to do not pass this via DT I agreed that the pbl is quite sensitive but you do need to pass them via DT Best Regards, J.
Viresh, On 7/13/2012 4:00 PM, viresh kumar wrote: > On Fri, Jul 13, 2012 at 10:23 AM, Vipul Kumar Samar > <vipulkumar.samar@st.com> wrote: >> Use AUXDATA to pass platform data for Ethernet controller. > Adding Stefan and Peppe. > > I understand why you can't send all platform data from DT. > Let me elaborate the problem statement > > stmmac is used by platforms with and without DT. > - Without DT will pass platform data directly, without any issues. > - With DT have to pass all data, some of that via DT and other without > DT, like routines > (atleast for now) > > For now what I suggest is, update DT support for whatever we can.. > i.e. support Maximum > properties there. As finally we will support everything via DT, no > platform data. > > Whatever is left, that can't be passed via DT, like routine, pass it > from platform data > and merge both these versions of platform data in driver, keeping DT > ones in priority. > > i.e. Whatever is defined in DT properties must come from there and > left outs from > platform data. I do differ on the point over here. I do believe that the code for now should be left as into mutually exclusive sections. As you said, without DT, the platform data would exist without any problem, Thats fine But with DT also, as of now since the DT is still evolving we should not merge the data and keep it at two places. The reasons being. The stmmac driver is being used by multiple platforms, lets say within spear we have different variants requiring different configurations and dividing those configurations at two different places will require larger maintenance.Any more changes in driver that has dependency on the platform data will require updates, and more such conflicts will arrive related to maintenance. Lets keep the platform data as a part of AUXDATA for now, till the tree evolves fully specifically if DT is being used. Deepak > -- > viresh > . >
Hi Arnd, On 7/13/2012 7:52 PM, Arnd Bergmann wrote: > On Friday 13 July 2012, viresh kumar wrote: >> Adding Stefan and Peppe. >> >> I understand why you can't send all platform data from DT. >> Let me elaborate the problem statement >> >> stmmac is used by platforms with and without DT. >> - Without DT will pass platform data directly, without any issues. >> - With DT have to pass all data, some of that via DT and other without >> DT, like routines >> (atleast for now) >> >> For now what I suggest is, update DT support for whatever we can.. >> i.e. support Maximum >> properties there. As finally we will support everything via DT, no >> platform data. >> >> Whatever is left, that can't be passed via DT, like routine, pass it >> from platform data >> and merge both these versions of platform data in driver, keeping DT >> ones in priority. >> >> i.e. Whatever is defined in DT properties must come from there and >> left outs from >> platform data. > Absolutely agreed. > > The one part I think you both have wrong is the idea that the > spear13xx_eth_phy_clk_cfg function is needed. > > I believe the correct answer for this is to introduce a driver for > the phy in drivers/net/phy/, and have the phy be described correctly > in the device tree and referenced found using the of_phy_find_device() > function. SPEAr platform has been using the generic phy framework, and it just requires a register into mdio bus and provide the necessary callback w.r.t mdio read and write. These mdio read and write are more into the GMAC registers, and hence a part of stmmac driver. As far as my understanding is concerned, addition to the phy framework could have been done had it been some phy from stmicro, which requires some special addressing to be done. The function we are talking about is more related to handling the interface that phy has. The stmmac core has been configured as rmii/smii/rgmii and requires the system to generate corresponding clock to support data transfers over PHY. This was specifically the reason of aligning the phy clock configuration function with stmmac driver, and could well be taken as functional interface clock for mac core. I do think that we should not add a new driver for aligning the clock settings of the interfaces. Regards Deepak > > Arnd > . >
On 15:55 Tue 17 Jul , deepaksi wrote: > Hi Arnd, > > > On 7/13/2012 7:52 PM, Arnd Bergmann wrote: > >On Friday 13 July 2012, viresh kumar wrote: > >>Adding Stefan and Peppe. > >> > >>I understand why you can't send all platform data from DT. > >>Let me elaborate the problem statement > >> > >>stmmac is used by platforms with and without DT. > >>- Without DT will pass platform data directly, without any issues. > >>- With DT have to pass all data, some of that via DT and other without > >>DT, like routines > >> (atleast for now) > >> > >>For now what I suggest is, update DT support for whatever we can.. > >>i.e. support Maximum > >>properties there. As finally we will support everything via DT, no > >>platform data. > >> > >>Whatever is left, that can't be passed via DT, like routine, pass it > >>from platform data > >>and merge both these versions of platform data in driver, keeping DT > >>ones in priority. > >> > >>i.e. Whatever is defined in DT properties must come from there and > >>left outs from > >>platform data. > >Absolutely agreed. > > > >The one part I think you both have wrong is the idea that the > >spear13xx_eth_phy_clk_cfg function is needed. > > > >I believe the correct answer for this is to introduce a driver for > >the phy in drivers/net/phy/, and have the phy be described correctly > >in the device tree and referenced found using the of_phy_find_device() > >function. > > SPEAr platform has been using the generic phy framework, and it just > requires a register into mdio > bus and provide the necessary callback w.r.t mdio read and write. > These mdio read and write are more > into the GMAC registers, and hence a part of stmmac driver. > > As far as my understanding is concerned, addition to the phy > framework could have been done had it been > some phy from stmicro, which requires some special addressing to be done. > The function we are talking about is more related to handling the > interface that phy has. The stmmac core has > been configured as rmii/smii/rgmii and requires the system to > generate corresponding clock to support data > transfers over PHY. > This was specifically the reason of aligning the phy clock > configuration function with stmmac driver, and could > well be taken as functional interface clock for mac core. > I do think that we should not add a new driver for aligning the > clock settings of the interfaces. this is not spear related but IP related this need to move the stmmac IIRC I see it on other ST SoC Best Regards, J.
On Tuesday 17 July 2012, deepaksi wrote: > I do differ on the point over here. I do believe that the code for now > should be left as into mutually > exclusive sections. > As you said, without DT, the platform data would exist without any > problem, Thats fine > > But with DT also, as of now since the DT is still evolving we should not > merge the data and keep it at > two places. The reasons being. > The stmmac driver is being used by multiple platforms, lets say within > spear we have different variants > requiring different configurations and dividing those configurations at > two different places will require larger > maintenance. I don't think that the STMMAC_PLATFORM part is used by any other platform in the mainline kernel, so we are definitely free to rip out (parts of) the platform_data and replace them with DT properties. There is also no platform defining plat_stmmacenet_data yet, which means that the code is completely untested at the moment. Don't worry about any out-of-tree platforms, they can keep their out of tree patches to add back the platform data if they don't want to move to DT booting. Since all the data you are adding to spear1340.c is constant anyway, I suppose that means this data is specific to the spear1340 soc, not to a particular board or configuration. I would suggest you add a preset like static const struct of_device_id stmmac_dt_ids[] = { { .compatible = "st,spear600-gmac", .data = &spear600_data}, { .compatible = "st,spear1340-gmac", .data = &spear1340_data} }; that contains all the soc-specific data. You can probably make that "const" and just kill off the platform_data path in the driver. > Any more changes in driver that has dependency on the > platform data will require updates, > and more such conflicts will arrive related to maintenance. > > Lets keep the platform data as a part of AUXDATA for now, till the tree > evolves fully specifically if DT is being > used. SPEAr is already fully using DT for everything except DMA channels. Please don't add any more such exceptions. Arnd
Hi Arnd, On 7/17/2012 10:23 PM, Arnd Bergmann wrote: > On Tuesday 17 July 2012, deepaksi wrote: >> I do differ on the point over here. I do believe that the code for now >> should be left as into mutually >> exclusive sections. >> As you said, without DT, the platform data would exist without any >> problem, Thats fine >> >> But with DT also, as of now since the DT is still evolving we should not >> merge the data and keep it at >> two places. The reasons being. >> The stmmac driver is being used by multiple platforms, lets say within >> spear we have different variants >> requiring different configurations and dividing those configurations at >> two different places will require larger >> maintenance. > I don't think that the STMMAC_PLATFORM part is used by any other > platform in the mainline kernel, so we are definitely free to > rip out (parts of) the platform_data and replace them with DT > properties. > There is also no platform defining plat_stmmacenet_data > yet, which means that the code is completely untested at > the moment. > > Don't worry about any out-of-tree platforms, they can keep > their out of tree patches to add back the platform data if > they don't want to move to DT booting. > > Since all the data you are adding to spear1340.c is constant > anyway, I suppose that means this data is specific to > the spear1340 soc, not to a particular board or configuration. > I would suggest you add a preset like > > static const struct of_device_id stmmac_dt_ids[] = { > { .compatible = "st,spear600-gmac", .data =&spear600_data}, > { .compatible = "st,spear1340-gmac", .data =&spear1340_data} > }; > > that contains all the soc-specific data. You can probably make > that "const" and just kill off the platform_data path in the > driver. Sure, we will do it as per the suggestions. Regards Deepak >> Any more changes in driver that has dependency on the >> platform data will require updates, >> and more such conflicts will arrive related to maintenance. >> >> Lets keep the platform data as a part of AUXDATA for now, till the tree >> evolves fully specifically if DT is being >> used. > SPEAr is already fully using DT for everything except DMA channels. > Please don't add any more such exceptions. > > Arnd > . >
Hi Arnd, >>> >> I don't think that the STMMAC_PLATFORM part is used by any other >> platform in the mainline kernel, so we are definitely free to >> rip out (parts of) the platform_data and replace them with DT >> properties. >> There is also no platform defining plat_stmmacenet_data >> yet, which means that the code is completely untested at >> the moment. >> >> Don't worry about any out-of-tree platforms, they can keep >> their out of tree patches to add back the platform data if >> they don't want to move to DT booting. >> >> Since all the data you are adding to spear1340.c is constant >> anyway, I suppose that means this data is specific to >> the spear1340 soc, not to a particular board or configuration. >> I would suggest you add a preset like >> >> static const struct of_device_id stmmac_dt_ids[] = { >> { .compatible = "st,spear600-gmac", .data =&spear600_data}, >> { .compatible = "st,spear1340-gmac", .data =&spear1340_data} >> }; >> >> that contains all the soc-specific data. You can probably make >> that "const" and just kill off the platform_data path in the >> driver. > I am sorry but bringing alive this discussion once again, as we have some implementation specific open points. 1. In case we go by the above proposal, it kind of solves a few things for us, such as - Through the DT blob it was difficult to plug in some of the platform specific callbacks which kind of did few things that driver needed before it could be operational. In SPEAr architecture we have a miscellaneous space which provides SOC specific initializations used by devices which can be handled by these callbacks. Now with the approach suggested, we have the flexibility to add on these callbacks, where DT is not sufficient and move further. *But there are few catches/ query I have with me.* 1. Within the driver space now, even if it is a stmmac_platform.c file we are kind of adding a non driver related miscellaneous space (SoC specific) to do some initializations. Will that be fine to do all such initializations in driver ? 2. If yes, then it is kind of moving all the platform related initializations into driver, and these could be varying across machines, for example with SPEAr, 6xx /3xx/13xx may all have different callbacks; and then take it to next level when the stmmac driver would be shared across the platforms, and all initializations will plug into this file. It sounds more of collaborating of all the ethernet driver information which previously existed across platforms into one area, and making the driver space untidy. I would seek your opinion and suggestions on this. Regards Deepak
On Wednesday 25 July 2012, deepaksi wrote: > *But there are few catches/ query I have with me.* > > 1. Within the driver space now, even if it is a stmmac_platform.c file > we are kind of adding > a non driver related miscellaneous space (SoC specific) to do some > initializations. > Will that be fine to do all such initializations in driver ? It depends a lot on what you want to do with those callbacks. We generally try hard to make those callbacks unnecessary, but I agree that this is not always easy or obvious how to do it. There are cases where you have one SoC doing something very obscure with an otherwise generic piece of hardware, and in that case, it may be better to keep a callback in platform code and use some method (auxdata or a notifier) to hand a pointer to the driver. In other cases, you have a number of platforms that essentially want to do the same thing (set up clocks or voltages, pick a PHY, ...) and in that case you should not be using callback functions at all but instead describe the differences in the device tree using the bindings for whatever you want to do (adding bindings if necessary). You can also have a case where the callback is just doing something that is part of that driver in different ways, and in that case using the "compatible" property to determine which way is right is the best option. Can you post here the complete set of callback implementations that you have in your development trees at the moment? That should help us determine which case we're talking about here. Arnd
Hi Arnd, On Wed, Jul 25, 2012 at 06:31:53AM +0000, Arnd Bergmann wrote: > Can you post here the complete set of callback implementations > that you have in your development trees at the moment? That should > help us determine which case we're talking about here. There are following types of callbacks (involving SoC/platform) seen generally in our case, * parent clk selection - driver is not aware about various clock sources (which also varies from platform to platform) and hence can only be programmed by corresponding platform. This is the case in Ethernet, audio, etc. * spi controller This is little hack in the h/w. As part of s/w controlled chip select/deselect in spi-pl022, we need to write to some system specific register. * OTG controller phy clock initialization which involves series of steps including powering down, etc. * NAND controller bank selection on runtime by writing to system registers * sata controller Similar to OTG case which involves clock initialization. -- regards Shiraz
On Wednesday 25 July 2012, Shiraz Hashim wrote: > > On Wed, Jul 25, 2012 at 06:31:53AM +0000, Arnd Bergmann wrote: > > Can you post here the complete set of callback implementations > > that you have in your development trees at the moment? That should > > help us determine which case we're talking about here. > > There are following types of callbacks (involving SoC/platform) seen > generally in our case, > > * parent clk selection - > driver is not aware about various clock sources (which also > varies from platform to platform) and hence can only be > programmed by corresponding platform. > > This is the case in Ethernet, audio, etc. > > * sata controller > Similar to OTG case which involves clock initialization. This should definitely be moved into the drivers. A lot of drivers enable and program clocks using the generic clock interfaces, so there is no point using a callback. Note that since stmmac is an architecture independent driver, this depends on Mark Brown's patch to provide empty stubs for the clock management functions on architectures that don't yet use it. > * spi controller > This is little hack in the h/w. As part of s/w controlled chip > select/deselect in spi-pl022, we need to write to some system > specific register. > > * OTG controller > phy clock initialization which involves series of steps > including powering down, etc. > > * NAND controller > bank selection on runtime by writing to system registers I don't understand any of these, so unless you post the code that you want help with as I asked above, I'll assume that you find the solution yourself and don't need a callback ;-) Arnd
Hi Arnd, On Wed, Jul 25, 2012 at 05:10:53PM +0000, Arnd Bergmann wrote: > On Wednesday 25 July 2012, Shiraz Hashim wrote: > > On Wed, Jul 25, 2012 at 06:31:53AM +0000, Arnd Bergmann wrote: > > > Can you post here the complete set of callback implementations > > > that you have in your development trees at the moment? That should > > > help us determine which case we're talking about here. > > > > There are following types of callbacks (involving SoC/platform) seen > > generally in our case, > > > > * parent clk selection - > > driver is not aware about various clock sources (which also > > varies from platform to platform) and hence can only be > > programmed by corresponding platform. > > > > This is the case in Ethernet, audio, etc. > > > > * sata controller > > Similar to OTG case which involves clock initialization. > > This should definitely be moved into the drivers. A lot of drivers > enable and program clocks using the generic clock interfaces, so > there is no point using a callback. > > Note that since stmmac is an architecture independent driver, this > depends on Mark Brown's patch to provide empty stubs for > the clock management functions on architectures that don't yet > use it. Yes, this is true for clocks associated with devices and we do handle that in this way. I was talking more of cases where, we need to * select clock sources (parents) about which driver has no knowledge and may vary across boards. * perform initializations which are more than clock, like phy initialization/programming etc. This is SoC dependant. > > * spi controller > > This is little hack in the h/w. As part of s/w controlled chip > > select/deselect in spi-pl022, we need to write to some system > > specific register. > > > > * OTG controller > > phy clock initialization which involves series of steps > > including powering down, etc. > > > > * NAND controller > > bank selection on runtime by writing to system registers > > I don't understand any of these, so unless you post the code > that you want help with as I asked above, I'll assume that you find > the solution yourself and don't need a callback ;-) Some of them are, - fsmc NAND controller -- include/linux/mtd/fsmc.h struct fsmc_nand_platform_data { ... void (*select_bank)(uint32_t bank, uint32_t busw); ... }; -- arch/arm/mach-spear13xx/spear13xx.c void nand_select_bank(u32 bank, u32 busw) { u32 fsmc_cfg; if (cpu_is_spear1340()) { #ifdef CONFIG_CPU_SPEAR1340 fsmc_cfg = readl(VA_SPEAR1340_FSMC_CFG); #endif } else fsmc_cfg = readl(VA_FSMC_CFG); fsmc_cfg &= ~(NAND_BANK_MASK << NAND_BANK_SHIFT); fsmc_cfg |= (bank << NAND_BANK_SHIFT); if (busw) fsmc_cfg |= 1 << NAND_DEV_WIDTH16; else fsmc_cfg &= ~(1 << NAND_DEV_WIDTH16); if (cpu_is_spear1340()) { #ifdef CONFIG_CPU_SPEAR1340 writel(fsmc_cfg, VA_SPEAR1340_FSMC_CFG); #endif } else writel(fsmc_cfg, VA_FSMC_CFG); } - SATA Controller -- include/linux/ahci_platform.h struct ahci_platform_data { int (*init)(struct device *dev, void __iomem *addr); void (*exit)(struct device *dev); const struct ata_port_info *ata_port_info; unsigned int force_port_map; unsigned int mask_port_map; }; -- arch/arm/mach-spear13xx/spear1340.c void sata_miphy_exit(struct device *dev) { writel(0, VA_SPEAR1340_PCIE_SATA_CFG); writel(0, VA_SPEAR1340_PCIE_MIPHY_CFG); /* Enable PCIE SATA Controller reset */ writel((readl(VA_SPEAR1340_PERIP1_SW_RST) | (0x1000)), VA_SPEAR1340_PERIP1_SW_RST); msleep(20); /* Switch off sata power domain */ writel((readl(VA_SPEAR1340_PCM_CFG) & (~0x800)), VA_SPEAR1340_PCM_CFG); msleep(20); } static int sata_miphy_init(struct device *dev, void __iomem *addr) { writel(SPEAR1340_SATA_CFG_VAL, VA_SPEAR1340_PCIE_SATA_CFG); writel(SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK, VA_SPEAR1340_PCIE_MIPHY_CFG); /* Switch on sata power domain */ writel((readl(VA_SPEAR1340_PCM_CFG) | (0x800)), VA_SPEAR1340_PCM_CFG); msleep(20); /* Disable PCIE SATA Controller reset */ writel((readl(VA_SPEAR1340_PERIP1_SW_RST) & (~0x1000)), VA_SPEAR1340_PERIP1_SW_RST); msleep(20); return 0; } static struct ahci_platform_data sata_pdata = { .init = sata_miphy_init, .exit = sata_miphy_exit, }; -- regards Shiraz
On Thursday 26 July 2012, Shiraz Hashim wrote: > Hi Arnd, > > On Wed, Jul 25, 2012 at 05:10:53PM +0000, Arnd Bergmann wrote: > > On Wednesday 25 July 2012, Shiraz Hashim wrote: > > > On Wed, Jul 25, 2012 at 06:31:53AM +0000, Arnd Bergmann wrote: > > > > Can you post here the complete set of callback implementations > > > > that you have in your development trees at the moment? That should > > > > help us determine which case we're talking about here. > > > > > > There are following types of callbacks (involving SoC/platform) seen > > > generally in our case, > > > > > > * parent clk selection - > > > driver is not aware about various clock sources (which also > > > varies from platform to platform) and hence can only be > > > programmed by corresponding platform. > > > > > > This is the case in Ethernet, audio, etc. > > > > > > * sata controller > > > Similar to OTG case which involves clock initialization. > > > > This should definitely be moved into the drivers. A lot of drivers > > enable and program clocks using the generic clock interfaces, so > > there is no point using a callback. > > > > Note that since stmmac is an architecture independent driver, this > > depends on Mark Brown's patch to provide empty stubs for > > the clock management functions on architectures that don't yet > > use it. > > Yes, this is true for clocks associated with devices and we do handle > that in this way. > > I was talking more of cases where, we need to > > * select clock sources (parents) about which driver has no > knowledge and may vary across boards. I think we should be able to describe this now with the clock bindings for device tree. If not, then we need to update those bindings rather than work around the shortcomings. > * perform initializations which are more than clock, like phy > initialization/programming etc. This is SoC dependant. Setting up the phy is different, but again I think there is a better way than using callbacks if you put the initialization for the phy into the driver that deals with that phy. > > > * spi controller > > > This is little hack in the h/w. As part of s/w controlled chip > > > select/deselect in spi-pl022, we need to write to some system > > > specific register. > > > > > > * OTG controller > > > phy clock initialization which involves series of steps > > > including powering down, etc. > > > > > > * NAND controller > > > bank selection on runtime by writing to system registers > > > > I don't understand any of these, so unless you post the code > > that you want help with as I asked above, I'll assume that you find > > the solution yourself and don't need a callback ;-) > > Some of them are, > > - fsmc NAND controller > > -- include/linux/mtd/fsmc.h > > struct fsmc_nand_platform_data { > ... > > void (*select_bank)(uint32_t bank, uint32_t busw); > > ... > > }; > > > -- arch/arm/mach-spear13xx/spear13xx.c > > void nand_select_bank(u32 bank, u32 busw) > { > u32 fsmc_cfg; > > if (cpu_is_spear1340()) { > #ifdef CONFIG_CPU_SPEAR1340 > fsmc_cfg = readl(VA_SPEAR1340_FSMC_CFG); > #endif > } else > fsmc_cfg = readl(VA_FSMC_CFG); > > fsmc_cfg &= ~(NAND_BANK_MASK << NAND_BANK_SHIFT); > fsmc_cfg |= (bank << NAND_BANK_SHIFT); > > if (busw) > fsmc_cfg |= 1 << NAND_DEV_WIDTH16; > else > fsmc_cfg &= ~(1 << NAND_DEV_WIDTH16); > > if (cpu_is_spear1340()) { > #ifdef CONFIG_CPU_SPEAR1340 > writel(fsmc_cfg, VA_SPEAR1340_FSMC_CFG); > #endif > } else > writel(fsmc_cfg, VA_FSMC_CFG); > } The FSMC driver is ST Microelectronics specific, and this function looks fairly straightforward. I think this can easily go into the driver, with a few modifications: * Find the FSMC_CFG address using the device tree. You should never have any hardcoded virtual addresses like the one in this file, but instead ioremap a resource that gets passed from the device tree. Where is this register located? I assume that it's not part of the normal fsmc registers, but how to get to this address depends a bit on where it's located. * Remove the cpu_is_spear1340() hacks. If you have to tell the difference between variations, make it depend on different "compatible" properties on the presence or absence of some other property or register range. > > > -- arch/arm/mach-spear13xx/spear1340.c > > void sata_miphy_exit(struct device *dev) > { > writel(0, VA_SPEAR1340_PCIE_SATA_CFG); > writel(0, VA_SPEAR1340_PCIE_MIPHY_CFG); > > /* Enable PCIE SATA Controller reset */ > writel((readl(VA_SPEAR1340_PERIP1_SW_RST) | (0x1000)), > VA_SPEAR1340_PERIP1_SW_RST); > msleep(20); > > /* Switch off sata power domain */ > writel((readl(VA_SPEAR1340_PCM_CFG) & (~0x800)), > VA_SPEAR1340_PCM_CFG); > msleep(20); > } Again, you should move the accesses to VA_SPEAR1340_PERIP1_SW_RST etc to some device driver that deals with the specific register range. This can be the driver for the actual peripheral itself, or it can be some kind of "system controller" that exports an interface. In the case of power domains and getting stuff in and out of reset, the subsystems that you most likely should be using do do this are the regulator and the powerdomain framework. Regulators already have device tree bindings, while the power domains still need them. Arnd
diff --git a/arch/arm/mach-spear13xx/include/mach/generic.h b/arch/arm/mach-spear13xx/include/mach/generic.h index dac57fd..8c8fbaa 100644 --- a/arch/arm/mach-spear13xx/include/mach/generic.h +++ b/arch/arm/mach-spear13xx/include/mach/generic.h @@ -15,6 +15,7 @@ #define __MACH_GENERIC_H #include <linux/dmaengine.h> +#include <linux/platform_device.h> #include <asm/mach/time.h> /* Add spear13xx structure declarations here */ @@ -31,6 +32,7 @@ void __init spear13xx_map_io(void); void __init spear13xx_dt_init_irq(void); void __init spear13xx_l2x0_init(void); bool dw_dma_filter(struct dma_chan *chan, void *slave); +int spear13xx_eth_phy_clk_cfg(struct platform_device *pdev); void spear_restart(char, const char *); void spear13xx_secondary_startup(void); diff --git a/arch/arm/mach-spear13xx/include/mach/spear.h b/arch/arm/mach-spear13xx/include/mach/spear.h index 65f27de..b0b6f91 100644 --- a/arch/arm/mach-spear13xx/include/mach/spear.h +++ b/arch/arm/mach-spear13xx/include/mach/spear.h @@ -46,6 +46,7 @@ #define DMAC0_BASE UL(0xEA800000) #define DMAC1_BASE UL(0xEB000000) #define MCIF_CF_BASE UL(0xB2800000) +#define SPEAR13XX_GETH_BASE UL(0xE2000000) /* Devices present in SPEAr1310 */ #ifdef CONFIG_MACH_SPEAR1310 diff --git a/arch/arm/mach-spear13xx/spear1340.c b/arch/arm/mach-spear13xx/spear1340.c index 81e4ed7..ab282ed 100644 --- a/arch/arm/mach-spear13xx/spear1340.c +++ b/arch/arm/mach-spear13xx/spear1340.c @@ -18,6 +18,9 @@ #include <linux/delay.h> #include <linux/dw_dmac.h> #include <linux/of_platform.h> +#include <linux/phy.h> +#include <linux/platform_device.h> +#include <linux/stmmac.h> #include <asm/hardware/gic.h> #include <asm/mach/arch.h> #include <mach/dma.h> @@ -100,6 +103,34 @@ static struct amba_pl011_data uart1_data = { .dma_rx_param = &uart1_dma_param[1], }; +/* Ethernet platform data */ +static struct stmmac_mdio_bus_data mdio0_private_data = { + .bus_id = 0, + .phy_mask = 0, +}; + +static struct stmmac_dma_cfg dma0_private_data = { + .pbl = 16, + .fixed_burst = 1, + .burst_len = DMA_AXI_BLEN_ALL, +}; + +static struct plat_stmmacenet_data eth_data = { + .bus_id = 0, + .phy_addr = -1, + .interface = PHY_INTERFACE_MODE_RGMII, + .has_gmac = 1, + .enh_desc = 1, + .tx_coe = 1, + .dma_cfg = &dma0_private_data, + .rx_coe = STMMAC_RX_COE_TYPE2, + .bugged_jumbo = 1, + .pmt = 1, + .mdio_bus_data = &mdio0_private_data, + .init = spear13xx_eth_phy_clk_cfg, + .clk_csr = STMMAC_CSR_150_250M, +}; + /* SATA device registration */ static int sata_miphy_init(struct device *dev, void __iomem *addr) { @@ -166,6 +197,7 @@ static struct of_dev_auxdata spear1340_auxdata_lookup[] __initdata = { OF_DEV_AUXDATA("snps,spear-ahci", SPEAR1340_SATA_BASE, NULL, &sata_pdata), OF_DEV_AUXDATA("arm,pl011", SPEAR1340_UART1_BASE, NULL, &uart1_data), + OF_DEV_AUXDATA("st,spear600-gmac", SPEAR13XX_GETH_BASE, NULL, ð_data), {} }; diff --git a/arch/arm/mach-spear13xx/spear13xx.c b/arch/arm/mach-spear13xx/spear13xx.c index cf936b1..9ab3b47 100644 --- a/arch/arm/mach-spear13xx/spear13xx.c +++ b/arch/arm/mach-spear13xx/spear13xx.c @@ -18,6 +18,9 @@ #include <linux/dw_dmac.h> #include <linux/err.h> #include <linux/of_irq.h> +#include <linux/phy.h> +#include <linux/platform_device.h> +#include <linux/stmmac.h> #include <asm/hardware/cache-l2x0.h> #include <asm/hardware/gic.h> #include <asm/mach/map.h> @@ -80,6 +83,107 @@ struct dw_dma_platform_data dmac_plat_data = { .chan_priority = CHAN_PRIORITY_DESCENDING, }; +/* Ethernet clock initialization */ +int spear13xx_eth_phy_clk_cfg(struct platform_device *pdev) +{ + int ret; + struct clk *input_clk, *input_pclk, *phy_pclk, *phy_clk; + struct plat_stmmacenet_data *pdata = dev_get_platdata(&pdev->dev); + const char *phy_clk_src_name[] = { + "phy_input_mclk", + "phy_syn_gclk", + }; + const char *input_clk_src_name[] = { + "pll2_clk", + "gmii_pad_clk", + "osc_25m_clk", + }; + const char *phy_clk_name[] = { + "stmmacphy.0" + }; + + if (!pdata) + return -EINVAL; + + /* Get the Pll-2 Clock as parent for PHY Input Clock Source */ + input_pclk = clk_get(NULL, input_clk_src_name[0]); + if (IS_ERR(input_pclk)) { + ret = PTR_ERR(input_pclk); + goto fail_get_input_pclk; + } + + /* + * Get the Phy Input clock source as parent for Phy clock. Default + * selection is gmac_phy_input_clk. This selection would be driving both + * the synthesizer and phy clock. + */ + input_clk = clk_get(NULL, phy_clk_src_name[0]); + if (IS_ERR(input_clk)) { + ret = PTR_ERR(input_clk); + goto fail_get_input_clk; + } + + /* Fetch the phy clock */ + phy_clk = clk_get(NULL, phy_clk_name[pdata->bus_id]); + if (IS_ERR(phy_clk)) { + ret = PTR_ERR(phy_clk); + goto fail_get_phy_clk; + } + + /* Set the pll-2 to 125 MHz */ + ret = clk_set_rate(input_pclk, 125000000); + if (IS_ERR_VALUE(ret)) { + pr_err("%s:couldn't set rate for input phy clk\n", __func__); + goto fail_set_rate; + } + + /* Set the Pll-2 as parent for gmac_phy_input_clk */ + ret = clk_set_parent(input_clk, input_pclk); + if (IS_ERR_VALUE(ret)) { + pr_err("%s:couldn't set parent for inout phy clk \n", __func__); + goto fail_set_rate; + } + if (pdata->interface == PHY_INTERFACE_MODE_RMII) { + /* + * For the rmii interface select gmac_phy_synth_clk + * as the parent and set the clock to 50 Mhz + */ + phy_pclk = clk_get(NULL, phy_clk_src_name[1]); + ret = clk_set_rate(phy_pclk, 50000000); + if (IS_ERR_VALUE(ret)) { + pr_err("%s:couldn't set rate for phy synth clk\n", + __func__); + goto fail_set_rate; + } + } else { + /* + * Set the gmac_phy_input_clk as the parent, + * and pll-2 is already running as parent of + * gmac_phy_input_clk at 125 Mhz + */ + phy_pclk = input_clk; + } + + /* Select the parent for phy clock */ + ret = clk_set_parent(phy_clk, phy_pclk); + if (IS_ERR_VALUE(ret)) { + pr_err("%s:couldn't set parent for phy clk \n", __func__); + goto fail_set_rate; + } + + ret = clk_prepare_enable(phy_clk); + + return ret; +fail_set_rate: + clk_put(phy_clk); +fail_get_phy_clk: + clk_put(input_clk); +fail_get_input_clk: + clk_put(input_pclk); +fail_get_input_pclk: + return ret; +} + void __init spear13xx_l2x0_init(void) { /*
Use AUXDATA to pass platform data for Ethernet controller. Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> --- arch/arm/mach-spear13xx/include/mach/generic.h | 2 + arch/arm/mach-spear13xx/include/mach/spear.h | 1 + arch/arm/mach-spear13xx/spear1340.c | 32 +++++++ arch/arm/mach-spear13xx/spear13xx.c | 104 ++++++++++++++++++++++++ 4 files changed, 139 insertions(+), 0 deletions(-)