| Message ID | 1392736109-3981-1-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 18 February 2014 16:08:29 Thomas Petazzoni wrote: > From: Marcin Wojtas <mw@semihalf.com> > > The SDHCI unit used on the Armada 380 and 385 Marvell SoC is similar > to the PXAv3 unit. The only difference is that on Armada 38x, the > PXAv3 unit accesses memory through MBus windows which must be > configured prior to using the device. Without this, DMA would not > work. > > In order to achieve this, the sdhci-pxav3 driver is extended with an > additional compatible string "marvell,armada-380-sdhci". When this > compatible string is used, the MBus windows are initialized in a way > that is identical to what all other DMA-capable drivers for Marvell > EBU platforms do. It seems odd to do this in the sdhci driver, when the configuration is done in registers that belong to mbus. > +/* > + * These registers are relative to the second register region, for the > + * MBus bridge. > + */ > +#define SDHCI_WINDOW_CTRL(i) (0x80 + ((i) << 3)) > +#define SDHCI_WINDOW_BASE(i) (0x84 + ((i) << 3)) > +#define SDHCI_MAX_WIN_NUM 8 These look similar to the outbound mbus windows that are used for MMIO, but it's not really clear from the code what they really do. > + for (i = 0; i < SDHCI_MAX_WIN_NUM; i++) { > + writel(0, regs + SDHCI_WINDOW_CTRL(i)); > + writel(0, regs + SDHCI_WINDOW_BASE(i)); > + } > + > + for (i = 0; i < dram->num_cs; i++) { > + const struct mbus_dram_window *cs = dram->cs + i; > + > + /* Write size, attributes and target id to control register > */ + writel(((cs->size - 1) & 0xffff0000) | > + (cs->mbus_attr << 8) | > + (dram->mbus_dram_target_id << 4) | 1, > + regs + SDHCI_WINDOW_CTRL(i)); > + /* Write base address to base register */ > + writel(cs->base, regs + SDHCI_WINDOW_BASE(i)); > + } Accessing the mbus_dram_window() is also something that seems to fit better into the mbus driver. I assume there are more the same register ranges for each bus master behind mbus (PCI being special again). How about adding an exported function to the mbus driver that sets up all the windows for one bus master correctly, passing only the number of the bus master? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Arnd Bergmann, On Tue, 18 Feb 2014 19:02:47 +0100, Arnd Bergmann wrote: > > In order to achieve this, the sdhci-pxav3 driver is extended with an > > additional compatible string "marvell,armada-380-sdhci". When this > > compatible string is used, the MBus windows are initialized in a way > > that is identical to what all other DMA-capable drivers for Marvell > > EBU platforms do. > > It seems odd to do this in the sdhci driver, when the configuration > is done in registers that belong to mbus. No, those registers don't belong to MBus. They belong to the device itself, as they configure the device -> memory windows. Because SDHCI has a standard register interface, they separated the SDHCI registers (standard) from the registers to configure the device -> Mbus windows (Marvell-specific). But in all other IPs, the device -> memory windows are configured in registers that are just in the middle of the other registers for this device. Examples: * In the mvneta driver, the register definitions: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/marvell/mvneta.c#n57 and the corresponding code: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/marvell/mvneta.c#n2717 * In the XOR driver, the register definitions: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/dma/mv_xor.h#n53 and the corresponding code: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/dma/mv_xor.c#n1116 * In the mvsdio driver, the register definitions: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/mvsdio.h#n66 and the corresponding code: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/mvsdio.c#n658 And there are many more examples throughout the kernel. These registers are in the middle of the other registers for the IP. The SDHCI IP is an exception here. > > > +/* > > + * These registers are relative to the second register region, for the > > + * MBus bridge. > > + */ > > +#define SDHCI_WINDOW_CTRL(i) (0x80 + ((i) << 3)) > > +#define SDHCI_WINDOW_BASE(i) (0x84 + ((i) << 3)) > > +#define SDHCI_MAX_WIN_NUM 8 > > These look similar to the outbound mbus windows that are used for MMIO, > but it's not really clear from the code what they really do. There are two completely different mechanisms: * The CPU -> { memory, device } windows. These windows are managed by the mvebu-mbus driver, as they are configured using global registers, owned by the mvebu-driver. * The device -> memory windows. These windows are needed for a given device to access memory in order to do DMA. These windows are configured through registers that are part of each peripheral register area. > > > + for (i = 0; i < SDHCI_MAX_WIN_NUM; i++) { > > + writel(0, regs + SDHCI_WINDOW_CTRL(i)); > > + writel(0, regs + SDHCI_WINDOW_BASE(i)); > > + } > > + > > + for (i = 0; i < dram->num_cs; i++) { > > + const struct mbus_dram_window *cs = dram->cs + i; > > + > > + /* Write size, attributes and target id to control register > > */ + writel(((cs->size - 1) & 0xffff0000) | > > + (cs->mbus_attr << 8) | > > + (dram->mbus_dram_target_id << 4) | 1, > > + regs + SDHCI_WINDOW_CTRL(i)); > > + /* Write base address to base register */ > > + writel(cs->base, regs + SDHCI_WINDOW_BASE(i)); > > + } > > Accessing the mbus_dram_window() is also something that seems to fit > better into the mbus driver. > > I assume there are more the same register ranges for each bus master > behind mbus (PCI being special again). How about adding an exported > function to the mbus driver that sets up all the windows for one > bus master correctly, passing only the number of the bus master? This is certainly a possible refactoring, but it involves changing a fairly large number of drivers, since many drivers are using mv_mbus_dram_info() (this function and all the code spread in drivers to configure windows predates the mach-mvebu thing and all the DT conversion). Therefore, I'd like to have the possibility of handling sdhci-pxav3.c like all other drivers for now, and then do a cleanup of this area. Would this be possible? Thanks, Thomas
On Tuesday 18 February 2014 20:26:16 Thomas Petazzoni wrote: > On Tue, 18 Feb 2014 19:02:47 +0100, Arnd Bergmann wrote: > > > > In order to achieve this, the sdhci-pxav3 driver is extended with an > > > additional compatible string "marvell,armada-380-sdhci". When this > > > compatible string is used, the MBus windows are initialized in a way > > > that is identical to what all other DMA-capable drivers for Marvell > > > EBU platforms do. > > > > It seems odd to do this in the sdhci driver, when the configuration > > is done in registers that belong to mbus. > > No, those registers don't belong to MBus. They belong to the device > itself, as they configure the device -> memory windows. > > Because SDHCI has a standard register interface, they separated the > SDHCI registers (standard) from the registers to configure the device > -> Mbus windows (Marvell-specific). > > But in all other IPs, the device -> memory windows are configured in > registers that are just in the middle of the other registers for this > device. Ok, got it. > > > > > +/* > > > + * These registers are relative to the second register region, for the > > > + * MBus bridge. > > > + */ > > > +#define SDHCI_WINDOW_CTRL(i) (0x80 + ((i) << 3)) > > > +#define SDHCI_WINDOW_BASE(i) (0x84 + ((i) << 3)) > > > +#define SDHCI_MAX_WIN_NUM 8 > > > > These look similar to the outbound mbus windows that are used for MMIO, > > but it's not really clear from the code what they really do. > > There are two completely different mechanisms: > > * The CPU -> { memory, device } windows. These windows are managed by > the mvebu-mbus driver, as they are configured using global > registers, owned by the mvebu-driver. > > * The device -> memory windows. These windows are needed for a given > device to access memory in order to do DMA. These windows are > configured through registers that are part of each peripheral > register area. Yes, I understand the difference. The former corresponds to the DT 'ranges' property, while the latter is the 'dma-ranges' property. > > I assume there are more the same register ranges for each bus master > > behind mbus (PCI being special again). How about adding an exported > > function to the mbus driver that sets up all the windows for one > > bus master correctly, passing only the number of the bus master? > > This is certainly a possible refactoring, but it involves changing a > fairly large number of drivers, since many drivers are using > mv_mbus_dram_info() (this function and all the code spread in drivers > to configure windows predates the mach-mvebu thing and all the DT > conversion). Is the layout of the mbus configuration windows in each device the same? > Therefore, I'd like to have the possibility of handling sdhci-pxav3.c > like all other drivers for now, and then do a cleanup of this area. > Would this be possible? Yes, sounds reasonable. Thanks for the clarification. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Arnd Bergmann, On Tue, 18 Feb 2014 20:43:04 +0100, Arnd Bergmann wrote: > > There are two completely different mechanisms: > > > > * The CPU -> { memory, device } windows. These windows are managed by > > the mvebu-mbus driver, as they are configured using global > > registers, owned by the mvebu-driver. > > > > * The device -> memory windows. These windows are needed for a given > > device to access memory in order to do DMA. These windows are > > configured through registers that are part of each peripheral > > register area. > > Yes, I understand the difference. The former corresponds to > the DT 'ranges' property, while the latter is the 'dma-ranges' > property. Hum, possible. I must admit I've never looked at the dma-ranges property. > > > I assume there are more the same register ranges for each bus master > > > behind mbus (PCI being special again). How about adding an exported > > > function to the mbus driver that sets up all the windows for one > > > bus master correctly, passing only the number of the bus master? > > > > This is certainly a possible refactoring, but it involves changing a > > fairly large number of drivers, since many drivers are using > > mv_mbus_dram_info() (this function and all the code spread in drivers > > to configure windows predates the mach-mvebu thing and all the DT > > conversion). > > Is the layout of the mbus configuration windows in each device > the same? The number of windows is different, and for some devices, there are additional registers to poke. The simple example is: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/mvsdio.c#n658 this one has only 4 windows, no remappable windows, no special register to poke. Another example is: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/dma/mv_xor.c#n1116 this one has 8 windows, 4 are remappable, and there are special registers to poke: WINDOW_BAR_ENABLE(x) and WINDOW_OVERRIDE_CTRL(x). Yet another example is: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/marvell/mvneta.c#n2717 this one has 6 windows, 4 are remappable, and there is a special register to poke: MVNETA_BASE_ADDR_ENABLE. So, I believe some refactoring is possible, but we cannot completely eliminate a per-driver handling of some of these registers. > > Therefore, I'd like to have the possibility of handling sdhci-pxav3.c > > like all other drivers for now, and then do a cleanup of this area. > > Would this be possible? > > Yes, sounds reasonable. Great, thanks! > Thanks for the clarification. You're welcome, thanks for reviewing the patches! Thomas
Hello Chris, Do you think it would be possible to have the below patch merged for 3.15 ? It would allow us to enable SDHCI support in the new Marvell Armada 38x SOCs. The patch is fairly simple I believe, just adding an additional compatible string to the driver, and adding a little bit of logic to configure Marvell-specific windows when this compatible string is being used. Thanks a lot! Thomas On Tue, 18 Feb 2014 16:08:29 +0100, Thomas Petazzoni wrote: > From: Marcin Wojtas <mw@semihalf.com> > > The SDHCI unit used on the Armada 380 and 385 Marvell SoC is similar > to the PXAv3 unit. The only difference is that on Armada 38x, the > PXAv3 unit accesses memory through MBus windows which must be > configured prior to using the device. Without this, DMA would not > work. > > In order to achieve this, the sdhci-pxav3 driver is extended with an > additional compatible string "marvell,armada-380-sdhci". When this > compatible string is used, the MBus windows are initialized in a way > that is identical to what all other DMA-capable drivers for Marvell > EBU platforms do. > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > .../devicetree/bindings/mmc/sdhci-pxa.txt | 17 +++++- > drivers/mmc/host/sdhci-pxav3.c | 68 ++++++++++++++++++++++ > 2 files changed, 84 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > index dbe98a3..86223c3 100644 > --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > @@ -4,7 +4,14 @@ This file documents differences between the core properties in mmc.txt > and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers. > > Required properties: > -- compatible: Should be "mrvl,pxav2-mmc" or "mrvl,pxav3-mmc". > +- compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or > + "marvell,armada-380-sdhci". > +- reg: > + * for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for > + the SDHCI registers. > + * for "marvell,armada-380-sdhci", two register areas. The first one > + for the SDHCI registers themselves, and the second one for the > + AXI/Mbus bridge registers of the SDHCI unit. > > Optional properties: > - mrvl,clk-delay-cycles: Specify a number of cycles to delay for tuning. > @@ -19,3 +26,11 @@ sdhci@d4280800 { > non-removable; > mrvl,clk-delay-cycles = <31>; > }; > + > +sdhci@d8000 { > + compatible = "marvell,armada-380-sdhci"; > + reg = <0xd8000 0x1000>, <0xdc000 0x100>; > + interrupts = <0 25 0x4>; > + clocks = <&gateclk 17>; > + mrvl,clk-delay-cycles = <0x1F>; > +}; > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > index 793dacd..2fd73b3 100644 > --- a/drivers/mmc/host/sdhci-pxav3.c > +++ b/drivers/mmc/host/sdhci-pxav3.c > @@ -34,6 +34,7 @@ > #include <linux/of_gpio.h> > #include <linux/pm.h> > #include <linux/pm_runtime.h> > +#include <linux/mbus.h> > > #include "sdhci.h" > #include "sdhci-pltfm.h" > @@ -57,6 +58,60 @@ > #define SDCE_MISC_INT (1<<2) > #define SDCE_MISC_INT_EN (1<<1) > > +/* > + * These registers are relative to the second register region, for the > + * MBus bridge. > + */ > +#define SDHCI_WINDOW_CTRL(i) (0x80 + ((i) << 3)) > +#define SDHCI_WINDOW_BASE(i) (0x84 + ((i) << 3)) > +#define SDHCI_MAX_WIN_NUM 8 > + > +static int mv_conf_mbus_windows(struct platform_device *pdev, > + const struct mbus_dram_target_info *dram) > +{ > + int i; > + void __iomem *regs; > + struct resource *res; > + > + if (!dram) { > + dev_err(&pdev->dev, "no mbus dram info\n"); > + return -EINVAL; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) { > + dev_err(&pdev->dev, "cannot get mbus registers\n"); > + return -EINVAL; > + } > + > + regs = ioremap(res->start, resource_size(res)); > + if (!regs) { > + dev_err(&pdev->dev, "cannot map mbus registers\n"); > + return -ENOMEM; > + } > + > + for (i = 0; i < SDHCI_MAX_WIN_NUM; i++) { > + writel(0, regs + SDHCI_WINDOW_CTRL(i)); > + writel(0, regs + SDHCI_WINDOW_BASE(i)); > + } > + > + for (i = 0; i < dram->num_cs; i++) { > + const struct mbus_dram_window *cs = dram->cs + i; > + > + /* Write size, attributes and target id to control register */ > + writel(((cs->size - 1) & 0xffff0000) | > + (cs->mbus_attr << 8) | > + (dram->mbus_dram_target_id << 4) | 1, > + regs + SDHCI_WINDOW_CTRL(i)); > + /* Write base address to base register */ > + writel(cs->base, regs + SDHCI_WINDOW_BASE(i)); > + } > + > + iounmap(regs); > + > + return 0; > +} > + > static void pxav3_set_private_registers(struct sdhci_host *host, u8 mask) > { > struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc)); > @@ -187,6 +242,9 @@ static const struct of_device_id sdhci_pxav3_of_match[] = { > { > .compatible = "mrvl,pxav3-mmc", > }, > + { > + .compatible = "marvell,armada-380-sdhci", > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, sdhci_pxav3_of_match); > @@ -219,6 +277,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; > struct device *dev = &pdev->dev; > + struct device_node *np = pdev->dev.of_node; > struct sdhci_host *host = NULL; > struct sdhci_pxa *pxa = NULL; > const struct of_device_id *match; > @@ -235,6 +294,14 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > kfree(pxa); > return PTR_ERR(host); > } > + > + if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) { > + ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info()); > + if (ret < 0) > + goto err_mbus_win; > + } > + > + > pltfm_host = sdhci_priv(host); > pltfm_host->priv = pxa; > > @@ -321,6 +388,7 @@ err_add_host: > clk_disable_unprepare(clk); > clk_put(clk); > err_clk_get: > +err_mbus_win: > sdhci_pltfm_free(pdev); > kfree(pxa); > return ret;
Hello Chris, The patch below has been sent more than a month ago, and despite a ping on February, 25th, I haven't received any feedback. Would it be possible to get it merged, or at least get some review comments so that some progress can be made? Or would you accept if this patch was pushed through the ARM mvebu maintainers tree, with your Acked-by? Thanks a lot, Thomas On Tue, 25 Feb 2014 19:40:13 +0100, Thomas Petazzoni wrote: > Hello Chris, > > Do you think it would be possible to have the below patch merged for > 3.15 ? > > It would allow us to enable SDHCI support in the new Marvell Armada 38x > SOCs. The patch is fairly simple I believe, just adding an additional > compatible string to the driver, and adding a little bit of logic to > configure Marvell-specific windows when this compatible string is being > used. > > Thanks a lot! > > Thomas > > On Tue, 18 Feb 2014 16:08:29 +0100, Thomas Petazzoni wrote: > > From: Marcin Wojtas <mw@semihalf.com> > > > > The SDHCI unit used on the Armada 380 and 385 Marvell SoC is similar > > to the PXAv3 unit. The only difference is that on Armada 38x, the > > PXAv3 unit accesses memory through MBus windows which must be > > configured prior to using the device. Without this, DMA would not > > work. > > > > In order to achieve this, the sdhci-pxav3 driver is extended with an > > additional compatible string "marvell,armada-380-sdhci". When this > > compatible string is used, the MBus windows are initialized in a way > > that is identical to what all other DMA-capable drivers for Marvell > > EBU platforms do. > > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > --- > > .../devicetree/bindings/mmc/sdhci-pxa.txt | 17 +++++- > > drivers/mmc/host/sdhci-pxav3.c | 68 ++++++++++++++++++++++ > > 2 files changed, 84 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > > index dbe98a3..86223c3 100644 > > --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > > +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > > @@ -4,7 +4,14 @@ This file documents differences between the core properties in mmc.txt > > and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers. > > > > Required properties: > > -- compatible: Should be "mrvl,pxav2-mmc" or "mrvl,pxav3-mmc". > > +- compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or > > + "marvell,armada-380-sdhci". > > +- reg: > > + * for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for > > + the SDHCI registers. > > + * for "marvell,armada-380-sdhci", two register areas. The first one > > + for the SDHCI registers themselves, and the second one for the > > + AXI/Mbus bridge registers of the SDHCI unit. > > > > Optional properties: > > - mrvl,clk-delay-cycles: Specify a number of cycles to delay for tuning. > > @@ -19,3 +26,11 @@ sdhci@d4280800 { > > non-removable; > > mrvl,clk-delay-cycles = <31>; > > }; > > + > > +sdhci@d8000 { > > + compatible = "marvell,armada-380-sdhci"; > > + reg = <0xd8000 0x1000>, <0xdc000 0x100>; > > + interrupts = <0 25 0x4>; > > + clocks = <&gateclk 17>; > > + mrvl,clk-delay-cycles = <0x1F>; > > +}; > > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > > index 793dacd..2fd73b3 100644 > > --- a/drivers/mmc/host/sdhci-pxav3.c > > +++ b/drivers/mmc/host/sdhci-pxav3.c > > @@ -34,6 +34,7 @@ > > #include <linux/of_gpio.h> > > #include <linux/pm.h> > > #include <linux/pm_runtime.h> > > +#include <linux/mbus.h> > > > > #include "sdhci.h" > > #include "sdhci-pltfm.h" > > @@ -57,6 +58,60 @@ > > #define SDCE_MISC_INT (1<<2) > > #define SDCE_MISC_INT_EN (1<<1) > > > > +/* > > + * These registers are relative to the second register region, for the > > + * MBus bridge. > > + */ > > +#define SDHCI_WINDOW_CTRL(i) (0x80 + ((i) << 3)) > > +#define SDHCI_WINDOW_BASE(i) (0x84 + ((i) << 3)) > > +#define SDHCI_MAX_WIN_NUM 8 > > + > > +static int mv_conf_mbus_windows(struct platform_device *pdev, > > + const struct mbus_dram_target_info *dram) > > +{ > > + int i; > > + void __iomem *regs; > > + struct resource *res; > > + > > + if (!dram) { > > + dev_err(&pdev->dev, "no mbus dram info\n"); > > + return -EINVAL; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + if (!res) { > > + dev_err(&pdev->dev, "cannot get mbus registers\n"); > > + return -EINVAL; > > + } > > + > > + regs = ioremap(res->start, resource_size(res)); > > + if (!regs) { > > + dev_err(&pdev->dev, "cannot map mbus registers\n"); > > + return -ENOMEM; > > + } > > + > > + for (i = 0; i < SDHCI_MAX_WIN_NUM; i++) { > > + writel(0, regs + SDHCI_WINDOW_CTRL(i)); > > + writel(0, regs + SDHCI_WINDOW_BASE(i)); > > + } > > + > > + for (i = 0; i < dram->num_cs; i++) { > > + const struct mbus_dram_window *cs = dram->cs + i; > > + > > + /* Write size, attributes and target id to control register */ > > + writel(((cs->size - 1) & 0xffff0000) | > > + (cs->mbus_attr << 8) | > > + (dram->mbus_dram_target_id << 4) | 1, > > + regs + SDHCI_WINDOW_CTRL(i)); > > + /* Write base address to base register */ > > + writel(cs->base, regs + SDHCI_WINDOW_BASE(i)); > > + } > > + > > + iounmap(regs); > > + > > + return 0; > > +} > > + > > static void pxav3_set_private_registers(struct sdhci_host *host, u8 mask) > > { > > struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc)); > > @@ -187,6 +242,9 @@ static const struct of_device_id sdhci_pxav3_of_match[] = { > > { > > .compatible = "mrvl,pxav3-mmc", > > }, > > + { > > + .compatible = "marvell,armada-380-sdhci", > > + }, > > {}, > > }; > > MODULE_DEVICE_TABLE(of, sdhci_pxav3_of_match); > > @@ -219,6 +277,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > > struct sdhci_pltfm_host *pltfm_host; > > struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; > > struct device *dev = &pdev->dev; > > + struct device_node *np = pdev->dev.of_node; > > struct sdhci_host *host = NULL; > > struct sdhci_pxa *pxa = NULL; > > const struct of_device_id *match; > > @@ -235,6 +294,14 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > > kfree(pxa); > > return PTR_ERR(host); > > } > > + > > + if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) { > > + ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info()); > > + if (ret < 0) > > + goto err_mbus_win; > > + } > > + > > + > > pltfm_host = sdhci_priv(host); > > pltfm_host->priv = pxa; > > > > @@ -321,6 +388,7 @@ err_add_host: > > clk_disable_unprepare(clk); > > clk_put(clk); > > err_clk_get: > > +err_mbus_win: > > sdhci_pltfm_free(pdev); > > kfree(pxa); > > return ret; > > >
Dear Chris Ball, The below patch was originally sent on February, 18th. On February 25th, I sent a reply to get some news. Then, again on March, 20th, I sent again a reply to get some news. I have never heard back from you about this patch since a month and a half. The patch is fairly simple, and not having it merged prevents from enabling SDHCI on a new platform we're working. Could you consider applying the patch for 3.15, or at least, give some review comments if any? I see that on March, 17th you have applied a series of 6 patches, so it looks like you are still maintaining actively the MMC subsystem. Is there any specific problem about this patch? Thanks, Thomas On Tue, 18 Feb 2014 16:08:29 +0100, Thomas Petazzoni wrote: > From: Marcin Wojtas <mw@semihalf.com> > > The SDHCI unit used on the Armada 380 and 385 Marvell SoC is similar > to the PXAv3 unit. The only difference is that on Armada 38x, the > PXAv3 unit accesses memory through MBus windows which must be > configured prior to using the device. Without this, DMA would not > work. > > In order to achieve this, the sdhci-pxav3 driver is extended with an > additional compatible string "marvell,armada-380-sdhci". When this > compatible string is used, the MBus windows are initialized in a way > that is identical to what all other DMA-capable drivers for Marvell > EBU platforms do. > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > .../devicetree/bindings/mmc/sdhci-pxa.txt | 17 +++++- > drivers/mmc/host/sdhci-pxav3.c | 68 ++++++++++++++++++++++ > 2 files changed, 84 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > index dbe98a3..86223c3 100644 > --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt > @@ -4,7 +4,14 @@ This file documents differences between the core properties in mmc.txt > and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers. > > Required properties: > -- compatible: Should be "mrvl,pxav2-mmc" or "mrvl,pxav3-mmc". > +- compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or > + "marvell,armada-380-sdhci". > +- reg: > + * for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for > + the SDHCI registers. > + * for "marvell,armada-380-sdhci", two register areas. The first one > + for the SDHCI registers themselves, and the second one for the > + AXI/Mbus bridge registers of the SDHCI unit. > > Optional properties: > - mrvl,clk-delay-cycles: Specify a number of cycles to delay for tuning. > @@ -19,3 +26,11 @@ sdhci@d4280800 { > non-removable; > mrvl,clk-delay-cycles = <31>; > }; > + > +sdhci@d8000 { > + compatible = "marvell,armada-380-sdhci"; > + reg = <0xd8000 0x1000>, <0xdc000 0x100>; > + interrupts = <0 25 0x4>; > + clocks = <&gateclk 17>; > + mrvl,clk-delay-cycles = <0x1F>; > +}; > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > index 793dacd..2fd73b3 100644 > --- a/drivers/mmc/host/sdhci-pxav3.c > +++ b/drivers/mmc/host/sdhci-pxav3.c > @@ -34,6 +34,7 @@ > #include <linux/of_gpio.h> > #include <linux/pm.h> > #include <linux/pm_runtime.h> > +#include <linux/mbus.h> > > #include "sdhci.h" > #include "sdhci-pltfm.h" > @@ -57,6 +58,60 @@ > #define SDCE_MISC_INT (1<<2) > #define SDCE_MISC_INT_EN (1<<1) > > +/* > + * These registers are relative to the second register region, for the > + * MBus bridge. > + */ > +#define SDHCI_WINDOW_CTRL(i) (0x80 + ((i) << 3)) > +#define SDHCI_WINDOW_BASE(i) (0x84 + ((i) << 3)) > +#define SDHCI_MAX_WIN_NUM 8 > + > +static int mv_conf_mbus_windows(struct platform_device *pdev, > + const struct mbus_dram_target_info *dram) > +{ > + int i; > + void __iomem *regs; > + struct resource *res; > + > + if (!dram) { > + dev_err(&pdev->dev, "no mbus dram info\n"); > + return -EINVAL; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) { > + dev_err(&pdev->dev, "cannot get mbus registers\n"); > + return -EINVAL; > + } > + > + regs = ioremap(res->start, resource_size(res)); > + if (!regs) { > + dev_err(&pdev->dev, "cannot map mbus registers\n"); > + return -ENOMEM; > + } > + > + for (i = 0; i < SDHCI_MAX_WIN_NUM; i++) { > + writel(0, regs + SDHCI_WINDOW_CTRL(i)); > + writel(0, regs + SDHCI_WINDOW_BASE(i)); > + } > + > + for (i = 0; i < dram->num_cs; i++) { > + const struct mbus_dram_window *cs = dram->cs + i; > + > + /* Write size, attributes and target id to control register */ > + writel(((cs->size - 1) & 0xffff0000) | > + (cs->mbus_attr << 8) | > + (dram->mbus_dram_target_id << 4) | 1, > + regs + SDHCI_WINDOW_CTRL(i)); > + /* Write base address to base register */ > + writel(cs->base, regs + SDHCI_WINDOW_BASE(i)); > + } > + > + iounmap(regs); > + > + return 0; > +} > + > static void pxav3_set_private_registers(struct sdhci_host *host, u8 mask) > { > struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc)); > @@ -187,6 +242,9 @@ static const struct of_device_id sdhci_pxav3_of_match[] = { > { > .compatible = "mrvl,pxav3-mmc", > }, > + { > + .compatible = "marvell,armada-380-sdhci", > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, sdhci_pxav3_of_match); > @@ -219,6 +277,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; > struct device *dev = &pdev->dev; > + struct device_node *np = pdev->dev.of_node; > struct sdhci_host *host = NULL; > struct sdhci_pxa *pxa = NULL; > const struct of_device_id *match; > @@ -235,6 +294,14 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) > kfree(pxa); > return PTR_ERR(host); > } > + > + if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) { > + ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info()); > + if (ret < 0) > + goto err_mbus_win; > + } > + > + > pltfm_host = sdhci_priv(host); > pltfm_host->priv = pxa; > > @@ -321,6 +388,7 @@ err_add_host: > clk_disable_unprepare(clk); > clk_put(clk); > err_clk_get: > +err_mbus_win: > sdhci_pltfm_free(pdev); > kfree(pxa); > return ret;
Hi Thomas, On Sat, Mar 29 2014, Thomas Petazzoni wrote: > The below patch was originally sent on February, 18th. On February > 25th, I sent a reply to get some news. Then, again on March, 20th, I > sent again a reply to get some news. I have never heard back from you > about this patch since a month and a half. The patch is fairly simple, > and not having it merged prevents from enabling SDHCI on a new platform > we're working. I'm sorry about this. I didn't see either of your previous pings -- they were both marked as spam by SpamAssassin due to a failing RCVD_ILLEGAL_IP test. That test is supposed to signify that there is an invalid IP in a Received: header, but I don't see such an IP, so I'm suspecting it might be a SpamAssassin bug. It looks like between the first two pings and this one, free-electrons.com moved from Hetzner to OVH, and that stopped the test failing. Just letting you know in case it's widespread. I should try upgrading SpamAssassin. (Still, I should have noticed your pings on the list too.) > Could you consider applying the patch for 3.15, or at least, give some > review comments if any? Yes, looks good, pushed to mmc-next for 3.15, thanks. - Chris.
Dear Chris Ball, On Sat, 29 Mar 2014 16:19:20 +0000, Chris Ball wrote: > On Sat, Mar 29 2014, Thomas Petazzoni wrote: > > The below patch was originally sent on February, 18th. On February > > 25th, I sent a reply to get some news. Then, again on March, 20th, I > > sent again a reply to get some news. I have never heard back from you > > about this patch since a month and a half. The patch is fairly simple, > > and not having it merged prevents from enabling SDHCI on a new platform > > we're working. > > I'm sorry about this. I didn't see either of your previous pings -- > they were both marked as spam by SpamAssassin due to a failing > RCVD_ILLEGAL_IP test. That test is supposed to signify that there is > an invalid IP in a Received: header, but I don't see such an IP, so > I'm suspecting it might be a SpamAssassin bug. It looks like between > the first two pings and this one, free-electrons.com moved from > Hetzner to OVH, and that stopped the test failing. Just letting you > know in case it's widespread. I should try upgrading SpamAssassin. > > (Still, I should have noticed your pings on the list too.) Ah, sorry. I'll notify the person who handles our mail server about this. > > Could you consider applying the patch for 3.15, or at least, give some > > review comments if any? > > Yes, looks good, pushed to mmc-next for 3.15, thanks. Thanks a lot! Thomas
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt index dbe98a3..86223c3 100644 --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.txt @@ -4,7 +4,14 @@ This file documents differences between the core properties in mmc.txt and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers. Required properties: -- compatible: Should be "mrvl,pxav2-mmc" or "mrvl,pxav3-mmc". +- compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or + "marvell,armada-380-sdhci". +- reg: + * for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for + the SDHCI registers. + * for "marvell,armada-380-sdhci", two register areas. The first one + for the SDHCI registers themselves, and the second one for the + AXI/Mbus bridge registers of the SDHCI unit. Optional properties: - mrvl,clk-delay-cycles: Specify a number of cycles to delay for tuning. @@ -19,3 +26,11 @@ sdhci@d4280800 { non-removable; mrvl,clk-delay-cycles = <31>; }; + +sdhci@d8000 { + compatible = "marvell,armada-380-sdhci"; + reg = <0xd8000 0x1000>, <0xdc000 0x100>; + interrupts = <0 25 0x4>; + clocks = <&gateclk 17>; + mrvl,clk-delay-cycles = <0x1F>; +}; diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c index 793dacd..2fd73b3 100644 --- a/drivers/mmc/host/sdhci-pxav3.c +++ b/drivers/mmc/host/sdhci-pxav3.c @@ -34,6 +34,7 @@ #include <linux/of_gpio.h> #include <linux/pm.h> #include <linux/pm_runtime.h> +#include <linux/mbus.h> #include "sdhci.h" #include "sdhci-pltfm.h" @@ -57,6 +58,60 @@ #define SDCE_MISC_INT (1<<2) #define SDCE_MISC_INT_EN (1<<1) +/* + * These registers are relative to the second register region, for the + * MBus bridge. + */ +#define SDHCI_WINDOW_CTRL(i) (0x80 + ((i) << 3)) +#define SDHCI_WINDOW_BASE(i) (0x84 + ((i) << 3)) +#define SDHCI_MAX_WIN_NUM 8 + +static int mv_conf_mbus_windows(struct platform_device *pdev, + const struct mbus_dram_target_info *dram) +{ + int i; + void __iomem *regs; + struct resource *res; + + if (!dram) { + dev_err(&pdev->dev, "no mbus dram info\n"); + return -EINVAL; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + if (!res) { + dev_err(&pdev->dev, "cannot get mbus registers\n"); + return -EINVAL; + } + + regs = ioremap(res->start, resource_size(res)); + if (!regs) { + dev_err(&pdev->dev, "cannot map mbus registers\n"); + return -ENOMEM; + } + + for (i = 0; i < SDHCI_MAX_WIN_NUM; i++) { + writel(0, regs + SDHCI_WINDOW_CTRL(i)); + writel(0, regs + SDHCI_WINDOW_BASE(i)); + } + + for (i = 0; i < dram->num_cs; i++) { + const struct mbus_dram_window *cs = dram->cs + i; + + /* Write size, attributes and target id to control register */ + writel(((cs->size - 1) & 0xffff0000) | + (cs->mbus_attr << 8) | + (dram->mbus_dram_target_id << 4) | 1, + regs + SDHCI_WINDOW_CTRL(i)); + /* Write base address to base register */ + writel(cs->base, regs + SDHCI_WINDOW_BASE(i)); + } + + iounmap(regs); + + return 0; +} + static void pxav3_set_private_registers(struct sdhci_host *host, u8 mask) { struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc)); @@ -187,6 +242,9 @@ static const struct of_device_id sdhci_pxav3_of_match[] = { { .compatible = "mrvl,pxav3-mmc", }, + { + .compatible = "marvell,armada-380-sdhci", + }, {}, }; MODULE_DEVICE_TABLE(of, sdhci_pxav3_of_match); @@ -219,6 +277,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) struct sdhci_pltfm_host *pltfm_host; struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data; struct device *dev = &pdev->dev; + struct device_node *np = pdev->dev.of_node; struct sdhci_host *host = NULL; struct sdhci_pxa *pxa = NULL; const struct of_device_id *match; @@ -235,6 +294,14 @@ static int sdhci_pxav3_probe(struct platform_device *pdev) kfree(pxa); return PTR_ERR(host); } + + if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) { + ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info()); + if (ret < 0) + goto err_mbus_win; + } + + pltfm_host = sdhci_priv(host); pltfm_host->priv = pxa; @@ -321,6 +388,7 @@ err_add_host: clk_disable_unprepare(clk); clk_put(clk); err_clk_get: +err_mbus_win: sdhci_pltfm_free(pdev); kfree(pxa); return ret;