spi: orion.c: Add direct access mode

This patch adds support for the direct access mode to the Orion SPI driver which is used on the Marvell Armada based SoCs. In this direct mode, all data written to (or read from) a specifically mapped MBus window (linked to one SPI chip-select on one of the SPI controllers) will be transferred directly to the SPI bus. Without the need to control the SPI registers in between. This can improve the SPI transfer rate in such cases.

Both, direct-read and -write mode are supported. But only the write mode has been tested. This mode especially benefits from the SPI direct mode, as the data bytes are written head-to-head to the SPI bus, without any additional addresses.

One use-case for this direct write mode is, programming a FPGA bitstream image into the FPGA connected to the SPI bus at maximum speed.

This mode is described in chapter "22.5.2 Direct Write to SPI" in the Marvell Armada XP Functional Spec Datasheet.

Signed-off-by Stefan Roese <[email protected]>

Cc Nadav Haklai <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Mark Brown <[email protected]>

---
Mark, sorry for the huge delay for v2 of this direct-access patch. I was busy with other tasks in the meantime. And only found now the time to address (hopefully all) of your comments.

Quoted 24 lines · Stefan Roese

This patch adds support for the direct access mode to the Orion SPI driver which is used on the Marvell Armada based SoCs. In this direct mode, all data written to (or read from) a specifically mapped MBus window (linked to one SPI chip-select on one of the SPI controllers) will be transferred directly to the SPI bus. Without the need to control the SPI registers in between. This can improve the SPI transfer rate in such cases.

Both, direct-read and -write mode are supported. But only the write mode has been tested. This mode especially benefits from the SPI direct mode, as the data bytes are written head-to-head to the SPI bus, without any additional addresses.

One use-case for this direct write mode is, programming a FPGA bitstream image into the FPGA connected to the SPI bus at maximum speed.

This mode is described in chapter "22.5.2 Direct Write to SPI" in the Marvell Armada XP Functional Spec Datasheet.

Signed-off-by Stefan Roese <[email protected]>

Cc Nadav Haklai <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Mark Brown <[email protected]>

Quoted 4 lines

---
Mark, sorry for the huge delay for v2 of this direct-access patch. I was busy with other tasks in the meantime. And only found now the time to address (hopefully all) of your comments.

Thanks for this new version! To be honest, I don't remember all the discussions that took place on the v1, so maybe I'll just be re-asking the same question.

Has there been any discussion on whether dynamically adding the MBus window is a good idea, as opposed to statically defining it in the board .dts ?

So far, the only driver that was using dynamically allocated MBus is the PCIe controller driver, because there is no way in advanced to know the number and memory window requirements of the PCIe devices that will be connected to the system.

For all other windows (BootROM, crypto SRAM and more recently network related SRAM), we are using statically allocated windows.

So I'm wondering if we should add this additional DT binding that describes the necessary information to allow the driver to dynamically allocate a window, or if we shouldn't rely on a statically allocated window.

This is really an open discussion, I don't have a very well-defined opinion on the matter.

Let's Cc: Arnd Bergmann on this question, he has followed the whole MBus story and might have some interesting insights.

Quoted 3 lines · Thomas Petazzoni

Has there been any discussion on whether dynamically adding the MBus window is a good idea, as opposed to statically defining it in the board .dts ?

Yes. My 1st patch version (still RFC) used fixed MBus windows instead:

http://www.spinics.net/lists/linux-spi/msg06536.html

Mark suggested to use dynamic windows, so that one area could be used for all SPI devices by switching (re-configuring) the MBus window:

http://www.spinics.net/lists/linux-spi/msg06537.html

Quoted 13 lines · Stefan Roese

Quoted 3 lines · Thomas Petazzoni

Has there been any discussion on whether dynamically adding the MBus window is a good idea, as opposed to statically defining it in the board .dts ?

Yes. My 1st patch version (still RFC) used fixed MBus windows instead:

http://www.spinics.net/lists/linux-spi/msg06536.html

Mark suggested to use dynamic windows, so that one area could be used for all SPI devices by switching (re-configuring) the MBus window:

Quoted 1 line

http://www.spinics.net/lists/linux-spi/msg06537.html

No, there's two separate things here. The big problem with what you originally sent was that you were defining a window per SPI device but this is a part of the SPI controller so having to change windows per device is just going to make the code more complex.

There's also the
separate issue of where any MBus windows we get come up with.

Quoted 4 lines · Mark Brown

No, there's two separate things here. The big problem with what you originally sent was that you were defining a window per SPI device but this is a part of the SPI controller so having to change windows per device is just going to make the code more complex.

I'm sorry, but this sentence is not totally clear to me: Do you mean that the "more complex" code in v2 with the MBus window reconfiguration is not good? Or do you mean that in the RFC version, the switching between the SPI devices was too complex?

Quoted 2 lines

There's also the
separate issue of where any MBus windows we get come up with.

Could you also please explain this in more detail? What do you mean with that? Do you mean the physical address, that is defined in the DT for this MBus window?

Quoted 6 lines · Stefan Roese

Quoted 4 lines · Mark Brown

