| Message ID | 20180403154449.2443-4-ssuloev@orpaltech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote: > There is no need to handle 3/4 empty interrupt as the maximum > supported transfer length in PIO mode is equal to FIFO depth, > i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs. > > Changes in v3: > 1) Restored processing of 3/4 FIFO full interrupt. > > Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com> > --- > drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------ > 1 file changed, 17 insertions(+), 24 deletions(-) > > diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c > index 78acc1f..c09ad10 100644 > --- a/drivers/spi/spi-sun6i.c > +++ b/drivers/spi/spi-sun6i.c > @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable) > > static size_t sun6i_spi_max_transfer_size(struct spi_device *spi) > { > - return SUN6I_MAX_XFER_SIZE - 1; > + struct spi_master *master = spi->master; > + struct sun6i_spi *sspi = spi_master_get_devdata(master); > + > + return sspi->fifo_depth; Doesn't that effectively revert 3288d5cb40c0 ? Why do you need to do so? > } > > static int sun6i_spi_prepare_message(struct spi_master *master, > @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > int ret = 0; > u32 reg; > > - if (tfr->len > SUN6I_MAX_XFER_SIZE) > - return -EINVAL; > + /* A zero length transfer never finishes if programmed > + in the hardware */ Improper comment style here. Please make sure to run checkpatch before sending your patches. > + if (!tfr->len) > + return 0; Can that case even happen? > + /* Don't support transfer larger than the FIFO */ > + if (tfr->len > sspi->fifo_depth) > + return -EMSGSIZE; You're changing the return type, why? > reinit_completion(&sspi->done); > sspi->tx_buf = tfr->tx_buf; > @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > */ > trig_level = sspi->fifo_depth / 4 * 3; > sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, > - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) | > - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS)); > + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS)); > > > reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); > @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > sun6i_spi_fill_fifo(sspi, sspi->fifo_depth); > > /* Enable the interrupts */ > - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC); > sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC | > SUN6I_INT_CTL_RF_RDY); > - if (tx_len > sspi->fifo_depth) > - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ); This would also need to be explained in your commit log. > > /* Start the transfer */ > reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); > @@ -376,7 +381,9 @@ out: > static irqreturn_t sun6i_spi_handler(int irq, void *dev_id) > { > struct sun6i_spi *sspi = dev_id; > - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); > + u32 status; > + > + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); Why is this change needed? Maxime
On 04/04/2018 09:50 AM, Maxime Ripard wrote: > On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote: >> There is no need to handle 3/4 empty interrupt as the maximum >> supported transfer length in PIO mode is equal to FIFO depth, >> i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs. >> >> Changes in v3: >> 1) Restored processing of 3/4 FIFO full interrupt. >> >> Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com> >> --- >> drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------ >> 1 file changed, 17 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c >> index 78acc1f..c09ad10 100644 >> --- a/drivers/spi/spi-sun6i.c >> +++ b/drivers/spi/spi-sun6i.c >> @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable) >> >> static size_t sun6i_spi_max_transfer_size(struct spi_device *spi) >> { >> - return SUN6I_MAX_XFER_SIZE - 1; >> + struct spi_master *master = spi->master; >> + struct sun6i_spi *sspi = spi_master_get_devdata(master); >> + >> + return sspi->fifo_depth; > Doesn't that effectively revert 3288d5cb40c0 ? > > Why do you need to do so? Need what? Change is supposed to restrict max transfer size for PIO mode otherwise it will fail. The maximum transfer length = FIFO depth for PIO mode. > >> } >> >> static int sun6i_spi_prepare_message(struct spi_master *master, >> @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master, >> int ret = 0; >> u32 reg; >> >> - if (tfr->len > SUN6I_MAX_XFER_SIZE) >> - return -EINVAL; >> + /* A zero length transfer never finishes if programmed >> + in the hardware */ > Improper comment style here. Please make sure to run checkpatch before > sending your patches. ok > >> + if (!tfr->len) >> + return 0; > Can that case even happen? Not sure, just for safety. > >> + /* Don't support transfer larger than the FIFO */ >> + if (tfr->len > sspi->fifo_depth) >> + return -EMSGSIZE; > You're changing the return type, why? As a more appropriate one > >> reinit_completion(&sspi->done); >> sspi->tx_buf = tfr->tx_buf; >> @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master, >> */ >> trig_level = sspi->fifo_depth / 4 * 3; >> sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, >> - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) | >> - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS)); >> + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS)); >> >> >> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); >> @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master, >> sun6i_spi_fill_fifo(sspi, sspi->fifo_depth); >> >> /* Enable the interrupts */ >> - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC); >> sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC | >> SUN6I_INT_CTL_RF_RDY); >> - if (tx_len > sspi->fifo_depth) >> - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ); > This would also need to be explained in your commit log. What exactly and in what way ? > >> >> /* Start the transfer */ >> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); >> @@ -376,7 +381,9 @@ out: >> static irqreturn_t sun6i_spi_handler(int irq, void *dev_id) >> { >> struct sun6i_spi *sspi = dev_id; >> - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); >> + u32 status; >> + >> + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); > Why is this change needed? A minor one, for readability. > > Maxime >
On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote: > On 04/04/2018 09:50 AM, Maxime Ripard wrote: > > On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote: > > > There is no need to handle 3/4 empty interrupt as the maximum > > > supported transfer length in PIO mode is equal to FIFO depth, > > > i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs. > > > > > > Changes in v3: > > > 1) Restored processing of 3/4 FIFO full interrupt. > > > > > > Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com> > > > --- > > > drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------ > > > 1 file changed, 17 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c > > > index 78acc1f..c09ad10 100644 > > > --- a/drivers/spi/spi-sun6i.c > > > +++ b/drivers/spi/spi-sun6i.c > > > @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable) > > > static size_t sun6i_spi_max_transfer_size(struct spi_device *spi) > > > { > > > - return SUN6I_MAX_XFER_SIZE - 1; > > > + struct spi_master *master = spi->master; > > > + struct sun6i_spi *sspi = spi_master_get_devdata(master); > > > + > > > + return sspi->fifo_depth; > > Doesn't that effectively revert 3288d5cb40c0 ? > > > > Why do you need to do so? > > Need what? Why do you need to revert that change. > Change is supposed to restrict max transfer size for PIO mode otherwise it > will fail. > The maximum transfer length = FIFO depth for PIO mode. The point of that patch was precisely to allow to send more data than the FIFO. You're breaking that behaviour without any justification, and this is not ok. > > > > > } > > > static int sun6i_spi_prepare_message(struct spi_master *master, > > > @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > > > int ret = 0; > > > u32 reg; > > > - if (tfr->len > SUN6I_MAX_XFER_SIZE) > > > - return -EINVAL; > > > + /* A zero length transfer never finishes if programmed > > > + in the hardware */ > > Improper comment style here. Please make sure to run checkpatch before > > sending your patches. > ok > > > > > + if (!tfr->len) > > > + return 0; > > Can that case even happen? > Not sure, just for safety. > > > > > + /* Don't support transfer larger than the FIFO */ > > > + if (tfr->len > sspi->fifo_depth) > > > + return -EMSGSIZE; > > You're changing the return type, why? > As a more appropriate one The fact that it's more appropriate should be justified. > > > > > reinit_completion(&sspi->done); > > > sspi->tx_buf = tfr->tx_buf; > > > @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > > > */ > > > trig_level = sspi->fifo_depth / 4 * 3; > > > sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, > > > - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) | > > > - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS)); > > > + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS)); > > > reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); > > > @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > > > sun6i_spi_fill_fifo(sspi, sspi->fifo_depth); > > > /* Enable the interrupts */ > > > - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC); > > > sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC | > > > SUN6I_INT_CTL_RF_RDY); > > > - if (tx_len > sspi->fifo_depth) > > > - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ); > > This would also need to be explained in your commit log. > What exactly and in what way ? You should explain, at least: A) What is the current behaviour B) Why that is a problem, or what problem does it cause C) What solution you implement and why you think it's justified > > > > > /* Start the transfer */ > > > reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); > > > @@ -376,7 +381,9 @@ out: > > > static irqreturn_t sun6i_spi_handler(int irq, void *dev_id) > > > { > > > struct sun6i_spi *sspi = dev_id; > > > - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); > > > + u32 status; > > > + > > > + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); > > Why is this change needed? > A minor one, for readability. That's arguable, and you should have a single logical change per patch. So this doesn't belong in this one. Maxime
On 04/05/2018 12:19 PM, Maxime Ripard wrote: > On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote: >> On 04/04/2018 09:50 AM, Maxime Ripard wrote: >>> On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote: >>>> There is no need to handle 3/4 empty interrupt as the maximum >>>> supported transfer length in PIO mode is equal to FIFO depth, >>>> i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs. >>>> >>>> Changes in v3: >>>> 1) Restored processing of 3/4 FIFO full interrupt. >>>> >>>> Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com> >>>> --- >>>> drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------ >>>> 1 file changed, 17 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c >>>> index 78acc1f..c09ad10 100644 >>>> --- a/drivers/spi/spi-sun6i.c >>>> +++ b/drivers/spi/spi-sun6i.c >>>> @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable) >>>> static size_t sun6i_spi_max_transfer_size(struct spi_device *spi) >>>> { >>>> - return SUN6I_MAX_XFER_SIZE - 1; >>>> + struct spi_master *master = spi->master; >>>> + struct sun6i_spi *sspi = spi_master_get_devdata(master); >>>> + >>>> + return sspi->fifo_depth; >>> Doesn't that effectively revert 3288d5cb40c0 ? >>> >>> Why do you need to do so? >> Need what? > Why do you need to revert that change. > >> Change is supposed to restrict max transfer size for PIO mode otherwise it >> will fail. >> The maximum transfer length = FIFO depth for PIO mode. > The point of that patch was precisely to allow to send more data than > the FIFO. You're breaking that behaviour without any justification, > and this is not ok. I am sorry, but you can't. That's a hardware limitation. >>>> } >>>> static int sun6i_spi_prepare_message(struct spi_master *master, >>>> @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master, >>>> int ret = 0; >>>> u32 reg; >>>> - if (tfr->len > SUN6I_MAX_XFER_SIZE) >>>> - return -EINVAL; >>>> + /* A zero length transfer never finishes if programmed >>>> + in the hardware */ >>> Improper comment style here. Please make sure to run checkpatch before >>> sending your patches. >> ok >>>> + if (!tfr->len) >>>> + return 0; >>> Can that case even happen? >> Not sure, just for safety. >>>> + /* Don't support transfer larger than the FIFO */ >>>> + if (tfr->len > sspi->fifo_depth) >>>> + return -EMSGSIZE; >>> You're changing the return type, why? >> As a more appropriate one > The fact that it's more appropriate should be justified. > >>>> reinit_completion(&sspi->done); >>>> sspi->tx_buf = tfr->tx_buf; >>>> @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master, >>>> */ >>>> trig_level = sspi->fifo_depth / 4 * 3; >>>> sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, >>>> - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) | >>>> - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS)); >>>> + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS)); >>>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); >>>> @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master, >>>> sun6i_spi_fill_fifo(sspi, sspi->fifo_depth); >>>> /* Enable the interrupts */ >>>> - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC); >>>> sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC | >>>> SUN6I_INT_CTL_RF_RDY); >>>> - if (tx_len > sspi->fifo_depth) >>>> - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ); >>> This would also need to be explained in your commit log. >> What exactly and in what way ? > You should explain, at least: > A) What is the current behaviour > B) Why that is a problem, or what problem does it cause > C) What solution you implement and why you think it's justified > >>>> /* Start the transfer */ >>>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); >>>> @@ -376,7 +381,9 @@ out: >>>> static irqreturn_t sun6i_spi_handler(int irq, void *dev_id) >>>> { >>>> struct sun6i_spi *sspi = dev_id; >>>> - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); >>>> + u32 status; >>>> + >>>> + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); >>> Why is this change needed? >> A minor one, for readability. > That's arguable, and you should have a single logical change per > patch. So this doesn't belong in this one. > > Maxime > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Apr 05, 2018 at 11:19:13AM +0200, Maxime Ripard wrote: > On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote: > > What exactly and in what way ? > You should explain, at least: > A) What is the current behaviour > B) Why that is a problem, or what problem does it cause > C) What solution you implement and why you think it's justified Right, this is key - the top level problem with most of this patch set is that it's hard to understand what the changes are intended to do or why. It's really important that people reading the changes be able to understand what's going on, especially if technical problems have been found since that tends to make people look more closely. Part of this is about splitting the changes out so that each patch does one thing (which makes it easier to understand them) and part of it is about explaining those changes clearly.
On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote: > On 04/05/2018 12:19 PM, Maxime Ripard wrote: > > The point of that patch was precisely to allow to send more data than > > the FIFO. You're breaking that behaviour without any justification, > > and this is not ok. > I am sorry, but you can't. That's a hardware limitation. Are you positive about that? Normally you can add things to hardware FIFOs while they're being drained so so long as you can keep data flowing in at least as fast as it's being consumed.
On 04/05/2018 04:17 PM, Mark Brown wrote: > On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote: >> On 04/05/2018 12:19 PM, Maxime Ripard wrote: >>> The point of that patch was precisely to allow to send more data than >>> the FIFO. You're breaking that behaviour without any justification, >>> and this is not ok. >> I am sorry, but you can't. That's a hardware limitation. > Are you positive about that? Normally you can add things to hardware > FIFOs while they're being drained so so long as you can keep data > flowing in at least as fast as it's being consumed. Well, normally yes, but this is not the case with the hardware that I own. My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer larger than FIFO then TC interrupt never happens.
On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote: > On 04/05/2018 04:17 PM, Mark Brown wrote: > > On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote: > > > On 04/05/2018 12:19 PM, Maxime Ripard wrote: > > > > The point of that patch was precisely to allow to send more data than > > > > the FIFO. You're breaking that behaviour without any justification, > > > > and this is not ok. > > > I am sorry, but you can't. That's a hardware limitation. > > Are you positive about that? Normally you can add things to hardware > > FIFOs while they're being drained so so long as you can keep data > > flowing in at least as fast as it's being consumed. > > Well, normally yes, but this is not the case with the hardware that I own. > My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer > larger than FIFO then TC interrupt never happens. Because you're not supposed to have a transfer larger than the FIFO, but to have to setup at first a transfer the size of the FIFO, and then when it's (or starts to be) depleted, fill it up again. That's the point of the patch you're reverting, and if it doesn't work, you should make it work and not simply revert it. Maxime
On 04/06/2018 10:34 AM, Maxime Ripard wrote: > On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote: >> On 04/05/2018 04:17 PM, Mark Brown wrote: >>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote: >>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote: >>>>> The point of that patch was precisely to allow to send more data than >>>>> the FIFO. You're breaking that behaviour without any justification, >>>>> and this is not ok. >>>> I am sorry, but you can't. That's a hardware limitation. >>> Are you positive about that? Normally you can add things to hardware >>> FIFOs while they're being drained so so long as you can keep data >>> flowing in at least as fast as it's being consumed. >> Well, normally yes, but this is not the case with the hardware that I own. >> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer >> larger than FIFO then TC interrupt never happens. > Because you're not supposed to have a transfer larger than the FIFO, > but to have to setup at first a transfer the size of the FIFO, and > then when it's (or starts to be) depleted, fill it up again. According to what you said the driver must implement "transfer_one_message" instead of "transfer_one" > > That's the point of the patch you're reverting, and if it doesn't > work, you should make it work and not simply revert it. > > Maxime >
On 04/06/2018 10:34 AM, Maxime Ripard wrote: > On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote: >> On 04/05/2018 04:17 PM, Mark Brown wrote: >>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote: >>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote: >>>>> The point of that patch was precisely to allow to send more data than >>>>> the FIFO. You're breaking that behaviour without any justification, >>>>> and this is not ok. >>>> I am sorry, but you can't. That's a hardware limitation. >>> Are you positive about that? Normally you can add things to hardware >>> FIFOs while they're being drained so so long as you can keep data >>> flowing in at least as fast as it's being consumed. >> Well, normally yes, but this is not the case with the hardware that I own. >> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer >> larger than FIFO then TC interrupt never happens. > Because you're not supposed to have a transfer larger than the FIFO, > but to have to setup at first a transfer the size of the FIFO, and > then when it's (or starts to be) depleted, fill it up again. According to what you said the driver must implement "transfer_one_message" instead of "transfer_one", because the maximum transfer length is 64 bytes (for sun4i) and we shouldn't allow "transfer_one" handle more than 64 bytes. Otherwise it breaks the concept. > > That's the point of the patch you're reverting, and if it doesn't > work, you should make it work and not simply revert it. > > Maxime >
On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote: > On 04/06/2018 10:34 AM, Maxime Ripard wrote: > > On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote: > > > On 04/05/2018 04:17 PM, Mark Brown wrote: > > > > On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote: > > > > > On 04/05/2018 12:19 PM, Maxime Ripard wrote: > > > > > > The point of that patch was precisely to allow to send more data than > > > > > > the FIFO. You're breaking that behaviour without any justification, > > > > > > and this is not ok. > > > > > I am sorry, but you can't. That's a hardware limitation. > > > > Are you positive about that? Normally you can add things to hardware > > > > FIFOs while they're being drained so so long as you can keep data > > > > flowing in at least as fast as it's being consumed. > > > Well, normally yes, but this is not the case with the hardware that I own. > > > My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer > > > larger than FIFO then TC interrupt never happens. > > Because you're not supposed to have a transfer larger than the FIFO, > > but to have to setup at first a transfer the size of the FIFO, and > > then when it's (or starts to be) depleted, fill it up again. > > According to what you said the driver must implement > "transfer_one_message" instead of "transfer_one" I'm not sure what makes you think that I said that. Maxime
On 04/09/2018 12:27 PM, Maxime Ripard wrote: > On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote: >> On 04/06/2018 10:34 AM, Maxime Ripard wrote: >>> On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote: >>>> On 04/05/2018 04:17 PM, Mark Brown wrote: >>>>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote: >>>>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote: >>>>>>> The point of that patch was precisely to allow to send more data than >>>>>>> the FIFO. You're breaking that behaviour without any justification, >>>>>>> and this is not ok. >>>>>> I am sorry, but you can't. That's a hardware limitation. >>>>> Are you positive about that? Normally you can add things to hardware >>>>> FIFOs while they're being drained so so long as you can keep data >>>>> flowing in at least as fast as it's being consumed. >>>> Well, normally yes, but this is not the case with the hardware that I own. >>>> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer >>>> larger than FIFO then TC interrupt never happens. >>> Because you're not supposed to have a transfer larger than the FIFO, >>> but to have to setup at first a transfer the size of the FIFO, and >>> then when it's (or starts to be) depleted, fill it up again. >> According to what you said the driver must implement >> "transfer_one_message" instead of "transfer_one" > I'm not sure what makes you think that I said that. > > Maxime > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel Because current implementation tries to send more than FIFO-depth of data in a single call to "transfer_one" which is wrong.
On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote: > On 04/09/2018 12:27 PM, Maxime Ripard wrote: > > On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote: > > > On 04/06/2018 10:34 AM, Maxime Ripard wrote: > > > According to what you said the driver must implement > > > "transfer_one_message" instead of "transfer_one" > > I'm not sure what makes you think that I said that. > Because current implementation tries to send more than FIFO-depth of data in > a single call to "transfer_one" which is wrong. No, that's absolutely not the case. All any of these functions has to do is transfer whatever they were asked to, how they do it is not at all important to the framework.
On 04/09/2018 01:50 PM, Mark Brown wrote: > On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote: >> On 04/09/2018 12:27 PM, Maxime Ripard wrote: >>> On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote: >>>> On 04/06/2018 10:34 AM, Maxime Ripard wrote: >>>> According to what you said the driver must implement >>>> "transfer_one_message" instead of "transfer_one" >>> I'm not sure what makes you think that I said that. >> Because current implementation tries to send more than FIFO-depth of data in >> a single call to "transfer_one" which is wrong. > No, that's absolutely not the case. All any of these functions has to > do is transfer whatever they were asked to, how they do it is not at all > important to the framework. I think you don't fully understand the issue. Let's talk about sun4i and sun6i SPI drivers separately. sun4i 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode but transfer_one() function doesn't follow the declaration allowing PIO transfers longer than FIFO depth by just refilling FIFO using 3/4 FIFO empty interrupt. I can definitely state here that long transfers WON'T WORK on real hardware. I tested it and that's why I can say that. But as soon as sun4i SPI driver is correctly declaring max_transfer_size then "smart" clients will work well by limiting a single transfer size to FIFO depth. I tested it with real hardware, again. sun6i 2) it allows PIO transfers of any length by declaring max_transfer_size to a huge number, i.e. you can ONLY make this driver work in PIO mode by limiting a single transfer size to FIFO depth (64 or 128 bytes) on client side and ignore max_transfer_size exposed by the driver. Again, tested with real hardware. All above doesn't work for DMA mode as there is no such limitation. I can't clearly explain what is happening in the hardware in PIO mode but it seems that TC interrupt doesn't arrive in time when refilling FIFO multiple times takes place and every long transfer will end up with a timeout error.
On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote: > On 04/09/2018 01:50 PM, Mark Brown wrote: > > On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote: > > > Because current implementation tries to send more than FIFO-depth of data in > > > a single call to "transfer_one" which is wrong. > > No, that's absolutely not the case. All any of these functions has to > > do is transfer whatever they were asked to, how they do it is not at all > > important to the framework. > I think you don't fully understand the issue. Let's talk about sun4i and > sun6i SPI drivers separately. > sun4i > 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode but > transfer_one() function doesn't follow the declaration allowing PIO > transfers longer than FIFO depth by just refilling FIFO using 3/4 FIFO > empty interrupt. I can definitely state here that long transfers WON'T WORK > on real hardware. I tested it and that's why I can say that. But as soon as > sun4i SPI driver is correctly declaring max_transfer_size then "smart" > clients will work well by limiting a single transfer size to FIFO depth. I > tested it with real hardware, again. None of this is in the slightest bit related to the use of transfer_one() vs transfer_one_message(). These are all implementation details and will so far as I can tell apply equally to both APIs.
On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote: > On 04/09/2018 01:50 PM, Mark Brown wrote: > > On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote: > > > On 04/09/2018 12:27 PM, Maxime Ripard wrote: > > > > On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote: > > > > > On 04/06/2018 10:34 AM, Maxime Ripard wrote: > > > > > According to what you said the driver must implement > > > > > "transfer_one_message" instead of "transfer_one" > > > > I'm not sure what makes you think that I said that. > > > Because current implementation tries to send more than FIFO-depth of data in > > > a single call to "transfer_one" which is wrong. > > No, that's absolutely not the case. All any of these functions has to > > do is transfer whatever they were asked to, how they do it is not at all > > important to the framework. > > I think you don't fully understand the issue. Let's talk about sun4i and > sun6i SPI drivers separately. > > sun4i > > 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode but > transfer_one() function doesn't follow the declaration allowing PIO > transfers longer than FIFO depth by just refilling FIFO using 3/4 FIFO > empty interrupt. I can definitely state here that long transfers WON'T WORK > on real hardware. Surely the original author of the patch allowing to do just that disagrees with you. And it's not about the hardware itself, it's about how the driver operates as well. > I tested it and that's why I can say that. Then it must be fixed, and not silently reverted. > But as soon as sun4i SPI driver is correctly declaring > max_transfer_size then "smart" clients will work well by limiting a > single transfer size to FIFO depth. I tested it with real hardware, > again. This is really not my point. What would prevent you from doing multiple transfers in that case, and filling the FIFO entirely, waiting for it to be done, then resuming until you have sent the right number of bytes? Maxime
On 04/09/2018 02:36 PM, Maxime Ripard wrote: > On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote: >> On 04/09/2018 01:50 PM, Mark Brown wrote: >>> On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote: >>>> On 04/09/2018 12:27 PM, Maxime Ripard wrote: >>>>> On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote: >>>>>> On 04/06/2018 10:34 AM, Maxime Ripard wrote: >>>>>> According to what you said the driver must implement >>>>>> "transfer_one_message" instead of "transfer_one" >>>>> I'm not sure what makes you think that I said that. >>>> Because current implementation tries to send more than FIFO-depth of data in >>>> a single call to "transfer_one" which is wrong. >>> No, that's absolutely not the case. All any of these functions has to >>> do is transfer whatever they were asked to, how they do it is not at all >>> important to the framework. >> I think you don't fully understand the issue. Let's talk about sun4i and >> sun6i SPI drivers separately. >> >> sun4i >> >> 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode but >> transfer_one() function doesn't follow the declaration allowing PIO >> transfers longer than FIFO depth by just refilling FIFO using 3/4 FIFO >> empty interrupt. I can definitely state here that long transfers WON'T WORK >> on real hardware. > Surely the original author of the patch allowing to do just that > disagrees with you. I am not getting the point why the driver is declaring the max transfer length value and not following the rule. > And it's not about the hardware itself, it's about > how the driver operates as well. > >> I tested it and that's why I can say that. > Then it must be fixed, and not silently reverted. > >> But as soon as sun4i SPI driver is correctly declaring >> max_transfer_size then "smart" clients will work well by limiting a >> single transfer size to FIFO depth. I tested it with real hardware, >> again. > This is really not my point. What would prevent you from doing > multiple transfers in that case, and filling the FIFO entirely, > waiting for it to be done, then resuming until you have sent the right > number of bytes? Because it makes no sense IMHO. I can't see any single point in allowing long PIO transfers. Can you find at least one ? I think we should reuse as much SPI core code as possible. The SPI core can handle an SPI message with multiple transfers, all we need is to have max_transfer_size = FIFO depth and restrict it in transfer_one(). > > Maxime > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Apr 09, 2018 at 02:59:57PM +0300, Sergey Suloev wrote: > > > But as soon as sun4i SPI driver is correctly declaring > > > max_transfer_size then "smart" clients will work well by limiting a > > > single transfer size to FIFO depth. I tested it with real hardware, > > > again. > > This is really not my point. What would prevent you from doing > > multiple transfers in that case, and filling the FIFO entirely, > > waiting for it to be done, then resuming until you have sent the right > > number of bytes? > > Because it makes no sense IMHO. I can't see any single point in allowing > long PIO transfers. Can you find at least one ? I'm probably going to state the obvious here, but to allow long transfers? > I think we should reuse as much SPI core code as possible. The SPI > core can handle an SPI message with multiple transfers, all we need > is to have max_transfer_size = FIFO depth and restrict it in > transfer_one(). There's not a single call to the max_transfer_size hook in the SPI core in 4.16, so that seems a bit too optimistic. Maxime
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c index 78acc1f..c09ad10 100644 --- a/drivers/spi/spi-sun6i.c +++ b/drivers/spi/spi-sun6i.c @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable) static size_t sun6i_spi_max_transfer_size(struct spi_device *spi) { - return SUN6I_MAX_XFER_SIZE - 1; + struct spi_master *master = spi->master; + struct sun6i_spi *sspi = spi_master_get_devdata(master); + + return sspi->fifo_depth; } static int sun6i_spi_prepare_message(struct spi_master *master, @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master, int ret = 0; u32 reg; - if (tfr->len > SUN6I_MAX_XFER_SIZE) - return -EINVAL; + /* A zero length transfer never finishes if programmed + in the hardware */ + if (!tfr->len) + return 0; + + /* Don't support transfer larger than the FIFO */ + if (tfr->len > sspi->fifo_depth) + return -EMSGSIZE; reinit_completion(&sspi->done); sspi->tx_buf = tfr->tx_buf; @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master, */ trig_level = sspi->fifo_depth / 4 * 3; sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) | - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS)); + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS)); reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master, sun6i_spi_fill_fifo(sspi, sspi->fifo_depth); /* Enable the interrupts */ - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC); sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC | SUN6I_INT_CTL_RF_RDY); - if (tx_len > sspi->fifo_depth) - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ); /* Start the transfer */ reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG); @@ -376,7 +381,9 @@ out: static irqreturn_t sun6i_spi_handler(int irq, void *dev_id) { struct sun6i_spi *sspi = dev_id; - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); + u32 status; + + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG); /* Transfer complete */ if (status & SUN6I_INT_CTL_TC) { @@ -388,26 +395,12 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id) /* Receive FIFO 3/4 full */ if (status & SUN6I_INT_CTL_RF_RDY) { - sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH); + sun6i_spi_drain_fifo(sspi, sspi->fifo_depth); /* Only clear the interrupt _after_ draining the FIFO */ sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_RF_RDY); return IRQ_HANDLED; } - /* Transmit FIFO 3/4 empty */ - if (status & SUN6I_INT_CTL_TF_ERQ) { - sun6i_spi_fill_fifo(sspi, SUN6I_FIFO_DEPTH); - - if (!sspi->len) - /* nothing left to transmit */ - sun6i_spi_disable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ); - - /* Only clear the interrupt _after_ re-seeding the FIFO */ - sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TF_ERQ); - - return IRQ_HANDLED; - } - return IRQ_NONE; }
There is no need to handle 3/4 empty interrupt as the maximum supported transfer length in PIO mode is equal to FIFO depth, i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs. Changes in v3: 1) Restored processing of 3/4 FIFO full interrupt. Signed-off-by: Sergey Suloev <ssuloev@orpaltech.com> --- drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-)