[PATCH v2 06/10] drivers: spi: Add SPI controller driver for Octeon
Stefan Roese
sr at denx.de
Thu Jul 30 13:42:12 CEST 2020
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 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.
>
> 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
More information about the U-Boot
mailing list