| Message ID | 20210930120700.2564-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "spi: modify set_cs_timing parameter" | expand |
On Thu, Sep 30, 2021 at 02:07:00PM +0200, Dafna Hirschfeld wrote: > This reverts commit 04e6bb0d6bb127bac929fb35edd2dd01613c9520. Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone. > This revert the commit 'spi: modify set_cs_timing parameter' > and its following commit > 'spi: mediatek: fix build warnning in set cs timing'. Which is not what the commit message nor the paste of the full hash claimed :/ > Those commits cause regression on mt8173 elm device. The EC either is not > able to register or it sends numerous amount of errors: > cros-ec-i2c-tunnel 1100a000.spi:ec@0:i2c-tunnel0: Error transferring EC i2c message -71 > cros-ec-spi spi0.0: EC failed to respond in time. Do we have any analysis as to why? Do these devices use timing parameters in some way for example, or do the values written out to the device change in some way? You've provided no analysis here so it's hard to tell if this is just some random change that happens to change code generation slighly or if there's some actual reason why this might fix something. I'll note that as far as I can see there are no users of this API upstream so I'm guessing that you've got some out of tree consumer driver which uses the API, it's possible that there was some error in updating that driver to the new interface which is causing the issue.
hi, thanks for the fast feedback On 30.09.21 14:25, Mark Brown wrote: > On Thu, Sep 30, 2021 at 02:07:00PM +0200, Dafna Hirschfeld wrote: >> This reverts commit 04e6bb0d6bb127bac929fb35edd2dd01613c9520. > > Please submit patches using subject lines reflecting the style for the > subsystem, this makes it easier for people to identify relevant patches. > Look at what existing commits in the area you're changing are doing and > make sure your subject lines visually resemble what they're doing. > There's no need to resubmit to fix this alone. > >> This revert the commit 'spi: modify set_cs_timing parameter' >> and its following commit >> 'spi: mediatek: fix build warnning in set cs timing'. > > Which is not what the commit message nor the paste of the full hash > claimed :/ What is the paste of the full hash? Since the second commit is only a warning fixes I thought it is cumbersome to send two separate reverting patches. Should I? > >> Those commits cause regression on mt8173 elm device. The EC either is not >> able to register or it sends numerous amount of errors: > >> cros-ec-i2c-tunnel 1100a000.spi:ec@0:i2c-tunnel0: Error transferring EC i2c message -71 >> cros-ec-spi spi0.0: EC failed to respond in time. > > Do we have any analysis as to why? Do these devices use timing > parameters in some way for example, or do the values written out to the > device change in some way? > > You've provided no analysis here so it's hard to tell if this is just > some random change that happens to change code generation slighly or if > there's some actual reason why this might fix something. I'll note that > as far as I can see there are no users of this API upstream so I'm > guessing that you've got some out of tree consumer driver which uses the > API, it's possible that there was some error in updating that driver to > the new interface which is causing the issue. Actually the original commit not only change that callback 'set_cs_timing' but it also calls 'mtk_spi_set_hw_cs_timing' directly from the function "mtk_spi_prepare_message". So this actually influences all devices bound to this driver (in upstream) I did some printing and it does change values that are written to registers. Thanks, Dafna >
On Thu, Sep 30, 2021 at 02:36:01PM +0200, Dafna Hirschfeld wrote: > hi, thanks for the fast feedback > > On 30.09.21 14:25, Mark Brown wrote: > > On Thu, Sep 30, 2021 at 02:07:00PM +0200, Dafna Hirschfeld wrote: > > > This reverts commit 04e6bb0d6bb127bac929fb35edd2dd01613c9520. > > Which is not what the commit message nor the paste of the full hash > > claimed :/ > What is the paste of the full hash? The above. > Since the second commit is only a warning fixes I thought it is cumbersome to > send two separate reverting patches. Should I? No, you should write a proper commit log with (like I said) a normal subject line - basically, follow the process in submitting-patches.rst. > > Do we have any analysis as to why? Do these devices use timing > > parameters in some way for example, or do the values written out to the > > device change in some way? > > You've provided no analysis here so it's hard to tell if this is just > > some random change that happens to change code generation slighly or if > > there's some actual reason why this might fix something. I'll note that > > as far as I can see there are no users of this API upstream so I'm > > guessing that you've got some out of tree consumer driver which uses the > > API, it's possible that there was some error in updating that driver to > > the new interface which is causing the issue. > Actually the original commit not only change that callback 'set_cs_timing' but it also > calls 'mtk_spi_set_hw_cs_timing' directly from the function "mtk_spi_prepare_message". > So this actually influences all devices bound to this driver (in upstream) > I did some printing and it does change values that are written to registers. OK, so that's something that should have been in the commit log, preferrably in a more detailed form that identifies what the change is. However changing the values written out is clearly not the intent of the patch and it is a substantially better API so can we not just fix things so that the old values are written out? Why are we jumping straight to a revert here?
On 30.09.21 14:46, Mark Brown wrote: > On Thu, Sep 30, 2021 at 02:36:01PM +0200, Dafna Hirschfeld wrote: >> hi, thanks for the fast feedback >> >> On 30.09.21 14:25, Mark Brown wrote: >>> On Thu, Sep 30, 2021 at 02:07:00PM +0200, Dafna Hirschfeld wrote: >>>> This reverts commit 04e6bb0d6bb127bac929fb35edd2dd01613c9520. > >>> Which is not what the commit message nor the paste of the full hash >>> claimed :/ > >> What is the paste of the full hash? > > The above. > >> Since the second commit is only a warning fixes I thought it is cumbersome to >> send two separate reverting patches. Should I? > > No, you should write a proper commit log with (like I said) a normal > subject line - basically, follow the process in submitting-patches.rst. > >>> Do we have any analysis as to why? Do these devices use timing >>> parameters in some way for example, or do the values written out to the >>> device change in some way? > >>> You've provided no analysis here so it's hard to tell if this is just >>> some random change that happens to change code generation slighly or if >>> there's some actual reason why this might fix something. I'll note that >>> as far as I can see there are no users of this API upstream so I'm >>> guessing that you've got some out of tree consumer driver which uses the >>> API, it's possible that there was some error in updating that driver to >>> the new interface which is causing the issue. > >> Actually the original commit not only change that callback 'set_cs_timing' but it also >> calls 'mtk_spi_set_hw_cs_timing' directly from the function "mtk_spi_prepare_message". >> So this actually influences all devices bound to this driver (in upstream) >> I did some printing and it does change values that are written to registers. > > OK, so that's something that should have been in the commit log, > preferrably in a more detailed form that identifies what the change is. > However changing the values written out is clearly not the intent of the > patch and it is a substantially better API so can we not just fix things > so that the old values are written out? Why are we jumping straight to > a revert here? It could be that the values written to the register in the new version of "mtk_spi_set_hw_cs_timing" are the same as with the previous version. I didn't check that. The difference is that before that patch the function was not called so it was a dead code. Now it is called and causes erros. Without the datasheet it is hard to know how to fix it. I responded to that patch two days ago explaining that but Mason Zhang didn't respond yet. Thanks, Dafna >
On Thu, Sep 30, 2021 at 03:06:52PM +0200, Dafna Hirschfeld wrote: > On 30.09.21 14:46, Mark Brown wrote: > > On Thu, Sep 30, 2021 at 02:36:01PM +0200, Dafna Hirschfeld wrote: > > > Actually the original commit not only change that callback 'set_cs_timing' but it also > > > calls 'mtk_spi_set_hw_cs_timing' directly from the function "mtk_spi_prepare_message". > > > So this actually influences all devices bound to this driver (in upstream) > > > I did some printing and it does change values that are written to registers. > > OK, so that's something that should have been in the commit log, > > preferrably in a more detailed form that identifies what the change is. > > However changing the values written out is clearly not the intent of the > > patch and it is a substantially better API so can we not just fix things > > so that the old values are written out? Why are we jumping straight to > > a revert here? > It could be that the values written to the register in the new version of "mtk_spi_set_hw_cs_timing" are the same > as with the previous version. I didn't check that. The difference is that before that patch > the function was not called so it was a dead code. Now it is called and causes erros. > Without the datasheet it is hard to know how to fix it. I responded to that patch two days ago explaining > that but Mason Zhang didn't respond yet. The hardware supports register readback (it's doing a read/modify/write to set the new values) so it should be possible to look at what the default values that get written out are and work out what the default parameters should be set to to give the same effect (instead of 1 which is what's currently used). I would say just skip the delays if they are zero but that means that if only one device on a bus has a delay configured there'd be problems. Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c index 386e8c84be0a..aa7da41a8bd0 100644 --- a/drivers/spi/spi-mt65xx.c +++ b/drivers/spi/spi-mt65xx.c @@ -208,65 +208,6 @@ static void mtk_spi_reset(struct mtk_spi *mdata) writel(reg_val, mdata->base + SPI_CMD_REG); } -static int mtk_spi_set_hw_cs_timing(struct spi_device *spi) -{ - struct mtk_spi *mdata = spi_master_get_devdata(spi->master); - struct spi_delay *cs_setup = &spi->cs_setup; - struct spi_delay *cs_hold = &spi->cs_hold; - struct spi_delay *cs_inactive = &spi->cs_inactive; - u32 setup, hold, inactive; - u32 reg_val; - int delay; - - delay = spi_delay_to_ns(cs_setup, NULL); - if (delay < 0) - return delay; - setup = (delay * DIV_ROUND_UP(mdata->spi_clk_hz, 1000000)) / 1000; - - delay = spi_delay_to_ns(cs_hold, NULL); - if (delay < 0) - return delay; - hold = (delay * DIV_ROUND_UP(mdata->spi_clk_hz, 1000000)) / 1000; - - delay = spi_delay_to_ns(cs_inactive, NULL); - if (delay < 0) - return delay; - inactive = (delay * DIV_ROUND_UP(mdata->spi_clk_hz, 1000000)) / 1000; - - setup = setup ? setup : 1; - hold = hold ? hold : 1; - inactive = inactive ? inactive : 1; - - reg_val = readl(mdata->base + SPI_CFG0_REG); - if (mdata->dev_comp->enhance_timing) { - hold = min_t(u32, hold, 0x10000); - setup = min_t(u32, setup, 0x10000); - reg_val &= ~(0xffff << SPI_ADJUST_CFG0_CS_HOLD_OFFSET); - reg_val |= (((hold - 1) & 0xffff) - << SPI_ADJUST_CFG0_CS_HOLD_OFFSET); - reg_val &= ~(0xffff << SPI_ADJUST_CFG0_CS_SETUP_OFFSET); - reg_val |= (((setup - 1) & 0xffff) - << SPI_ADJUST_CFG0_CS_SETUP_OFFSET); - } else { - hold = min_t(u32, hold, 0x100); - setup = min_t(u32, setup, 0x100); - reg_val &= ~(0xff << SPI_CFG0_CS_HOLD_OFFSET); - reg_val |= (((hold - 1) & 0xff) << SPI_CFG0_CS_HOLD_OFFSET); - reg_val &= ~(0xff << SPI_CFG0_CS_SETUP_OFFSET); - reg_val |= (((setup - 1) & 0xff) - << SPI_CFG0_CS_SETUP_OFFSET); - } - writel(reg_val, mdata->base + SPI_CFG0_REG); - - inactive = min_t(u32, inactive, 0x100); - reg_val = readl(mdata->base + SPI_CFG1_REG); - reg_val &= ~SPI_CFG1_CS_IDLE_MASK; - reg_val |= (((inactive - 1) & 0xff) << SPI_CFG1_CS_IDLE_OFFSET); - writel(reg_val, mdata->base + SPI_CFG1_REG); - - return 0; -} - static int mtk_spi_prepare_message(struct spi_master *master, struct spi_message *msg) { @@ -343,8 +284,6 @@ static int mtk_spi_prepare_message(struct spi_master *master, << SPI_CFG1_GET_TICK_DLY_OFFSET); writel(reg_val, mdata->base + SPI_CFG1_REG); - /* set hw cs timing */ - mtk_spi_set_hw_cs_timing(spi); return 0; } @@ -590,6 +529,52 @@ static bool mtk_spi_can_dma(struct spi_master *master, (unsigned long)xfer->rx_buf % 4 == 0); } +static int mtk_spi_set_hw_cs_timing(struct spi_device *spi, + struct spi_delay *setup, + struct spi_delay *hold, + struct spi_delay *inactive) +{ + struct mtk_spi *mdata = spi_master_get_devdata(spi->master); + u16 setup_dly, hold_dly, inactive_dly; + u32 reg_val; + + if ((setup && setup->unit != SPI_DELAY_UNIT_SCK) || + (hold && hold->unit != SPI_DELAY_UNIT_SCK) || + (inactive && inactive->unit != SPI_DELAY_UNIT_SCK)) { + dev_err(&spi->dev, + "Invalid delay unit, should be SPI_DELAY_UNIT_SCK\n"); + return -EINVAL; + } + + setup_dly = setup ? setup->value : 1; + hold_dly = hold ? hold->value : 1; + inactive_dly = inactive ? inactive->value : 1; + + reg_val = readl(mdata->base + SPI_CFG0_REG); + if (mdata->dev_comp->enhance_timing) { + reg_val &= ~(0xffff << SPI_ADJUST_CFG0_CS_HOLD_OFFSET); + reg_val |= (((hold_dly - 1) & 0xffff) + << SPI_ADJUST_CFG0_CS_HOLD_OFFSET); + reg_val &= ~(0xffff << SPI_ADJUST_CFG0_CS_SETUP_OFFSET); + reg_val |= (((setup_dly - 1) & 0xffff) + << SPI_ADJUST_CFG0_CS_SETUP_OFFSET); + } else { + reg_val &= ~(0xff << SPI_CFG0_CS_HOLD_OFFSET); + reg_val |= (((hold_dly - 1) & 0xff) << SPI_CFG0_CS_HOLD_OFFSET); + reg_val &= ~(0xff << SPI_CFG0_CS_SETUP_OFFSET); + reg_val |= (((setup_dly - 1) & 0xff) + << SPI_CFG0_CS_SETUP_OFFSET); + } + writel(reg_val, mdata->base + SPI_CFG0_REG); + + reg_val = readl(mdata->base + SPI_CFG1_REG); + reg_val &= ~SPI_CFG1_CS_IDLE_MASK; + reg_val |= (((inactive_dly - 1) & 0xff) << SPI_CFG1_CS_IDLE_OFFSET); + writel(reg_val, mdata->base + SPI_CFG1_REG); + + return 0; +} + static int mtk_spi_setup(struct spi_device *spi) { struct mtk_spi *mdata = spi_master_get_devdata(spi->master); diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c index e9de1d958bbd..5131141bbf0d 100644 --- a/drivers/spi/spi-tegra114.c +++ b/drivers/spi/spi-tegra114.c @@ -717,12 +717,12 @@ static void tegra_spi_deinit_dma_param(struct tegra_spi_data *tspi, dma_release_channel(dma_chan); } -static int tegra_spi_set_hw_cs_timing(struct spi_device *spi) +static int tegra_spi_set_hw_cs_timing(struct spi_device *spi, + struct spi_delay *setup, + struct spi_delay *hold, + struct spi_delay *inactive) { struct tegra_spi_data *tspi = spi_master_get_devdata(spi->master); - struct spi_delay *setup = &spi->cs_setup; - struct spi_delay *hold = &spi->cs_hold; - struct spi_delay *inactive = &spi->cs_inactive; u8 setup_dly, hold_dly, inactive_dly; u32 setup_hold; u32 spi_cs_timing; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 8371bca13729..1efe2e08957e 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -554,7 +554,8 @@ struct spi_controller { * to configure specific CS timing through spi_set_cs_timing() after * spi_setup(). */ - int (*set_cs_timing)(struct spi_device *spi); + int (*set_cs_timing)(struct spi_device *spi, struct spi_delay *setup, + struct spi_delay *hold, struct spi_delay *inactive); /* bidirectional bulk transfers *
This reverts commit 04e6bb0d6bb127bac929fb35edd2dd01613c9520. This revert the commit 'spi: modify set_cs_timing parameter' and its following commit 'spi: mediatek: fix build warnning in set cs timing'. Those commits cause regression on mt8173 elm device. The EC either is not able to register or it sends numerous amount of errors: cros-ec-i2c-tunnel 1100a000.spi:ec@0:i2c-tunnel0: Error transferring EC i2c message -71 cros-ec-spi spi0.0: EC failed to respond in time. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/spi/spi-mt65xx.c | 107 ++++++++++++++++--------------------- drivers/spi/spi-tegra114.c | 8 +-- include/linux/spi/spi.h | 3 +- 3 files changed, 52 insertions(+), 66 deletions(-)