No, there's two separate things here. The big problem with what you originally sent was that you were defining a window per SPI device but this is a part of the SPI controller so having to change windows per device is just going to make the code more complex.

Quoted 4 lines

I'm sorry, but this sentence is not totally clear to me: Do you mean that the "more complex" code in v2 with the MBus window reconfiguration is not good? Or do you mean that in the RFC version, the switching between the SPI devices was too complex?

I haven't looked at your new code at all. What I'm saying is that specifying a per-device MBus window seems like pointless complexity.

Hi Mark,

Quoted 8 lines · Mark Brown

Quoted 6 lines · Stefan Roese

Quoted 4 lines · Mark Brown

No, there's two separate things here. The big problem with what you originally sent was that you were defining a window per SPI device but this is a part of the SPI controller so having to change windows per device is just going to make the code more complex.

Quoted 7 lines

Quoted 4 lines

I'm sorry, but this sentence is not totally clear to me: Do you mean that the "more complex" code in v2 with the MBus window reconfiguration is not good? Or do you mean that in the RFC version, the switching between the SPI devices was too complex?

I haven't looked at your new code at all. What I'm saying is that specifying a per-device MBus window seems like pointless complexity.

I don't necessarily share this opinions. Code-wise, its less complex that re-configuring (removing the old and creating the new) the MBus window. But I have no strong feeling here. Whatever is decided that should be used, I can go with.

Thomas, Arnd (or anyone else?), do you have any comments or preferences which way to go here?

The number of windows is limited. On i think the Armada XP, if you put a PCIe device on every available PCIe bus, you can run out of windows. This is why Thomas implemented dynamic allocation of the Windows, so that only those that are needed are used.

So i would not statically and globally allocate as many windows as possible SPI devices.

The fact that SPI can fall back to another mechanism if there are no available windows, were as PCIe cannot, suggests that SPI should dynamically allocate a window, and be prepared for it to fail.

Since only one SPI device can be active at once on a SPI bus, one window per bus makes sense, and keeps the required number of windows to a minimum.

Quoted 8 lines · Andrew Lunn

The number of windows is limited. On i think the Armada XP, if you put a PCIe device on every available PCIe bus, you can run out of windows. This is why Thomas implemented dynamic allocation of the Windows, so that only those that are needed are used.

So i would not statically and globally allocate as many windows as possible SPI devices.

Quoted 7 lines

The fact that SPI can fall back to another mechanism if there are no available windows, were as PCIe cannot, suggests that SPI should dynamically allocate a window, and be prepared for it to fail.

Since only one SPI device can be active at once on a SPI bus, one window per bus makes sense, and keeps the required number of windows to a minimum.

If we're under pressure for windows I'd go further and say that we should be dynamically allocating the windows only when they're actually in use (and modifying other drivers to do the same if that makes sense for them), unless it's somehow expensive to allocate windows that means that we should reduce the overall pressure.

Quoted 18 lines · Mark Brown

Quoted 8 lines · Andrew Lunn

The number of windows is limited. On i think the Armada XP, if you put a PCIe device on every available PCIe bus, you can run out of windows. This is why Thomas implemented dynamic allocation of the Windows, so that only those that are needed are used.

So i would not statically and globally allocate as many windows as possible SPI devices.

Quoted 7 lines

The fact that SPI can fall back to another mechanism if there are no available windows, were as PCIe cannot, suggests that SPI should dynamically allocate a window, and be prepared for it to fail.

Since only one SPI device can be active at once on a SPI bus, one window per bus makes sense, and keeps the required number of windows to a minimum.

Quoted 5 lines

If we're under pressure for windows I'd go further and say that we should be dynamically allocating the windows only when they're actually in use (and modifying other drivers to do the same if that makes sense for them), unless it's somehow expensive to allocate windows that means that we should reduce the overall pressure.

Hi Mark

We are only under pressure in the extremes, i.e 10 PCIe busses in use. Only allocating PCIe Windows when needed means in practice, we have not had issues. We need to be mindful, and don't waste them, but we don't need to consider them a scarce resource.

I don't see it being an issue for the SPI driver to allocate one on probe and keeping it until release. I probably would object to it allocating one per chip select line.

Quoted 27 lines · On Wednesday 23 March 2016 14:56:06 Andrew Lunn wrote:

Quoted 18 lines · Mark Brown

Quoted 8 lines · Andrew Lunn

The number of windows is limited. On i think the Armada XP, if you put a PCIe device on every available PCIe bus, you can run out of windows. This is why Thomas implemented dynamic allocation of the Windows, so that only those that are needed are used.

So i would not statically and globally allocate as many windows as possible SPI devices.

Quoted 7 lines

The fact that SPI can fall back to another mechanism if there are no available windows, were as PCIe cannot, suggests that SPI should dynamically allocate a window, and be prepared for it to fail.

Since only one SPI device can be active at once on a SPI bus, one window per bus makes sense, and keeps the required number of windows to a minimum.

Quoted 5 lines

If we're under pressure for windows I'd go further and say that we should be dynamically allocating the windows only when they're actually in use (and modifying other drivers to do the same if that makes sense for them), unless it's somehow expensive to allocate windows that means that we should reduce the overall pressure.

