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

[Daniel Schwierzeck <daniel.schwierzeck@gmail.com>]

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. 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,
 };
 
> > 
> > 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.

-- 
- Daniel