| Message ID | 1352446306-19945-1-git-send-email-horms@verge.net.au |
|---|---|
| State | New |
| Headers | show |
On Friday 09 November 2012, Simon Horman wrote: > Hi Olof, Hi Arnd, > > please consider the following board enhancements for 3.8. > > * This pull request is based on a merge of > a) The renesas/boards branch of the arm-soc tree > b) The soc branch of the renesas tree, > which I have sent a separate pull requst for > > * "sh: clkfwk: add sh_clk_fsidiv_register()" is a driver patch > which is a dependency of > - ARM: shmobile: sh7372: use sh_clk_fsidiv_register() for FSI-DIV clocks > - ARM: shmobile: r8a7740: add FSI-DVI clocks > I have spoken to the driver maintainer, Paul Mundt, and he has indicated > that he thinks it is best to merge all in one go and acked the patch for > inclusion in this pull-request. > > * The following patches > - ARM: shmobile: use FSI driver's audio clock on ap4evb > - ARM: shmobile: use FSI driver's audio clock on mackerel > - ARM: shmobile: use FSI driver's audio clock on armadillo800eva > Have a compile-time dependency on the following patch which is present in > the for-next branch of Mark Brown's sound tree on kernel.org > - ASoC: fsi: add master clock control functions > (ab6f6d85210c4d0265cf48e9958c04e08595055a) Hi Simon, I've pulled this into a new next/boards2 branch because of the new dependency, but I'm not entirely happy with the way that the dependency came in through your tree. It's generally ok to have external dependencies where they can't be avoided, but please remember these rules: * When you send a branch that has an external dependency, actually base your branch on top of the other one, so that it can be independently verified. If you have a dependency and send your patches without that one included, it's clear that your code can't be tested in the version you are sending, and it breaks any attempt to test just the arm-soc tree or your branch rather than the entire for-next tree. * Make sure that the branch you depend on will not get rebased before it gets submitted to the mainline kernel. * Always let the person that owns the dependency know that the changes in their tree are also included elsewhere and that things go bad if those changes get rebased after all, or won't make it into the merge window for some reason. I have taken Mark on Cc to let him know about the dependency now, and I've merged ab6f6d85210c4d0265cf48e9958c04e08595055a (which has only shmobile specific ASoC patches) into the next/boards2 branch before merging your branch. This is still not perfect because it breaks bisection, but it's the best I could do aside from forcing you do do another round-trip. Arnd
On Mon, Nov 12, 2012 at 09:11:03PM +0000, Arnd Bergmann wrote: > On Friday 09 November 2012, Simon Horman wrote: > > Hi Olof, Hi Arnd, > > > > please consider the following board enhancements for 3.8. > > > > * This pull request is based on a merge of > > a) The renesas/boards branch of the arm-soc tree > > b) The soc branch of the renesas tree, > > which I have sent a separate pull requst for > > > > * "sh: clkfwk: add sh_clk_fsidiv_register()" is a driver patch > > which is a dependency of > > - ARM: shmobile: sh7372: use sh_clk_fsidiv_register() for FSI-DIV clocks > > - ARM: shmobile: r8a7740: add FSI-DVI clocks > > I have spoken to the driver maintainer, Paul Mundt, and he has indicated > > that he thinks it is best to merge all in one go and acked the patch for > > inclusion in this pull-request. > > > > * The following patches > > - ARM: shmobile: use FSI driver's audio clock on ap4evb > > - ARM: shmobile: use FSI driver's audio clock on mackerel > > - ARM: shmobile: use FSI driver's audio clock on armadillo800eva > > Have a compile-time dependency on the following patch which is present in > > the for-next branch of Mark Brown's sound tree on kernel.org > > - ASoC: fsi: add master clock control functions > > (ab6f6d85210c4d0265cf48e9958c04e08595055a) > > Hi Simon, > > I've pulled this into a new next/boards2 branch because of the new dependency, > but I'm not entirely happy with the way that the dependency came in through > your tree. > > It's generally ok to have external dependencies where they can't be avoided, > but please remember these rules: > > * When you send a branch that has an external dependency, actually base your > branch on top of the other one, so that it can be independently verified. > If you have a dependency and send your patches without that one included, > it's clear that your code can't be tested in the version you are sending, > and it breaks any attempt to test just the arm-soc tree or your branch > rather than the entire for-next tree. > > * Make sure that the branch you depend on will not get rebased before it > gets submitted to the mainline kernel. > > * Always let the person that owns the dependency know that the changes in > their tree are also included elsewhere and that things go bad if those > changes get rebased after all, or won't make it into the merge window > for some reason. > > I have taken Mark on Cc to let him know about the dependency now, and I've > merged ab6f6d85210c4d0265cf48e9958c04e08595055a (which has only shmobile > specific ASoC patches) into the next/boards2 branch before merging your > branch. This is still not perfect because it breaks bisection, but it's > the best I could do aside from forcing you do do another round-trip. Thanks and sorry about that. I'll be more careful next time.
On Mon, Nov 12, 2012 at 09:11:03PM +0000, Arnd Bergmann wrote: > * Always let the person that owns the dependency know that the changes in > their tree are also included elsewhere and that things go bad if those > changes get rebased after all, or won't make it into the merge window > for some reason. Please never pull anything from me unless there's a signed tag for it, anything else I won't know about and may be rebased or vanish or something. > I have taken Mark on Cc to let him know about the dependency now, and I've > merged ab6f6d85210c4d0265cf48e9958c04e08595055a (which has only shmobile > specific ASoC patches) into the next/boards2 branch before merging your > branch. This is still not perfect because it breaks bisection, but it's > the best I could do aside from forcing you do do another round-trip. What's the bisection problem?
On Tuesday 13 November 2012, Mark Brown wrote: > Show Details > On Mon, Nov 12, 2012 at 09:11:03PM +0000, Arnd Bergmann wrote: > > > * Always let the person that owns the dependency know that the changes in > > their tree are also included elsewhere and that things go bad if those > > changes get rebased after all, or won't make it into the merge window > > for some reason. > > Please never pull anything from me unless there's a signed tag for it, > anything else I won't know about and may be rebased or vanish or > something. Ok, thanks for the reminder. Do you have a signed tag containing ab6f6d8521? > > I have taken Mark on Cc to let him know about the dependency now, and I've > > merged ab6f6d85210c4d0265cf48e9958c04e08595055a (which has only shmobile > > specific ASoC patches) into the next/boards2 branch before merging your > > branch. This is still not perfect because it breaks bisection, but it's > > the best I could do aside from forcing you do do another round-trip. > > What's the bisection problem? I merged ab6f6d8521 first, and then Simon's branch, which means that the linear (first-parent) history is ok, but bisecting the entire state of the tree may end up in a commit in his branch in a state before it was merged into mine, where it doesn't contain the patches it depends on, and as Simon mentioned in his pull request, that will cause a build failure. Arnd
On Tue, Nov 13, 2012 at 08:48:03AM +0000, Arnd Bergmann wrote: > On Tuesday 13 November 2012, Mark Brown wrote: > > Please never pull anything from me unless there's a signed tag for it, > > anything else I won't know about and may be rebased or vanish or > > something. > Ok, thanks for the reminder. Do you have a signed tag containing ab6f6d8521? Nope, your e-mail was the first I'd heard of the cross-merge. I guess I could create one if this is going to get redone for some reason... > > > I have taken Mark on Cc to let him know about the dependency now, and I've > > > merged ab6f6d85210c4d0265cf48e9958c04e08595055a (which has only shmobile > > > specific ASoC patches) into the next/boards2 branch before merging your > > > branch. This is still not perfect because it breaks bisection, but it's > > > the best I could do aside from forcing you do do another round-trip. > > What's the bisection problem? > I merged ab6f6d8521 first, and then Simon's branch, which means that the > linear (first-parent) history is ok, but bisecting the entire state of > the tree may end up in a commit in his branch in a state before it was > merged into mine, where it doesn't contain the patches it depends on, > and as Simon mentioned in his pull request, that will cause a build > failure. Oh, I'd missed the fact that this wasn't coming in via a merge into Simon's tree.
On Tuesday 13 November 2012, Mark Brown wrote: > On Tue, Nov 13, 2012 at 08:48:03AM +0000, Arnd Bergmann wrote: > > On Tuesday 13 November 2012, Mark Brown wrote: > > > > Please never pull anything from me unless there's a signed tag for it, > > > anything else I won't know about and may be rebased or vanish or > > > something. > > > Ok, thanks for the reminder. Do you have a signed tag containing ab6f6d8521? > > Nope, your e-mail was the first I'd heard of the cross-merge. I guess I > could create one if this is going to get redone for some reason... I'm not planning to pull this one into somewhere else, just want to make sure that you remeber it's been pulled into arm-soc. You mentioned above that having a tag helps you with your bookkeeping. Arnd
On Tue, Nov 13, 2012 at 10:27:10AM +0000, Arnd Bergmann wrote: > I'm not planning to pull this one into somewhere else, just want to make > sure that you remeber it's been pulled into arm-soc. You mentioned above > that having a tag helps you with your bookkeeping. It's not that it's particularly helpful for my bookkeeping (I rarely if ever look at the tags), it's more that if I've not signed a tag then it means that I'm either not aware of or not OK with the branch being used.
Hello. On 11/09/2012 10:31 AM, Simon Horman wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > This patch supports CN21/CN22 USB 2.0 (port 0/1/2), > and enable USB momery on defconfig Sorry for late comment but I'm studying this code just now (in order to add R-Car M1A USB support). > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Signed-off-by: Simon Horman <horms@verge.net.au> > --- > arch/arm/configs/marzen_defconfig | 6 ++ > arch/arm/mach-shmobile/Kconfig | 1 + > arch/arm/mach-shmobile/board-marzen.c | 108 ++++++++++++++++++++++++++++++++- > 3 files changed, 114 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c > index 74c7f0b..707b3bd 100644 > --- a/arch/arm/mach-shmobile/board-marzen.c > +++ b/arch/arm/mach-shmobile/board-marzen.c > @@ -34,6 +34,9 @@ [...] > @@ -172,6 +175,101 @@ static struct platform_device *marzen_devices[] __initdata = { > &usb_phy_device, > }; > > +/* USB */ > +static struct usb_phy *phy; > +static int usb_power_on(struct platform_device *pdev) > +{ > + if (!phy) > + return -EIO; > + > + pm_runtime_enable(&pdev->dev); > + pm_runtime_get_sync(&pdev->dev); > + > + usb_phy_init(phy); > + > + return 0; > +} > + > +static void usb_power_off(struct platform_device *pdev) > +{ > + if (!phy) > + return; > + > + usb_phy_shutdown(phy); > + > + pm_runtime_put_sync(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > +} > + > +static struct usb_ehci_pdata ehcix_pdata = { > + .power_on = usb_power_on, > + .power_off = usb_power_off, > + .power_suspend = usb_power_off, > +}; > + > +static struct resource ehci0_resources[] = { > + [0] = { > + .start = 0xffe70000, > + .end = 0xffe70400 - 1, > + .flags = IORESOURCE_MEM, > + }, > + [1] = { > + .start = gic_spi(44), > + .flags = IORESOURCE_IRQ, > + }, > +}; > + > +static struct platform_device ehci0_device = { > + .name = "ehci-platform", > + .id = 0, > + .dev = { > + .dma_mask = &ehci0_device.dev.coherent_dma_mask, > + .coherent_dma_mask = 0xffffffff, > + .platform_data = &ehcix_pdata, > + }, > + .num_resources = ARRAY_SIZE(ehci0_resources), > + .resource = ehci0_resources, > +}; > + > +static struct resource ehci1_resources[] = { > + [0] = { > + .start = 0xfff70000, > + .end = 0xfff70400 - 1, > + .flags = IORESOURCE_MEM, > + }, > + [1] = { > + .start = gic_spi(45), > + .flags = IORESOURCE_IRQ, > + }, > +}; > + > +static struct platform_device ehci1_device = { > + .name = "ehci-platform", > + .id = 1, > + .dev = { > + .dma_mask = &ehci1_device.dev.coherent_dma_mask, > + .coherent_dma_mask = 0xffffffff, > + .platform_data = &ehcix_pdata, > + }, > + .num_resources = ARRAY_SIZE(ehci1_resources), > + .resource = ehci1_resources, > +}; > + > +static struct platform_device *marzen_late_devices[] __initdata = { > + &ehci0_device, > + &ehci1_device, > +}; > + > +void __init marzen_init_late(void) > +{ > + /* get usb phy */ > + phy = usb_get_phy(USB_PHY_TYPE_USB2); > + > + shmobile_init_late(); > + platform_add_devices(marzen_late_devices, > + ARRAY_SIZE(marzen_late_devices)); > +} > + > Morimoto-san, I don't understand why this SoC specific platform device ended up in the board file? Could you explain your reasons please? I think this is generally a bad practice as this approach scales badly. WVR, Sergei
Hello. On 11/09/2012 10:31 AM, Simon Horman wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Signed-off-by: Simon Horman <horms@verge.net.au> > --- > arch/arm/configs/marzen_defconfig | 3 ++- > arch/arm/mach-shmobile/board-marzen.c | 21 +++++++++++++++++++++ > 2 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-shmobile/board-marzen.c b/arch/arm/mach-shmobile/board-marzen.c > index 69f7f46..74c7f0b 100644 > --- a/arch/arm/mach-shmobile/board-marzen.c > +++ b/arch/arm/mach-shmobile/board-marzen.c > @@ -144,11 +144,32 @@ static struct platform_device hspi_device = { > .num_resources = ARRAY_SIZE(hspi_resources), > }; > > +/* USB PHY */ > +static struct resource usb_phy_resources[] = { > + [0] = { > + .start = 0xffe70000, Why not 0xffe70800? 0xffe70000 is where the EHCI registers start. I don't see why they should be overlapped like this. > + .end = 0xffe70900 - 1, > + .flags = IORESOURCE_MEM, > + }, > + [1] = { > + .start = 0xfff70000, > + .end = 0xfff70900 - 1, Hm, this PHY register range doesn't seem to be documented? > + .flags = IORESOURCE_MEM, > + }, > +}; > + > +static struct platform_device usb_phy_device = { > + .name = "rcar_usb_phy", > + .resource = usb_phy_resources, > + .num_resources = ARRAY_SIZE(usb_phy_resources), > +}; > + > static struct platform_device *marzen_devices[] __initdata = { > ð_device, > &sdhi0_device, > &thermal_device, > &hspi_device, > + &usb_phy_device, > }; > > static void __init marzen_init(void) Finally, the same question as for EHCI: why this ended up in the board file instead of setup-r8a7779.c? WBR, Sergei
Hi Sergei > > +/* USB */ > > +static struct usb_phy *phy; > > +static int usb_power_on(struct platform_device *pdev) > > +{ > > + if (!phy) > > + return -EIO; > > + > > + pm_runtime_enable(&pdev->dev); > > + pm_runtime_get_sync(&pdev->dev); > > + > > + usb_phy_init(phy); > > + > > + return 0; > > +} > > + > > +static void usb_power_off(struct platform_device *pdev) > > +{ > > + if (!phy) > > + return; > > + > > + usb_phy_shutdown(phy); > > + > > + pm_runtime_put_sync(&pdev->dev); > > + pm_runtime_disable(&pdev->dev); > > +} > > + > > +static struct usb_ehci_pdata ehcix_pdata = { > > + .power_on = usb_power_on, > > + .power_off = usb_power_off, > > + .power_suspend = usb_power_off, > > +}; (snip) > Morimoto-san, I don't understand why this SoC specific platform device > ended up in the board file? Could you explain your reasons please? > I think this is generally a bad practice as this approach scales badly. Do you mean it should exist in setup-r8a7779.c ? I forgot detail of it, but these usb is using callback function, and it is using *phy*. This phy came from marzen_init_late() with usb_get_phy(USB_PHY_TYPE_USB2); This usb_get_phy() is not needed if board doesn't have USB. You can modify it if you want Best regards --- Kuninori Morimoto
Hi Sergei > > +/* USB PHY */ > > +static struct resource usb_phy_resources[] = { > > + [0] = { > > + .start = 0xffe70000, > > Why not 0xffe70800? 0xffe70000 is where the EHCI registers start. > I don't see why they should be overlapped like this. Current rcar-phy try to access 0xffe7009x > > + [1] = { > > + .start = 0xfff70000, > > + .end = 0xfff70900 - 1, > > Hm, this PHY register range doesn't seem to be documented? 0xffe70000 is for port0/1 0xfff70000 is for port2 But this 0xfff70000 is only for 0xfff7009x access Best regards --- Kuninori Morimoto
Hi Sergei > > Morimoto-san, I don't understand why this SoC specific platform device > > ended up in the board file? Could you explain your reasons please? > > I think this is generally a bad practice as this approach scales badly. > > Do you mean it should exist in setup-r8a7779.c ? > > I forgot detail of it, but these usb is using callback function, > and it is using *phy*. > This phy came from marzen_init_late() with usb_get_phy(USB_PHY_TYPE_USB2); > This usb_get_phy() is not needed if board doesn't have USB. > > You can modify it if you want And, as you pointed in USB ML, This rcar_usb_phy driver will need board specific initialization. (it is not implemented at this point...) Best regards --- Kuninori Morimoto
Hello. On 14-03-2013 4:44, Kuninori Morimoto wrote: >>> +/* USB PHY */ >>> +static struct resource usb_phy_resources[] = { >>> + [0] = { >>> + .start = 0xffe70000, >> Why not 0xffe70800? 0xffe70000 is where the EHCI registers start. >> I don't see why they should be overlapped like this. > Current rcar-phy try to access 0xffe7009x That should be changed too IHO, as these registers are not really PHY related, they control EHCI itself. These writes should be done somewhere in setup-r8a7779.c probably. >>> + [1] = { >>> + .start = 0xfff70000, >>> + .end = 0xfff70900 - 1, >> Hm, this PHY register range doesn't seem to be documented? > 0xffe70000 is for port0/1 > 0xfff70000 is for port2 > But this 0xfff70000 is only for 0xfff7009x access Yeah, I understood that it's only for the extra EHCI registers. > Best regards > --- > Kuninori Morimoto WBR, Sergei
Hello. On 14-03-2013 4:29, Kuninori Morimoto wrote: >>> +/* USB */ >>> +static struct usb_phy *phy; >>> +static int usb_power_on(struct platform_device *pdev) >>> +{ >>> + if (!phy) >>> + return -EIO; >>> + >>> + pm_runtime_enable(&pdev->dev); >>> + pm_runtime_get_sync(&pdev->dev); >>> + >>> + usb_phy_init(phy); >>> + >>> + return 0; >>> +} >>> + >>> +static void usb_power_off(struct platform_device *pdev) >>> +{ >>> + if (!phy) >>> + return; >>> + >>> + usb_phy_shutdown(phy); >>> + >>> + pm_runtime_put_sync(&pdev->dev); >>> + pm_runtime_disable(&pdev->dev); >>> +} >>> + >>> +static struct usb_ehci_pdata ehcix_pdata = { >>> + .power_on = usb_power_on, >>> + .power_off = usb_power_off, >>> + .power_suspend = usb_power_off, >>> +}; > (snip) >> Morimoto-san, I don't understand why this SoC specific platform device >> ended up in the board file? Could you explain your reasons please? >> I think this is generally a bad practice as this approach scales badly. > Do you mean it should exist in setup-r8a7779.c ? Yes. > I forgot detail of it, but these usb is using callback function, > and it is using *phy*. But this PHY also belongs to SoC. > This phy came from marzen_init_late() with usb_get_phy(USB_PHY_TYPE_USB2); > This usb_get_phy() is not needed if board doesn't have USB. Anyway, there should be ways to separate the board specific platform code and the SoC specific one. That's what other subarches do. > You can modify it if you want Yes, I definitely would like to try. WBR, Sergei
Hi Sergei > > I forgot detail of it, but these usb is using callback function, > > and it is using *phy*. > > But this PHY also belongs to SoC. > > > This phy came from marzen_init_late() with usb_get_phy(USB_PHY_TYPE_USB2); > > This usb_get_phy() is not needed if board doesn't have USB. > > Anyway, there should be ways to separate the board specific platform code > and the SoC specific one. That's what other subarches do. > > > You can modify it if you want > > Yes, I definitely would like to try. I checked CPG :: MSTP for USB0/1/2 and USB Host/Function, and GIC number for it. It seems no conflict each other I guess EHCI/OHCI goes to setup seems no problem. But, as you pointed on USB ML, USB-PHY :: USBPCTRL0 depends on platform (Host/Function selection, and OVC pin setting) rcar_usb_phy driver belongs to platform Best regards --- Kuninori Morimoto
Hello. On 15-03-2013 4:52, Kuninori Morimoto wrote: >>> I forgot detail of it, but these usb is using callback function, >>> and it is using *phy*. >> But this PHY also belongs to SoC. >>> This phy came from marzen_init_late() with usb_get_phy(USB_PHY_TYPE_USB2); >>> This usb_get_phy() is not needed if board doesn't have USB. >> Anyway, there should be ways to separate the board specific platform code >> and the SoC specific one. That's what other subarches do. >>> You can modify it if you want >> Yes, I definitely would like to try. > I checked CPG :: MSTP for USB0/1/2 and USB Host/Function, > and GIC number for it. > It seems no conflict each other > I guess EHCI/OHCI goes to setup seems no problem. > But, as you pointed on USB ML, > USB-PHY :: USBPCTRL0 depends on platform > (Host/Function selection, and OVC pin setting) > rcar_usb_phy driver belongs to platform No, it doesn't. Only it's (future) platform data will belong to the board -- that's what they were designed for. WBR, Sergei
Hi Olof, Hi Arnd, please consider the following board enhancements for 3.8. * This pull request is based on a merge of a) The renesas/boards branch of the arm-soc tree b) The soc branch of the renesas tree, which I have sent a separate pull requst for * "sh: clkfwk: add sh_clk_fsidiv_register()" is a driver patch which is a dependency of - ARM: shmobile: sh7372: use sh_clk_fsidiv_register() for FSI-DIV clocks - ARM: shmobile: r8a7740: add FSI-DVI clocks I have spoken to the driver maintainer, Paul Mundt, and he has indicated that he thinks it is best to merge all in one go and acked the patch for inclusion in this pull-request. * The following patches - ARM: shmobile: use FSI driver's audio clock on ap4evb - ARM: shmobile: use FSI driver's audio clock on mackerel - ARM: shmobile: use FSI driver's audio clock on armadillo800eva Have a compile-time dependency on the following patch which is present in the for-next branch of Mark Brown's sound tree on kernel.org - ASoC: fsi: add master clock control functions (ab6f6d85210c4d0265cf48e9958c04e08595055a) ---------------------------------------------------------------- The following changes since commit ab3ff12a78a64b851ab22726ab99dca6ad37bf94: Merge branch 'soc' into boards (2012-11-08 17:49:35 +0900) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git boards for you to fetch changes up to 12818d82ccad9868a820dbb193b4f9b797cd02d4: ARM: shmobile: use FSI driver's audio clock on ap4evb (2012-11-08 17:52:05 +0900) ---------------------------------------------------------------- Kuninori Morimoto (7): ARM: shmobile: marzen: add USB phy support ARM: shmobile: marzen: add USB EHCI driver support ARM: shmobile: marzen: add USB OHCI driver support ARM: shmobile: mackerel: enable DMAEngine on USB Host ARM: shmobile: use FSI driver's audio clock on armadillo800eva ARM: shmobile: use FSI driver's audio clock on mackerel ARM: shmobile: use FSI driver's audio clock on ap4evb arch/arm/configs/marzen_defconfig | 14 +- arch/arm/mach-shmobile/Kconfig | 2 + arch/arm/mach-shmobile/board-ap4evb.c | 139 +----------------- arch/arm/mach-shmobile/board-armadillo800eva.c | 38 +---- arch/arm/mach-shmobile/board-mackerel.c | 74 +--------- arch/arm/mach-shmobile/board-marzen.c | 186 +++++++++++++++++++++++- 6 files changed, 205 insertions(+), 248 deletions(-)