Hi Mark

Quoted 8 lines

We are only under pressure in the extremes, i.e 10 PCIe busses in use. Only allocating PCIe Windows when needed means in practice, we have not had issues. We need to be mindful, and don't waste them, but we don't need to consider them a scarce resource.

I don't see it being an issue for the SPI driver to allocate one on probe and keeping it until release. I probably would object to it allocating one per chip select line.

I agree. We had a very long debate about this when we first added the support for MBus, and not much has changed. As far as I'm concerned, this is where we're at:

The binding defines a reasonable set of default settings for each device that uses MBus, and the OS can use them, or define its own. Coming up with an algorithm to do this right however is very hard and not really worth it, as defining the defaults works well enough.

Ideally we'd let the boot loader set up the windows statically and have the DT describe what they are, but unfortunately that is not what boot loaders in the field do.

The method that was picked for MBus is similar to how we typically handle PCI host bridges that have the same problem: you can set up translation windows between CPU addresses and the various PCI address spaces (I/O, mem, prefetchable, config, ...) in all sorts of ways, with various tradeoffs, but instead we just pick a reasonable default and describe it in the DT, because the kernel has no good generic way of picking a particular setting over another.

Arnd, thanks for your comments.

So we seem to agree, that one MBus window per SPI controller is the way to go. Only how should this be described in the DT? I've come up with these new DT properties in v2:

+- da-reg : The physical memory area that will be used for the direct

(quoted diff) +19 -0

+           access mode, if enabled in one of the SPI devices.
+
+Per SPI device:
+- da-target-attribute : The target and attribute value for a specific
+                        SPI-controller / SPI-device combination.
+                       E.g. <0x01 0x5e>: SPI0-CS1 target and attribute
 
...
 
+Example with direct access mode:
+       spi at 10600 {
+               compatible = "marvell,orion-spi";
+               status = "okay";
+               da-reg = <0xf2000000 0x100000>;
+
+               spi-fpga at 1 {
+                       compatible = "altera,stratix-v";
+                       reg = <1>;
+                       /* 0x01 0x5e: SPI0-CS1 target and attribute */
+                       da-target-attribute = <0x01 0x5e>;
+               };
+       };

I've added the "da-*" (Direct Access) properties to enable the SPI driver to dynamically allocate the MBus windows. Do you find these new bindings reasonable? Or do you have better suggestions for this per-SPI-device dynamic MBus allocation, perhaps by using MBUS_ID somehow?

Quoted 31 lines · On Thursday 24 March 2016 08:22:36 Stefan Roese wrote:

Arnd, thanks for your comments.

So we seem to agree, that one MBus window per SPI controller is the way to go. Only how should this be described in the DT? I've come up with these new DT properties in v2:

+- da-reg : The physical memory area that will be used for the direct

(quoted diff) +19 -0

+           access mode, if enabled in one of the SPI devices.
+
+Per SPI device:
+- da-target-attribute : The target and attribute value for a specific
+                        SPI-controller / SPI-device combination.
+                       E.g. <0x01 0x5e>: SPI0-CS1 target and attribute
 
...
 
+Example with direct access mode:
+       spi at 10600 {
+               compatible = "marvell,orion-spi";
+               status = "okay";
+               da-reg = <0xf2000000 0x100000>;
+
+               spi-fpga at 1 {
+                       compatible = "altera,stratix-v";
+                       reg = <1>;
+                       /* 0x01 0x5e: SPI0-CS1 target and attribute */
+                       da-target-attribute = <0x01 0x5e>;
+               };
+       };

Quoted 5 lines

I've added the "da-*" (Direct Access) properties to enable the SPI driver to dynamically allocate the MBus windows. Do you find these new bindings reasonable? Or do you have better suggestions for this per-SPI-device dynamic MBus allocation, perhaps by using MBUS_ID somehow?

I was thinking it would be statically set up,

but then we have a
problem with how it uses both internal-regs and and its own mbus based reg, so we probably have to move the spi node outside of the internal-regs node to achieve that, similar to how we handle the devbus devices:

	soc@ {
		spi0 {
                        compatible = "marvell,armada-370-spi",
                                     "marvell,orion-spi";
			reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28>,
			      <MBUS_ID(0x01, 0x5e) 0 0x100000>;
                        #address-cells = <1>;
                        #size-cells = <0>;
                        pinctrl-0 = <&spi0_pins1>;
                        pinctrl-names = "default";
                        cell-index = <0>;
                        interrupts = <30>;
                        clocks = <&coreclk 0>;
                        status = "disabled";
		};
	};

Quoted 7 lines

but then we have a
problem with how it uses both internal-regs and and its own mbus based reg, so we probably have to move the spi node outside of the internal-regs node to achieve that, similar to how we handle the devbus devices:

Quoted 16 lines

      soc@ {
              spi0 {
                        compatible = "marvell,armada-370-spi",
                                     "marvell,orion-spi";
                      reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28>,
                            <MBUS_ID(0x01, 0x5e) 0 0x100000>;
                        #address-cells = <1>;
                        #size-cells = <0>;
                        pinctrl-0 = <&spi0_pins1>;
                        pinctrl-names = "default";
                        cell-index = <0>;
                        interrupts = <30>;
                        clocks = <&coreclk 0>;
                        status = "disabled";
              };
      };

