Message ID | 1409835268-29555-1-git-send-email-gerg@uclinux.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Greg, On Thu, Sep 4, 2014 at 2:54 PM, <gerg@uclinux.org> wrote: > The Armada SoC family implementation of this SPI hardware module has > extended the configuration register to allow for a wider range of SPI > clock rates. Specifically the Serial Baud Rate Pre-selection bits in the > SPI Interface Configuration Register now also use bits 6 and 7 as well. > > Modify the baud rate calculation to handle these differences for the > Armada case. Potentially a baud rate can be setup using a number of > different pre-scalar and scalar combinations. This code tries all > possible pre-scalar divisors (8 in total) to try and find the most > accuate set. Are the Orion rates and register bits a subset of the Armada ones? I.e. does the Armada work with the Orion setup? > This change introduces a new device tree compatible device name > "armada-spi". This is used in place of "orion-spi" on the Armada SoC > parts. > - compatible = "marvell,orion-spi"; > + compatible = "marvell,armada-spi"; If the answer to my above question is yes, you want to keep "marvell,orion-spi" as a fallback. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 04/09/14 23:17, Geert Uytterhoeven wrote: > On Thu, Sep 4, 2014 at 2:54 PM, <gerg@uclinux.org> wrote: >> The Armada SoC family implementation of this SPI hardware module has >> extended the configuration register to allow for a wider range of SPI >> clock rates. Specifically the Serial Baud Rate Pre-selection bits in the >> SPI Interface Configuration Register now also use bits 6 and 7 as well. >> >> Modify the baud rate calculation to handle these differences for the >> Armada case. Potentially a baud rate can be setup using a number of >> different pre-scalar and scalar combinations. This code tries all >> possible pre-scalar divisors (8 in total) to try and find the most >> accuate set. > > Are the Orion rates and register bits a subset of the Armada ones? > I.e. does the Armada work with the Orion setup? The bits used by the orion are a subset (bits 0-4 of the SPI configuration register) of those used by the armada (bits 0-4 and 6-7). In practice the defined prescalar values for the orion do give the same divisor for the same bits set on armada. The extra bits on the armada let you get to lower SPI baudrates (which is more of a problem on the armada with its higher core clocks). >> This change introduces a new device tree compatible device name >> "armada-spi". This is used in place of "orion-spi" on the Armada SoC >> parts. > >> - compatible = "marvell,orion-spi"; >> + compatible = "marvell,armada-spi"; > > If the answer to my above question is yes, you want to keep > "marvell,orion-spi" as a fallback. Do you mean like this? compatible = "marvell,orion-spi", "marvell,armada-spi"; Regards Greg -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, On Fri, Sep 5, 2014 at 2:36 AM, Greg Ungerer <gerg@uclinux.org> wrote: >> Are the Orion rates and register bits a subset of the Armada ones? >> I.e. does the Armada work with the Orion setup? > > The bits used by the orion are a subset (bits 0-4 of the SPI > configuration register) of those used by the armada (bits 0-4 and > 6-7). > > In practice the defined prescalar values for the orion do give > the same divisor for the same bits set on armada. The extra bits > on the armada let you get to lower SPI baudrates (which is more of > a problem on the armada with its higher core clocks). OK, so the answer is yes. >>> This change introduces a new device tree compatible device name >>> "armada-spi". This is used in place of "orion-spi" on the Armada SoC >>> parts. >> >>> - compatible = "marvell,orion-spi"; >>> + compatible = "marvell,armada-spi"; >> >> If the answer to my above question is yes, you want to keep >> "marvell,orion-spi" as a fallback. > > Do you mean like this? > > compatible = "marvell,orion-spi", "marvell,armada-spi"; No, the other way around (from most-specific to least-specific): compatible = "marvell,armada-spi", "marvell,orion-spi" So the Armada version is used if available, and the Orion version is used as a fallback. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 05/09/14 17:36, Geert Uytterhoeven wrote: > Hi Greg, > > On Fri, Sep 5, 2014 at 2:36 AM, Greg Ungerer <gerg@uclinux.org> wrote: >>> Are the Orion rates and register bits a subset of the Armada ones? >>> I.e. does the Armada work with the Orion setup? >> The bits used by the orion are a subset (bits 0-4 of the SPI >> configuration register) of those used by the armada (bits 0-4 and >> 6-7). >> >> In practice the defined prescalar values for the orion do give >> the same divisor for the same bits set on armada. The extra bits >> on the armada let you get to lower SPI baudrates (which is more of >> a problem on the armada with its higher core clocks). > OK, so the answer is yes. > >>>> This change introduces a new device tree compatible device name >>>> "armada-spi". This is used in place of "orion-spi" on the Armada SoC >>>> parts. >>>> - compatible = "marvell,orion-spi"; >>>> + compatible = "marvell,armada-spi"; >>> If the answer to my above question is yes, you want to keep >>> "marvell,orion-spi" as a fallback. >> Do you mean like this? >> >> compatible = "marvell,orion-spi", "marvell,armada-spi"; > No, the other way around (from most-specific to least-specific): > > compatible = "marvell,armada-spi", "marvell,orion-spi" > > So the Armada version is used if available, and the Orion version > is used as a fallback. Ok, got it. Thanks. Regards Greg > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, (Ccing mvebu guys, full discussion here: http://www.spinics.net/lists/devicetree/msg47806.html) On 05 Sep 11:11 PM, Greg Ungerer wrote: > On 05/09/14 17:36, Geert Uytterhoeven wrote: > >No, the other way around (from most-specific to least-specific): > > > > compatible = "marvell,armada-spi", "marvell,orion-spi" > > > >So the Armada version is used if available, and the Orion version > >is used as a fallback. Since you are doing another spin, please split this in two patches. The driver and devicetree binding documentation should be in one patch, and the devicetree .dtsi change in another patch. By following Geert's request about the compatible, you make sure both patches can be merged through different maintainers, and that an old devicetree blob can be used safely. > > Also what about armada370-xp, armada375, armada380 > >and any other variants out there? > > I don't know. I don't have access to functional specs for anything > other than the 370. I expect they are probably the same though. > (Can anyone confirm?) > Confirmed. Judging from the specs, seems the same SPI IP is on all the Armada SoCs; you can do the 'armada-spi' compatible addition on 375 and 38x. Did a few tests on the AXP-GP board (which has an SPI flash) using 'orion-spi' and 'armada-spi' compatibles. Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> Reviewed-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> I haven't been able to spot any speed difference, though (perhaps my test is too limited?), so I wonder if there's any improvement with this patch? Thanks,
Hello, On Sat, 6 Sep 2014 09:06:00 -0300, Ezequiel Garcia wrote: > Confirmed. Judging from the specs, seems the same SPI IP is on all the > Armada SoCs; you can do the 'armada-spi' compatible addition on 375 and 38x. However, I believe a compatible string like "armada-370-spi" should be used instead of "armada-spi". The normal rule is to use the oldest SoC that introduced a certain feature as the compatible string. Remember for example that Armada 1500 (Berlin) is a completely different line of SoC, which cannot use this SPI driver. Thanks Greg for working on this! I noticed this as well recently when doing an SPI development, but you were faster than me in proposing a patch! Thanks, Thomas
Hi Ezequiel, On 06/09/14 22:06, Ezequiel Garcia wrote: > Hi Greg, > > (Ccing mvebu guys, full discussion here: http://www.spinics.net/lists/devicetree/msg47806.html) > > On 05 Sep 11:11 PM, Greg Ungerer wrote: >> On 05/09/14 17:36, Geert Uytterhoeven wrote: >>> No, the other way around (from most-specific to least-specific): >>> >>> compatible = "marvell,armada-spi", "marvell,orion-spi" >>> >>> So the Armada version is used if available, and the Orion version >>> is used as a fallback. > Since you are doing another spin, please split this in two patches. > The driver and devicetree binding documentation should be in one patch, > and the devicetree .dtsi change in another patch. > > By following Geert's request about the compatible, you make sure both > patches can be merged through different maintainers, and that an old devicetree > blob can be used safely. Sure, yes, I would prefer that too. I didn't do it for this because checkpatch complained for the spi-orion.c change in its own: WARNING: DT compatible string "marvell,armada-spi" appears un-documented -- check ./Documentation/devicetree/bindings/ #170: FILE: drivers/spi/spi-orion.c:412: + { .compatible = "marvell,armada-spi", .data = &armada_spi_dev_data, }, But I am happy to ignore that :-) >>> Also what about armada370-xp, armada375, armada380 >>> and any other variants out there? >> I don't know. I don't have access to functional specs for anything >> other than the 370. I expect they are probably the same though. >> (Can anyone confirm?) >> > Confirmed. Judging from the specs, seems the same SPI IP is on all the > Armada SoCs; you can do the 'armada-spi' compatible addition on 375 and 38x. > > Did a few tests on the AXP-GP board (which has an SPI flash) using > 'orion-spi' and 'armada-spi' compatibles. > > Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > Reviewed-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> Thanks, appreciate that. > I haven't been able to spot any speed difference, though (perhaps my > test is too limited?), so I wonder if there's any improvement with this > patch? Try setting a speed like 2MHz for an SPI device. It will fail because that is below the minimum divisor it can calculate with the old orion style behavior. I have an armada 370 based board with an ethernet switch that is controlled by SPI. Its typical SPI bus speed is 2MHz, that is how I came up against this. Regards Greg -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thomas, On 06/09/14 23:16, Thomas Petazzoni wrote: > Hello, > > On Sat, 6 Sep 2014 09:06:00 -0300, Ezequiel Garcia wrote: > >> Confirmed. Judging from the specs, seems the same SPI IP is on all the >> Armada SoCs; you can do the 'armada-spi' compatible addition on 375 and 38x. > However, I believe a compatible string like "armada-370-spi" should be > used instead of "armada-spi". The normal rule is to use the oldest SoC > that introduced a certain feature as the compatible string. Ok, will make it that then. > Remember for example that Armada 1500 (Berlin) is a completely different > line of SoC, which cannot use this SPI driver. > > Thanks Greg for working on this! I noticed this as well recently when > doing an SPI development, but you were faster than me in proposing a > patch! > No problem :-) Regards Greg -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, On 07 Sep 10:14 PM, Greg Ungerer wrote: > Hi Ezequiel, > > On 06/09/14 22:06, Ezequiel Garcia wrote: > >Hi Greg, > > > >(Ccing mvebu guys, full discussion here: http://www.spinics.net/lists/devicetree/msg47806.html) > > > >On 05 Sep 11:11 PM, Greg Ungerer wrote: > >>On 05/09/14 17:36, Geert Uytterhoeven wrote: > >>>No, the other way around (from most-specific to least-specific): > >>> > >>> compatible = "marvell,armada-spi", "marvell,orion-spi" > >>> > >>>So the Armada version is used if available, and the Orion version > >>>is used as a fallback. > >Since you are doing another spin, please split this in two patches. > >The driver and devicetree binding documentation should be in one patch, > >and the devicetree .dtsi change in another patch. > > > >By following Geert's request about the compatible, you make sure both > >patches can be merged through different maintainers, and that an old devicetree > >blob can be used safely. > > Sure, yes, I would prefer that too. I didn't do it for this because > checkpatch > complained for the spi-orion.c change in its own: > > WARNING: DT compatible string "marvell,armada-spi" appears un-documented -- > check ./Documentation/devicetree/bindings/ > #170: FILE: drivers/spi/spi-orion.c:412: > + { .compatible = "marvell,armada-spi", .data = &armada_spi_dev_data, }, > > But I am happy to ignore that :-) > If you split in two patches like this: Patch #1: "spi: orion: Add a new foo feature" .../devicetree/bindings/spi/spi-orion.txt | 2 +- drivers/spi/spi-orion.c | 116 +++++++++++++++++---- Patch #2: "ARM: mvebu: Add new compatible string for SPI" arch/arm/boot/dts/armada-370-xp.dtsi | 4 +- Then checkpatch shouldn't complain. As you can see, it makes more sense like this. The change to the devicetree binding in the driver and properly documented, but the devicetree files per-se, get modified in another patch.
Hi Ezequiel, On 08/09/14 01:12, Ezequiel Garcia wrote: > Hi Greg, > > On 07 Sep 10:14 PM, Greg Ungerer wrote: >> Hi Ezequiel, >> >> On 06/09/14 22:06, Ezequiel Garcia wrote: >>> Hi Greg, >>> >>> (Ccing mvebu guys, full discussion here: http://www.spinics.net/lists/devicetree/msg47806.html) >>> >>> On 05 Sep 11:11 PM, Greg Ungerer wrote: >>>> On 05/09/14 17:36, Geert Uytterhoeven wrote: >>>>> No, the other way around (from most-specific to least-specific): >>>>> >>>>> compatible = "marvell,armada-spi", "marvell,orion-spi" >>>>> >>>>> So the Armada version is used if available, and the Orion version >>>>> is used as a fallback. >>> Since you are doing another spin, please split this in two patches. >>> The driver and devicetree binding documentation should be in one patch, >>> and the devicetree .dtsi change in another patch. >>> >>> By following Geert's request about the compatible, you make sure both >>> patches can be merged through different maintainers, and that an old devicetree >>> blob can be used safely. >> >> Sure, yes, I would prefer that too. I didn't do it for this because >> checkpatch >> complained for the spi-orion.c change in its own: >> >> WARNING: DT compatible string "marvell,armada-spi" appears un-documented -- >> check ./Documentation/devicetree/bindings/ >> #170: FILE: drivers/spi/spi-orion.c:412: >> + { .compatible = "marvell,armada-spi", .data = &armada_spi_dev_data, }, >> >> But I am happy to ignore that :-) >> > > If you split in two patches like this: > > Patch #1: "spi: orion: Add a new foo feature" > > .../devicetree/bindings/spi/spi-orion.txt | 2 +- > drivers/spi/spi-orion.c | 116 +++++++++++++++++---- > > Patch #2: "ARM: mvebu: Add new compatible string for SPI" > > arch/arm/boot/dts/armada-370-xp.dtsi | 4 +- > > Then checkpatch shouldn't complain. As you can see, it makes more sense like > this. The change to the devicetree binding in the driver and properly > documented, but the devicetree files per-se, get modified in another patch. Ok, I see. Thanks. Regards Greg -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt index a3ff50f..674b64f 100644 --- a/Documentation/devicetree/bindings/spi/spi-orion.txt +++ b/Documentation/devicetree/bindings/spi/spi-orion.txt @@ -1,7 +1,7 @@ Marvell Orion SPI device Required properties: -- compatible : should be "marvell,orion-spi". +- compatible : should be "marvell,orion-spi" or "marvell,armada-spi". - reg : offset and length of the register set for the device - cell-index : Which of multiple SPI controllers is this. Optional properties: diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi index 23227e0..d2a9f50 100644 --- a/arch/arm/boot/dts/armada-370-xp.dtsi +++ b/arch/arm/boot/dts/armada-370-xp.dtsi @@ -110,7 +110,7 @@ }; spi0: spi@10600 { - compatible = "marvell,orion-spi"; + compatible = "marvell,armada-spi"; reg = <0x10600 0x28>; #address-cells = <1>; #size-cells = <0>; @@ -121,7 +121,7 @@ }; spi1: spi@10680 { - compatible = "marvell,orion-spi"; + compatible = "marvell,armada-spi"; reg = <0x10680 0x28>; #address-cells = <1>; #size-cells = <0>; diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c index c4675fa..566747f 100644 --- a/drivers/spi/spi-orion.c +++ b/drivers/spi/spi-orion.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/pm_runtime.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/clk.h> #include <linux/sizes.h> #include <asm/unaligned.h> @@ -40,13 +41,27 @@ #define ORION_SPI_MODE_CPHA (1 << 12) #define ORION_SPI_IF_8_16_BIT_MODE (1 << 5) #define ORION_SPI_CLK_PRESCALE_MASK 0x1F +#define ARMADA_SPI_CLK_PRESCALE_MASK 0xDF #define ORION_SPI_MODE_MASK (ORION_SPI_MODE_CPOL | \ ORION_SPI_MODE_CPHA) +enum orion_spi_type { + ORION_SPI, + ARMADA_SPI, +}; + +struct orion_spi_dev { + enum orion_spi_type typ; + unsigned int min_divisor; + unsigned int max_divisor; + u32 prescale_mask; +}; + struct orion_spi { struct spi_master *master; void __iomem *base; struct clk *clk; + const struct orion_spi_dev *devdata; }; static inline void __iomem *spi_reg(struct orion_spi *orion_spi, u32 reg) @@ -83,30 +98,66 @@ static int orion_spi_baudrate_set(struct spi_device *spi, unsigned int speed) u32 prescale; u32 reg; struct orion_spi *orion_spi; + const struct orion_spi_dev *devdata; orion_spi = spi_master_get_devdata(spi->master); + devdata = orion_spi->devdata; tclk_hz = clk_get_rate(orion_spi->clk); - /* - * the supported rates are: 4,6,8...30 - * round up as we look for equal or less speed - */ - rate = DIV_ROUND_UP(tclk_hz, speed); - rate = roundup(rate, 2); + if (devdata->typ == ARMADA_SPI) { + unsigned int clk, spr, sppr, sppr2, err; + unsigned int best_spr, best_sppr, best_err; + + best_err = speed; + best_spr = 0; + best_sppr = 0; + + /* Iterate over the valid range looking for best fit */ + for (sppr = 0; sppr < 8; sppr++) { + sppr2 = 0x1 << sppr; + + spr = tclk_hz / sppr2; + spr = DIV_ROUND_UP(spr, speed); + if ((spr == 0) || (spr > 15)) + continue; - /* check if requested speed is too small */ - if (rate > 30) - return -EINVAL; + clk = tclk_hz / (spr * sppr2); + err = speed - clk; - if (rate < 4) - rate = 4; + if (err < best_err) { + best_spr = spr; + best_sppr = sppr; + best_err = err; + } + } + + if ((best_sppr == 0) && (best_spr == 0)) + return -EINVAL; + + prescale = ((best_sppr & 0x6) << 5) | + ((best_sppr & 0x1) << 4) | best_spr; + } else { + /* + * the supported rates are: 4,6,8...30 + * round up as we look for equal or less speed + */ + rate = DIV_ROUND_UP(tclk_hz, speed); + rate = roundup(rate, 2); + + /* check if requested speed is too small */ + if (rate > 30) + return -EINVAL; - /* Convert the rate to SPI clock divisor value. */ - prescale = 0x10 + rate/2; + if (rate < 4) + rate = 4; + + /* Convert the rate to SPI clock divisor value. */ + prescale = 0x10 + rate/2; + } reg = readl(spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG)); - reg = ((reg & ~ORION_SPI_CLK_PRESCALE_MASK) | prescale); + reg = ((reg & ~devdata->prescale_mask) | prescale); writel(reg, spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG)); return 0; @@ -342,8 +393,31 @@ static int orion_spi_reset(struct orion_spi *orion_spi) return 0; } +static const struct orion_spi_dev orion_spi_dev_data = { + .typ = ORION_SPI, + .min_divisor = 4, + .max_divisor = 30, + .prescale_mask = ORION_SPI_CLK_PRESCALE_MASK, +}; + +static const struct orion_spi_dev armada_spi_dev_data = { + .typ = ARMADA_SPI, + .min_divisor = 1, + .max_divisor = 1920, + .prescale_mask = ARMADA_SPI_CLK_PRESCALE_MASK, +}; + +static const struct of_device_id orion_spi_of_match_table[] = { + { .compatible = "marvell,orion-spi", .data = &orion_spi_dev_data, }, + { .compatible = "marvell,armada-spi", .data = &armada_spi_dev_data, }, + {} +}; +MODULE_DEVICE_TABLE(of, orion_spi_of_match_table); + static int orion_spi_probe(struct platform_device *pdev) { + const struct of_device_id *of_id; + const struct orion_spi_dev *devdata; struct spi_master *master; struct orion_spi *spi; struct resource *r; @@ -378,6 +452,10 @@ static int orion_spi_probe(struct platform_device *pdev) spi = spi_master_get_devdata(master); spi->master = master; + of_id = of_match_device(orion_spi_of_match_table, &pdev->dev); + devdata = of_id->data; + spi->devdata = devdata; + spi->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(spi->clk)) { status = PTR_ERR(spi->clk); @@ -389,8 +467,8 @@ static int orion_spi_probe(struct platform_device *pdev) goto out; tclk_hz = clk_get_rate(spi->clk); - master->max_speed_hz = DIV_ROUND_UP(tclk_hz, 4); - master->min_speed_hz = DIV_ROUND_UP(tclk_hz, 30); + master->max_speed_hz = DIV_ROUND_UP(tclk_hz, devdata->min_divisor); + master->min_speed_hz = DIV_ROUND_UP(tclk_hz, devdata->max_divisor); r = platform_get_resource(pdev, IORESOURCE_MEM, 0); spi->base = devm_ioremap_resource(&pdev->dev, r); @@ -469,12 +547,6 @@ static const struct dev_pm_ops orion_spi_pm_ops = { NULL) }; -static const struct of_device_id orion_spi_of_match_table[] = { - { .compatible = "marvell,orion-spi", }, - {} -}; -MODULE_DEVICE_TABLE(of, orion_spi_of_match_table); - static struct platform_driver orion_spi_driver = { .driver = { .name = DRIVER_NAME,