From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Schwierzeck Date: Thu, 30 Jul 2020 13:00:03 +0200 Subject: [PATCH v2 06/10] drivers: spi: Add SPI controller driver for Octeon In-Reply-To: References: <20200723101724.953325-1-sr@denx.de> <20200723101724.953325-7-sr@denx.de>

Message-ID: <962416102381326caf3fbb43d93233ccbaf860b7.camel@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Am Donnerstag, den 30.07.2020, 08:23 +0200 schrieb Stefan Roese: > Hi Daniel, > > On 30.07.20 07:49, Stefan Roese wrote: > > Hi Daniel, > > > > On 24.07.20 15:56, Daniel Schwierzeck wrote: > > > Am Donnerstag, den 23.07.2020, 12:17 +0200 schrieb Stefan Roese: > > > > From: Suneel Garapati > > > > > > > > Adds support for SPI controllers found on Octeon II/III and Octeon TX > > > > TX2 SoC platforms. > > > > > > > > Signed-off-by: Aaron Williams > > > > Signed-off-by: Suneel Garapati > > > > Signed-off-by: Stefan Roese > > > > Cc: Daniel Schwierzeck > > > > Cc: Aaron Williams > > > > Cc: Chandrakala Chavva > > > > Cc: Jagan Teki > > > > > > > > > > +static const struct dm_spi_ops octeon_spi_ops = { > > > > + .claim_bus = octeon_spi_claim_bus, > > > > + .release_bus = octeon_spi_release_bus, > > > > + .set_speed = octeon_spi_set_speed, > > > > + .set_mode = octeon_spi_set_mode, > > > > + .xfer = octeon_spi_xfer, > > > > +}; > > > > + > > > > +static const struct dm_spi_ops octeontx2_spi_ops = { > > > > + .claim_bus = octeon_spi_claim_bus, > > > > + .release_bus = octeon_spi_release_bus, > > > > + .set_speed = octeon_spi_set_speed, > > > > + .set_mode = octeon_spi_set_mode, > > > > + .xfer = octeontx2_spi_xfer, > > > > + .mem_ops = &octeontx2_spi_mem_ops, > > > > +}; > > > > + > > > > +static int octeon_spi_probe(struct udevice *dev) > > > > +{ > > > > + struct octeon_spi *priv = dev_get_priv(dev); > > > > + const struct octeon_spi_data *data; > > > > + int ret; > > > > + > > > > + data = (const struct octeon_spi_data *)dev_get_driver_data(dev); > > > > + if (data->probe == PROBE_PCI) { > > > > + pci_dev_t bdf = dm_pci_get_bdf(dev); > > > > + > > > > + debug("SPI PCI device: %x\n", bdf); > > > > + priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0, > > > > + PCI_REGION_MEM); > > > > + } else { > > > > + priv->base = dev_remap_addr(dev); > > > > + } > > > > + > > > > + priv->base += data->reg_offs; > > > > + > > > > + /* Octeon TX2 needs a different ops struct */ > > > > + if (device_is_compatible(dev, "cavium,thunderx-spi")) { > > > > + /* > > > > + * "ops" is const and can't be written directly. So we need > > > > + * to write the Octeon TX2 ops value using this trick > > > > + */ > > > > + writeq((u64)&octeontx2_spi_ops, (void *)&dev->driver->ops); > > > > + } > > > > > > can't you simply add a xfer() function pointer to "struct > > > octeon_spi_data" and assign the according xfer function to it? Then > > > in octeon_spi_xfer() you can simply call that function pointer. With > > > this you only need one instance of "struct dm_spi_ops" and don't need > > > this ugly hack ;) > > > > Unfortuantely its not that easy, as the Octeon TX2 ops struct also has > > a " mem_ops" member, which the driver does not support for the other > > Octeon models. I could clear this "mem_ops" member in the non Octeon > > TX2 case, which is a bit better than the 2nd ops struct. But its still > > not really elegent. I had a look and the struct dm_spi_ops don't have a strict requirement to be const. It doesn't matter for "struct driver : const void *ops" if it points to struct dm_spi_ops linked to .data or .rodata, it can only be assigned once. If struct dm_spi_ops is linked to .data, you can simply re-assign some function pointers during probe. I would suggest the following change: @@ -82,6 +82,7 @@ void board_acquire_flash_arb(bool acquire); struct octeon_spi_data { int probe; u32 reg_offs; + bool is_octeontx; }; /* Local driver data structure */ @@ -559,7 +560,7 @@ static int octeon_spi_set_mode(struct udevice *bus, uint mode) return 0; } -static const struct dm_spi_ops octeon_spi_ops = { +static struct dm_spi_ops octeon_spi_ops = { .claim_bus = octeon_spi_claim_bus, .release_bus = octeon_spi_release_bus, .set_speed = octeon_spi_set_speed, @@ -567,15 +568,6 @@ static const struct dm_spi_ops octeon_spi_ops = { .xfer = octeon_spi_xfer, }; -static const struct dm_spi_ops octeontx2_spi_ops = { - .claim_bus = octeon_spi_claim_bus, - .release_bus = octeon_spi_release_bus, - .set_speed = octeon_spi_set_speed, - .set_mode = octeon_spi_set_mode, - .xfer = octeontx2_spi_xfer, - .mem_ops = &octeontx2_spi_mem_ops, -}; - static int octeon_spi_probe(struct udevice *dev) { struct octeon_spi *priv = dev_get_priv(dev); @@ -596,12 +588,9 @@ static int octeon_spi_probe(struct udevice *dev) priv->base += data->reg_offs; /* Octeon TX2 needs a different ops struct */ - if (device_is_compatible(dev, "cavium,thunderx-spi")) { - /* - * "ops" is const and can't be written directly. So we need - * to write the Octeon TX2 ops value using this trick - */ - writeq((u64)&octeontx2_spi_ops, (void *)&dev->driver- >ops); + if (data->is_octeontx) { + octeon_spi_ops.xfer = octeontx2_spi_xfer; + octeon_spi_ops.mem_ops = &octeontx2_spi_mem_ops; } ret = clk_get_by_index(dev, 0, &priv->clk); @@ -620,11 +609,13 @@ static int octeon_spi_probe(struct udevice *dev) static const struct octeon_spi_data spi_octeon_data = { .probe = PROBE_DT, .reg_offs = 0x0000, + .is_octeontx = false, }; static const struct octeon_spi_data spi_octeontx_data = { .probe = PROBE_PCI, .reg_offs = 0x1000, + .is_octeontx = true, }; > > > > Or do you have some other idea on how to implement this in a "better > > way"? > > BTW: If this SPI driver is the only patch in this series, that you feel > is not ready, then I suggest to drop this one from this patch series > and push the remaining ones to mainline (if they have no issues of > course). > if you are okay with my suggestion you could send a v3 of just the SPI driver. I think I'll have time tomorrow to prepare another pull request. -- - Daniel