Hi Sergey,
Thanks again for the reviews. I've been able to work on this recently
and test the changes using 5.10.28 on a production server. I'm going
back to the beginning to reply to each comment and work towards
closure of open issues before preparing patchset v3 which will need to
be re-done against the latest linux-next.
On Wed, Mar 3, 2021 at 10:44 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
>
> Hello Brad.
> Thanks for the patch. See my comments below.
>
> On Wed, Mar 03, 2021 at 07:41:36PM -0800, Brad Larson wrote:
> > The Pensando Elba SoC uses a GPIO based chip select
> > for two DW SPI busses with each bus having two
> > chip selects.
>
> I see a contradiction here. Normally GPIO-based chip-select is a
> property of a platform, but not a SoC/CPU/MCU/etc. Most of the time
> SoC SPI interfaces still get to have native CS pins, while at some
> platform configurations (like in case of DW APB SPI, which doesn't
> provide a way to directly toggle its native CSs) it's easier or even
> safer to use GPIOs as CS signals. Of course theoretically a SoC could
> be synthesized so it doesn't have native CS output pins, but only some
> virtual internal CS flags, but I've never seen such. Anyway according
> to the custom CS method below it's not your case because you still
> provide support for SPI-devices handled by native CS (else branch in
> the if (spi->cs_gpiod) {} else {} statement).
The native DW CS is not supported, that code is removed which caused
the confusion. The existing dw_spi_set_cs() works fine with the
updated version of this function being
/*
* Using a GPIO-based chip-select, the DW SPI controller still needs
* its own CS bit selected to start the serial engine. On Elba the
* specific CS doesn't matter, so use CS0.
*/
static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
{
spi->chip_select = 0;
dw_spi_set_cs(spi, enable);
}
which is much better than the original version shown below
> > +static void dw_spi_elba_set_cs(struct spi_device *spi, bool enable)
> > +{
> > + struct dw_spi *dws = spi_master_get_devdata(spi->master);
> > +
> > + if (!enable) {
> > + if (spi->cs_gpiod) {
> > + /*
> > + * Using a GPIO-based chip-select, the DW SPI
> > + * controller still needs its own CS bit selected
> > + * to start the serial engine. On Elba the specific
> > + * CS doesn't matter, so use CS0.
> > + */
> > + dw_writel(dws, DW_SPI_SER, BIT(0));
> > + } else {
> > + /*
> > + * Using the intrinsic DW chip-select; set the
> > + * appropriate CS.
> > + */
> > + dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
> > + }
> > - } else
> + } else {
> > + dw_writel(dws, DW_SPI_SER, 0);
> + } /* See [1] */
> > +}
>
> The custom CS-method above doesn't look much different from the
> dw_spi_set_cs() method defined in the spi-dw-core.o driver, except
> having at least two problems:
> 1) It assumes that "enable" argument means the CS-enabling flag, while
> in fact it's the CS-level which depending on the SPI_CS_HIGH flag
> set/cleared will be 1/0 respectively if CS is supposed to be enabled.
> That aspect has already been fixed in the dw_spi_set_cs() method.
> 2) The method enables CS[0] if GPIO-CS is used for a particular SPI
> device. That will cause problems for a GPIO/native CS intermixed case
> of having for instance one SPI-device connected to native CS[0] and
> another one - to a GPIO. So trying to communicate with the second SPI
> device you'll end up having the native CS[0] activated too thus
> having an SPI transfer sent to two SPI-device at the same time.
> Of course that's not what you'd want.
>
> Anyway I don't really see why you even need a custom CS method here. A
> generic method dw_spi_set_cs() shall work for your SPI interface.
> If I am wrong, please explain why. Did you try the generic CS method
> on your platform?
>
> [1] Placing Braces and Spaces. Chapter 3). Documentation/process/coding-style.rst
Yes, exactly. The generic method dw_spi_set_cs() works ok and
correctly handles active high/low.
> > +static int dw_spi_elba_init(struct platform_device *pdev,
> > + struct dw_spi_mmio *dwsmmio)
> > +{
> > + dwsmmio->dws.set_cs = dw_spi_elba_set_cs;
> > +
> > + return 0;
> > +}
> > +
> > static int dw_spi_mmio_probe(struct platform_device *pdev)
> > {
> > int (*init_func)(struct platform_device *pdev,
> > @@ -351,6 +383,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
> > { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
> > { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> > { .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
>
> > + { .compatible = "pensando,elba-spi", .data = dw_spi_elba_init },
>
> If you agree with me and remove the custom CS-method defined above in
> this patch, then all you'll need is just to add "pensando,elba-spi" here
> with generic init-callback set - dw_spi_dw_apb_init.
The existing dw_spi_set_cs() is now being used. Using
dw_spi_dw_apb_init results in every spi transfer failing which is why
dw_spi_elba_init() is still proposed which results in set_cs calling
dw_spi_elba_set_cs().
> Finally defining new compatible string requires the bindings update.
> In the framework of DW APB SPI interface they are defined in:
> Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> So you need to have that DT-schema accordingly altered.
>
> The bindings note concerns the rest of the updates in your patchset too.
>
> -Sergey
Patchset v2 separated out the bindings updates. There will be more
bindings needed for v3 of the patchset. I won't be sending v3 until
all discussions are resolved.
Regards,
Brad