Do I understand this correctly, that you suggest to list all MBus windows here, that the SoC supports (e.g. 8 for the Armada XP). And let the SPI driver then extract and dynamically enable (map) the one that is currently used?

Quoted 24 lines · On Thursday 24 March 2016 17:15:49 Stefan Roese wrote:

Quoted 23 lines

but then we have a
problem with how it uses both internal-regs and and its own mbus based reg, so we probably have to move the spi node outside of the internal-regs node to achieve that, similar to how we handle the devbus devices:

       soc@ {
               spi0 {
                         compatible = "marvell,armada-370-spi",
                                      "marvell,orion-spi";
                       reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28>,
                             <MBUS_ID(0x01, 0x5e) 0 0x100000>;
                         #address-cells = <1>;
                         #size-cells = <0>;
                         pinctrl-0 = <&spi0_pins1>;
                         pinctrl-names = "default";
                         cell-index = <0>;
                         interrupts = <30>;
                         clocks = <&coreclk 0>;
                         status = "disabled";
               };
       };

Quoted 4 lines

Do I understand this correctly, that you suggest to list all MBus windows here, that the SoC supports (e.g. 8 for the Armada XP). And let the SPI driver then extract and dynamically enable (map) the one that is currently used?

I had not realize that there is more than one per controller. Is it one per chip-select, or how do you pick the right ones?

Quoted 17 lines

      soc@ {
              spi0 {
                        compatible = "marvell,armada-370-spi",
                                     "marvell,orion-spi";
                      reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28>,
                            <MBUS_ID(0x01, 0x5e) 0 0x100000>;
                        #address-cells = <1>;
                        #size-cells = <0>;
                        pinctrl-0 = <&spi0_pins1>;
                        pinctrl-names = "default";
                        cell-index = <0>;
                        interrupts = <30>;
                        clocks = <&coreclk 0>;
                        status = "disabled";
              };
      };

Do I understand this correctly, that you suggest to list all MBus windows here, that the SoC supports (e.g. 8 for the Armada XP). And let the SPI driver then extract and dynamically enable (map) the one that is currently used?

We also need a per-SPI-device DT property to enable this direct access mode for this specific SPI device. As not all SPI devices support this mode - at least not yet. How about this one:

                flash0: flash at 0 {
                        compatible = "m25p128";
                        reg = <0>;
                        direct-access-enable;
                        ...
                };

?

Quoted 18 lines · On Thursday 24 March 2016 17:15:49 Stefan Roese wrote:

Quoted 17 lines

       soc@ {
               spi0 {
                         compatible = "marvell,armada-370-spi",
                                      "marvell,orion-spi";
                       reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28>,
                             <MBUS_ID(0x01, 0x5e) 0 0x100000>;
                         #address-cells = <1>;
                         #size-cells = <0>;
                         pinctrl-0 = <&spi0_pins1>;
                         pinctrl-names = "default";
                         cell-index = <0>;
                         interrupts = <30>;
                         clocks = <&coreclk 0>;
                         status = "disabled";
               };
       };

Quoted 4 lines

Do I understand this correctly, that you suggest to list all MBus windows here, that the SoC supports (e.g. 8 for the Armada XP). And let the SPI driver then extract and dynamically enable (map) the one that is currently used?

Actually the syntax above doesn't imply any mapping at all, it just describes how the hardware is wired up, so that really needs to list all windows, and then the ranges property in the soc node can statically map the ones that are used on the particular machine.

Quoted 11 lines

We also need a per-SPI-device DT property to enable this direct access mode for this specific SPI device. As not all SPI devices support this mode - at least not yet. How about this one:

                 flash0: flash at 0 {
                         compatible = "m25p128";
                         reg = <0>;
                         direct-access-enable;
                         ...
                 };

Quoted 1 line

?

Maybe the spi driver can just check whether the window is mapped or not, and then have no ranges entry for the devices that don't support it?

Alternatively, we might encode this in the 'reg' property in some way?

How do we know whether a device supports the mode or not?

Quoted 24 lines · Arnd Bergmann

Quoted 18 lines · On Thursday 24 March 2016 17:15:49 Stefan Roese wrote:

Quoted 17 lines

       soc@ {
               spi0 {
                         compatible = "marvell,armada-370-spi",
                                      "marvell,orion-spi";
                       reg = <MBUS_ID(0xf0, 0x01) 0x10600 0x28>,
                             <MBUS_ID(0x01, 0x5e) 0 0x100000>;
                         #address-cells = <1>;
                         #size-cells = <0>;
                         pinctrl-0 = <&spi0_pins1>;
                         pinctrl-names = "default";
                         cell-index = <0>;
                         interrupts = <30>;
                         clocks = <&coreclk 0>;
                         status = "disabled";
               };
       };

Quoted 4 lines

Do I understand this correctly, that you suggest to list all MBus windows here, that the SoC supports (e.g. 8 for the Armada XP). And let the SPI driver then extract and dynamically enable (map) the one that is currently used?

