[PATCH v2 06/10] drivers: spi: Add SPI controller driver for Octeon

[Stefan Roese]

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 at marvell.com>
>>
>> Adds support for SPI controllers found on Octeon II/III and Octeon TX
>> TX2 SoC platforms.
>>
>> Signed-off-by: Aaron Williams <awilliams at marvell.com>
>> Signed-off-by: Suneel Garapati <sgarapati at marvell.com>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>> Cc: Aaron Williams <awilliams at marvell.com>
>> Cc: Chandrakala Chavva <cchavva at marvell.com>
>> Cc: Jagan Teki <jagan at 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.

Or do you have some other idea on how to implement this in a "better
way"?

Thanks,
Stefan