[PATCH v3 02/21] spi: dw: Add DWC SSI capability
Currently DWC SSI core is supported by means of setting up the
core-specific update_cr0() callback. It isn't suitable for multiple
reasons. First of all having exported several methods doing the same thing
but for different chips makes the code harder to maintain. Secondly the
spi-dw-core driver exports the methods, then the spi-dw-mmio driver sets
the private data callback with one of them so to be called by the core
driver again. That makes the code logic too complicated. Thirdly using
callbacks for just updating the CR0 register is problematic, since in case
if the register needed to be updated from different parts of the code,
we'd have to create another callback (for instance the SPI device-specific
parameters don't need to be calculated each time the SPI transfer is
submitted, so it's better to pre-calculate the CR0 data at the SPI-device
setup stage).
So keeping all the above in mind let's discard the update_cr0() callbacks,
define a generic and static dw_spi_update_cr0() method and create the
DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the
alternative CR0 register layout.
While at it add the comments to the code path of the normal DW APB SSI
controller setup to make the dw_spi_update_cr0() method looking coherent.
Signed-off-by: Serge Semin
---
Changelog v2:
- Get back the in-code comments to the dw_spi_update_cr0() method and it'
further derivatives.
Changelog v3:
- Remove dw_spi_update_cr0() callback assignment from the DW APB SSI PCI
glue-driver.
---
drivers/spi/spi-dw-core.c | 80 ++-
drivers/spi/spi-dw-mmio.c | 20 +-
drivers/spi/spi-dw-pci.c | 6 ---
drivers/spi/spi-dw.h | 9 +
4 files changed, 40 insertions(+), 75 deletions(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 3a7fdca8d335..be16fdaf7ce0 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -228,60 +228,56 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
return dws->transfer_handler(dws);
}
-/* Configure CTRLR0 for DW_apb_ssi */
-u32 dw_spi_update_cr0(struct spi_controller *master, struct spi_device *spi,
- struct spi_transfer *transfer)
+static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi,
+ struct spi_transfer *transfer)
{
struct chip_data *chip = spi_get_ctldata(spi);
u32 cr0;
- /* Default SPI mode is SCPOL = 0, SCPH = 0 */
- cr0 = (transfer->bits_per_word - 1)
- | (SSI_MOTO_SPI << SPI_FRF_OFFSET)
- | spi->mode & SPI_CPOL) ? 1 : 0) << SPI_SCOL_OFFSET) |
- (((spi->mode & SPI_CPHA) ? 1 : 0) << SPI_SCPH_OFFSET) |
- (((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET))
- | (chip->tmode << SPI_TMOD_OFFSET);
+ /* CTRLR0[ 4/3: 0] Data Frame Size */
+ cr0 = (transfer->bits_per_word - 1);
- return cr0;
-}
-EXPORT_SYMBOL_GPL(dw_spi_update_cr0);
+ if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
+ /* CTRLR0[ 5: 4] Frame Format */
+ cr0 |= SSI_MOTO_SPI << SPI_FRF_OFFSET;
-/* Configure CTRLR0 for DWC_ssi */
-u32 dw_spi_update_cr0_v1_01a(struct spi_controller *master,
-struct spi_device *spi,
-struct spi_transfer *transfer)
-{
- struct dw_spi *dws = spi_controller_get_devdata(master);
- struct chip_data *chip = spi_get_ctldata(spi);
- u32 cr0;
+ /*
+* SPI mode (SCPOL|SCPH)
+* CTRLR0[ 6] Serial Clock Phase
+* CTRLR0[ 7] Serial Clock Polarity
+*/
+ cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << SPI_SCOL_OFFSET;
+ cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << SPI_SCPH_OFFSET;
- /* CTRLR0[ 4: 0] Data Frame Size */
- cr0 = (transfer->bits_per_word - 1);
+ /* CTRLR0[11] Shift Register Loop */
+ cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET;
- /* CTRLR0[ 7: 6] Frame Format */
- cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
+ /* CTRLR0[ 9:8] Transfer Mode */
+ cr0 |= chip->tmode << SPI_TMOD_OFFSET;
+ } else {
+ /* CTRLR0[ 7: 6] Frame Format */
+ cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;
- /*
-* SPI mode (SCPOL|SCPH)
-* CTRLR0[ 8] Serial Clock Phase
-* CTRLR0[ 9] Serial Clock Polarity
-*/
- cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) << DWC_SSI_CTRLR0_SCPOL_OFFSET;
- cr0 |= ((spi->mode & SPI_CPHA) ? 1 : 0) << DWC_SSI_CTRLR0_SCPH_OFFSET;
+ /*
+* SPI mode (SCPOL|SCPH)
+* CTRLR0[ 8] Serial Clock Phase
+* CTRLR0[ 9] Serial Clock Polarity
+*/
+ cr0 |= ((spi->mode & SPI_CPOL) ? 1 : 0) <<
DWC_SSI_CTRLR0_SCPOL_OFFSET;
+ c
Re: [PATCH v3 02/21] spi: dw: Add DWC SSI capability
On Fri, Oct 02, 2020 at 01:28:10AM +0300, Serge Semin wrote: > Currently DWC SSI core is supported by means of setting up the > core-specific update_cr0() callback. It isn't suitable for multiple > reasons. First of all having exported several methods doing the same thing > but for different chips makes the code harder to maintain. Secondly the > spi-dw-core driver exports the methods, then the spi-dw-mmio driver sets > the private data callback with one of them so to be called by the core > driver again. That makes the code logic too complicated. Thirdly using > callbacks for just updating the CR0 register is problematic, since in case > if the register needed to be updated from different parts of the code, > we'd have to create another callback (for instance the SPI device-specific > parameters don't need to be calculated each time the SPI transfer is > submitted, so it's better to pre-calculate the CR0 data at the SPI-device > setup stage). > > So keeping all the above in mind let's discard the update_cr0() callbacks, > define a generic and static dw_spi_update_cr0() method and create the > DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the > alternative CR0 register layout. > > While at it add the comments to the code path of the normal DW APB SSI > controller setup to make the dw_spi_update_cr0() method looking coherent. What the point to increase indentation level and produce additional churn? Can't you simply leave functions, unexport them, and call in one conditional of whatever new function is called? I have an impression that split of the series is done in a way that first patches in the series are not optimized to what is done in the last patches in the series. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 02/21] spi: dw: Add DWC SSI capability
On Fri, Oct 02, 2020 at 01:19:29PM +0300, Andy Shevchenko wrote: > On Fri, Oct 02, 2020 at 01:28:10AM +0300, Serge Semin wrote: > > Currently DWC SSI core is supported by means of setting up the > > core-specific update_cr0() callback. It isn't suitable for multiple > > reasons. First of all having exported several methods doing the same thing > > but for different chips makes the code harder to maintain. Secondly the > > spi-dw-core driver exports the methods, then the spi-dw-mmio driver sets > > the private data callback with one of them so to be called by the core > > driver again. That makes the code logic too complicated. Thirdly using > > callbacks for just updating the CR0 register is problematic, since in case > > if the register needed to be updated from different parts of the code, > > we'd have to create another callback (for instance the SPI device-specific > > parameters don't need to be calculated each time the SPI transfer is > > submitted, so it's better to pre-calculate the CR0 data at the SPI-device > > setup stage). > > > > So keeping all the above in mind let's discard the update_cr0() callbacks, > > define a generic and static dw_spi_update_cr0() method and create the > > DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the > > alternative CR0 register layout. > > > > While at it add the comments to the code path of the normal DW APB SSI > > controller setup to make the dw_spi_update_cr0() method looking coherent. > > What the point to increase indentation level and produce additional churn? > Can't you simply leave functions, unexport them, and call in one conditional > of > whatever new function is called? I forgot to mention that in the commit log, there is another reason why it's better to create a generic dw_spi_update_cr0() instead of doing what you suggest. As it will be seen from the following up patches, the dw_spi_update_cr0() function (to be more precise it's successor, but anyway) will be used from the SPI memory ops implementation. So if-else-ing here and there isn't a good idea for maintainability. For the same reason of the maintainability it's better to have a generic method which reflects all the config peculiarities, so in case of any changes they would be not be forgotten to be introduced for both DWC SSI and DW APB SSI parts of the setup procedures. As I see it that overbeats the additional indentation level drawback. -Sergey > > I have an impression that split of the series is done in a way that first > patches in the series are not optimized to what is done in the last patches in > the series. > > -- > With Best Regards, > Andy Shevchenko > >
Re: [PATCH v3 02/21] spi: dw: Add DWC SSI capability
On Fri, Oct 2, 2020 at 8:18 PM Serge Semin wrote: > > On Fri, Oct 02, 2020 at 01:19:29PM +0300, Andy Shevchenko wrote: > > On Fri, Oct 02, 2020 at 01:28:10AM +0300, Serge Semin wrote: > > > Currently DWC SSI core is supported by means of setting up the > > > core-specific update_cr0() callback. It isn't suitable for multiple > > > reasons. First of all having exported several methods doing the same thing > > > but for different chips makes the code harder to maintain. Secondly the > > > spi-dw-core driver exports the methods, then the spi-dw-mmio driver sets > > > the private data callback with one of them so to be called by the core > > > driver again. That makes the code logic too complicated. Thirdly using > > > callbacks for just updating the CR0 register is problematic, since in case > > > if the register needed to be updated from different parts of the code, > > > we'd have to create another callback (for instance the SPI device-specific > > > parameters don't need to be calculated each time the SPI transfer is > > > submitted, so it's better to pre-calculate the CR0 data at the SPI-device > > > setup stage). > > > > > > So keeping all the above in mind let's discard the update_cr0() callbacks, > > > define a generic and static dw_spi_update_cr0() method and create the > > > DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the > > > alternative CR0 register layout. > > > > > > While at it add the comments to the code path of the normal DW APB SSI > > > controller setup to make the dw_spi_update_cr0() method looking coherent. > > > > > What the point to increase indentation level and produce additional churn? > > Can't you simply leave functions, unexport them, and call in one > > conditional of > > whatever new function is called? > > I forgot to mention that in the commit log, there is another reason why it's > better to create a generic dw_spi_update_cr0() instead of doing what you > suggest. > As it will be seen from the following up patches, the dw_spi_update_cr0() > function > (to be more precise it's successor, but anyway) will be used from the SPI > memory > ops implementation. So if-else-ing here and there isn't a good idea for > maintainability. For the same reason of the maintainability it's better to > have a > generic method which reflects all the config peculiarities, so in case of any > changes they would be not be forgotten to be introduced for both DWC SSI and > DW > APB SSI parts of the setup procedures. As I see it that overbeats the > additional > indentation level drawback. What I meant is to leave functions as is and call them under conditional if () call one else call another -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 02/21] spi: dw: Add DWC SSI capability
On Fri, Oct 02, 2020 at 09:26:07PM +0300, Andy Shevchenko wrote: > On Fri, Oct 2, 2020 at 8:18 PM Serge Semin > wrote: > > > > On Fri, Oct 02, 2020 at 01:19:29PM +0300, Andy Shevchenko wrote: > > > On Fri, Oct 02, 2020 at 01:28:10AM +0300, Serge Semin wrote: > > > > Currently DWC SSI core is supported by means of setting up the > > > > core-specific update_cr0() callback. It isn't suitable for multiple > > > > reasons. First of all having exported several methods doing the same > > > > thing > > > > but for different chips makes the code harder to maintain. Secondly the > > > > spi-dw-core driver exports the methods, then the spi-dw-mmio driver sets > > > > the private data callback with one of them so to be called by the core > > > > driver again. That makes the code logic too complicated. Thirdly using > > > > callbacks for just updating the CR0 register is problematic, since in > > > > case > > > > if the register needed to be updated from different parts of the code, > > > > we'd have to create another callback (for instance the SPI > > > > device-specific > > > > parameters don't need to be calculated each time the SPI transfer is > > > > submitted, so it's better to pre-calculate the CR0 data at the > > > > SPI-device > > > > setup stage). > > > > > > > > So keeping all the above in mind let's discard the update_cr0() > > > > callbacks, > > > > define a generic and static dw_spi_update_cr0() method and create the > > > > DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the > > > > alternative CR0 register layout. > > > > > > > > While at it add the comments to the code path of the normal DW APB SSI > > > > controller setup to make the dw_spi_update_cr0() method looking > > > > coherent. > > > > > > > > What the point to increase indentation level and produce additional churn? > > > Can't you simply leave functions, unexport them, and call in one > > > conditional of > > > whatever new function is called? > > > > I forgot to mention that in the commit log, there is another reason why it's > > better to create a generic dw_spi_update_cr0() instead of doing what you > > suggest. > > As it will be seen from the following up patches, the dw_spi_update_cr0() > > function > > (to be more precise it's successor, but anyway) will be used from the SPI > > memory > > ops implementation. So if-else-ing here and there isn't a good idea for > > maintainability. For the same reason of the maintainability it's better to > > have a > > generic method which reflects all the config peculiarities, so in case of > > any > > changes they would be not be forgotten to be introduced for both DWC SSI > > and DW > > APB SSI parts of the setup procedures. As I see it that overbeats the > > additional > > indentation level drawback. > > What I meant is to leave functions as is and call them under conditional > > if () > call one > else > call another Yeah, I understood what you meant. What you suggest would be a better solution if I needed to call the dw_spi_update_cr0() method just from a single place of the driver (but in that case I wouldn't need to replace the callback-based approach with the Capabilities-based one at all). The thing is that the dw_spi_update_cr0() will be also called from the SPI memory exec_op() callback (see patch "[PATCH v3 17/21] spi: dw: Add memory operations support" and the method dw_spi_update_config() invocation) in the same way as it is called from the SPI core transfer-one callback. Following your suggestion I would have to implement the same "if () call one else call another" pattern there too. Copying it here and there would be a weak design from the maintainability point of view and from the coding style too. Much better is to create a generic dw_spi_update_cr0() (later in this patchset it will be renamed to dw_spi_update_config()...), which would work for both DWC SSI and DW APB SSI by embedding the "if (is_CAP) call one else call another" into the method itself as I suggested in this patch. -Sergey > > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH v3 02/21] spi: dw: Add DWC SSI capability
On Fri, Oct 02, 2020 at 10:46:09PM +0300, Serge Semin wrote: > On Fri, Oct 02, 2020 at 09:26:07PM +0300, Andy Shevchenko wrote: > > On Fri, Oct 2, 2020 at 8:18 PM Serge Semin > > wrote: > > > > > > On Fri, Oct 02, 2020 at 01:19:29PM +0300, Andy Shevchenko wrote: > > > > On Fri, Oct 02, 2020 at 01:28:10AM +0300, Serge Semin wrote: > > > > > Currently DWC SSI core is supported by means of setting up the > > > > > core-specific update_cr0() callback. It isn't suitable for multiple > > > > > reasons. First of all having exported several methods doing the same > > > > > thing > > > > > but for different chips makes the code harder to maintain. Secondly > > > > > the > > > > > spi-dw-core driver exports the methods, then the spi-dw-mmio driver > > > > > sets > > > > > the private data callback with one of them so to be called by the core > > > > > driver again. That makes the code logic too complicated. Thirdly using > > > > > callbacks for just updating the CR0 register is problematic, since in > > > > > case > > > > > if the register needed to be updated from different parts of the code, > > > > > we'd have to create another callback (for instance the SPI > > > > > device-specific > > > > > parameters don't need to be calculated each time the SPI transfer is > > > > > submitted, so it's better to pre-calculate the CR0 data at the > > > > > SPI-device > > > > > setup stage). > > > > > > > > > > So keeping all the above in mind let's discard the update_cr0() > > > > > callbacks, > > > > > define a generic and static dw_spi_update_cr0() method and create the > > > > > DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the > > > > > alternative CR0 register layout. > > > > > > > > > > While at it add the comments to the code path of the normal DW APB SSI > > > > > controller setup to make the dw_spi_update_cr0() method looking > > > > > coherent. > > > > > > > > > > > What the point to increase indentation level and produce additional > > > > churn? > > > > Can't you simply leave functions, unexport them, and call in one > > > > conditional of > > > > whatever new function is called? > > > > > > I forgot to mention that in the commit log, there is another reason why > > > it's > > > better to create a generic dw_spi_update_cr0() instead of doing what you > > > suggest. > > > As it will be seen from the following up patches, the dw_spi_update_cr0() > > > function > > > (to be more precise it's successor, but anyway) will be used from the SPI > > > memory > > > ops implementation. So if-else-ing here and there isn't a good idea for > > > maintainability. For the same reason of the maintainability it's better > > > to have a > > > generic method which reflects all the config peculiarities, so in case of > > > any > > > changes they would be not be forgotten to be introduced for both DWC SSI > > > and DW > > > APB SSI parts of the setup procedures. As I see it that overbeats the > > > additional > > > indentation level drawback. > > > > > What I meant is to leave functions as is and call them under conditional > > > > if () > > call one > > else > > call another > > Yeah, I understood what you meant. What you suggest would be a better solution > if I needed to call the dw_spi_update_cr0() method just from a single place of > the driver (but in that case I wouldn't need to replace the callback-based > approach with the Capabilities-based one at all). The thing is that the > dw_spi_update_cr0() will be also called from the SPI memory exec_op() callback > (see patch "[PATCH v3 17/21] spi: dw: Add memory operations support" and the > method dw_spi_update_config() invocation) in the same way as it is called from > the SPI core transfer-one callback. Following your suggestion I would have to > implement the same "if () call one else call another" pattern there too. > Copying > it here and there would be a weak design from the maintainability point of > view > and from the coding style too. Much better is to create a generic > dw_spi_update_cr0() (later in this patchset it will be renamed to > dw_spi_update_config()...), which would work for both DWC SSI and DW APB SSI > by > embedding the "if (is_CAP) call one else call another" into the method itself > as > I suggested in this patch. Oh, and the same "if-else" pattern would need to be either left in the dw_spi_get_cr0()/dw_spi_prepare_cr0() or added around the dw_spi_prepare_cr0() method invocation with creating two versions of it. So no, I'd stick with the design I suggested in this patch: just two "if-else"s and the generic versions of the dw_spi_prepare_cr0() and dw_spi_update_cr0() functions. -Sergey > > -Sergey > > > > > > > -- > > With Best Regards, > > Andy Shevchenko
Re: [PATCH v3 02/21] spi: dw: Add DWC SSI capability
On Fri, Oct 02, 2020 at 11:08:29PM +0300, Serge Semin wrote: > On Fri, Oct 02, 2020 at 10:46:09PM +0300, Serge Semin wrote: > > On Fri, Oct 02, 2020 at 09:26:07PM +0300, Andy Shevchenko wrote: > > > On Fri, Oct 2, 2020 at 8:18 PM Serge Semin > > > wrote: > > > > > > > > On Fri, Oct 02, 2020 at 01:19:29PM +0300, Andy Shevchenko wrote: > > > > > On Fri, Oct 02, 2020 at 01:28:10AM +0300, Serge Semin wrote: > > > > > > Currently DWC SSI core is supported by means of setting up the > > > > > > core-specific update_cr0() callback. It isn't suitable for multiple > > > > > > reasons. First of all having exported several methods doing the > > > > > > same thing > > > > > > but for different chips makes the code harder to maintain. Secondly > > > > > > the > > > > > > spi-dw-core driver exports the methods, then the spi-dw-mmio driver > > > > > > sets > > > > > > the private data callback with one of them so to be called by the > > > > > > core > > > > > > driver again. That makes the code logic too complicated. Thirdly > > > > > > using > > > > > > callbacks for just updating the CR0 register is problematic, since > > > > > > in case > > > > > > if the register needed to be updated from different parts of the > > > > > > code, > > > > > > we'd have to create another callback (for instance the SPI > > > > > > device-specific > > > > > > parameters don't need to be calculated each time the SPI transfer is > > > > > > submitted, so it's better to pre-calculate the CR0 data at the > > > > > > SPI-device > > > > > > setup stage). > > > > > > > > > > > > So keeping all the above in mind let's discard the update_cr0() > > > > > > callbacks, > > > > > > define a generic and static dw_spi_update_cr0() method and create > > > > > > the > > > > > > DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the > > > > > > alternative CR0 register layout. > > > > > > > > > > > > While at it add the comments to the code path of the normal DW APB > > > > > > SSI > > > > > > controller setup to make the dw_spi_update_cr0() method looking > > > > > > coherent. > > > > > > > > > > > > > > What the point to increase indentation level and produce additional > > > > > churn? > > > > > Can't you simply leave functions, unexport them, and call in one > > > > > conditional of > > > > > whatever new function is called? > > > > > > > > I forgot to mention that in the commit log, there is another reason why > > > > it's > > > > better to create a generic dw_spi_update_cr0() instead of doing what > > > > you suggest. > > > > As it will be seen from the following up patches, the > > > > dw_spi_update_cr0() function > > > > (to be more precise it's successor, but anyway) will be used from the > > > > SPI memory > > > > ops implementation. So if-else-ing here and there isn't a good idea for > > > > maintainability. For the same reason of the maintainability it's better > > > > to have a > > > > generic method which reflects all the config peculiarities, so in case > > > > of any > > > > changes they would be not be forgotten to be introduced for both DWC > > > > SSI and DW > > > > APB SSI parts of the setup procedures. As I see it that overbeats the > > > > additional > > > > indentation level drawback. > > > > > > > > What I meant is to leave functions as is and call them under conditional > > > > > > if () > > > call one > > > else > > > call another > > > > Yeah, I understood what you meant. What you suggest would be a better > > solution > > if I needed to call the dw_spi_update_cr0() method just from a single place > > of > > the driver (but in that case I wouldn't need to replace the callback-based > > approach with the Capabilities-based one at all). The thing is that the > > dw_spi_update_cr0() will be also called from the SPI memory exec_op() > > callback > > (see patch "[PATCH v3 17/21] spi: dw: Add memory operations support" and the > > method dw_spi_update_config() invocation) in the same way as it is called > > from > > the SPI core transfer-one callback. Following your suggestion I would have > > to > > implement the same "if () call one else call another" pattern there too. > > Copying > > it here and there would be a weak design from the maintainability point of > > view > > and from the coding style too. Much better is to create a generic > > dw_spi_update_cr0() (later in this patchset it will be renamed to > > dw_spi_update_config()...), which would work for both DWC SSI and DW APB > > SSI by > > embedding the "if (is_CAP) call one else call another" into the method > > itself as > > I suggested in this patch. > > Oh, and the same "if-else" pattern would need to be either left in the > dw_spi_get_cr0()/dw_spi_prepare_cr0() or added around the dw_spi_prepare_cr0() * I meant dw_spi_update_cr0() here... -Sergey > method invocation with creating two versions of it. So no, I'd stick with the > design I suggested in this patch: just two "if-else"s and the generic versions > of the dw_