Quoted 5 lines

Actually the syntax above doesn't imply any mapping at all, it just describes how the hardware is wired up, so that really needs to list all windows, and then the ranges property in the soc node can statically map the ones that are used on the particular machine.

I see. But with this we are back to statically mapping the MBus windows that are used. Resulting in potentially multiple windows per SPI controller, which is not necessary (Andrew and Mark objected against that).

Quoted 4 lines

Alternatively, we might encode this in the 'reg' property in some way?

How do we know whether a device supports the mode or not?

Currently, only a very limited number of SPI devices will support it. As its really only useful right now for full-speed tx-transfer. In my case its downloading a bitstream into a FPGA (only SPI writes). In the future, SPI NOR flash devices can be supported. But this needs additional work. So SPI devices should not use this direct access mode as default for now. It needs to get opted-in via some DT property.

Quoted 4 lines · On Thursday 24 March 2016 18:51:53 Stefan Roese wrote:

I see. But with this we are back to statically mapping the MBus windows that are used. Resulting in potentially multiple windows per SPI controller, which is not necessary (Andrew and Mark objected against that).

I'm not following here. Do you mean we should set up and tear down the windows in the runtime PM callbacks so they are only present when we actually access a device instead?

Among the dts files we ship with the kernel, how many would actually use more than one mapping in practice if we decide to do the static ranges property? I had a quick look and could not even find one that has more than one chip-select connected.

Ok, so with the static mapping it could be done very easily, or we need a more complex solution for the dynamic mapping.

Quoted 10 lines · Arnd Bergmann

Quoted 4 lines · On Thursday 24 March 2016 18:51:53 Stefan Roese wrote:

I see. But with this we are back to statically mapping the MBus windows that are used. Resulting in potentially multiple windows per SPI controller, which is not necessary (Andrew and Mark objected against that).

I'm not following here. Do you mean we should set up and tear down the windows in the runtime PM callbacks so they are only present when we actually access a device instead?

What we're trying to avoid is allocating a window to every single client device attached to SPI, the controller can only talk to one device at once anyway so having one per device is wasteful when this is a limited resource. Looking at this from the point of view of a SPI driver it just seems obviously bad, in order to get the best performance with this controller we need to do a special per-device magic binding. That's not good for usability, it's not ideal having to do it per controller but at least then it's done for the SoC.

We also already apparently have some dynamic code for PCIe.

Quoted 4 lines

Among the dts files we ship with the kernel, how many would actually use more than one mapping in practice if we decide to do the static ranges property? I had a quick look and could not even find one that has more than one chip-select connected.

Do any of these boards have things like expansion connectors?

Quoted 2 lines

Ok, so with the static mapping it could be done very easily, or we need a more complex solution for the dynamic mapping.

Part of what I personally don't understand is why this is complicated?

There is nothing magic in the binding if we just do the same thing the flash driver does, and describe the memory range that is associated with a chipselect.

Quoted 4 lines

Quoted 2 lines

Ok, so with the static mapping it could be done very easily, or we need a more complex solution for the dynamic mapping.

Part of what I personally don't understand is why this is complicated?

I think we'd need to add another special case in the bus driver for it, which otherwise should be able to handle this in a generic
way: if we just use the existing binding, the spi host driver can simply call devm_ioremap_resource() to see if there is a map for a given chipselect and otherwise fall back to the current mode.

Quoted 3 lines · Arnd Bergmann

There is nothing magic in the binding if we just do the same thing the flash driver does, and describe the memory range that is associated with a chipselect.

Why are we even doing that though? Like I say it just seems pointlessly unhelpful for users. Are we really saying that every single system has to go through and manually modify their DT so that they can use this entirely in SoC feature? That doesn't seem like winning...

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Ok, so with the static mapping it could be done very easily, or we need a more complex solution for the dynamic mapping.

Part of what I personally don't understand is why this is complicated?

Quoted 6 lines

I think we'd need to add another special case in the bus driver for it, which otherwise should be able to handle this in a generic
way: if we just use the existing binding, the spi host driver can simply call devm_ioremap_resource() to see if there is a map for a given chipselect and otherwise fall back to the current mode.

Why does the binding have to be in the client device to do that?

Quoted 4 lines · On Friday 25 March 2016 15:50:32 Mark Brown wrote:

Why are we even doing that though? Like I say it just seems pointlessly unhelpful for users. Are we really saying that every single system has to go through and manually modify their DT so that they can use this entirely in SoC feature? That doesn't seem like winning...

I don't remember the exact details, but I think we had come up with a generic constraint solving algorithm to pick appropriate windows for each device with a (close to) minimal use of physical address space,

Quoted 4 lines

Quoted 4 lines

way: if we just use the existing binding, the spi host driver can simply call devm_ioremap_resource() to see if there is a map for a given chipselect and otherwise fall back to the current mode.

Quoted 1 line

Why does the binding have to be in the client device to do that?

I don't understand the question. The client device here is the SPI controller, right? That doesn't need to have a special binding, it just needs to list the registers relative to the mbus address space like any other device, and that is independent of how we implement it.

