[PATCH v2 06/10] drivers: spi: Add SPI controller driver for Octeon
Daniel Schwierzeck
daniel.schwierzeck at gmail.com
Thu Jul 30 13:00:03 CEST 2020
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. 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
More information about the U-Boot
mailing list