From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Thu, 30 Jul 2020 08:23:09 +0200
Subject: [PATCH v2 06/10] drivers: spi: Add SPI controller driver for
 Octeon
In-Reply-To: <e3ebed29-534d-4741-bdc3-b26230021b03@denx.de>
References: <20200723101724.953325-1-sr@denx.de>
 <20200723101724.953325-7-sr@denx.de>
 <af15c11a2657009f45e9d4cec4c8d8e58a6cb27c.camel@gmail.com>
 <e3ebed29-534d-4741-bdc3-b26230021b03@denx.de>
Message-ID: <d44958d9-1050-63c0-5b3d-68d58d098918@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.
> 
> 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).

Thanks,
Stefan