It looks like Stefan's patch tricks here by creating an incompatible binding for mbus that bypasses the address mapping.

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · On Friday 25 March 2016 15:50:32 Mark Brown wrote:

Why are we even doing that though? Like I say it just seems pointlessly unhelpful for users. Are we really saying that every single system has to go through and manually modify their DT so that they can use this entirely in SoC feature? That doesn't seem like winning...

Quoted 3 lines

I don't remember the exact details, but I think we had come up with a generic constraint solving algorithm to pick appropriate windows for each device with a (close to) minimal use of physical address space,

So there are some complicated constraints that make it hard to allocate from the bus space?

From the Armada XP data sheet, I can also see that it's possible to map

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

way: if we just use the existing binding, the spi host driver can simply call devm_ioremap_resource() to see if there is a map for a given chipselect and otherwise fall back to the current mode.

Quoted 7 lines

Quoted 1 line

Why does the binding have to be in the client device to do that?

I don't understand the question. The client device here is the SPI controller, right? That doesn't need to have a special binding, it just needs to list the registers relative to the mbus address space like any other device, and that is independent of how we implement it.

If you're saying we're doing a binding per chip select presumably that binding appears in the SPI device even if it's the controller driver that parses it. This was what the original patch did, I pushed back since it's a fairly common pattern for poor quality bindings where people for some reason want to add per device properties for whatever new feature they want to support instead of doing it at the controller level (and ideally just doing it without requring any configuration).

Quoted 2 lines

It looks like Stefan's patch tricks here by creating an incompatible binding for mbus that bypasses the address mapping.

I've not looked at the patch and honestly nobody has been talking about any generic binding, all I've seen is the per device property in the original patch.

Quoted 2 lines · On Friday 25 March 2016 22:39:22 Mark Brown wrote:

So there are some complicated constraints that make it hard to allocate from the bus space?

Yes, it's basically an optimization problem: the closer you can squeeze everything together, the more address space you have left for RAM and/or PCI.

What we really have though is additional registers that belong to the SPI controller and that are not part of internal-regs, so we need to move it up one level out of the internal-regs node in order to add more registers:

I think it makes a lot of sense to define a separate window for each CS at boot time, just so you don't have to reprogram the windows manually. After all, the entire point of the direct mode is to avoid having to do any of the setup work, and have the SPI master set the right CS itself based on the MBus ID that is used for accessing the slave mmio window. This is also required if we want to enable things like XIP (DaX) mappings for file systems on a SPI-NOR flash.

Quoted 8 lines

I think it makes a lot of sense to define a separate window for each CS at boot time, just so you don't have to reprogram the windows manually. After all, the entire point of the direct mode is to avoid having to do any of the setup work, and have the SPI master set the right CS itself based on the MBus ID that is used for accessing the slave mmio window. This is also required if we want to enable things like XIP (DaX) mappings for file systems on a SPI-NOR flash.

Well, in the cases where we have one device on the bus then it's not a big deal since we can check what the last thing we set was. The direct access stuff is going to have trouble if we have multiple devices on the bus since we try to mix it with non-MMIO access we run the risk of conflicting simultaneous use unless we continue to route everything through the SPI subsystem (like we do with the current flash read support).

It really only makes a difference if the reprogramming process happens a lot and is expensive relative to the transfers and that doesn't seem like something I'd expect.

Quoted 7 lines · On Tuesday 29 March 2016 09:47:58 Mark Brown wrote:

Well, in the cases where we have one device on the bus then it's not a big deal since we can check what the last thing we set was. The direct access stuff is going to have trouble if we have multiple devices on the bus since we try to mix it with non-MMIO access we run the risk of conflicting simultaneous use unless we continue to route everything through the SPI subsystem (like we do with the current flash read support).

Maybe we can avoid that if we enforce at the driver level that we use the same mode for all slaves? The way I read the manual, I think that's how it is intended at least.

Also, as mentioned we don't have any machine with more than one SPI slave so far, so we don't really need to overengineer it and can go for the simplest implementation in the SPI master driver.

Quoted 9 lines · Arnd Bergmann

Quoted 7 lines · On Tuesday 29 March 2016 09:47:58 Mark Brown wrote:

Well, in the cases where we have one device on the bus then it's not a big deal since we can check what the last thing we set was. The direct access stuff is going to have trouble if we have multiple devices on the bus since we try to mix it with non-MMIO access we run the risk of conflicting simultaneous use unless we continue to route everything through the SPI subsystem (like we do with the current flash read support).

Quoted 3 lines

Maybe we can avoid that if we enforce at the driver level that we use the same mode for all slaves? The way I read the manual, I think that's how it is intended at least.

Well, we currently don't have any XIP or whatever support at all, it's still only in the SPI operations so it's academic - that'd be a future issue if we did start doing things that accessed the memory map outside of the SPI flow.

Quoted 3 lines

Also, as mentioned we don't have any machine with more than one SPI slave so far, so we don't really need to overengineer it and can go for the simplest implementation in the SPI master driver.

For me the simplest thing seems like just using one window for all the devices, it seems more likely to deliver useful results without the system integrator having to think about how to configure this for optimisation.

