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 <[email protected]>
Adds support for SPI controllers found on Octeon II/III and Octeon TX
TX2 SoC platforms.
Signed-off-by: Aaron Williams <[email protected]>
Signed-off-by: Suneel Garapati <[email protected]>
Signed-off-by: Stefan Roese <[email protected]>
Cc: Daniel Schwierzeck <[email protected]>
Cc: Aaron Williams <[email protected]>
Cc: Chandrakala Chavva <[email protected]>
Cc: Jagan Teki <[email protected]>
<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 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,
};
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
