Re: [PATCH v2 3/6] spi: dw: Add Microchip Sparx5 support

[Lars Povlsen <lars.povlsen@microchip.com>]

Mark Brown writes:

> On Tue, Jun 23, 2020 at 03:53:22PM +0200, Lars Povlsen wrote:
> > Mark Brown writes:
> 
> > >If there's a mux that needs to be handled specially that mux should be
> > >described in DT on the relevant boards, there shouldn't just be
> > >something hard coded in the controller driver.
> 
> > I looked at the spi-mux driver, but that is more for muxing the CS's, as
> > I understand - not the actual bus segment. I could use it, but it would
> 
> It doesn't matter that much exactly what signals get switched I think,
> we can't really tell by the time we get back to the controller.
> 
> > require encoding the bus segment into the CS (double the normal
> > range). Also, selecting the bus interface is tightly coupled to the
> > controller - its not an externally constructed board mux.
> 
> It sounds like this controller should be describing a mux all the time -
> if there's completely separate output buses that you can switch between
> then we'll need to know about that if someone wires up a GPIO chip
> select.

Mark,

I had to tinker a bit with this to get my head around it.

I added the mux driver, and made the cs/bus configuration reside here -
all well and done.

For our reference config we have something like:

  mux: mux-controller {
          compatible = "microchip,sparx5-spi-mux";
          #mux-control-cells = <0>;
          mux@0 {
                  reg = <0>;
                  microchip,bus-interface = <0>;
          };
          mux@e {
                  reg = <14>;
                  microchip,bus-interface = <1>;
          };
  };

Then I tried to use the existing spi-mux as you suggested. But I
realized as its really designed for CS muxing I had to instantiate each
SPI device in its own spi-mux instance, repeating the CS (as we don't
want that to change). The result was kinda bulky.

An example would be:

  spi0: spi@600104000 {
          compatible = "microchip,sparx5-spi";
          spi@0 {
                  compatible = "spi-mux";
                  mux-controls = <&mux>;
                  reg = <0>;
                  spi-flash@0 {
                          compatible = "jedec,spi-nor";
                          reg = <0>;
                  };
          };
          spi@e {
                  compatible = "spi-mux";
                  mux-controls = <&mux>;
                  reg = <14>;
                  spi-flash@e {
                          compatible = "spi-nand";
                          reg = <14>;
                  };
          };
  };

I then looked a bit at other users of the mux framework,
drivers/mtd/hyperbus/hbmc-am654.c specifically.

I then added direct use of a mux, by the well-established "mux-controls"
DT property. The result was much cleaner, IMHO, and allows the spi and
mux to be connected directly in the base sparx5 DT. Code impact was
really small.

Both examples use the same mux configuration, and as the "mux-controls"
is established by default in the base spi0 node, this (directly) yields:

&spi0 {
	spi-flash@0 {
		compatible = "jedec,spi-nor";
		reg = <0>;
	};
	spi-flash@e {
		compatible = "spi-nand";
		reg = <14>;
	};
};

I will be sending the new revision of the patches shortly. I look
forward to your comments.

I also CC'ed Peter Rosin as the MUX subsystem maintainer. Peter, sorry
for sticking you halfway into a conversation, but I thought you might
want to be informed. You are also on the recipient list of the v3
patches, so now you know why...

Sincerely,

-- 
Lars Povlsen,
Microchip