Quoted 4 lines · On Tuesday 29 March 2016 12:52:30 Mark Brown wrote:

Well, we currently don't have any XIP or whatever support at all, it's still only in the SPI operations so it's academic - that'd be a future issue if we did start doing things that accessed the memory map outside of the SPI flow.

Isn't this just about implementing .point()/.unpoint() in the spi-nor driver?

Quoted 4 lines

For me the simplest thing seems like just using one window for all the devices, it seems more likely to deliver useful results without the system integrator having to think about how to configure this for optimisation.

Do you mean single-window mode? I don't see the difference to the other modes, but that's certainly fine with me if it helps.

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · On Tuesday 29 March 2016 12:52:30 Mark Brown wrote:

Well, we currently don't have any XIP or whatever support at all, it's still only in the SPI operations so it's academic - that'd be a future issue if we did start doing things that accessed the memory map outside of the SPI flow.

Quoted 2 lines

Isn't this just about implementing .point()/.unpoint() in the spi-nor driver?

Implementing that would require the SPI core to export something providing access to the memory regions. We don't currently do that, it'd require us to deal with the interaction with access as a SPI device (this hardware often seems to only accelerate reads so we still need to use regular SPI access for writes).

Quoted 7 lines

Quoted 4 lines

For me the simplest thing seems like just using one window for all the devices, it seems more likely to deliver useful results without the system integrator having to think about how to configure this for optimisation.

Do you mean single-window mode? I don't see the difference to the other modes, but that's certainly fine with me if it helps.

I don't like having to assign windows per device, like I say it seems like more work.

Quoted 5 lines · On Tuesday 29 March 2016 14:00:18 Mark Brown wrote:

Implementing that would require the SPI core to export something providing access to the memory regions. We don't currently do that, it'd require us to deal with the interaction with access as a SPI device (this hardware often seems to only accelerate reads so we still need to use regular SPI access for writes).

The manual lists both read and write accesses as handled through direct mode.

Quoted 2 lines

I don't like having to assign windows per device, like I say it seems like more work.

Ok, but that's something else that can be implemented independently as I explained earlier: We currently rely on the windows to be assigned in the DT, but it's also possible to extend the mbus driver in a way that lets it pick its own windows in one way or another. It should

not impact the binding for the SPI host controller in any way, the binding just describes how the internal addressing is wired up to the mbus in hardware, and the mbus driver takes care of mapping those into CPU addresses.

Quoted 7 lines · Arnd Bergmann

Quoted 5 lines · On Tuesday 29 March 2016 14:00:18 Mark Brown wrote:

Implementing that would require the SPI core to export something providing access to the memory regions. We don't currently do that, it'd require us to deal with the interaction with access as a SPI device (this hardware often seems to only accelerate reads so we still need to use regular SPI access for writes).

Quoted 2 lines

The manual lists both read and write accesses as handled through direct mode.

That is not in general the case for this sort of hardware, and there's also erase support to consider here.

Quoted 7 lines

Quoted 2 lines

I don't like having to assign windows per device, like I say it seems like more work.

Ok, but that's something else that can be implemented independently as I explained earlier: We currently rely on the windows to be assigned in the DT, but it's also possible to extend the mbus driver in a way that lets it pick its own windows in one way or another. It should

That's what I've been asking for but people keep saying it's not possible...

Quoted 4 lines

not impact the binding for the SPI host controller in any way, the binding just describes how the internal addressing is wired up to the mbus in hardware, and the mbus driver takes care of mapping those into CPU addresses.

If we're able to do it automatically without baking this into the DT and there's enough space to cope with this and PCI together then that's more reasonable.

Quoted 2 lines · On Tuesday 29 March 2016 14:28:42 Mark Brown wrote:

Quoted 1 line · Arnd Bergmann

On Tuesday 29 March 2016 14:00:18 Mark Brown wrote:

Quoted 8 lines

Quoted 4 lines

not impact the binding for the SPI host controller in any way, the binding just describes how the internal addressing is wired up to the mbus in hardware, and the mbus driver takes care of mapping those into CPU addresses.

If we're able to do it automatically without baking this into the DT and there's enough space to cope with this and PCI together then that's more reasonable.

Ok, good. Then let's just do the SPI host driver like all the other mbus slaves using whatever mbus provides, and implement dynamic remapping of mbus windows whenever someone gets too annoyed with having to come up with a mapping for their board.

At least by doing SPI like all the other devices, we don't make it harder to come up with a generic algorithm for assigning the windows at boot time.

This will make the SPI driver patch very simple but will generate quite a big Armada dts change, moving all the SPI controllers from the 'internal-regs' node to the 'soc' node though.

Quoted 3 lines

This will make the SPI driver patch very simple but will generate quite a big Armada dts change, moving all the SPI controllers from the 'internal-regs' node to the 'soc' node though.

Hi Stefan

There was been talk about making use of the labels in dts files, rather than keeping with the tree structure. So while you are moving the spi nodes, made you can also change the style.

Yes, I wanted to suggest this change as well. I hope its acceptable that I only change the SPI nodes to using the "labels style" in this attempt though?

Quoted 2 lines · Mark Brown

