Hi Daniel,
On 30.07.20 13:00, Daniel Schwierzeck wrote:
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 sgarapati@marvell.com
Adds support for SPI controllers found on Octeon II/III and Octeon TX TX2 SoC platforms.
Signed-off-by: Aaron Williams awilliams@marvell.com Signed-off-by: Suneel Garapati sgarapati@marvell.com Signed-off-by: Stefan Roese sr@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Aaron Williams awilliams@marvell.com Cc: Chandrakala Chavva cchavva@marvell.com Cc: Jagan Teki jagan@amarulasolutions.com
<snip>
+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.
A, nice. Thanks for looking into this.
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 weneed
* 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,};
This selection via driver_data does not work, as Octeon TX and Octeon TX2 have the same compatible for probing. I've changed this to a runtime compatible detection (TX2 only) instead and it works just fine.
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.
Great. I'll make the necessary changes and will send out v3 shortly.
Thanks, Stefan