I haven't looked at your new code at all. What I'm saying is that specifying a per-device MBus window seems like pointless complexity.

I don't necessarily share this opinions. Code-wise, its less complex that re-configuring (removing the old and creating the new) the MBus window. But I have no strong feeling here. Whatever is decided that should be used, I can go with.

Quoted 9 lines · Stefan Roese

Quoted 2 lines · Mark Brown

I haven't looked at your new code at all. What I'm saying is that specifying a per-device MBus window seems like pointless complexity.

I don't necessarily share this opinions. Code-wise, its less complex that re-configuring (removing the old and creating the new) the MBus window. But I have no strong feeling here. Whatever is decided that should be used, I can go with.

No, really - it's just unhelpful. Putting this in the ABI means that every single system integrator who cares about performance is going to need to go and manually figure out how to configure this in DT and manually select values. That's not doing a good job for users, it's making their lives harder for no gain. If there are no physical constraints then how we allocate space in the MBus should be a runtime thing, it shouldn't be part of the ABI.

Quoted 7 lines · Mark Brown

No, really - it's just unhelpful. Putting this in the ABI means that every single system integrator who cares about performance is going to need to go and manually figure out how to configure this in DT and manually select values. That's not doing a good job for users, it's making their lives harder for no gain. If there are no physical constraints then how we allocate space in the MBus should be a runtime thing, it shouldn't be part of the ABI.

Do I understand you correctly, that you want the complete MBus allocation and configuration to be included in the SPI driver instead of using the DT to describe the window (target, attribute, area)? If yes, then the SPI driver would be the first driver to do this "internally". All other drivers using MBus resources parse these from the DT (AFAIK) as this is highly SoC specific.

Please note that this direct access mode is really not suitable for "normal" SPI devices like SPI NOR chips. As the direct read mode (as described in chapter 22.5.1 of the Armada XP datasheet) always generates:

a) a write command to the SPI bus (opcode configurable via register)

and

b) writes 1-4 address bytes to the SPI bus

before the data is read from the SPI bus.

So its definitely nothing that should be enabled per default for all SPI devices.

Quoted 7 lines

Please note that this direct access mode is really not suitable for "normal" SPI devices like SPI NOR chips. As the direct read mode (as described in chapter 22.5.1 of the Armada XP datasheet) always generates:

a) a write command to the SPI bus (opcode configurable via register)

Quoted 8 lines

and

b) writes 1-4 address bytes to the SPI bus

before the data is read from the SPI bus.

So its definitely nothing that should be enabled per default for all SPI devices.

Looking at the Kirkwood datasheet, it suggests it can be used with SPI flash:

If using SPI flash, set the <Attr> field in the CPU address decoding windows to match the SPI interface (see Section 2, Address Map, on page 34 for more details). Any CPU read to this address space is converted by the SPI controller to a SPI flash read transaction, composed of an address phase, followed by a data phase.

The actual sequence that is driven on the SPI interface is:

1. Assert SPI_CSn. Write command to the SPI. The command is either Read or Fast_Read, based on the configuration of the <DirectRdCommand> field.

2. Write the address. The Address is driven in 1 to 4 phases based on the configuration of the <DirectAddrLen>> field. The order of the bytes is MSB to LSB.

3. In Fast_Read mode, add a one-byte dummy write.

4. Read the data, according to the length given by the request.

5. De-assert the SPI_CSn signal.

There is also something interesting in the Direct Write to SPI section:

4. Address Phase.

This is a 1-4 byte field that is taken from the address of the request. The size of the address is configured via the <DirectAddrLen> field in the Serial Memory Interface Configuration Register (Table 629 p. 686). The entire Address phase is omitted when <Direct Wr Addr Enable> field in the Serial Memory Direct Write Configuration Register (Table 634 p. 688) is cleared.

Turning off the address phase may mean it is possible to do normal SPI writes using the direct mode.

Quite a few Kirkwood boards have SPI flash, so if this brings better performance, it would be nice to be able to use it.

Right. It is of course possible to use this direct mode to access SPI flash. Its just not the way how the SPI MTD driver uses the SPI controller. All information that is written to the SPI flash (e.g. commands, addresses) are passed as plain tx-data in the SPI message to the SPI controller. And it would be not easy to detect commands / addresses vs. "real data" in this orion SPI driver to put these values into the specific registers.

Quoted 7 lines · Stefan Roese

Right. It is of course possible to use this direct mode to access SPI flash. Its just not the way how the SPI MTD driver uses the SPI controller. All information that is written to the SPI flash (e.g. commands, addresses) are passed as plain tx-data in the SPI message to the SPI controller. And it would be not easy to detect commands / addresses vs. "real data" in this orion SPI driver to put these values into the specific registers.

We now have extensions for passing commands through directly (see spi_flash_read()).

Has there been any discussion on whether dynamically adding the MBus window is a good idea, as opposed to statically defining it in the board .dts ?

So far, the only driver that was using dynamically allocated MBus is the PCIe controller driver, because there is no way in advanced to know the number and memory window requirements of the PCIe devices that will be connected to the system.

For all other windows (BootROM, crypto SRAM and more recently network related SRAM), we are using statically allocated windows.