Re: Regression: spi: core: avoid waking pump thread from spi_sync instead run teardown delayed

[Martin Sperl]

Hi Mark!

> On 23.01.2019, at 18:56, Mark Brown  wrote:
> 
>> On Sun, Jan 20, 2019 at 12:24:23PM +0100, ker...@martin.sperl.org wrote:
>> 
>> These kind of changes it requires are consuming a bit more time than
>> I was hoping for.
> 
> Thanks for trying.
> 
>> So maybe at this very moment the best is reverting the patch.
> 
> Yes, I'm just going to do that for now.
> 
>> As for the root cause of the regression: my guess is that spi-mem is
>> just not triggering a shutdown any more because of how message_pump works.
> 
> I'm fairly sure that's what's going on but not been able to get my head
> around things enough to figure out what's going wrong yet.

While thinking about this again maybe an idea:
What about implement a second spi_transfer_one implementation (together
with a message pump implementation) that would handle things correctly.

Any driver then can select the old (default) or new implementation and thus
would allow the optimizations to take place only for verified working drivers...

At least this way we would not be blocked because no hw exposing this
Behavior is available to us - at the cost of extra code to get maintained.

What I would then also like to do for the new implementation is modify the
API a bit - ideally I would like to:
* Make spi_sync the primary interface which the message pump is also 
  using directly
* move all the prepare stuff early into spi-sync, so that for example the
  Preparing (including dma mapping) would get done in the calling thread
  And only the prepared message would get submitted to the queue
  - special processing would be needed for the spi-async case.

This should optimize the computations out of the central loop faster.

Adding spi-nand support could get added later by someone who has
access to a device making use of this.

If that sounds as somewhat acceptable then I will try get something
Implemented.

Any other ideas where we could improve as well?

Martin

Re: Regression: spi: core: avoid waking pump thread from spi_sync instead run teardown delayed

[Martin Sperl]

> On 15.01.2019, at 20:26, Mark Brown  wrote:
> 
>> On Tue, Jan 15, 2019 at 06:39:27PM +0100, ker...@martin.sperl.org wrote:
>> 
>> Is it possible that the specific flash is not using the “normal” 
>> spi_pump_message, but spi_controller_mem_ops operations? 
> 
> Right, that's my best guess at the minute as well.
> 
>> Maybe we are missing the teardown in that driver or something is
>> changing flags there.
> 
>> grepping a bit:
> 
>> I see spi_mem_access_start calling spi_flush_queue, which is calling
>> pump_message, which - if there is no transfer - will trigger a delayed
>> tear-down, while it maybe should not be doing that.
> 
> If nothing else it's inefficient.
> 
>> Maybe spi_mem_access_end needs a call to spi_flush_queue as well?
> 
> Hrm, or needs to schedule the queue at any rate (though this will only
> have an impact in the fairly unusual case where there's something
> sharing the bus with a flash).
> 
>> Unfortunately this is theoretical work and quite a lot of guesswork
>> without an actual device available for testing...
> 
> Indeed.

Maybe a bigger change to the reduce the complexity of
the state machine would solve that problem and also
reduce code complexity... 

I may find some time over the weekend if no solution
has been found until then.

The way I would envision it it would have a “state”
as a level (0=shutdown, 1=hw enabled, 2=in pump, 
3=in transfer, 4=in hw-mode,...) and a complete
to allow waking the shutdown thread (and by this
avoiding the busy wait loop we have now).
This would replace those idling, busy, and running flags.

Drawback: it is invasive, but let us see what it
really looks like...

Martin

Re: [PATCH v3 3/7] i2c: bcm2835: Use dev_dbg logging on transfer errors

[Martin Sperl]

On 28.09.2016, at 19:50, Noralf Trønnes <nor...@tronnes.org> wrote:
> 
> Writing to an AT24C32 generates on average 2x i2c transfer errors per
> 32-byte page write. Which amounts to a lot for a 4k write. This is due
> to the fact that the chip doesn't respond during it's internal write
> cycle when the at24 driver tries and retries the next write.
> Only a handful drivers use dev_err() on transfer error, so switch to
> dev_dbg() instead.
> 
> Signed-off-by: Noralf Trønnes <nor...@tronnes.org>
Reviewed-by: Martin Sperl <ker...@martin.sperl.org>

Re: [PATCH v3 3/7] i2c: bcm2835: Use dev_dbg logging on transfer errors

[Martin Sperl]

On 28.09.2016, at 19:50, Noralf Trønnes  wrote:
> 
> Writing to an AT24C32 generates on average 2x i2c transfer errors per
> 32-byte page write. Which amounts to a lot for a 4k write. This is due
> to the fact that the chip doesn't respond during it's internal write
> cycle when the at24 driver tries and retries the next write.
> Only a handful drivers use dev_err() on transfer error, so switch to
> dev_dbg() instead.
> 
> Signed-off-by: Noralf Trønnes 
Reviewed-by: Martin Sperl 

Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Martin Sperl]

On 28.09.2016 23:24, Eric Anholt wrote:

Noralf Trønnes <nor...@tronnes.org> writes:


Support a dynamic clock by reading the frequency and setting the
divisor in the transfer function instead of during probe.

Is this fixing some particular case you could note in the commit
message?  As is, it makes me think that we should be using a notifier
for when the parent clock (that's the one I assume you're talking about
being dynamic) changes.  But maybe you're working around a variable VPU
clock being set by the firmware, because we don't have a notifier for
it?

I'm a bit worried because I think this is going to be pretty expensive
to be doing per transfer.


Well, the clocks are all configured without CLK_GET_RATE_NOCACHE et. al.,
so the value is read from cache, which is not (very) expensive
(see clk_core_get_rate).

This also means that any clock change of the VPU done by the firmware
does not propagate to the linux kernel anyway and the unchanged
cached value is returned.

To make this work it would require a notification mechanism from the
firmware to trigger a re-validation of all the caches. (or some sort of 
watchdog

process).

Adding a notifier to each driver (I2C, SPI) instead is - imo - a lot of 
unnecessary

code complexity, as any currently running transfer would still be impacted,
because changing the clock-divider in flight is a asking for trouble.
But then changing the vpu-clock speed while a I2s/SPI/... transfer is 
running is

also asking for trouble

The only place where - IMO - a notifier would make sense is with the 
auxiliar

UART driver(8250_bcm2835aux.c), as there we only read the clock rates
when setting/changing the baud rate. But - again -  this would require some
notification by the firmware in the first place and any reception in the
window of change would go wrong because of unexpected effective baud
rate changes.

So as far as I can tell the change to read the current clock rate in the
transfer function is a reasonable approach and the clock framework should
handle the communication with the firmware about such changes.
(And I remember having had some discussions around this subject
with Phil Elwell or popcornmix some time ago on github where it boiled
down to: what is the "right" interface? - I can not find the reference
right now)

Reviewed-by: Martin Sperl <ker...@martin.sperl.org>

Thanks, Martin

Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Martin Sperl]

On 28.09.2016 23:24, Eric Anholt wrote:

Noralf Trønnes  writes:


Support a dynamic clock by reading the frequency and setting the
divisor in the transfer function instead of during probe.

Is this fixing some particular case you could note in the commit
message?  As is, it makes me think that we should be using a notifier
for when the parent clock (that's the one I assume you're talking about
being dynamic) changes.  But maybe you're working around a variable VPU
clock being set by the firmware, because we don't have a notifier for
it?

I'm a bit worried because I think this is going to be pretty expensive
to be doing per transfer.


Well, the clocks are all configured without CLK_GET_RATE_NOCACHE et. al.,
so the value is read from cache, which is not (very) expensive
(see clk_core_get_rate).

This also means that any clock change of the VPU done by the firmware
does not propagate to the linux kernel anyway and the unchanged
cached value is returned.

To make this work it would require a notification mechanism from the
firmware to trigger a re-validation of all the caches. (or some sort of 
watchdog

process).

Adding a notifier to each driver (I2C, SPI) instead is - imo - a lot of 
unnecessary

code complexity, as any currently running transfer would still be impacted,
because changing the clock-divider in flight is a asking for trouble.
But then changing the vpu-clock speed while a I2s/SPI/... transfer is 
running is

also asking for trouble

The only place where - IMO - a notifier would make sense is with the 
auxiliar

UART driver(8250_bcm2835aux.c), as there we only read the clock rates
when setting/changing the baud rate. But - again -  this would require some
notification by the firmware in the first place and any reception in the
window of change would go wrong because of unexpected effective baud
rate changes.

So as far as I can tell the change to read the current clock rate in the
transfer function is a reasonable approach and the clock framework should
handle the communication with the firmware about such changes.
(And I remember having had some discussions around this subject
with Phil Elwell or popcornmix some time ago on github where it boiled
down to: what is the "right" interface? - I can not find the reference
right now)

Reviewed-by: Martin Sperl 

Thanks, Martin

Re: [PATCH v2 3/8] i2c: bcm2835: Use ratelimited logging on transfer errors

[Martin Sperl]

> On 27 Sep 2016, at 13:57, Noralf Trønnes  wrote:
> 
> Writing to an AT24C32 generates on average 2x i2c transfer errors per
> 32-byte page write. Which amounts to a lot for a 4k write. This is due
> to the fact that the chip doesn't respond during it's internal write
> cycle when the at24 driver tries and retries the next write.
> Reduce this flooding of the log by using dev_err_ratelimited().
> 
> Signed-off-by: Noralf Trønnes 
> Reviewed-by: Eric Anholt 
> ---
> drivers/i2c/busses/i2c-bcm2835.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm2835.c 
> b/drivers/i2c/busses/i2c-bcm2835.c
> index df036ed..370a322 100644
> --- a/drivers/i2c/busses/i2c-bcm2835.c
> +++ b/drivers/i2c/busses/i2c-bcm2835.c
> @@ -207,7 +207,8 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev 
> *i2c_dev,
>(msg->flags & I2C_M_IGNORE_NAK))
>return 0;
> 
> -dev_err(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
> +dev_err_ratelimited(i2c_dev->dev, "i2c transfer failed: %x\n",
> +i2c_dev->msg_err);
Do we really need this error message at all?

Maybe just remove it instead, because error messages during 
"normal"/successfull operations of at24 seems  strange.

Or make it a debug message instead.

Martin

Re: [PATCH v2 3/8] i2c: bcm2835: Use ratelimited logging on transfer errors

[Martin Sperl]

> On 27 Sep 2016, at 13:57, Noralf Trønnes  wrote:
> 
> Writing to an AT24C32 generates on average 2x i2c transfer errors per
> 32-byte page write. Which amounts to a lot for a 4k write. This is due
> to the fact that the chip doesn't respond during it's internal write
> cycle when the at24 driver tries and retries the next write.
> Reduce this flooding of the log by using dev_err_ratelimited().
> 
> Signed-off-by: Noralf Trønnes 
> Reviewed-by: Eric Anholt 
> ---
> drivers/i2c/busses/i2c-bcm2835.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm2835.c 
> b/drivers/i2c/busses/i2c-bcm2835.c
> index df036ed..370a322 100644
> --- a/drivers/i2c/busses/i2c-bcm2835.c
> +++ b/drivers/i2c/busses/i2c-bcm2835.c
> @@ -207,7 +207,8 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev 
> *i2c_dev,
>(msg->flags & I2C_M_IGNORE_NAK))
>return 0;
> 
> -dev_err(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
> +dev_err_ratelimited(i2c_dev->dev, "i2c transfer failed: %x\n",
> +i2c_dev->msg_err);
Do we really need this error message at all?

Maybe just remove it instead, because error messages during 
"normal"/successfull operations of at24 seems  strange.

Or make it a debug message instead.

Martin

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Martin Sperl]

On 20.09.2016 10:41, Noralf Trønnes wrote:


Den 20.09.2016 09:19, skrev Martin Sperl:

Hi Noralf!

On 19.09.2016 17:26, Noralf Trønnes wrote:

Some SMBus protocols use Repeated Start Condition to switch from write
mode to read mode. Devices like MMA8451 won't work without it.

When downstream implemented support for this in i2c-bcm2708, it broke
support for some devices, so a module parameter was added and combined
transfer was disabled by default.
See https://github.com/raspberrypi/linux/issues/599
It doesn't seem to have been any investigation into what the problem
really was. Later there was added a timeout on the polling loop.

One of the devices mentioned to partially stop working was DS1307.

I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
and AT24C32 (eeprom) in parallel without problems.

Signed-off-by: Noralf Trønnes <nor...@tronnes.org>
---
  drivers/i2c/busses/i2c-bcm2835.c | 107
+++
  1 file changed, 98 insertions(+), 9 deletions(-)

...

@@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter
*adap, struct i2c_msg msgs[],
  int i;
  int ret = 0;
  +/* Combined write-read to the same address (smbus) */
+if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+(msgs[0].len <= 16)) {
+ret = bcm2835_i2c_xfer_msg(i2c_dev, [0], [1]);
+
+return ret ? ret : 2;
+}
+
  for (i = 0; i < num; i++) {
-ret = bcm2835_i2c_xfer_msg(i2c_dev, [i]);
+ret = bcm2835_i2c_xfer_msg(i2c_dev, [i], NULL);
  if (ret)
  break;
  }

This does not seem to implement the i2c_msg api correctly.

As per comments in include/uapi/linux/i2c.h on line 58 only the last
message
in a group should - by default - send a STOP.



Apparently it's a known problem that the i2c controller doesn't support
Repeated Start. It will always issue a Stop when it has transferred DLEN
bytes.
Refs:
http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they


UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
and before DONE is set (or the last byte is shifted, I don't know excatly).
Refs:
https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
https://www.raspberrypi.org/forums/viewtopic.php?p=807834=2b612c7209f2175bf1a266359c72ae6c#p807834


I found this answer/report by joan that the downstream combined support
isn't reliable:
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they


My implementation differs from downstream in that I use local_irq_save()
to protect the polling loop. But that only protects from missing the TA
(downstream can miss the TA and issue a Stop).

So currently in mainline we have a driver that says it support the standard
(I2C_FUNC_I2C), but it really only supports one message transfers since it
can't do ReStart.

What I have done in this patch is to support ReStart for transfers with
2 messages: first write, then read. But maybe a better solution is to just
leave this alone if it is flaky and use bitbanging instead. I don't know.

I have not said that the approach you have taken is wrong or bad.

I was only telling you that the portion inside the bcm2835_i2c_xfer:
+   /* Combined write-read to the same address (smbus) */
+   if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+   !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+   (msgs[0].len <= 16)) {
+   ret = bcm2835_i2c_xfer_msg(i2c_dev, [0], [1]);
+
+   return ret ? ret : 2;
+   }
is very specific and maybe could be done in a "generic" manner
supporting more cases.

At least add a dev_warn_once for all num > 1 cases not handled by the
code above.

This gives people an opportunity to detect such a situation if they
find something is not working as expected.

Martin

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Martin Sperl]

On 20.09.2016 10:41, Noralf Trønnes wrote:


Den 20.09.2016 09:19, skrev Martin Sperl:

Hi Noralf!

On 19.09.2016 17:26, Noralf Trønnes wrote:

Some SMBus protocols use Repeated Start Condition to switch from write
mode to read mode. Devices like MMA8451 won't work without it.

When downstream implemented support for this in i2c-bcm2708, it broke
support for some devices, so a module parameter was added and combined
transfer was disabled by default.
See https://github.com/raspberrypi/linux/issues/599
It doesn't seem to have been any investigation into what the problem
really was. Later there was added a timeout on the polling loop.

One of the devices mentioned to partially stop working was DS1307.

I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
and AT24C32 (eeprom) in parallel without problems.

Signed-off-by: Noralf Trønnes 
---
  drivers/i2c/busses/i2c-bcm2835.c | 107
+++
  1 file changed, 98 insertions(+), 9 deletions(-)

...

@@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter
*adap, struct i2c_msg msgs[],
  int i;
  int ret = 0;
  +/* Combined write-read to the same address (smbus) */
+if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+(msgs[0].len <= 16)) {
+ret = bcm2835_i2c_xfer_msg(i2c_dev, [0], [1]);
+
+return ret ? ret : 2;
+}
+
  for (i = 0; i < num; i++) {
-ret = bcm2835_i2c_xfer_msg(i2c_dev, [i]);
+ret = bcm2835_i2c_xfer_msg(i2c_dev, [i], NULL);
  if (ret)
  break;
  }

This does not seem to implement the i2c_msg api correctly.

As per comments in include/uapi/linux/i2c.h on line 58 only the last
message
in a group should - by default - send a STOP.



Apparently it's a known problem that the i2c controller doesn't support
Repeated Start. It will always issue a Stop when it has transferred DLEN
bytes.
Refs:
http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they


UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
and before DONE is set (or the last byte is shifted, I don't know excatly).
Refs:
https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
https://www.raspberrypi.org/forums/viewtopic.php?p=807834=2b612c7209f2175bf1a266359c72ae6c#p807834


I found this answer/report by joan that the downstream combined support
isn't reliable:
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they


My implementation differs from downstream in that I use local_irq_save()
to protect the polling loop. But that only protects from missing the TA
(downstream can miss the TA and issue a Stop).

So currently in mainline we have a driver that says it support the standard
(I2C_FUNC_I2C), but it really only supports one message transfers since it
can't do ReStart.

What I have done in this patch is to support ReStart for transfers with
2 messages: first write, then read. But maybe a better solution is to just
leave this alone if it is flaky and use bitbanging instead. I don't know.

I have not said that the approach you have taken is wrong or bad.

I was only telling you that the portion inside the bcm2835_i2c_xfer:
+   /* Combined write-read to the same address (smbus) */
+   if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+   !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+   (msgs[0].len <= 16)) {
+   ret = bcm2835_i2c_xfer_msg(i2c_dev, [0], [1]);
+
+   return ret ? ret : 2;
+   }
is very specific and maybe could be done in a "generic" manner
supporting more cases.

At least add a dev_warn_once for all num > 1 cases not handled by the
code above.

This gives people an opportunity to detect such a situation if they
find something is not working as expected.

Martin

Re: [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes

[Martin Sperl]

On 19.09.2016 17:26, Noralf Trønnes wrote:

Writing messages larger than the FIFO size results in a hang, rendering
the machine unusable. This is because the RXD status flag is set on the
first interrupt which results in bcm2835_drain_rxfifo() stealing bytes
from the buffer. The controller continues to trigger interrupts waiting
for the missing bytes, but bcm2835_fill_txfifo() has none to give.

I remember having seen similar interrupt issues with the SPI HW-block.

In this situation wait_for_completion_timeout() apparently is unable to
stop the madness.
That is because it is a level irq that immediately triggers another 
interrupt giving

no CPU no time to do other (threaded) OS activity - this might be slightly
different for multi-core machines...

The BCM2835 ARM Peripherals datasheet has this to say about the flags:
   TXD: is set when the FIFO has space for at least one byte of data.
   RXD: is set when the FIFO contains at least one byte of data.
   TXW: is set during a write transfer and the FIFO is less than full.
   RXR: is set during a read transfer and the FIFO is or more full.

Implementing the logic from the downstream i2c-bcm2708 driver solved
the hang problem.

Signed-off-by: Noralf Trønnes<nor...@tronnes.org>

Reviewed-by: Martin Sperl <ker...@martin.sperl.org>

Re: [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes

[Martin Sperl]

On 19.09.2016 17:26, Noralf Trønnes wrote:

Writing messages larger than the FIFO size results in a hang, rendering
the machine unusable. This is because the RXD status flag is set on the
first interrupt which results in bcm2835_drain_rxfifo() stealing bytes
from the buffer. The controller continues to trigger interrupts waiting
for the missing bytes, but bcm2835_fill_txfifo() has none to give.

I remember having seen similar interrupt issues with the SPI HW-block.

In this situation wait_for_completion_timeout() apparently is unable to
stop the madness.
That is because it is a level irq that immediately triggers another 
interrupt giving

no CPU no time to do other (threaded) OS activity - this might be slightly
different for multi-core machines...

The BCM2835 ARM Peripherals datasheet has this to say about the flags:
   TXD: is set when the FIFO has space for at least one byte of data.
   RXD: is set when the FIFO contains at least one byte of data.
   TXW: is set during a write transfer and the FIFO is less than full.
   RXR: is set during a read transfer and the FIFO is or more full.

Implementing the logic from the downstream i2c-bcm2708 driver solved
the hang problem.

Signed-off-by: Noralf Trønnes

Reviewed-by: Martin Sperl 

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Martin Sperl]

Hi Noralf!

On 19.09.2016 17:26, Noralf Trønnes wrote:

Some SMBus protocols use Repeated Start Condition to switch from write
mode to read mode. Devices like MMA8451 won't work without it.

When downstream implemented support for this in i2c-bcm2708, it broke
support for some devices, so a module parameter was added and combined
transfer was disabled by default.
See https://github.com/raspberrypi/linux/issues/599
It doesn't seem to have been any investigation into what the problem
really was. Later there was added a timeout on the polling loop.

One of the devices mentioned to partially stop working was DS1307.

I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
and AT24C32 (eeprom) in parallel without problems.

Signed-off-by: Noralf Trønnes 
---
  drivers/i2c/busses/i2c-bcm2835.c | 107 +++
  1 file changed, 98 insertions(+), 9 deletions(-)

...

@@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, 
struct i2c_msg msgs[],
int i;
int ret = 0;
  
+	/* Combined write-read to the same address (smbus) */

+   if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+   !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+   (msgs[0].len <= 16)) {
+   ret = bcm2835_i2c_xfer_msg(i2c_dev, [0], [1]);
+
+   return ret ? ret : 2;
+   }
+
for (i = 0; i < num; i++) {
-   ret = bcm2835_i2c_xfer_msg(i2c_dev, [i]);
+   ret = bcm2835_i2c_xfer_msg(i2c_dev, [i], NULL);
if (ret)
break;
}

This does not seem to implement the i2c_msg api correctly.

As per comments in include/uapi/linux/i2c.h on line 58 only the last message
in a group should - by default - send a STOP.

As far as I understand  you would need to implement the I2C_M_STOP flag
(by exposing  I2C_FUNC_PROTOCOL_MANGLING in bcm2835_i2c_func)
to make this work correctly:

for (i = 0; i < num; i++) {
+   bool send_stop = (i == num - 1) ||msgs[i] 
->flags 
  _M_STOP 
;
-   ret = bcm2835_i2c_xfer_msg(i2c_dev, [i]);
+   ret = bcm2835_i2c_xfer_msg(i2c_dev, [i], send_stop);
if (ret)
break;
}

The corresponding device driver (or userspace) will need to set the flag 
correctly.


Martin

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Martin Sperl]

Hi Noralf!

On 19.09.2016 17:26, Noralf Trønnes wrote:

Some SMBus protocols use Repeated Start Condition to switch from write
mode to read mode. Devices like MMA8451 won't work without it.

When downstream implemented support for this in i2c-bcm2708, it broke
support for some devices, so a module parameter was added and combined
transfer was disabled by default.
See https://github.com/raspberrypi/linux/issues/599
It doesn't seem to have been any investigation into what the problem
really was. Later there was added a timeout on the polling loop.

One of the devices mentioned to partially stop working was DS1307.

I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
and AT24C32 (eeprom) in parallel without problems.

Signed-off-by: Noralf Trønnes 
---
  drivers/i2c/busses/i2c-bcm2835.c | 107 +++
  1 file changed, 98 insertions(+), 9 deletions(-)

...

@@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, 
struct i2c_msg msgs[],
int i;
int ret = 0;
  
+	/* Combined write-read to the same address (smbus) */

+   if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+   !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+   (msgs[0].len <= 16)) {
+   ret = bcm2835_i2c_xfer_msg(i2c_dev, [0], [1]);
+
+   return ret ? ret : 2;
+   }
+
for (i = 0; i < num; i++) {
-   ret = bcm2835_i2c_xfer_msg(i2c_dev, [i]);
+   ret = bcm2835_i2c_xfer_msg(i2c_dev, [i], NULL);
if (ret)
break;
}

This does not seem to implement the i2c_msg api correctly.

As per comments in include/uapi/linux/i2c.h on line 58 only the last message
in a group should - by default - send a STOP.

As far as I understand  you would need to implement the I2C_M_STOP flag
(by exposing  I2C_FUNC_PROTOCOL_MANGLING in bcm2835_i2c_func)
to make this work correctly:

for (i = 0; i < num; i++) {
+   bool send_stop = (i == num - 1) ||msgs[i] 
->flags 
  _M_STOP 
;
-   ret = bcm2835_i2c_xfer_msg(i2c_dev, [i]);
+   ret = bcm2835_i2c_xfer_msg(i2c_dev, [i], send_stop);
if (ret)
break;
}

The corresponding device driver (or userspace) will need to set the flag 
correctly.


Martin

Re: [PATCH v3 0/4] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 01.06.2016, at 21:05, Eric Anholt <e...@anholt.net> wrote:
> 
> I figured out another critical clock (patch 3), but didn't use the
> CLK_IS_CRITICAL flag since I want to just protect whatever clock
> happens to be the parent (there are #ifdefs in the firmware indicating
> that they've experimented with using different clocks as the parent).
> 
> I think these fixes are all suitable for 4.7.
> 
> Eric Anholt (4):
>  clk: bcm2835: Mark the VPU clock as critical
>  clk: bcm2835: Mark GPIO clocks enabled at boot as critical
>  clk: bcm2835: Mark the CM SDRAM clock's parent as critical
>  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
> 
> drivers/clk/bcm/clk-bcm2835.c | 63 +--
> 1 file changed, 61 insertions(+), 2 deletions(-)

Whole series:
Acked-by: Martin Sperl <ker...@martin.sperl.org>

Note that these patches are also seeing more testing downstream in 4.7
and there have been no hiccups seen either. Clock selection is working
as expected for I2S as well.

Re: [PATCH v3 0/4] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 01.06.2016, at 21:05, Eric Anholt  wrote:
> 
> I figured out another critical clock (patch 3), but didn't use the
> CLK_IS_CRITICAL flag since I want to just protect whatever clock
> happens to be the parent (there are #ifdefs in the firmware indicating
> that they've experimented with using different clocks as the parent).
> 
> I think these fixes are all suitable for 4.7.
> 
> Eric Anholt (4):
>  clk: bcm2835: Mark the VPU clock as critical
>  clk: bcm2835: Mark GPIO clocks enabled at boot as critical
>  clk: bcm2835: Mark the CM SDRAM clock's parent as critical
>  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
> 
> drivers/clk/bcm/clk-bcm2835.c | 63 +--
> 1 file changed, 61 insertions(+), 2 deletions(-)

Whole series:
Acked-by: Martin Sperl 

Note that these patches are also seeing more testing downstream in 4.7
and there have been no hiccups seen either. Clock selection is working
as expected for I2S as well.

Re: [PATCH 0/4] add minimal bcm2835-sdram driver

[Martin Sperl]

> On 12.05.2016, at 20:15, Eric Anholt <e...@anholt.net> wrote:
> 
> ker...@martin.sperl.org writes:
> 
>> From: Martin Sperl <ker...@martin.sperl.org>
>> 
>> As the sdram clock is a critical clock to the system
>> the minimal bcm2835-sdram driver claims (and enables)
>> this clock and also exposes the corresponding sdram
>> registers via debugfs.
> 
> I don't think this is a good solution to the problem you are trying to
> work around.  You're relying on the fact that this driver gets
> successfully probed before a driver that would -EPROBE_DEFER on a
> sibling clock, which is not a guarantee.
Ah - that is probably the reason why hand_off has not gone in...

> 
> Let's please continue the debugging of clock management on linux-clk.

I am just trying to find a solution that works...

Still I think that having a driver that claims the clock and register range
is good to have - at least this way we can have a peek at what
the firmware does set up.

Re: [PATCH 0/4] add minimal bcm2835-sdram driver

[Martin Sperl]

> On 12.05.2016, at 20:15, Eric Anholt  wrote:
> 
> ker...@martin.sperl.org writes:
> 
>> From: Martin Sperl 
>> 
>> As the sdram clock is a critical clock to the system
>> the minimal bcm2835-sdram driver claims (and enables)
>> this clock and also exposes the corresponding sdram
>> registers via debugfs.
> 
> I don't think this is a good solution to the problem you are trying to
> work around.  You're relying on the fact that this driver gets
> successfully probed before a driver that would -EPROBE_DEFER on a
> sibling clock, which is not a guarantee.
Ah - that is probably the reason why hand_off has not gone in...

> 
> Let's please continue the debugging of clock management on linux-clk.

I am just trying to find a solution that works...

Still I think that having a driver that claims the clock and register range
is good to have - at least this way we can have a peek at what
the firmware does set up.

Re: [PATCH 0/4] add minimal bcm2835-sdram driver

[Martin Sperl]

> On 12.05.2016, at 17:55, Stefan Wahren <stefan.wah...@i2se.com> wrote:
> 
> Hi,
> 
>> Martin Sperl <ker...@martin.sperl.org> hat am 12. Mai 2016 um 17:28
>> geschrieben:
>> 
>> 
>> 
>>> On 12.05.2016, at 16:50, Stefan Wahren <stefan.wah...@i2se.com> wrote:
>>> 
>>> Hi Martin,
>>> 
>>>> ker...@martin.sperl.org hat am 12. Mai 2016 um 14:38 geschrieben:
>>>> 
>>>> 
>>>> From: Martin Sperl <ker...@martin.sperl.org>
>>>> 
>>>> As the sdram clock is a critical clock to the system
>>>> the minimal bcm2835-sdram driver claims (and enables)
>>>> this clock and also exposes the corresponding sdram
>>>> registers via debugfs.
>>> 
>>> sounds like this driver should fix an clock handling issue. Unfortunately
>>> this
>>> isn't a solution in case the driver is disabled.
>> Unfortunately there is no way around this - the driver has
>> to be enabled so that the sdram clock or the parent pll,
>> which typically is plld_core, never gets disabled.
>> 
>> The only other option would be marking the clock as critical
>> for those legacy drivers.
> 
> i would prefer this option. Since this would be more a fix we could get this
> faster in, it's more clear why we are doing that and less to review.
> 
> Did i miss a drawback?

We had long discussions already on that - and HAND_OFF did not make
it into the kernel either.

I had hoped that this was simple enough.

Martin

Re: [PATCH 0/4] add minimal bcm2835-sdram driver

[Martin Sperl]

> On 12.05.2016, at 17:55, Stefan Wahren  wrote:
> 
> Hi,
> 
>> Martin Sperl  hat am 12. Mai 2016 um 17:28
>> geschrieben:
>> 
>> 
>> 
>>> On 12.05.2016, at 16:50, Stefan Wahren  wrote:
>>> 
>>> Hi Martin,
>>> 
>>>> ker...@martin.sperl.org hat am 12. Mai 2016 um 14:38 geschrieben:
>>>> 
>>>> 
>>>> From: Martin Sperl 
>>>> 
>>>> As the sdram clock is a critical clock to the system
>>>> the minimal bcm2835-sdram driver claims (and enables)
>>>> this clock and also exposes the corresponding sdram
>>>> registers via debugfs.
>>> 
>>> sounds like this driver should fix an clock handling issue. Unfortunately
>>> this
>>> isn't a solution in case the driver is disabled.
>> Unfortunately there is no way around this - the driver has
>> to be enabled so that the sdram clock or the parent pll,
>> which typically is plld_core, never gets disabled.
>> 
>> The only other option would be marking the clock as critical
>> for those legacy drivers.
> 
> i would prefer this option. Since this would be more a fix we could get this
> faster in, it's more clear why we are doing that and less to review.
> 
> Did i miss a drawback?

We had long discussions already on that - and HAND_OFF did not make
it into the kernel either.

I had hoped that this was simple enough.

Martin

Re: [PATCH 3/4] ARM: dts: bcm2835: add the bcm2835-sdram-controller to the dt

[Martin Sperl]

> On 12.05.2016, at 16:56, Stefan Wahren <stefan.wah...@i2se.com> wrote:
> 
> Hi,
> 
>> ker...@martin.sperl.org hat am 12. Mai 2016 um 14:38 geschrieben:
>> 
>> 
>> From: Martin Sperl <ker...@martin.sperl.org>
>> 
>> Add the bcm2835 sdram controller to the device tree.
>> 
>> Signed-off-by: Martin Sperl <ker...@martin.sperl.org>
>> ---
>> arch/arm/boot/dts/bcm283x.dtsi | 6 ++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
>> index 8aaf193..9db9d97 100644
>> --- a/arch/arm/boot/dts/bcm283x.dtsi
>> +++ b/arch/arm/boot/dts/bcm283x.dtsi
>> @@ -22,6 +22,12 @@
>>  #address-cells = <1>;
>>  #size-cells = <1>;
>> 
>> +memory-controller@7e002000 {
>> +compatible = "brcm,bcm2835-sdram";
>> +reg = <0x7e002000 0x58>, <0x7e002800 0x58>;
> 
> where do you get these addresses?
I took this info based on the shared headers by broadcom:
https://github.com/msperl/rpi-registers/blob/master/md/README.md

And the bootrom code showed something similar that would point to
these registers.

Ic0 and ic1 register values also show the valid memory regions:
Especially IC[01]_SRC0 contains reference to the SRAM
size on the CM - 0x2000
here the fump of devmem2: 
Value at address 0x3F00200C (0x76f3300c): 0x2000.

But as you point out that may also be the interrupt controller for the VC4
cores…

It is probably the location where the VC4 interrupt table is situated.


> According to register documentation [1] the adresses belong to the interrupt
> controller.
> 
> Maybe 0x7ee0 is the right address.
This would also fall in range with the cache register ranges:
0x7ee0 SD
0x7ee01000 L2
0x7ee02000 L1 

L1 contains also IC0 and IC1 register names (L1_IC0_CONTROL et.al.)
so I guess this is what has set me in the wrong direction...

Thanks for pointing it out...

I guess I will need to amend the patchset for that…

Martin

Re: [PATCH 3/4] ARM: dts: bcm2835: add the bcm2835-sdram-controller to the dt

[Martin Sperl]

> On 12.05.2016, at 16:56, Stefan Wahren  wrote:
> 
> Hi,
> 
>> ker...@martin.sperl.org hat am 12. Mai 2016 um 14:38 geschrieben:
>> 
>> 
>> From: Martin Sperl 
>> 
>> Add the bcm2835 sdram controller to the device tree.
>> 
>> Signed-off-by: Martin Sperl 
>> ---
>> arch/arm/boot/dts/bcm283x.dtsi | 6 ++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
>> index 8aaf193..9db9d97 100644
>> --- a/arch/arm/boot/dts/bcm283x.dtsi
>> +++ b/arch/arm/boot/dts/bcm283x.dtsi
>> @@ -22,6 +22,12 @@
>>  #address-cells = <1>;
>>  #size-cells = <1>;
>> 
>> +memory-controller@7e002000 {
>> +compatible = "brcm,bcm2835-sdram";
>> +reg = <0x7e002000 0x58>, <0x7e002800 0x58>;
> 
> where do you get these addresses?
I took this info based on the shared headers by broadcom:
https://github.com/msperl/rpi-registers/blob/master/md/README.md

And the bootrom code showed something similar that would point to
these registers.

Ic0 and ic1 register values also show the valid memory regions:
Especially IC[01]_SRC0 contains reference to the SRAM
size on the CM - 0x2000
here the fump of devmem2: 
Value at address 0x3F00200C (0x76f3300c): 0x2000.

But as you point out that may also be the interrupt controller for the VC4
cores…

It is probably the location where the VC4 interrupt table is situated.


> According to register documentation [1] the adresses belong to the interrupt
> controller.
> 
> Maybe 0x7ee0 is the right address.
This would also fall in range with the cache register ranges:
0x7ee0 SD
0x7ee01000 L2
0x7ee02000 L1 

L1 contains also IC0 and IC1 register names (L1_IC0_CONTROL et.al.)
so I guess this is what has set me in the wrong direction...

Thanks for pointing it out...

I guess I will need to amend the patchset for that…

Martin

Re: [PATCH 0/4] add minimal bcm2835-sdram driver

[Martin Sperl]

> On 12.05.2016, at 16:50, Stefan Wahren <stefan.wah...@i2se.com> wrote:
> 
> Hi Martin,
> 
>> ker...@martin.sperl.org hat am 12. Mai 2016 um 14:38 geschrieben:
>> 
>> 
>> From: Martin Sperl <ker...@martin.sperl.org>
>> 
>> As the sdram clock is a critical clock to the system
>> the minimal bcm2835-sdram driver claims (and enables)
>> this clock and also exposes the corresponding sdram
>> registers via debugfs.
> 
> sounds like this driver should fix an clock handling issue. Unfortunately this
> isn't a solution in case the driver is disabled.
Unfortunately there is no way around this - the driver has
to be enabled so that the sdram clock or the parent pll,
which typically is plld_core, never gets disabled.

The only other option would be marking the clock as critical
for those legacy drivers.

See also the discussions around the clock register for the
sdram clock, where we have the “normal” clock and some
pll as well - even if the “normal” clock is disabled, 
then clearing the sdram register (or the parent) freezes
the system.

> 
> Does the GPU firmware handle the SDRAM controller or is it initialized by
> bootcode?
> 

AFAIK it is enabled in bootcode.bin and - as of now - the firmware
updates refresh cycles when SOC temperatures change.
FW checks every 30 seconds unless there is a key set in config.txt,
which - supposedly - produces a slight impact every 30 seconds to
the system.

Martin

Re: [PATCH 0/4] add minimal bcm2835-sdram driver

[Martin Sperl]

> On 12.05.2016, at 16:50, Stefan Wahren  wrote:
> 
> Hi Martin,
> 
>> ker...@martin.sperl.org hat am 12. Mai 2016 um 14:38 geschrieben:
>> 
>> 
>> From: Martin Sperl 
>> 
>> As the sdram clock is a critical clock to the system
>> the minimal bcm2835-sdram driver claims (and enables)
>> this clock and also exposes the corresponding sdram
>> registers via debugfs.
> 
> sounds like this driver should fix an clock handling issue. Unfortunately this
> isn't a solution in case the driver is disabled.
Unfortunately there is no way around this - the driver has
to be enabled so that the sdram clock or the parent pll,
which typically is plld_core, never gets disabled.

The only other option would be marking the clock as critical
for those legacy drivers.

See also the discussions around the clock register for the
sdram clock, where we have the “normal” clock and some
pll as well - even if the “normal” clock is disabled, 
then clearing the sdram register (or the parent) freezes
the system.

> 
> Does the GPU firmware handle the SDRAM controller or is it initialized by
> bootcode?
> 

AFAIK it is enabled in bootcode.bin and - as of now - the firmware
updates refresh cycles when SOC temperatures change.
FW checks every 30 seconds unless there is a key set in config.txt,
which - supposedly - produces a slight impact every 30 seconds to
the system.

Martin

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 11.05.2016, at 10:21, Martin Sperl <ker...@martin.sperl.org> wrote:
> 
> On 10.05.2016, at 21:58, Martin Sperl <ker...@martin.sperl.org> wrote:
>> 
>> 
>> 
>>> On 10.05.2016, at 19:37, Eric Anholt <e...@anholt.net> wrote:
>>> 
>>> Martin Sperl <ker...@martin.sperl.org> writes:
>>> 
>>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>>> With the new patch 2 inserted between my previous pair, I think this
>>>>> should cover Martin's bugs with clock disabling.
>>>>> 
>>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>>> EPROBE_DEFER.
>>>>> 
>>>>> Eric Anholt (3):
>>>>> clk: bcm2835: Mark the VPU clock as critical
>>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>>> 
>>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++--
>>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>>> clocks:
>>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> 
>>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>>> set - so why is it marked as critical for all devices?
>>>> So why apply patch2 for all possible devices?
>>> 
>>> According to the CLK_IS_CRITICAL patches, the author intended critical
>>> clocks not to use the included function for marking clocks as critical
>>> From the DT.  I'm not sure why, but writing patches using that when they
>>> say not to seemed like a waste.
>>> 
>>> We could check if gp1/gp2 are already on before marking them critical.
>> That may seem reasonable.
>>> 
>>>> Loading/unloading the amba_pl011 module does not crash the system,
>>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>>> 
>>>> Here the sequence:
>>>> root@raspcm:~# dmesg -C
>>>> root@raspcm:~# modprobe amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  141.708453] Serial: AMBA PL011 UART driver
>>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root@raspcm:~# rmmod  amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  150.511248] Trying to free nonexistent resource 
>>>> <20201000-20201fff>
>>>> root@raspcm:~# modprobe amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  159.385002] Serial: AMBA PL011 UART driver
>>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root@raspcm:~# stty -F /dev/ttyAMA0
>>>> speed 9600 baud; line = 0;
>>>> -brkint -imaxbel
>>>> root@raspcm:~# Timeout, server raspcm not responding.
>>>> 
>>>> The reason behind this is that the firmware pre-configured uart clock
>>>> looks like this:
>>>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>>> ctl = 0x0296
>>>> div = 0x000a6aab
>>>> so it is configured to use plld_per (which itself is running, even if 
>>>> not enabled
>>>> in the kernel)
>>>> 
>>>> But as plld_per is not among the enabled clocks then plld_per
>>>> gets disabled as soon as the tty device is closed (by stty) and
>>>> this also disables plld...
>>>> 
>>>> Similar effect when using PCM/i2s and use speaker-test:
>>>> root@raspcm:~# dmesg -C
>>>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
&

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 11.05.2016, at 10:21, Martin Sperl  wrote:
> 
> On 10.05.2016, at 21:58, Martin Sperl  wrote:
>> 
>> 
>> 
>>> On 10.05.2016, at 19:37, Eric Anholt  wrote:
>>> 
>>> Martin Sperl  writes:
>>> 
>>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>>> With the new patch 2 inserted between my previous pair, I think this
>>>>> should cover Martin's bugs with clock disabling.
>>>>> 
>>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>>> EPROBE_DEFER.
>>>>> 
>>>>> Eric Anholt (3):
>>>>> clk: bcm2835: Mark the VPU clock as critical
>>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>>> 
>>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++--
>>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>>> clocks:
>>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>>> 
>>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>>> set - so why is it marked as critical for all devices?
>>>> So why apply patch2 for all possible devices?
>>> 
>>> According to the CLK_IS_CRITICAL patches, the author intended critical
>>> clocks not to use the included function for marking clocks as critical
>>> From the DT.  I'm not sure why, but writing patches using that when they
>>> say not to seemed like a waste.
>>> 
>>> We could check if gp1/gp2 are already on before marking them critical.
>> That may seem reasonable.
>>> 
>>>> Loading/unloading the amba_pl011 module does not crash the system,
>>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>>> 
>>>> Here the sequence:
>>>> root@raspcm:~# dmesg -C
>>>> root@raspcm:~# modprobe amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  141.708453] Serial: AMBA PL011 UART driver
>>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root@raspcm:~# rmmod  amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  150.511248] Trying to free nonexistent resource 
>>>> <20201000-20201fff>
>>>> root@raspcm:~# modprobe amba_pl011
>>>> root@raspcm:~# dmesg -c
>>>> [  159.385002] Serial: AMBA PL011 UART driver
>>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>>> base_baud = 0) is a PL011 rev2
>>>> root@raspcm:~# stty -F /dev/ttyAMA0
>>>> speed 9600 baud; line = 0;
>>>> -brkint -imaxbel
>>>> root@raspcm:~# Timeout, server raspcm not responding.
>>>> 
>>>> The reason behind this is that the firmware pre-configured uart clock
>>>> looks like this:
>>>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>>> ctl = 0x0296
>>>> div = 0x000a6aab
>>>> so it is configured to use plld_per (which itself is running, even if 
>>>> not enabled
>>>> in the kernel)
>>>> 
>>>> But as plld_per is not among the enabled clocks then plld_per
>>>> gets disabled as soon as the tty device is closed (by stty) and
>>>> this also disables plld...
>>>> 
>>>> Similar effect when using PCM/i2s and use speaker-test:
>>>> root@raspcm:~# dmesg -C
>>>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>>> modprobe snd-soc-hifiberry-dac
>>>> root@raspcm:~# dmesg
>>>> [   81.968591] 

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

On 10.05.2016, at 21:58, Martin Sperl <ker...@martin.sperl.org> wrote:
> 
> 
> 
>> On 10.05.2016, at 19:37, Eric Anholt <e...@anholt.net> wrote:
>> 
>> Martin Sperl <ker...@martin.sperl.org> writes:
>> 
>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>> With the new patch 2 inserted between my previous pair, I think this
>>>> should cover Martin's bugs with clock disabling.
>>>> 
>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>> EPROBE_DEFER.
>>>> 
>>>> Eric Anholt (3):
>>>> clk: bcm2835: Mark the VPU clock as critical
>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>> 
>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++--
>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>> clocks:
>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>> 
>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>> set - so why is it marked as critical for all devices?
>>> So why apply patch2 for all possible devices?
>> 
>> According to the CLK_IS_CRITICAL patches, the author intended critical
>> clocks not to use the included function for marking clocks as critical
>> From the DT.  I'm not sure why, but writing patches using that when they
>> say not to seemed like a waste.
>> 
>> We could check if gp1/gp2 are already on before marking them critical.
> That may seem reasonable.
>> 
>>> Loading/unloading the amba_pl011 module does not crash the system,
>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>> 
>>> Here the sequence:
>>> root@raspcm:~# dmesg -C
>>> root@raspcm:~# modprobe amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  141.708453] Serial: AMBA PL011 UART driver
>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>> base_baud = 0) is a PL011 rev2
>>> root@raspcm:~# rmmod  amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  150.511248] Trying to free nonexistent resource 
>>> <20201000-20201fff>
>>> root@raspcm:~# modprobe amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  159.385002] Serial: AMBA PL011 UART driver
>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>> base_baud = 0) is a PL011 rev2
>>> root@raspcm:~# stty -F /dev/ttyAMA0
>>> speed 9600 baud; line = 0;
>>> -brkint -imaxbel
>>> root@raspcm:~# Timeout, server raspcm not responding.
>>> 
>>> The reason behind this is that the firmware pre-configured uart clock
>>> looks like this:
>>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>> ctl = 0x0296
>>> div = 0x000a6aab
>>> so it is configured to use plld_per (which itself is running, even if 
>>> not enabled
>>> in the kernel)
>>> 
>>> But as plld_per is not among the enabled clocks then plld_per
>>> gets disabled as soon as the tty device is closed (by stty) and
>>> this also disables plld...
>>> 
>>> Similar effect when using PCM/i2s and use speaker-test:
>>> root@raspcm:~# dmesg -C
>>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>> modprobe snd-soc-hifiberry-dac
>>> root@raspcm:~# dmesg
>>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>>> mapping ok
>>> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>>> [1] 579
>>> root@raspcm:~#
>>> speaker-test 1.0.28
>>> 
>>> Playback device is default
>>> Stream parameters 

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

On 10.05.2016, at 21:58, Martin Sperl  wrote:
> 
> 
> 
>> On 10.05.2016, at 19:37, Eric Anholt  wrote:
>> 
>> Martin Sperl  writes:
>> 
>>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>>> With the new patch 2 inserted between my previous pair, I think this
>>>> should cover Martin's bugs with clock disabling.
>>>> 
>>>> I tested patch 2 to be important on the downstream kernel: with the
>>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>>> EPROBE_DEFER.
>>>> 
>>>> Eric Anholt (3):
>>>> clk: bcm2835: Mark the VPU clock as critical
>>>> clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>> clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>>> 
>>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++--
>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>> I gave it a try - with all 3 patches applied I get the following enabled 
>>> clocks:
>>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>>> 
>>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>>> set - so why is it marked as critical for all devices?
>>> So why apply patch2 for all possible devices?
>> 
>> According to the CLK_IS_CRITICAL patches, the author intended critical
>> clocks not to use the included function for marking clocks as critical
>> From the DT.  I'm not sure why, but writing patches using that when they
>> say not to seemed like a waste.
>> 
>> We could check if gp1/gp2 are already on before marking them critical.
> That may seem reasonable.
>> 
>>> Loading/unloading the amba_pl011 module does not crash the system,
>>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>>> 
>>> Here the sequence:
>>> root@raspcm:~# dmesg -C
>>> root@raspcm:~# modprobe amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  141.708453] Serial: AMBA PL011 UART driver
>>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>> base_baud = 0) is a PL011 rev2
>>> root@raspcm:~# rmmod  amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  150.511248] Trying to free nonexistent resource 
>>> <20201000-20201fff>
>>> root@raspcm:~# modprobe amba_pl011
>>> root@raspcm:~# dmesg -c
>>> [  159.385002] Serial: AMBA PL011 UART driver
>>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>>> base_baud = 0) is a PL011 rev2
>>> root@raspcm:~# stty -F /dev/ttyAMA0
>>> speed 9600 baud; line = 0;
>>> -brkint -imaxbel
>>> root@raspcm:~# Timeout, server raspcm not responding.
>>> 
>>> The reason behind this is that the firmware pre-configured uart clock
>>> looks like this:
>>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>>> ctl = 0x0296
>>> div = 0x000a6aab
>>> so it is configured to use plld_per (which itself is running, even if 
>>> not enabled
>>> in the kernel)
>>> 
>>> But as plld_per is not among the enabled clocks then plld_per
>>> gets disabled as soon as the tty device is closed (by stty) and
>>> this also disables plld...
>>> 
>>> Similar effect when using PCM/i2s and use speaker-test:
>>> root@raspcm:~# dmesg -C
>>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>>> modprobe snd-soc-hifiberry-dac
>>> root@raspcm:~# dmesg
>>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>>> mapping ok
>>> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>>> [1] 579
>>> root@raspcm:~#
>>> speaker-test 1.0.28
>>> 
>>> Playback device is default
>>> Stream parameters are 44100Hz, S16_LE, 2 channels
>>> Sine wave rate is 440.Hz
>>> 

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 10.05.2016, at 19:37, Eric Anholt <e...@anholt.net> wrote:
> 
> Martin Sperl <ker...@martin.sperl.org> writes:
> 
>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>> With the new patch 2 inserted between my previous pair, I think this
>>> should cover Martin's bugs with clock disabling.
>>> 
>>> I tested patch 2 to be important on the downstream kernel: with the
>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>> EPROBE_DEFER.
>>> 
>>> Eric Anholt (3):
>>>  clk: bcm2835: Mark the VPU clock as critical
>>>  clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>> 
>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++--
>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>> I gave it a try - with all 3 patches applied I get the following enabled 
>> clocks:
>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>> 
>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>> set - so why is it marked as critical for all devices?
>> So why apply patch2 for all possible devices?
> 
> According to the CLK_IS_CRITICAL patches, the author intended critical
> clocks not to use the included function for marking clocks as critical
> From the DT.  I'm not sure why, but writing patches using that when they
> say not to seemed like a waste.
> 
> We could check if gp1/gp2 are already on before marking them critical.
That may seem reasonable.
> 
>> Loading/unloading the amba_pl011 module does not crash the system,
>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>> 
>> Here the sequence:
>> root@raspcm:~# dmesg -C
>> root@raspcm:~# modprobe amba_pl011
>> root@raspcm:~# dmesg -c
>> [  141.708453] Serial: AMBA PL011 UART driver
>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>> base_baud = 0) is a PL011 rev2
>> root@raspcm:~# rmmod  amba_pl011
>> root@raspcm:~# dmesg -c
>> [  150.511248] Trying to free nonexistent resource 
>> <20201000-20201fff>
>> root@raspcm:~# modprobe amba_pl011
>> root@raspcm:~# dmesg -c
>> [  159.385002] Serial: AMBA PL011 UART driver
>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>> base_baud = 0) is a PL011 rev2
>> root@raspcm:~# stty -F /dev/ttyAMA0
>> speed 9600 baud; line = 0;
>> -brkint -imaxbel
>> root@raspcm:~# Timeout, server raspcm not responding.
>> 
>> The reason behind this is that the firmware pre-configured uart clock
>> looks like this:
>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>> ctl = 0x0296
>> div = 0x000a6aab
>> so it is configured to use plld_per (which itself is running, even if 
>> not enabled
>> in the kernel)
>> 
>> But as plld_per is not among the enabled clocks then plld_per
>> gets disabled as soon as the tty device is closed (by stty) and
>> this also disables plld...
>> 
>> Similar effect when using PCM/i2s and use speaker-test:
>> root@raspcm:~# dmesg -C
>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>> modprobe snd-soc-hifiberry-dac
>> root@raspcm:~# dmesg
>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>> mapping ok
>> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>> [1] 579
>> root@raspcm:~#
>> speaker-test 1.0.28
>> 
>> Playback device is default
>> Stream parameters are 44100Hz, S16_LE, 2 channels
>> Sine wave rate is 440.Hz
>> Rate set to 44100Hz (requested 44100Hz)
>> Buffer size range from 128 to 131072
>> Period size range from 64 to 65536
>> Using max buffer size 131072
>> Periods = 4
>> was set period_size = 32768
>> was set buffer_size = 131072
>> 0 - Front Left
>> 1 - Front Right
>> 
>> root@raspcm:~#
>> root@raspcm:~# grep -vE ^0 /sys/kernel/d

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

> On 10.05.2016, at 19:37, Eric Anholt  wrote:
> 
> Martin Sperl  writes:
> 
>>> On 10.05.2016 03:01, Eric Anholt wrote:
>>> With the new patch 2 inserted between my previous pair, I think this
>>> should cover Martin's bugs with clock disabling.
>>> 
>>> I tested patch 2 to be important on the downstream kernel: with the
>>> DPI panel support added there, I was losing ethernet (my only I/O)
>>> when the HDMI HSM hanging off of PLLD_PER got disabled due to
>>> EPROBE_DEFER.
>>> 
>>> Eric Anholt (3):
>>>  clk: bcm2835: Mark the VPU clock as critical
>>>  clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
>>>  clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent
>>> 
>>> drivers/clk/bcm/clk-bcm2835.c | 32 ++--
>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>> I gave it a try - with all 3 patches applied I get the following enabled 
>> clocks:
>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>> /sys/kernel/debug/clk/aux_uart/clk_enable_count:1
>> /sys/kernel/debug/clk/emmc/clk_enable_count:1
>> /sys/kernel/debug/clk/gp1/clk_enable_count:1
>> /sys/kernel/debug/clk/gp2/clk_enable_count:1
>> /sys/kernel/debug/clk/osc/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc/clk_enable_count:2
>> /sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
>> /sys/kernel/debug/clk/pllc_per/clk_enable_count:1
>> /sys/kernel/debug/clk/vpu/clk_enable_count:2
>> 
>> At least on my compute module gp1/gp2 is enabled, but there is no rate
>> set - so why is it marked as critical for all devices?
>> So why apply patch2 for all possible devices?
> 
> According to the CLK_IS_CRITICAL patches, the author intended critical
> clocks not to use the included function for marking clocks as critical
> From the DT.  I'm not sure why, but writing patches using that when they
> say not to seemed like a waste.
> 
> We could check if gp1/gp2 are already on before marking them critical.
That may seem reasonable.
> 
>> Loading/unloading the amba_pl011 module does not crash the system,
>> but a simple stty -F /dev/ttyAMA0 does crash the system!
>> 
>> Here the sequence:
>> root@raspcm:~# dmesg -C
>> root@raspcm:~# modprobe amba_pl011
>> root@raspcm:~# dmesg -c
>> [  141.708453] Serial: AMBA PL011 UART driver
>> [  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>> base_baud = 0) is a PL011 rev2
>> root@raspcm:~# rmmod  amba_pl011
>> root@raspcm:~# dmesg -c
>> [  150.511248] Trying to free nonexistent resource 
>> <20201000-20201fff>
>> root@raspcm:~# modprobe amba_pl011
>> root@raspcm:~# dmesg -c
>> [  159.385002] Serial: AMBA PL011 UART driver
>> [  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
>> base_baud = 0) is a PL011 rev2
>> root@raspcm:~# stty -F /dev/ttyAMA0
>> speed 9600 baud; line = 0;
>> -brkint -imaxbel
>> root@raspcm:~# Timeout, server raspcm not responding.
>> 
>> The reason behind this is that the firmware pre-configured uart clock
>> looks like this:
>> root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
>> ctl = 0x0296
>> div = 0x000a6aab
>> so it is configured to use plld_per (which itself is running, even if 
>> not enabled
>> in the kernel)
>> 
>> But as plld_per is not among the enabled clocks then plld_per
>> gets disabled as soon as the tty device is closed (by stty) and
>> this also disables plld...
>> 
>> Similar effect when using PCM/i2s and use speaker-test:
>> root@raspcm:~# dmesg -C
>> root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
>> modprobe snd-soc-hifiberry-dac
>> root@raspcm:~# dmesg
>> [   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
>> mapping ok
>> root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
>> [1] 579
>> root@raspcm:~#
>> speaker-test 1.0.28
>> 
>> Playback device is default
>> Stream parameters are 44100Hz, S16_LE, 2 channels
>> Sine wave rate is 440.Hz
>> Rate set to 44100Hz (requested 44100Hz)
>> Buffer size range from 128 to 131072
>> Period size range from 64 to 65536
>> Using max buffer size 131072
>> Periods = 4
>> was set period_size = 32768
>> was set buffer_size = 131072
>> 0 - Front Left
>> 1 - Front Right
>> 
>> root@raspcm:~#
>> root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
>> /sys/kernel/de

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

On 10.05.2016 03:01, Eric Anholt wrote:

With the new patch 2 inserted between my previous pair, I think this
should cover Martin's bugs with clock disabling.

I tested patch 2 to be important on the downstream kernel: with the
DPI panel support added there, I was losing ethernet (my only I/O)
when the HDMI HSM hanging off of PLLD_PER got disabled due to
EPROBE_DEFER.

Eric Anholt (3):
   clk: bcm2835: Mark the VPU clock as critical
   clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
   clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

  drivers/clk/bcm/clk-bcm2835.c | 32 ++--
  1 file changed, 30 insertions(+), 2 deletions(-)

I gave it a try - with all 3 patches applied I get the following enabled 
clocks:

root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/aux_uart/clk_enable_count:1
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/gp1/clk_enable_count:1
/sys/kernel/debug/clk/gp2/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:2

At least on my compute module gp1/gp2 is enabled, but there is no rate
set - so why is it marked as critical for all devices?
So why apply patch2 for all possible devices?

Loading/unloading the amba_pl011 module does not crash the system,
but a simple stty -F /dev/ttyAMA0 does crash the system!

Here the sequence:
root@raspcm:~# dmesg -C
root@raspcm:~# modprobe amba_pl011
root@raspcm:~# dmesg -c
[  141.708453] Serial: AMBA PL011 UART driver
[  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
base_baud = 0) is a PL011 rev2

root@raspcm:~# rmmod  amba_pl011
root@raspcm:~# dmesg -c
[  150.511248] Trying to free nonexistent resource 
<20201000-20201fff>

root@raspcm:~# modprobe amba_pl011
root@raspcm:~# dmesg -c
[  159.385002] Serial: AMBA PL011 UART driver
[  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
base_baud = 0) is a PL011 rev2

root@raspcm:~# stty -F /dev/ttyAMA0
speed 9600 baud; line = 0;
-brkint -imaxbel
root@raspcm:~# Timeout, server raspcm not responding.

The reason behind this is that the firmware pre-configured uart clock
looks like this:
root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
ctl = 0x0296
div = 0x000a6aab
so it is configured to use plld_per (which itself is running, even if 
not enabled

in the kernel)

But as plld_per is not among the enabled clocks then plld_per
gets disabled as soon as the tty device is closed (by stty) and
this also disables plld...

Similar effect when using PCM/i2s and use speaker-test:
root@raspcm:~# dmesg -C
root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
modprobe snd-soc-hifiberry-dac

root@raspcm:~# dmesg
[   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
mapping ok

root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
[1] 579
root@raspcm:~#
speaker-test 1.0.28

Playback device is default
Stream parameters are 44100Hz, S16_LE, 2 channels
Sine wave rate is 440.Hz
Rate set to 44100Hz (requested 44100Hz)
Buffer size range from 128 to 131072
Period size range from 64 to 65536
Using max buffer size 131072
Periods = 4
was set period_size = 32768
was set buffer_size = 131072
 0 - Front Left
 1 - Front Right

root@raspcm:~#
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/aux_uart/clk_enable_count:1
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/gp1/clk_enable_count:1
/sys/kernel/debug/clk/gp2/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:2
/sys/kernel/debug/clk/pcm/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/plld/clk_enable_count:1
/sys/kernel/debug/clk/plld_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:2
root@raspcm:~# kill %1
root@raspcm:~# Time per period = 106.889502
Timeout, server raspcm not responding.

You see that plld gets now used and when I kill speaker-test
the machine crashes again.

So this patchset does not really solve any of the problems that
I have reported either.

That is why my patchset has taken the "HAND_OFF" approach
instead (which still just hides some of the issues), but at least
it does not crash the system on the use of plld and it allows
for custom parent and mash selection.

In reality it would require consumers of the corresponding
parent clocks in the kernel (arm, ...) and the knowledge which
clocks are really needed by the firmware - i.e plld.

Note that the sdram clock is using plld_core parent!
root@raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
ctl = 0x4006
div = 0x3000
root@raspcm:~# cat 

Re: [PATCH 0/3] clk: bcm2835: critical clocks and parent selection

[Martin Sperl]

On 10.05.2016 03:01, Eric Anholt wrote:

With the new patch 2 inserted between my previous pair, I think this
should cover Martin's bugs with clock disabling.

I tested patch 2 to be important on the downstream kernel: with the
DPI panel support added there, I was losing ethernet (my only I/O)
when the HDMI HSM hanging off of PLLD_PER got disabled due to
EPROBE_DEFER.

Eric Anholt (3):
   clk: bcm2835: Mark the VPU clock as critical
   clk: bcm2835: Mark GPIO clocks enabled at boot as critical.
   clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

  drivers/clk/bcm/clk-bcm2835.c | 32 ++--
  1 file changed, 30 insertions(+), 2 deletions(-)

I gave it a try - with all 3 patches applied I get the following enabled 
clocks:

root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/aux_uart/clk_enable_count:1
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/gp1/clk_enable_count:1
/sys/kernel/debug/clk/gp2/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:2

At least on my compute module gp1/gp2 is enabled, but there is no rate
set - so why is it marked as critical for all devices?
So why apply patch2 for all possible devices?

Loading/unloading the amba_pl011 module does not crash the system,
but a simple stty -F /dev/ttyAMA0 does crash the system!

Here the sequence:
root@raspcm:~# dmesg -C
root@raspcm:~# modprobe amba_pl011
root@raspcm:~# dmesg -c
[  141.708453] Serial: AMBA PL011 UART driver
[  141.709158] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
base_baud = 0) is a PL011 rev2

root@raspcm:~# rmmod  amba_pl011
root@raspcm:~# dmesg -c
[  150.511248] Trying to free nonexistent resource 
<20201000-20201fff>

root@raspcm:~# modprobe amba_pl011
root@raspcm:~# dmesg -c
[  159.385002] Serial: AMBA PL011 UART driver
[  159.385714] 20201000.uart: ttyAMA0 at MMIO 0x20201000 (irq = 81, 
base_baud = 0) is a PL011 rev2

root@raspcm:~# stty -F /dev/ttyAMA0
speed 9600 baud; line = 0;
-brkint -imaxbel
root@raspcm:~# Timeout, server raspcm not responding.

The reason behind this is that the firmware pre-configured uart clock
looks like this:
root@raspcm:~# cat /sys/kernel/debug/clk/uart/regdump
ctl = 0x0296
div = 0x000a6aab
so it is configured to use plld_per (which itself is running, even if 
not enabled

in the kernel)

But as plld_per is not among the enabled clocks then plld_per
gets disabled as soon as the tty device is closed (by stty) and
this also disables plld...

Similar effect when using PCM/i2s and use speaker-test:
root@raspcm:~# dmesg -C
root@raspcm:~# modprobe snd-soc-bcm2835-i2s; modprobe snd-soc-pcm5102a; 
modprobe snd-soc-hifiberry-dac

root@raspcm:~# dmesg
[   81.968591] snd-hifiberry-dac sound: pcm5102a-hifi <-> 20203000.i2s 
mapping ok

root@raspcm:~# speaker-test -c 2 -r 44100 -F S16_LE -f 440 -t sine&
[1] 579
root@raspcm:~#
speaker-test 1.0.28

Playback device is default
Stream parameters are 44100Hz, S16_LE, 2 channels
Sine wave rate is 440.Hz
Rate set to 44100Hz (requested 44100Hz)
Buffer size range from 128 to 131072
Period size range from 64 to 65536
Using max buffer size 131072
Periods = 4
was set period_size = 32768
was set buffer_size = 131072
 0 - Front Left
 1 - Front Right

root@raspcm:~#
root@raspcm:~# grep -vE ^0 /sys/kernel/debug/clk/*/clk_enable_count
/sys/kernel/debug/clk/aux_uart/clk_enable_count:1
/sys/kernel/debug/clk/emmc/clk_enable_count:1
/sys/kernel/debug/clk/gp1/clk_enable_count:1
/sys/kernel/debug/clk/gp2/clk_enable_count:1
/sys/kernel/debug/clk/osc/clk_enable_count:2
/sys/kernel/debug/clk/pcm/clk_enable_count:1
/sys/kernel/debug/clk/pllc/clk_enable_count:2
/sys/kernel/debug/clk/pllc_core0/clk_enable_count:1
/sys/kernel/debug/clk/pllc_per/clk_enable_count:1
/sys/kernel/debug/clk/plld/clk_enable_count:1
/sys/kernel/debug/clk/plld_per/clk_enable_count:1
/sys/kernel/debug/clk/vpu/clk_enable_count:2
root@raspcm:~# kill %1
root@raspcm:~# Time per period = 106.889502
Timeout, server raspcm not responding.

You see that plld gets now used and when I kill speaker-test
the machine crashes again.

So this patchset does not really solve any of the problems that
I have reported either.

That is why my patchset has taken the "HAND_OFF" approach
instead (which still just hides some of the issues), but at least
it does not crash the system on the use of plld and it allows
for custom parent and mash selection.

In reality it would require consumers of the corresponding
parent clocks in the kernel (arm, ...) and the knowledge which
clocks are really needed by the firmware - i.e plld.

Note that the sdram clock is using plld_core parent!
root@raspcm:~#  cat /sys/kernel/debug/clk/sdram/regdump
ctl = 0x4006
div = 0x3000
root@raspcm:~# cat 

Re: [PATCH 2/2] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Martin Sperl]

> On 02.05.2016, at 17:29, Eric Anholt <e...@anholt.net> wrote:
> 
> Martin Sperl <ker...@martin.sperl.org> writes:
> 
>>> On 26.04.2016, at 21:39, Eric Anholt <e...@anholt.net> wrote:
>>> 
>>> If the firmware had set up a clock to source from PLLC, go along with
>>> it.  But if we're looking for a new parent, we don't want to switch it
>>> to PLLC because the firmware will force PLLC (and thus the AXI bus
>>> clock) to different frequencies during over-temp/under-voltage,
>>> without notification to Linux.
>>> 
>>> On my system, this moves the Linux-enabled HDMI state machine and DSI1
>>> escape clock over to plld_per from pllc_per.  EMMC still ends up on
>>> pllc_per, because the firmware had set it up to use that.
>>> 
>>> Signed-off-by: Eric Anholt <e...@anholt.net>
>>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio 
>>> domain clocks")
>>> —
>> 
>> I guess this patch looks to me as if it is a policy inside the kernel,
>> which is AFAIK frowned upon.
> 
> Can you come up with a use for putting peripherals on PLLC ever, such
> that we need choice?

For PLLC not right now, but with clk_notifier_register drivers could
work around those clock changes (assuming we get that information
from the firmware somehow - or if we could move this decision into the
kernel: even better).

But I can come up with a scenario that would make use of the pllh_aux
under some circumstances - e.g when requesting 290039Hz on clock gp0/1/2.

Similarly: if we ever enable the testdebugX clocks these become immediate
candidates for parent-clocks as well which can result in more headache.

Being able to define which clocks to use at least give the dts author
a means also to control clock selection if he wants to enable the
testdebug clocks.

Another similar situation can occur with plla_per - under some
circumstances it may get used unexpectedly.

Martin

Re: [PATCH 2/2] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Martin Sperl]

> On 02.05.2016, at 17:29, Eric Anholt  wrote:
> 
> Martin Sperl  writes:
> 
>>> On 26.04.2016, at 21:39, Eric Anholt  wrote:
>>> 
>>> If the firmware had set up a clock to source from PLLC, go along with
>>> it.  But if we're looking for a new parent, we don't want to switch it
>>> to PLLC because the firmware will force PLLC (and thus the AXI bus
>>> clock) to different frequencies during over-temp/under-voltage,
>>> without notification to Linux.
>>> 
>>> On my system, this moves the Linux-enabled HDMI state machine and DSI1
>>> escape clock over to plld_per from pllc_per.  EMMC still ends up on
>>> pllc_per, because the firmware had set it up to use that.
>>> 
>>> Signed-off-by: Eric Anholt 
>>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio 
>>> domain clocks")
>>> —
>> 
>> I guess this patch looks to me as if it is a policy inside the kernel,
>> which is AFAIK frowned upon.
> 
> Can you come up with a use for putting peripherals on PLLC ever, such
> that we need choice?

For PLLC not right now, but with clk_notifier_register drivers could
work around those clock changes (assuming we get that information
from the firmware somehow - or if we could move this decision into the
kernel: even better).

But I can come up with a scenario that would make use of the pllh_aux
under some circumstances - e.g when requesting 290039Hz on clock gp0/1/2.

Similarly: if we ever enable the testdebugX clocks these become immediate
candidates for parent-clocks as well which can result in more headache.

Being able to define which clocks to use at least give the dts author
a means also to control clock selection if he wants to enable the
testdebug clocks.

Another similar situation can occur with plla_per - under some
circumstances it may get used unexpectedly.

Martin

Re: [PATCH 2/2] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Martin Sperl]

On 30.04.2016 11:28, Martin Sperl wrote:

On 26.04.2016, at 21:39, Eric Anholt <e...@anholt.net> wrote:

If the firmware had set up a clock to source from PLLC, go along with
it.  But if we're looking for a new parent, we don't want to switch it
to PLLC because the firmware will force PLLC (and thus the AXI bus
clock) to different frequencies during over-temp/under-voltage,
without notification to Linux.

On my system, this moves the Linux-enabled HDMI state machine and DSI1
escape clock over to plld_per from pllc_per.  EMMC still ends up on
pllc_per, because the firmware had set it up to use that.

Signed-off-by: Eric Anholt <e...@anholt.net>
Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain 
clocks")
—

I guess this patch looks to me as if it is a policy inside the kernel,
which is AFAIK frowned upon.

I am looking into making "assigned-clock-parents” inside the dt
work with the driver.

Could look something like this:
i2s: i2s@7e203000 {
assigned-clock-parents = < BCM2835_PLLD_PER>, <_osc>;
assigned-clocks = < BCM2835_CLOCK_PCM>, < 
BCM2835_CLOCK_PCM>;
};
(not sure if that works really - the same clock in assigned-clocks looks 
suspicious)

This would move the policy out of the kernel into the device-tree,
which - i guess is a better solution.


So after some more investigation it seems that we can not really use those
assigned-clock-parents properties for our purpose of filtering the parent
clocks, as it:
a) requires also assigned-clocks to be set (this may be OK)
b) it does not allow to define a list of clocks to get used - it will 
just set the
parent of the assigned-clock - if we take the example shown above, 
it would

call clk_set_parent 2 times for the PCM clock - once with PLLD_PER
and once with clk_osc.

So I start to wonder if it would not be better to use an approach like this:
cprman: cprman@7e101000 {
...
brcm,clock-flags = , ;
brcm,clock-index = , ;
}

the flags would be a bitfield that select the parent clocks.

So it could look like this:
cprman: cprman@7e101000 {
...
brcm,clock-flags = (BIT(BCM2835_PER_PARENT_OSC) |
BIT(BCM2835_PER_PARENT_PLLD_PER)), ...;
brcm,clock-index = , ;
}

BCM2835_PER_PARENT_PLLD_PER and BCM2835_PER_PARENT_OSC
would then be defined in include/dt-bindings/clock/bcm2835.h

In addition this would also allow us to add other flags to enable
higher order MASH clock dividers - we currently only allow simple
fractional dividers - we could even force the use of integer dividers
if there comes a need.

This would really allow us to define the parents freely and if the
firmware ever changes its behavior with regards to PLLC, then we can
easily change the device-tree.

Is this approach acceptable - maybe in a variation?

Thanks,
Martin

Re: [PATCH 2/2] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Martin Sperl]

On 30.04.2016 11:28, Martin Sperl wrote:

On 26.04.2016, at 21:39, Eric Anholt  wrote:

If the firmware had set up a clock to source from PLLC, go along with
it.  But if we're looking for a new parent, we don't want to switch it
to PLLC because the firmware will force PLLC (and thus the AXI bus
clock) to different frequencies during over-temp/under-voltage,
without notification to Linux.

On my system, this moves the Linux-enabled HDMI state machine and DSI1
escape clock over to plld_per from pllc_per.  EMMC still ends up on
pllc_per, because the firmware had set it up to use that.

Signed-off-by: Eric Anholt 
Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain 
clocks")
—

I guess this patch looks to me as if it is a policy inside the kernel,
which is AFAIK frowned upon.

I am looking into making "assigned-clock-parents” inside the dt
work with the driver.

Could look something like this:
i2s: i2s@7e203000 {
assigned-clock-parents = < BCM2835_PLLD_PER>, <_osc>;
assigned-clocks = < BCM2835_CLOCK_PCM>, < 
BCM2835_CLOCK_PCM>;
};
(not sure if that works really - the same clock in assigned-clocks looks 
suspicious)

This would move the policy out of the kernel into the device-tree,
which - i guess is a better solution.


So after some more investigation it seems that we can not really use those
assigned-clock-parents properties for our purpose of filtering the parent
clocks, as it:
a) requires also assigned-clocks to be set (this may be OK)
b) it does not allow to define a list of clocks to get used - it will 
just set the
parent of the assigned-clock - if we take the example shown above, 
it would

call clk_set_parent 2 times for the PCM clock - once with PLLD_PER
and once with clk_osc.

So I start to wonder if it would not be better to use an approach like this:
cprman: cprman@7e101000 {
...
brcm,clock-flags = , ;
brcm,clock-index = , ;
}

the flags would be a bitfield that select the parent clocks.

So it could look like this:
cprman: cprman@7e101000 {
...
brcm,clock-flags = (BIT(BCM2835_PER_PARENT_OSC) |
BIT(BCM2835_PER_PARENT_PLLD_PER)), ...;
brcm,clock-index = , ;
}

BCM2835_PER_PARENT_PLLD_PER and BCM2835_PER_PARENT_OSC
would then be defined in include/dt-bindings/clock/bcm2835.h

In addition this would also allow us to add other flags to enable
higher order MASH clock dividers - we currently only allow simple
fractional dividers - we could even force the use of integer dividers
if there comes a need.

This would really allow us to define the parents freely and if the
firmware ever changes its behavior with regards to PLLC, then we can
easily change the device-tree.

Is this approach acceptable - maybe in a variation?

Thanks,
Martin

Re: [PATCH 2/2] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Martin Sperl]

> On 26.04.2016, at 21:39, Eric Anholt  wrote:
> 
> If the firmware had set up a clock to source from PLLC, go along with
> it.  But if we're looking for a new parent, we don't want to switch it
> to PLLC because the firmware will force PLLC (and thus the AXI bus
> clock) to different frequencies during over-temp/under-voltage,
> without notification to Linux.
> 
> On my system, this moves the Linux-enabled HDMI state machine and DSI1
> escape clock over to plld_per from pllc_per.  EMMC still ends up on
> pllc_per, because the firmware had set it up to use that.
> 
> Signed-off-by: Eric Anholt 
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio 
> domain clocks")
> —

I guess this patch looks to me as if it is a policy inside the kernel,
which is AFAIK frowned upon.

I am looking into making "assigned-clock-parents” inside the dt 
work with the driver.

Could look something like this:
i2s: i2s@7e203000 {
assigned-clock-parents = < BCM2835_PLLD_PER>, <_osc>;
assigned-clocks = < BCM2835_CLOCK_PCM>, < 
BCM2835_CLOCK_PCM>;
};
(not sure if that works really - the same clock in assigned-clocks looks 
suspicious)

This would move the policy out of the kernel into the device-tree,
which - i guess is a better solution.

Martin

Re: [PATCH 2/2] clk: bcm2835: Skip PLLC clocks when deciding on a new clock parent

[Martin Sperl]

> On 26.04.2016, at 21:39, Eric Anholt  wrote:
> 
> If the firmware had set up a clock to source from PLLC, go along with
> it.  But if we're looking for a new parent, we don't want to switch it
> to PLLC because the firmware will force PLLC (and thus the AXI bus
> clock) to different frequencies during over-temp/under-voltage,
> without notification to Linux.
> 
> On my system, this moves the Linux-enabled HDMI state machine and DSI1
> escape clock over to plld_per from pllc_per.  EMMC still ends up on
> pllc_per, because the firmware had set it up to use that.
> 
> Signed-off-by: Eric Anholt 
> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio 
> domain clocks")
> —

I guess this patch looks to me as if it is a policy inside the kernel,
which is AFAIK frowned upon.

I am looking into making "assigned-clock-parents” inside the dt 
work with the driver.

Could look something like this:
i2s: i2s@7e203000 {
assigned-clock-parents = < BCM2835_PLLD_PER>, <_osc>;
assigned-clocks = < BCM2835_CLOCK_PCM>, < 
BCM2835_CLOCK_PCM>;
};
(not sure if that works really - the same clock in assigned-clocks looks 
suspicious)

This would move the policy out of the kernel into the device-tree,
which - i guess is a better solution.

Martin

Re: [PATCH 1/2] ASoC: dmaengine_pcm: Add support for packed transfers

[Martin Sperl]

> On 27.04.2016, at 15:26, Matthias Reichl <h...@horus.com> wrote:
> 
> dmaengine_pcm currently only supports setups where FIFO reads/writes
> correspond to exactly one sample, eg 16-bit sample data is transferred
> via 16-bit FIFO accesses, 32-bit data via 32-bit accesses.
> 
> This patch adds support for setups with fixed width FIFOs where
> multiple samples are packed into a larger word.
> 
> For example setups with a 32-bit wide FIFO register that expect
> 16-bit sample transfers to be done with the left+right sample data
> packed into a 32-bit word.
> 
> Support for packed transfers is controlled via the
> SND_DMAENGINE_PCM_DAI_FLAG_PACK flag in snd_dmaengine_dai_dma_data.flags
> 
> If this flag is set dmaengine_pcm doesn't put any restriction on the
> supported formats and sets the DMA transfer width to undefined.
> 
> This means control over the constraints is now transferred to the DAI
> driver and it's responsible to provide proper configuration and
> check for possible corner cases that aren't handled by the ALSA core.
> 
> Signed-off-by: Matthias Reichl <h...@horus.com>

Tested-by: Martin Sperl <ker...@martin.sperl.org>

Re: [PATCH 1/2] ASoC: dmaengine_pcm: Add support for packed transfers

[Martin Sperl]

> On 27.04.2016, at 15:26, Matthias Reichl  wrote:
> 
> dmaengine_pcm currently only supports setups where FIFO reads/writes
> correspond to exactly one sample, eg 16-bit sample data is transferred
> via 16-bit FIFO accesses, 32-bit data via 32-bit accesses.
> 
> This patch adds support for setups with fixed width FIFOs where
> multiple samples are packed into a larger word.
> 
> For example setups with a 32-bit wide FIFO register that expect
> 16-bit sample transfers to be done with the left+right sample data
> packed into a 32-bit word.
> 
> Support for packed transfers is controlled via the
> SND_DMAENGINE_PCM_DAI_FLAG_PACK flag in snd_dmaengine_dai_dma_data.flags
> 
> If this flag is set dmaengine_pcm doesn't put any restriction on the
> supported formats and sets the DMA transfer width to undefined.
> 
> This means control over the constraints is now transferred to the DAI
> driver and it's responsible to provide proper configuration and
> check for possible corner cases that aren't handled by the ALSA core.
> 
> Signed-off-by: Matthias Reichl 

Tested-by: Martin Sperl 

Re: [PATCH 2/2] ASoC: bcm2835: Add S16_LE support via packed DMA transfers

[Martin Sperl]

> On 27.04.2016, at 15:26, Matthias Reichl <h...@horus.com> wrote:
> 
> The bcm2835-i2s driver already has support for the S16_LE format but
> that format hasn't been made available because dmaengine_pcm didn't
> support packed data transfers.
> 
> bcm2835-i2s needs 16-bit left+right channel data to be packed into
> a 32-bit word, the FIFO register is 32-bit only and doesn't support
> 16-bit access.
> 
> Now that dmaengine_pcm supports packed transfers the format can
> be made available by setting the SND_DMAENGINE_PCM_DAI_FLAG_PACK flag.
> 
> No further configuration is necessary:
> - snd_dmaengine_dai_dma_data.addr_width is already set to
>  DMA_SLAVE_BUSWIDTH_4_BYTES to force 32-bit DMA transfers
> - dmaengine_pcm will pick up the S16_LE format from the DAI
>  configuration and make it available since it's no longer
>  masked out due to the PACK flag.
> - there are no further corner cases to catch in hw_params,
>  since the channel count is fixed at 2 we always have two
>  16-bit stereo samples that can be transferred via 32-bit DMA
> 
> Signed-off-by: Matthias Reichl <h...@horus.com>

Tested-by: Martin Sperl <ker...@martin.sperl.org>

Re: [PATCH 2/2] ASoC: bcm2835: Add S16_LE support via packed DMA transfers

[Martin Sperl]

> On 27.04.2016, at 15:26, Matthias Reichl  wrote:
> 
> The bcm2835-i2s driver already has support for the S16_LE format but
> that format hasn't been made available because dmaengine_pcm didn't
> support packed data transfers.
> 
> bcm2835-i2s needs 16-bit left+right channel data to be packed into
> a 32-bit word, the FIFO register is 32-bit only and doesn't support
> 16-bit access.
> 
> Now that dmaengine_pcm supports packed transfers the format can
> be made available by setting the SND_DMAENGINE_PCM_DAI_FLAG_PACK flag.
> 
> No further configuration is necessary:
> - snd_dmaengine_dai_dma_data.addr_width is already set to
>  DMA_SLAVE_BUSWIDTH_4_BYTES to force 32-bit DMA transfers
> - dmaengine_pcm will pick up the S16_LE format from the DAI
>  configuration and make it available since it's no longer
>  masked out due to the PACK flag.
> - there are no further corner cases to catch in hw_params,
>  since the channel count is fixed at 2 we always have two
>  16-bit stereo samples that can be transferred via 32-bit DMA
> 
> Signed-off-by: Matthias Reichl 

Tested-by: Martin Sperl 

Re: [PATCH 0/8 v4] bcm2835 DMA slave support

[Martin Sperl]

On 16.03.2016 20:24, Eric Anholt wrote:

Here's the series for DMA slave and memcpy support for 2835, with the
DT changes to enable the remaining channels dropped out while that
goes through review.  I had to do some minor conflict resolution, but
it was pretty mechanical, and I tested again with dmatest on the last
patch.
I guess I have got a list to implement that without a change to the 
device tree,

besides making use of platform_get_irq_byname and setting up the property:
interrupt-names = "dma0", "dma1", .. "dma10", "dma_shared", "dma_all";

As the property "interrupt-names" is a standard it should be acceptable.

Re: [PATCH 0/8 v4] bcm2835 DMA slave support

[Martin Sperl]

On 16.03.2016 20:24, Eric Anholt wrote:

Here's the series for DMA slave and memcpy support for 2835, with the
DT changes to enable the remaining channels dropped out while that
goes through review.  I had to do some minor conflict resolution, but
it was pretty mechanical, and I tested again with dmatest on the last
patch.
I guess I have got a list to implement that without a change to the 
device tree,

besides making use of platform_get_irq_byname and setting up the property:
interrupt-names = "dma0", "dma1", .. "dma10", "dma_shared", "dma_all";

As the property "interrupt-names" is a standard it should be acceptable.

Re: [RFT PATCH 2/2] Revert "usb: dwc2: Fix probe problem on bcm2835"

[Martin Sperl]

> On 19.03.2016, at 10:52, Stefan Wahren <stefan.wah...@i2se.com> wrote:
> 
> Hi,
> 
>> Martin Sperl <ker...@martin.sperl.org> hat am 19. März 2016 um 08:44
>> geschrieben:
>> 
>> 
>> 
>>> On 19.03.2016, at 03:17, Eric Anholt <e...@anholt.net> wrote:
>>> 
>>> Stefan Wahren <stefan.wah...@i2se.com> writes:
>>> 
>>>> Hi Eric,
>>>> hi Martin,
>>>> 
>>>>> John Youn <john.y...@synopsys.com> hat am 16. März 2016 um 19:28
>>>>> geschrieben:
>>>>> 
>>>>> 
>>>>> On 3/10/2016 11:14 AM, John Youn wrote:
>>>>>> On 3/9/2016 11:06 AM, Doug Anderson wrote:
>>>>>>> 
>>>>>>> John: it's pretty clear that there's something taking almost exactly
>>>>>>> 10ms on my system and almost exactly 50ms on Stefan's system. Is
>>>>>>> there some register we could poll to see when this process is done?
>>>>>>> ...or can we look at the dwc2 revision number / feature register and
>>>>>>> detect how long to delay?
>>>>>>> 
>> 
>> Maybe this difference is related to overclocking settings in the firmware?
>> 
>>>>> 
>>>>> 1. What is the AHB Clock frequency? Is the AHB Clock gated during
>>>>> Reset?
>>> 
>>> Low confidence here as I'm tracing lines across a ton of modules, but it
>>> looks like it comes from the USB AXI clock in peri_image, which is a
>>> gate on the normal 250Mhz APB clock, but nothing should be touching that
>>> gate register as part of USB reset as far as I know.
>>> 
>> Isn’t it possible that this clock (probably BCM2835_CLOCK_VPU) is
>> changed by the firmware due to overclocking settings in /boot/config.txt?
> 
> i don't use any overclocking settings. 
> 
> Are you able to reproduce the behavior on your Pi?

I did not have any problems with USB recently (using 4.5), 
so I would not have any idea how to reproduce it.

Note that I am using it with an AXIS USB-ethernet dongle plus USB-stick
all the time on my Compute Module connected via a tiny hub, 
so I wonder if that qualifies as being able to trigger the issue...

Martin

Re: [RFT PATCH 2/2] Revert "usb: dwc2: Fix probe problem on bcm2835"

[Martin Sperl]

> On 19.03.2016, at 10:52, Stefan Wahren  wrote:
> 
> Hi,
> 
>> Martin Sperl  hat am 19. März 2016 um 08:44
>> geschrieben:
>> 
>> 
>> 
>>> On 19.03.2016, at 03:17, Eric Anholt  wrote:
>>> 
>>> Stefan Wahren  writes:
>>> 
>>>> Hi Eric,
>>>> hi Martin,
>>>> 
>>>>> John Youn  hat am 16. März 2016 um 19:28
>>>>> geschrieben:
>>>>> 
>>>>> 
>>>>> On 3/10/2016 11:14 AM, John Youn wrote:
>>>>>> On 3/9/2016 11:06 AM, Doug Anderson wrote:
>>>>>>> 
>>>>>>> John: it's pretty clear that there's something taking almost exactly
>>>>>>> 10ms on my system and almost exactly 50ms on Stefan's system. Is
>>>>>>> there some register we could poll to see when this process is done?
>>>>>>> ...or can we look at the dwc2 revision number / feature register and
>>>>>>> detect how long to delay?
>>>>>>> 
>> 
>> Maybe this difference is related to overclocking settings in the firmware?
>> 
>>>>> 
>>>>> 1. What is the AHB Clock frequency? Is the AHB Clock gated during
>>>>> Reset?
>>> 
>>> Low confidence here as I'm tracing lines across a ton of modules, but it
>>> looks like it comes from the USB AXI clock in peri_image, which is a
>>> gate on the normal 250Mhz APB clock, but nothing should be touching that
>>> gate register as part of USB reset as far as I know.
>>> 
>> Isn’t it possible that this clock (probably BCM2835_CLOCK_VPU) is
>> changed by the firmware due to overclocking settings in /boot/config.txt?
> 
> i don't use any overclocking settings. 
> 
> Are you able to reproduce the behavior on your Pi?

I did not have any problems with USB recently (using 4.5), 
so I would not have any idea how to reproduce it.

Note that I am using it with an AXIS USB-ethernet dongle plus USB-stick
all the time on my Compute Module connected via a tiny hub, 
so I wonder if that qualifies as being able to trigger the issue...

Martin

Re: [RFT PATCH 2/2] Revert "usb: dwc2: Fix probe problem on bcm2835"

[Martin Sperl]

> On 19.03.2016, at 03:17, Eric Anholt  wrote:
> 
> Stefan Wahren  writes:
> 
>> Hi Eric,
>> hi Martin,
>> 
>>> John Youn  hat am 16. März 2016 um 19:28 
>>> geschrieben:
>>> 
>>> 
>>> On 3/10/2016 11:14 AM, John Youn wrote:
 On 3/9/2016 11:06 AM, Doug Anderson wrote:
> 
> John: it's pretty clear that there's something taking almost exactly
> 10ms on my system and almost exactly 50ms on Stefan's system. Is
> there some register we could poll to see when this process is done?
> ...or can we look at the dwc2 revision number / feature register and
> detect how long to delay?
> 

Maybe this difference is related to overclocking settings in the firmware?

>>> 
>>> 1. What is the AHB Clock frequency? Is the AHB Clock gated during
>>> Reset?
> 
> Low confidence here as I'm tracing lines across a ton of modules, but it
> looks like it comes from the USB AXI clock in peri_image, which is a
> gate on the normal 250Mhz APB clock, but nothing should be touching that
> gate register as part of USB reset as far as I know.
> 
Isn’t it possible that this clock (probably BCM2835_CLOCK_VPU) is 
changed by the firmware due to overclocking settings in /boot/config.txt?

Martin

Re: [RFT PATCH 2/2] Revert "usb: dwc2: Fix probe problem on bcm2835"

[Martin Sperl]

> On 19.03.2016, at 03:17, Eric Anholt  wrote:
> 
> Stefan Wahren  writes:
> 
>> Hi Eric,
>> hi Martin,
>> 
>>> John Youn  hat am 16. März 2016 um 19:28 
>>> geschrieben:
>>> 
>>> 
>>> On 3/10/2016 11:14 AM, John Youn wrote:
 On 3/9/2016 11:06 AM, Doug Anderson wrote:
> 
> John: it's pretty clear that there's something taking almost exactly
> 10ms on my system and almost exactly 50ms on Stefan's system. Is
> there some register we could poll to see when this process is done?
> ...or can we look at the dwc2 revision number / feature register and
> detect how long to delay?
> 

Maybe this difference is related to overclocking settings in the firmware?

>>> 
>>> 1. What is the AHB Clock frequency? Is the AHB Clock gated during
>>> Reset?
> 
> Low confidence here as I'm tracing lines across a ton of modules, but it
> looks like it comes from the USB AXI clock in peri_image, which is a
> gate on the normal 250Mhz APB clock, but nothing should be touching that
> gate register as part of USB reset as far as I know.
> 
Isn’t it possible that this clock (probably BCM2835_CLOCK_VPU) is 
changed by the firmware due to overclocking settings in /boot/config.txt?

Martin

Re: [PATCH 0/8 v4] bcm2835 DMA slave support

[Martin Sperl]

On 16.03.2016 20:24, Eric Anholt wrote:

Here's the series for DMA slave and memcpy support for 2835, with the
DT changes to enable the remaining channels dropped out while that
goes through review.  I had to do some minor conflict resolution, but
it was pretty mechanical, and I tested again with dmatest on the last
patch.

Martin Sperl (8):
   dmaengine: bcm2835: set residue_granularity field
   dmaengine: bcm2835: remove unnecessary masking of dma channels
   dmaengine: bcm2835: add additional defines for DMA-registers
   dmaengine: bcm2835: move cyclic member from bcm2835_chan into
 bcm2835_desc
   dmaengine: bcm2835: move controlblock chain generation into separate
 method
   dmaengine: bcm2835: limit max length based on channel type
   dmaengine: bcm2835: add slave_sg support to bcm2835-dma
   dmaengine: bcm2835: add dma_memcopy support to bcm2835-dma


I have successfully tested this modified patch-series
playing BigBuckBunny on:
* fb-tft device (fb_st7735r) - via spi-bcm2835 using slave_sg dma
* I2S Hifiberry DAC (snd_soc_hifiberry_dac) - via bcm2835-i2s using 
cyclic dma


Required additional patches to make this work
(especially I2S support, which is non-working since ):
* the clock-patchsets:
 * [PATCH 0/6] clk: bcm2835: fixes clk-bcm2835 driver issues
(most are reviewed by Eric)
 * [PATCH 0/3] reorganize clock initialization and add PCM clock
(no reviewed/acked so far)
* i2s patchset to enable the use of the clock framework
 * [PATCH V2 0/3] ASOC: bcm2835: move bcm2835-i2s to use clock framework
(if I remember correctly Mark Brown has merged the driver patches)
* out of tree drivers for Hifiberry DAC
   (I guess I should upstream those...)

Tested-by: Martin Sperl <ker...@martin.sperl.org>

Re: [PATCH 0/8 v4] bcm2835 DMA slave support

[Martin Sperl]

On 16.03.2016 20:24, Eric Anholt wrote:

Here's the series for DMA slave and memcpy support for 2835, with the
DT changes to enable the remaining channels dropped out while that
goes through review.  I had to do some minor conflict resolution, but
it was pretty mechanical, and I tested again with dmatest on the last
patch.

Martin Sperl (8):
   dmaengine: bcm2835: set residue_granularity field
   dmaengine: bcm2835: remove unnecessary masking of dma channels
   dmaengine: bcm2835: add additional defines for DMA-registers
   dmaengine: bcm2835: move cyclic member from bcm2835_chan into
 bcm2835_desc
   dmaengine: bcm2835: move controlblock chain generation into separate
 method
   dmaengine: bcm2835: limit max length based on channel type
   dmaengine: bcm2835: add slave_sg support to bcm2835-dma
   dmaengine: bcm2835: add dma_memcopy support to bcm2835-dma


I have successfully tested this modified patch-series
playing BigBuckBunny on:
* fb-tft device (fb_st7735r) - via spi-bcm2835 using slave_sg dma
* I2S Hifiberry DAC (snd_soc_hifiberry_dac) - via bcm2835-i2s using 
cyclic dma


Required additional patches to make this work
(especially I2S support, which is non-working since ):
* the clock-patchsets:
 * [PATCH 0/6] clk: bcm2835: fixes clk-bcm2835 driver issues
(most are reviewed by Eric)
 * [PATCH 0/3] reorganize clock initialization and add PCM clock
(no reviewed/acked so far)
* i2s patchset to enable the use of the clock framework
 * [PATCH V2 0/3] ASOC: bcm2835: move bcm2835-i2s to use clock framework
(if I remember correctly Mark Brown has merged the driver patches)
* out of tree drivers for Hifiberry DAC
   (I guess I should upstream those...)

Tested-by: Martin Sperl 

Re: [PATCH v7] clk: bcm2835: Add support for programming the audio domain clocks

[Martin Sperl]

I know it has already been merged, but while doing some 
clock investigations of the RPI3 I came accross this code:

> On 09.10.2015, at 03:37, Eric Anholt  wrote:
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index dd295e4..8502a4b 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> +static unsigned long bcm2835_pll_get_rate(struct clk_hw *hw,
> +   unsigned long parent_rate)
> +{
> + struct bcm2835_pll *pll = container_of(hw, struct bcm2835_pll, hw);
> + struct bcm2835_cprman *cprman = pll->cprman;
> + const struct bcm2835_pll_data *data = pll->data;
> + u32 a2wctrl = cprman_read(cprman, data->a2w_ctrl_reg);
> + u32 ndiv, pdiv, fdiv;
> + bool using_prediv;
> +
> + if (parent_rate == 0)
> + return 0;
> +
> + fdiv = cprman_read(cprman, data->frac_reg) & A2W_PLL_FRAC_MASK;
> + ndiv = (a2wctrl & A2W_PLL_CTRL_NDIV_MASK) >> A2W_PLL_CTRL_NDIV_SHIFT;
> + pdiv = (a2wctrl & A2W_PLL_CTRL_PDIV_MASK) >> A2W_PLL_CTRL_PDIV_SHIFT;
> + using_prediv = cprman_read(cprman, data->ana_reg_base + 4) &
> + data->ana->fb_prediv_mask;
> +
> + if (using_prediv)
> + ndiv *= 2;
Is this really correct?

As far as I interpret the name “fb_predic_mask" it should be either:
  pdiv * = 2; /* there is already a divider of 2 applied */
or
  ndiv *= 2; /* we need to multiply bot integer and fractional by 2 */
  fdiv *= 2;

As it is a pre-divider, I would assume it is the first,
but I could be wrong - not knowing a lot about PLLs.

Just want to raise this as a question.

Martin

Re: [PATCH v7] clk: bcm2835: Add support for programming the audio domain clocks

[Martin Sperl]

I know it has already been merged, but while doing some 
clock investigations of the RPI3 I came accross this code:

> On 09.10.2015, at 03:37, Eric Anholt  wrote:
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index dd295e4..8502a4b 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> +static unsigned long bcm2835_pll_get_rate(struct clk_hw *hw,
> +   unsigned long parent_rate)
> +{
> + struct bcm2835_pll *pll = container_of(hw, struct bcm2835_pll, hw);
> + struct bcm2835_cprman *cprman = pll->cprman;
> + const struct bcm2835_pll_data *data = pll->data;
> + u32 a2wctrl = cprman_read(cprman, data->a2w_ctrl_reg);
> + u32 ndiv, pdiv, fdiv;
> + bool using_prediv;
> +
> + if (parent_rate == 0)
> + return 0;
> +
> + fdiv = cprman_read(cprman, data->frac_reg) & A2W_PLL_FRAC_MASK;
> + ndiv = (a2wctrl & A2W_PLL_CTRL_NDIV_MASK) >> A2W_PLL_CTRL_NDIV_SHIFT;
> + pdiv = (a2wctrl & A2W_PLL_CTRL_PDIV_MASK) >> A2W_PLL_CTRL_PDIV_SHIFT;
> + using_prediv = cprman_read(cprman, data->ana_reg_base + 4) &
> + data->ana->fb_prediv_mask;
> +
> + if (using_prediv)
> + ndiv *= 2;
Is this really correct?

As far as I interpret the name “fb_predic_mask" it should be either:
  pdiv * = 2; /* there is already a divider of 2 applied */
or
  ndiv *= 2; /* we need to multiply bot integer and fractional by 2 */
  fdiv *= 2;

As it is a pre-divider, I would assume it is the first,
but I could be wrong - not knowing a lot about PLLs.

Just want to raise this as a question.

Martin

Re: [PATCH 1/5] ARM: bcm2835: Define standard pinctrl groups in the gpio node.

[Martin Sperl]

> On 03.03.2016, at 22:20, Stephen Warren  wrote:
> 
> On 02/26/2016 11:19 AM, Eric Anholt wrote:
>> The BCM2835-ARM-Peripherals.pdf documentation specifies what the
>> function selects do for the pins, and there are a bunch of obvious
>> groupings to be made.  With these created, we'll be able to replace
>> bcm2835-rpi.dtsi's main "set all of these pins to alt0" with
>> references to specific groups we want enabled.
> 
>> diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
> 
>> +spi0_gpio7: spi0_gpio7 {
>> +brcm,pins = <7 8 9 10 11>;
>> +brcm,function = ;
>> +};
> 
> This is too many pins.
> 
> - It includes both MOSI and MISO, although a particular use-case may only use 
> 1 of those.
> 
> - It includes both chip-select signals, whereas a particular use-case may use 
> 0, 1, or 2 of those. This is especially true since IIRC the mainline bcm283x 
> SPI driver wants to only use GPIOs for chip-selects, not 
> SPI-controller-generated chip-select signals, to avoid some issues with the 
> HW generation of these signals.
That is true: the spi-bcm2835 driver requires GPIO usage for chip-select
to make all those latency optimizations work (but also to avoid some
spi-dma issues).
The reason behind it is that there are observed short term “glitches”
on native CS whenever the SPI control register is touched - even with 
identical values.
And GPIO controlled CS solves this issue (and Mark Brown said that
the GPIO-cs interface is now preferred anyway - hence the auxiliary
spi only implement gpio-cs and requires the CS set as OUTPUT, but
unlike the main spi this does not have “remapping” support for
legacy device-trees (as there never was a driver-version that supported
native-cs).

Maybe split the SPI-portion into 2 sections:
* the SCK, MOSI, MISO (pin 9 to 11) with ALT_0
* the CS GPIOs (standard pins are 7 and 8) with OUTPUT.

That way it is easy to override only this section (plus the gpio-cs property 
inside the spi node) to extend the number of chip selects or use different 
mappings.

> 
> I believe a similar comment applies to other SPI nodes too.
I guess the same “splitting” approach should be taken here as well...

Re: [PATCH 1/5] ARM: bcm2835: Define standard pinctrl groups in the gpio node.

[Martin Sperl]

> On 03.03.2016, at 22:20, Stephen Warren  wrote:
> 
> On 02/26/2016 11:19 AM, Eric Anholt wrote:
>> The BCM2835-ARM-Peripherals.pdf documentation specifies what the
>> function selects do for the pins, and there are a bunch of obvious
>> groupings to be made.  With these created, we'll be able to replace
>> bcm2835-rpi.dtsi's main "set all of these pins to alt0" with
>> references to specific groups we want enabled.
> 
>> diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
> 
>> +spi0_gpio7: spi0_gpio7 {
>> +brcm,pins = <7 8 9 10 11>;
>> +brcm,function = ;
>> +};
> 
> This is too many pins.
> 
> - It includes both MOSI and MISO, although a particular use-case may only use 
> 1 of those.
> 
> - It includes both chip-select signals, whereas a particular use-case may use 
> 0, 1, or 2 of those. This is especially true since IIRC the mainline bcm283x 
> SPI driver wants to only use GPIOs for chip-selects, not 
> SPI-controller-generated chip-select signals, to avoid some issues with the 
> HW generation of these signals.
That is true: the spi-bcm2835 driver requires GPIO usage for chip-select
to make all those latency optimizations work (but also to avoid some
spi-dma issues).
The reason behind it is that there are observed short term “glitches”
on native CS whenever the SPI control register is touched - even with 
identical values.
And GPIO controlled CS solves this issue (and Mark Brown said that
the GPIO-cs interface is now preferred anyway - hence the auxiliary
spi only implement gpio-cs and requires the CS set as OUTPUT, but
unlike the main spi this does not have “remapping” support for
legacy device-trees (as there never was a driver-version that supported
native-cs).

Maybe split the SPI-portion into 2 sections:
* the SCK, MOSI, MISO (pin 9 to 11) with ALT_0
* the CS GPIOs (standard pins are 7 and 8) with OUTPUT.

That way it is easy to override only this section (plus the gpio-cs property 
inside the spi node) to extend the number of chip selects or use different 
mappings.

> 
> I believe a similar comment applies to other SPI nodes too.
I guess the same “splitting” approach should be taken here as well...

Re: [PATCH 0/4] bcm2835 SDHOST controller

[Martin Sperl]

> On 27.02.2016, at 00:05, Eric Anholt  wrote:
> 
> Here's a series to enable the SDHOST controller.  It gives us better
> performance than our old sdhci-bcm2835.c.  The downstream Raspberry Pi
> kernel appears to be using this controller by default at this point.
> 
> I've tried to do some testing on it (mounting filesystem,
> reading/writing files, speed tests), but I'm not sure what a good
> testing regimen for storage drivers would be.
> 
> Eric Anholt (4):
>  dt-bindings: Add binding for brcm,bcm2835-sdhost.
>  mmc: bcm2835-sdhost: Add new driver for the internal SD controller.
>  ARM: bcm2835: Include SDHOST in the device tree.
>  ARM: bcm2835: Enable SDHOST by default.

This patchset misses defconfig changes to enable
the new sdhost driver.

So we end up with a default kernel
without the sdhost driver compiled in
but with sdhost enabled in the default device-tree (patch4)
resulting in a system that does not boot.

Thanks,
Martin

Re: [PATCH 0/4] bcm2835 SDHOST controller

[Martin Sperl]

> On 27.02.2016, at 00:05, Eric Anholt  wrote:
> 
> Here's a series to enable the SDHOST controller.  It gives us better
> performance than our old sdhci-bcm2835.c.  The downstream Raspberry Pi
> kernel appears to be using this controller by default at this point.
> 
> I've tried to do some testing on it (mounting filesystem,
> reading/writing files, speed tests), but I'm not sure what a good
> testing regimen for storage drivers would be.
> 
> Eric Anholt (4):
>  dt-bindings: Add binding for brcm,bcm2835-sdhost.
>  mmc: bcm2835-sdhost: Add new driver for the internal SD controller.
>  ARM: bcm2835: Include SDHOST in the device tree.
>  ARM: bcm2835: Enable SDHOST by default.

This patchset misses defconfig changes to enable
the new sdhost driver.

So we end up with a default kernel
without the sdhost driver compiled in
but with sdhost enabled in the default device-tree (patch4)
resulting in a system that does not boot.

Thanks,
Martin

Re: [GIT PULL] bcm2835 DT changes for 4.6

[Martin Sperl]

> On 09.02.2016, at 01:32, Eric Anholt  wrote:
> 
> Hi Florian.  Here's the first set of patches for bcm2835 for 4.6.
> We've got more DT patches that are going to happen for new boards,
> too, but they're still getting polished.
> 
> The following changes since commit 92e963f50fc74041b5e9e744c330dca48e04f08d:
> 
>  Linux 4.5-rc1 (2016-01-24 13:06:47 -0800)
> 
> are available in the git repository at:
> 
>  g...@github.com:anholt/linux.git tags/bcm2835-dt-next-2016-02-04
> 
> for you to fetch changes up to 5ec6f2cd8ec4bcd38ba199ea8711a5ec906d85e7:
> 
>  ARM: bcm2835: Add the Raspberry Pi power domain driver to the DT. 
> (2016-02-02 20:02:45 -0800)
> 
> 
> This pull request covers mostly DT changes that didn't make it into
> 4.5 because required header files went through other trees.
> 
> 
> Alexander Aring (1):
>  ARM: bcm2835: Add the Raspberry Pi power domain driver to the DT.
> 
> Lubomir Rintel (1):
>  ARM: bcm2835: dt: Add Raspberry Pi Model A
> 
> Martin Sperl (2):
>  ARM: bcm2835: add the auxiliary spi1 and spi2 to the device tree
>  ARM: bcm2835: follow dt uart node-naming convention

Do you want me to resend a rebased version of:
 ARM: bcm2835: add bcm2835-aux-uart support to default DT

The corresponding driver has been added to tty/tty-next.

Thanks,
Martin

Re: [GIT PULL] bcm2835 DT changes for 4.6

[Martin Sperl]

> On 09.02.2016, at 01:32, Eric Anholt <e...@anholt.net> wrote:
> 
> Hi Florian.  Here's the first set of patches for bcm2835 for 4.6.
> We've got more DT patches that are going to happen for new boards,
> too, but they're still getting polished.
> 
> The following changes since commit 92e963f50fc74041b5e9e744c330dca48e04f08d:
> 
>  Linux 4.5-rc1 (2016-01-24 13:06:47 -0800)
> 
> are available in the git repository at:
> 
>  g...@github.com:anholt/linux.git tags/bcm2835-dt-next-2016-02-04
> 
> for you to fetch changes up to 5ec6f2cd8ec4bcd38ba199ea8711a5ec906d85e7:
> 
>  ARM: bcm2835: Add the Raspberry Pi power domain driver to the DT. 
> (2016-02-02 20:02:45 -0800)
> 
> 
> This pull request covers mostly DT changes that didn't make it into
> 4.5 because required header files went through other trees.
> 
> 
> Alexander Aring (1):
>  ARM: bcm2835: Add the Raspberry Pi power domain driver to the DT.
> 
> Lubomir Rintel (1):
>  ARM: bcm2835: dt: Add Raspberry Pi Model A
> 
> Martin Sperl (2):
>  ARM: bcm2835: add the auxiliary spi1 and spi2 to the device tree
>  ARM: bcm2835: follow dt uart node-naming convention

Do you want me to resend a rebased version of:
 ARM: bcm2835: add bcm2835-aux-uart support to default DT

The corresponding driver has been added to tty/tty-next.

Thanks,
Martin

Re: [RESEND][PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

> On 18.12.2015, at 07:05, Vinod Koul  wrote:
> 
> On Thu, Dec 17, 2015 at 07:11:48PM +0100, Martin Sperl wrote:
> 
>> +
>> +/* Setup addresses */
>> +if (d->dir == DMA_DEV_TO_MEM) {
>> +control_block->info = BCM2835_DMA_D_INC |
>> +  BCM2835_DMA_D_WIDTH |
>> +  BCM2835_DMA_S_DREQ;
>> +control_block->src = dev_addr;
>> +control_block->dst = addr + (dma_addr_t)j;
>> +} else {
>> +control_block->info = BCM2835_DMA_S_INC |
>> +  BCM2835_DMA_S_WIDTH |
>> +  BCM2835_DMA_D_DREQ;
>> +control_block->src = addr + (dma_addr_t)j;
>> +control_block->dst = dev_addr;
>> +}
>> +
>> +/* Common part */
>> +control_block->info |=
>> +BCM2835_DMA_WAITS(BCM2835_DMA_WAIT_CYCLES);
>> +control_block->info |= BCM2835_DMA_WAIT_RESP;
>> +
>> +/* Enable */
>> +if (i == sg_len - 1 && len - j <= max_size)
>> +control_block->info |= BCM2835_DMA_INT_EN;
>> +
>> +/* Setup synchronization */
>> +if (sync_type)
>> +control_block->info |= sync_type;
>> +
>> +/* Setup DREQ channel */
>> +if (c->dreq)
>> +control_block->info |=
>> +BCM2835_DMA_PER_MAP(c->dreq);
>> +
>> +/* Length of a frame */
>> +control_block->length = min(len - j, max_size);
>> +d->size += control_block->length;
>> +
>> +if (i < sg_len - 1 || len - j > max_size) {
>> +/* Next block is the next frame. */
>> +control_block->next =
>> +d->control_block_base_phys +
>> +sizeof(struct bcm2835_dma_cb) *
>> +(i + split_cnt + 1);
>> +} else {
>> +/* Next block is empty. */
>> +control_block->next = 0;
>> +}
>> +
>> +if (len - j > max_size)
>> +split_cnt++;
> 
> Most of this part is common with the cyclic and seems copy paste. Can we
> use common routine to do common work please

I agree - can have a look - the big question is: how would I document the 
individual changes by Gellert Weisz and myself correctly?
* just a single patch with an attribution tag (which?) or just a commit message?
* Patchset with 2 patches: the original by Gellert Weisz (this)
  plus the modification to consolidate the code above?

Thanks,
Martin--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RESEND][PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

Add slave transfer capability to BCM2835 dmaengine driver.

This patch is pulled from the bcm2708-dmaengine driver in the
Raspberry Pi repo. The work was done originally by Gellert Weisz.

Tested using the spi-bcm2835 driver that has dma support since:
commit 3ecd37edaa2a6ba3 
("spi: bcm2835: enable dma modes for transfers meeting certain conditions”)

Signed-off-by: Noralf Trønnes 
Tested-by: Martin Sperl 
—

Patch was originally submitted by: Noralf Trønnes 

Gellert Weisz has ended his internship with Raspberry Pi Trading and
was not available to sign off this patch.

The patch is made against mainline 4.0-rc7.

Changes from v1:
    Martin Sperl, Dom Cobley:
MAX_LITE_TRANSFER has to be 32-bit aligned

Stefan Wahren:
Variable es is not used
Change splitct to split_cnt
Add dev_err for unsupported buswidth
Rearrange d->frames formula for readability
Move j variable definition to loop

Changes from original code:
Remove slave_id use.
SDHCI_BCM_DMA_WAITS changed to BCM2835_DMA_WAIT_CYCLES.
Use SZ_* macros instead of decimal values.
Change MAX_LITE_TRANSFER from 32k to 64K - 1.
Fix several whitespace issues.

Lee Jones' comments in previous email to Piotr Król:
Remove __func__ from dev_err message.
Cleanup comments.

Tested extensively by Martin Sperl with the following spi devices
all of which trigger the use of DMA within spi-bcm2835:
* spi_loopback_test - in spi/for-next
* enc28j60 - ethernet via SPI
* fb_st7735r - framebuffer playing BigBuckBunny
* mmc-spi with an out of tree patch to work arround the mmc_spi
  internal dma mapping issues, that inhibits the driver from working
  correctly - this got introduced with commit 0589342c27944e50
("of: set dma_mask to point to coherent_dma_mask")

drivers/dma/bcm2835-dma.c | 206 ++
1 file changed, 192 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index c92d6a7..0d93a90 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -1,11 +1,10 @@
/*
 * BCM2835 DMA engine support
 *
- * This driver only supports cyclic DMA transfers
- * as needed for the I2S module.
- *
 * Author:  Florian Meier 
 *  Copyright 2013
+ *  Gellert Weisz 
+ *  Copyright 2013-2014
 *
 * Based on
 *  OMAP DMAengine support by Russell King
@@ -89,6 +88,8 @@ struct bcm2835_desc {
size_t size;
};

+#define BCM2835_DMA_WAIT_CYCLES0  /* Slow down DMA transfers: 0-31 */
+
#define BCM2835_DMA_CS  0x00
#define BCM2835_DMA_ADDR0x04
#define BCM2835_DMA_SOURCE_AD   0x0c
@@ -105,12 +106,16 @@ struct bcm2835_desc {
#define BCM2835_DMA_RESET   BIT(31) /* WO, self clearing */

#define BCM2835_DMA_INT_EN  BIT(0)
+#define BCM2835_DMA_WAIT_RESP  BIT(3)
#define BCM2835_DMA_D_INC   BIT(4)
+#define BCM2835_DMA_D_WIDTHBIT(5)
#define BCM2835_DMA_D_DREQ  BIT(6)
#define BCM2835_DMA_S_INC   BIT(8)
+#define BCM2835_DMA_S_WIDTHBIT(9)
#define BCM2835_DMA_S_DREQ  BIT(10)

#define BCM2835_DMA_PER_MAP(x)  ((x) << 16)
+#define BCM2835_DMA_WAITS(x)   (((x) & 0x1f) << 21)

#define BCM2835_DMA_DATA_TYPE_S81
#define BCM2835_DMA_DATA_TYPE_S16   2
@@ -124,6 +129,14 @@ struct bcm2835_desc {
#define BCM2835_DMA_CHAN(n) ((n) << 8) /* Base address */
#define BCM2835_DMA_CHANIO(base, n) ((base) + BCM2835_DMA_CHAN(n))

+#define MAX_NORMAL_TRANSFERSZ_1G
+/*
+ * Max length on a Lite channel is 65535 bytes.
+ * DMA handles byte-enables on SDRAM reads and writes even on 128-bit accesses,
+ * but byte-enables don't exist on peripheral addresses, so align to 32-bit.
+ */
+#define MAX_LITE_TRANSFER  (SZ_64K - 4)
+
static inline struct bcm2835_dmadev *to_bcm2835_dma_dev(struct dma_device *d)
{
return container_of(d, struct bcm2835_dmadev, ddev);
@@ -217,12 +230,18 @@ static irqreturn_t bcm2835_dma_callback(int irq, void 
*data)
d = c->desc;

if (d) {
-   /* TODO Only works for cyclic DMA */
-   vchan_cyclic_callback(>vd);
-   }
+   if (c->cyclic) {
+   vchan_cyclic_callback(>vd);

-   /* Keep the DMA engine running */
-   writel(BCM2835_DMA_ACTIVE, c->chan_base + BCM2835_DMA_CS);
+   /* Keep the DMA engine running */
+   writel(BCM2835_DMA_ACTIVE,
+  c->chan_base + BCM2835_DMA_CS);
+
+   } else {
+   vchan_cookie_complete(>desc->vd);
+   bcm2835_dma_start_desc(c);
+   }
+   }

spin_unlock_irqrestore(>vc.lock, flags);

@@ -323,8 +342,6 @@ static void bcm2835_dma_issue_pending(struct dma_chan *chan)
struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
unsigned lo

[RESEND][PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

Add slave transfer capability to BCM2835 dmaengine driver.

This patch is pulled from the bcm2708-dmaengine driver in the
Raspberry Pi repo. The work was done originally by Gellert Weisz.

Tested using the spi-bcm2835 driver that has dma support since:
commit 3ecd37edaa2a6ba3 
("spi: bcm2835: enable dma modes for transfers meeting certain conditions”)

Signed-off-by: Noralf Trønnes <nor...@tronnes.org>
Tested-by: Martin Sperl <ker...@martin.sperl.org>
—

Patch was originally submitted by: Noralf Trønnes <nor...@tronnes.org>

Gellert Weisz has ended his internship with Raspberry Pi Trading and
was not available to sign off this patch.

The patch is made against mainline 4.0-rc7.

Changes from v1:
Martin Sperl, Dom Cobley:
MAX_LITE_TRANSFER has to be 32-bit aligned

Stefan Wahren:
Variable es is not used
Change splitct to split_cnt
Add dev_err for unsupported buswidth
Rearrange d->frames formula for readability
Move j variable definition to loop

Changes from original code:
Remove slave_id use.
SDHCI_BCM_DMA_WAITS changed to BCM2835_DMA_WAIT_CYCLES.
Use SZ_* macros instead of decimal values.
Change MAX_LITE_TRANSFER from 32k to 64K - 1.
Fix several whitespace issues.

Lee Jones' comments in previous email to Piotr Król:
Remove __func__ from dev_err message.
Cleanup comments.

Tested extensively by Martin Sperl with the following spi devices
all of which trigger the use of DMA within spi-bcm2835:
* spi_loopback_test - in spi/for-next
* enc28j60 - ethernet via SPI
* fb_st7735r - framebuffer playing BigBuckBunny
* mmc-spi with an out of tree patch to work arround the mmc_spi
  internal dma mapping issues, that inhibits the driver from working
  correctly - this got introduced with commit 0589342c27944e50
("of: set dma_mask to point to coherent_dma_mask")

drivers/dma/bcm2835-dma.c | 206 ++
1 file changed, 192 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index c92d6a7..0d93a90 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -1,11 +1,10 @@
/*
 * BCM2835 DMA engine support
 *
- * This driver only supports cyclic DMA transfers
- * as needed for the I2S module.
- *
 * Author:  Florian Meier <florian.me...@koalo.de>
 *  Copyright 2013
+ *  Gellert Weisz <gell...@raspberrypi.org>
+ *  Copyright 2013-2014
 *
 * Based on
 *  OMAP DMAengine support by Russell King
@@ -89,6 +88,8 @@ struct bcm2835_desc {
size_t size;
};

+#define BCM2835_DMA_WAIT_CYCLES0  /* Slow down DMA transfers: 0-31 */
+
#define BCM2835_DMA_CS  0x00
#define BCM2835_DMA_ADDR0x04
#define BCM2835_DMA_SOURCE_AD   0x0c
@@ -105,12 +106,16 @@ struct bcm2835_desc {
#define BCM2835_DMA_RESET   BIT(31) /* WO, self clearing */

#define BCM2835_DMA_INT_EN  BIT(0)
+#define BCM2835_DMA_WAIT_RESP  BIT(3)
#define BCM2835_DMA_D_INC   BIT(4)
+#define BCM2835_DMA_D_WIDTHBIT(5)
#define BCM2835_DMA_D_DREQ  BIT(6)
#define BCM2835_DMA_S_INC   BIT(8)
+#define BCM2835_DMA_S_WIDTHBIT(9)
#define BCM2835_DMA_S_DREQ  BIT(10)

#define BCM2835_DMA_PER_MAP(x)  ((x) << 16)
+#define BCM2835_DMA_WAITS(x)   (((x) & 0x1f) << 21)

#define BCM2835_DMA_DATA_TYPE_S81
#define BCM2835_DMA_DATA_TYPE_S16   2
@@ -124,6 +129,14 @@ struct bcm2835_desc {
#define BCM2835_DMA_CHAN(n) ((n) << 8) /* Base address */
#define BCM2835_DMA_CHANIO(base, n) ((base) + BCM2835_DMA_CHAN(n))

+#define MAX_NORMAL_TRANSFERSZ_1G
+/*
+ * Max length on a Lite channel is 65535 bytes.
+ * DMA handles byte-enables on SDRAM reads and writes even on 128-bit accesses,
+ * but byte-enables don't exist on peripheral addresses, so align to 32-bit.
+ */
+#define MAX_LITE_TRANSFER  (SZ_64K - 4)
+
static inline struct bcm2835_dmadev *to_bcm2835_dma_dev(struct dma_device *d)
{
return container_of(d, struct bcm2835_dmadev, ddev);
@@ -217,12 +230,18 @@ static irqreturn_t bcm2835_dma_callback(int irq, void 
*data)
d = c->desc;

if (d) {
-   /* TODO Only works for cyclic DMA */
-   vchan_cyclic_callback(>vd);
-   }
+   if (c->cyclic) {
+   vchan_cyclic_callback(>vd);

-   /* Keep the DMA engine running */
-   writel(BCM2835_DMA_ACTIVE, c->chan_base + BCM2835_DMA_CS);
+   /* Keep the DMA engine running */
+   writel(BCM2835_DMA_ACTIVE,
+  c->chan_base + BCM2835_DMA_CS);
+
+   } else {
+   vchan_cookie_complete(>desc->vd);
+   bcm2835_dma_start_desc(c);
+   }
+   }

spin_unlock_irqrestore(>vc.lock, flags);

@@ -323,8 +342

Re: [RESEND][PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

> On 18.12.2015, at 07:05, Vinod Koul <vinod.k...@intel.com> wrote:
> 
> On Thu, Dec 17, 2015 at 07:11:48PM +0100, Martin Sperl wrote:
> 
>> +
>> +/* Setup addresses */
>> +if (d->dir == DMA_DEV_TO_MEM) {
>> +control_block->info = BCM2835_DMA_D_INC |
>> +  BCM2835_DMA_D_WIDTH |
>> +  BCM2835_DMA_S_DREQ;
>> +control_block->src = dev_addr;
>> +control_block->dst = addr + (dma_addr_t)j;
>> +} else {
>> +control_block->info = BCM2835_DMA_S_INC |
>> +  BCM2835_DMA_S_WIDTH |
>> +  BCM2835_DMA_D_DREQ;
>> +control_block->src = addr + (dma_addr_t)j;
>> +control_block->dst = dev_addr;
>> +}
>> +
>> +/* Common part */
>> +control_block->info |=
>> +BCM2835_DMA_WAITS(BCM2835_DMA_WAIT_CYCLES);
>> +control_block->info |= BCM2835_DMA_WAIT_RESP;
>> +
>> +/* Enable */
>> +if (i == sg_len - 1 && len - j <= max_size)
>> +control_block->info |= BCM2835_DMA_INT_EN;
>> +
>> +/* Setup synchronization */
>> +if (sync_type)
>> +control_block->info |= sync_type;
>> +
>> +/* Setup DREQ channel */
>> +if (c->dreq)
>> +control_block->info |=
>> +BCM2835_DMA_PER_MAP(c->dreq);
>> +
>> +/* Length of a frame */
>> +control_block->length = min(len - j, max_size);
>> +d->size += control_block->length;
>> +
>> +if (i < sg_len - 1 || len - j > max_size) {
>> +/* Next block is the next frame. */
>> +control_block->next =
>> +d->control_block_base_phys +
>> +sizeof(struct bcm2835_dma_cb) *
>> +(i + split_cnt + 1);
>> +} else {
>> +/* Next block is empty. */
>> +control_block->next = 0;
>> +}
>> +
>> +if (len - j > max_size)
>> +split_cnt++;
> 
> Most of this part is common with the cyclic and seems copy paste. Can we
> use common routine to do common work please

I agree - can have a look - the big question is: how would I document the 
individual changes by Gellert Weisz and myself correctly?
* just a single patch with an attribution tag (which?) or just a commit message?
* Patchset with 2 patches: the original by Gellert Weisz (this)
  plus the modification to consolidate the code above?

Thanks,
Martin--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Martin Sperl]

> On 02.12.2015, at 00:12, Mark Brown  wrote:
> 
> On Tue, Dec 01, 2015 at 04:51:06PM -, Michal Suchanek wrote:
> 
>> +static inline size_t
>> +spi_max_transfer_size(struct spi_device *spi)
>> +{
>> +struct spi_master *master = spi->master;
>> +if (!master->max_transfer_size)
>> +return 0;
>> +return master->max_transfer_size(spi);
>> +}
> 
> Can we change this to return SIZE_MAX instead (ie, the maximum value for
> a size_t)?  That way callers don't need to worry if there is a limit, if
> they want to handle it they can just unconditionally assume that a limit
> will be provided.

As I just came across: spi_master.max_dma_len, so I wonder how this 
value would differ from the proposed spi_master.max_transfer_size
on specific HW?

For all practical purposes I would assume both are identical.

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 08/10] spi: expose master transfer size limitation.

[Martin Sperl]

> On 02.12.2015, at 00:12, Mark Brown  wrote:
> 
> On Tue, Dec 01, 2015 at 04:51:06PM -, Michal Suchanek wrote:
> 
>> +static inline size_t
>> +spi_max_transfer_size(struct spi_device *spi)
>> +{
>> +struct spi_master *master = spi->master;
>> +if (!master->max_transfer_size)
>> +return 0;
>> +return master->max_transfer_size(spi);
>> +}
> 
> Can we change this to return SIZE_MAX instead (ie, the maximum value for
> a size_t)?  That way callers don't need to worry if there is a limit, if
> they want to handle it they can just unconditionally assume that a limit
> will be provided.

As I just came across: spi_master.max_dma_len, so I wonder how this 
value would differ from the proposed spi_master.max_transfer_size
on specific HW?

For all practical purposes I would assume both are identical.

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/5] dt/bindings: bcm2835: add binding documentation for bcm2835-aux

[Martin Sperl]

> On 16.09.2015, at 06:12, Stephen Warren  wrote:
> 
> On 09/04/2015 01:26 AM, Martin Sperl wrote:
>> 
>>> On 26.08.2015, at 03:44, Stephen Warren  wrote:
>>> 
>>> On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote:
>>> 
>>>> +Example:
>>>> +
>>>> +aux_enable: aux_enable@0x7e215004 {
>>>> +  compatible = "bcrm,bcm2835-aux";
>>>> +  reg = <0x7e215004 0x04>;
>>> 
>>> I'd expect that to be <0x7e215000 0x8>;
>> 
>> The reason is that we just handle enable with this driver,
>> which just requires access to the 0x7e215004 register.
>> 
>> The 0x7e215000 register (interrupt mask) could be used by a
>> cascaded interrupt-controller, but as the spi and uart drivers
>> can run with shared interrupts this is not a necessity.
> 
> The DT is supposed to describe the HW, not any particular SW's use of
> the HW. If the HW block has 2 registers, so must the DT reg property.

Please look at V6 of the patch-series, that uses Erics clock-patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/5] dt/bindings: bcm2835: add binding documentation for bcm2835-aux

[Martin Sperl]

> On 16.09.2015, at 06:12, Stephen Warren <swar...@wwwdotorg.org> wrote:
> 
> On 09/04/2015 01:26 AM, Martin Sperl wrote:
>> 
>>> On 26.08.2015, at 03:44, Stephen Warren <swar...@wwwdotorg.org> wrote:
>>> 
>>> On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote:
>>> 
>>>> +Example:
>>>> +
>>>> +aux_enable: aux_enable@0x7e215004 {
>>>> +  compatible = "bcrm,bcm2835-aux";
>>>> +  reg = <0x7e215004 0x04>;
>>> 
>>> I'd expect that to be <0x7e215000 0x8>;
>> 
>> The reason is that we just handle enable with this driver,
>> which just requires access to the 0x7e215004 register.
>> 
>> The 0x7e215000 register (interrupt mask) could be used by a
>> cascaded interrupt-controller, but as the spi and uart drivers
>> can run with shared interrupts this is not a necessity.
> 
> The DT is supposed to describe the HW, not any particular SW's use of
> the HW. If the HW block has 2 registers, so must the DT reg property.

Please look at V6 of the patch-series, that uses Erics clock-patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] clk: bcm2835: Add a driver for the auxiliary peripheral clock gates.

[Martin Sperl]

> On 11.09.2015, at 00:22, Eric Anholt  wrote:
> 
> There are a pair of SPI masters and a mini UART that were last minute
> additions.  As a result, they didn't get integrated in the same way as
> the other gates off of the VPU clock in CPRMAN.
> 
> Signed-off-by: Eric Anholt 
> ---

Tested with the spi-bcm2835aux driver and it works perfectly.
I will now post the latest incarnation of the driver for inclusion.

I have also tested with the uart1 and it works in principle.

Note that there is one issue (as mentioned elsewhere) with uart1:
the 8250 serial driver has a (hardcoded) internal clock divider 16,
while the hardware has an internal divider of 8.

This results in baud rates that are a factor of 2 higher than requested.

Tested-by: Martin Sperl 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.

[Martin Sperl]

> On 10.09.2015, at 23:58, Eric Anholt  wrote:
> 
> This adds support for enabling, disabling, and setting the rate of the
> audio domain clocks.  It will be necessary for setting the pixel clock
> for HDMI in the VC4 driver and let us write a cpufreq driver.  It will
> also improve compatibility with user changes to the firmware's
> config.txt, since our previous fixed clocks are unaware of it.
> 
> The firmware also has support for configuring the clocks through the
> mailbox channel, but the pixel clock setup by the firmware doesn't
> work, and it's Raspberry Pi specific anyway.  The only conflicts we
> should have with the firmware would be if we made firmware calls that
> result in clock management (like opening firmware V3D or ISP access,
> which we don't support in upstream), or on hardware over-thermal or
> under-voltage (when the firmware would rewrite PLLB to take the ARM
> out of overclock).  If that happens, our cached .recalc_rate() results
> would be incorrect, but that's no worse than our current state where
> we used fixed clocks.
> 
> The existing fixed clocks in the code are left in place to provide
> backwards compatibility with old device tree files.
> 
> Signed-off-by: Eric Anholt 

This patch-sets sets the emmc clock to correct frequency - no longer
overclocking SDCards by a factor of 2.5.
This means booting from the eMMC on the Compute module works now
without issues.

Note that the DT bcm2835-rpi-b-plus.dtb can get used to boot
the compute module.

Thanks

Tested-by: Martin Sperl 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] clk: bcm2835: Add support for programming the audio domain clocks.

[Martin Sperl]

> On 10.09.2015, at 23:58, Eric Anholt <e...@anholt.net> wrote:
> 
> This adds support for enabling, disabling, and setting the rate of the
> audio domain clocks.  It will be necessary for setting the pixel clock
> for HDMI in the VC4 driver and let us write a cpufreq driver.  It will
> also improve compatibility with user changes to the firmware's
> config.txt, since our previous fixed clocks are unaware of it.
> 
> The firmware also has support for configuring the clocks through the
> mailbox channel, but the pixel clock setup by the firmware doesn't
> work, and it's Raspberry Pi specific anyway.  The only conflicts we
> should have with the firmware would be if we made firmware calls that
> result in clock management (like opening firmware V3D or ISP access,
> which we don't support in upstream), or on hardware over-thermal or
> under-voltage (when the firmware would rewrite PLLB to take the ARM
> out of overclock).  If that happens, our cached .recalc_rate() results
> would be incorrect, but that's no worse than our current state where
> we used fixed clocks.
> 
> The existing fixed clocks in the code are left in place to provide
> backwards compatibility with old device tree files.
> 
> Signed-off-by: Eric Anholt <e...@anholt.net>

This patch-sets sets the emmc clock to correct frequency - no longer
overclocking SDCards by a factor of 2.5.
This means booting from the eMMC on the Compute module works now
without issues.

Note that the DT bcm2835-rpi-b-plus.dtb can get used to boot
the compute module.

Thanks

Tested-by: Martin Sperl <ker...@martin.sperl.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] clk: bcm2835: Add a driver for the auxiliary peripheral clock gates.

[Martin Sperl]

> On 11.09.2015, at 00:22, Eric Anholt <e...@anholt.net> wrote:
> 
> There are a pair of SPI masters and a mini UART that were last minute
> additions.  As a result, they didn't get integrated in the same way as
> the other gates off of the VPU clock in CPRMAN.
> 
> Signed-off-by: Eric Anholt <e...@anholt.net>
> ---

Tested with the spi-bcm2835aux driver and it works perfectly.
I will now post the latest incarnation of the driver for inclusion.

I have also tested with the uart1 and it works in principle.

Note that there is one issue (as mentioned elsewhere) with uart1:
the 8250 serial driver has a (hardcoded) internal clock divider 16,
while the hardware has an internal divider of 8.

This results in baud rates that are a factor of 2 higher than requested.

Tested-by: Martin Sperl <ker...@martin.sperl.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 0/6] bcm2835: auxiliar device support for spi

[Martin Sperl]

> On 10.09.2015, at 17:48, Noralf Trønnes  wrote:
> 
> This looks interesting.
> But there's a challenge with the uart1 and the 8250 driver.
> 
> Phil Elwell has this to say:
> This means that that UART1 isn't an exact clone of a 8250 UART.
> In a particular, the clock divisor is calculated differently.
> A standard 8250 derives the baud rate as clock/(divisor16),
> whereas the BCM2835 mini UART uses clock/(divisor8). This means
> that if you want to use the standard driver then you need to lie
> about the clock frequency, providing a value is twice the real
> value, in order for a suitable divisor to be calculated.
> 
> Ref: https://github.com/raspberrypi/linux/pull/1008#issuecomment-139234607
> 
> So either we need a new uart1 driver or a doubled clock freq. somehow.

Found out the same thing and communicated it to Eric - not 
knowing about the different divider…

Martin--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 0/6] bcm2835: auxiliar device support for spi

[Martin Sperl]

> On 10.09.2015, at 17:48, Noralf Trønnes  wrote:
> 
> This looks interesting.
> But there's a challenge with the uart1 and the 8250 driver.
> 
> Phil Elwell has this to say:
> This means that that UART1 isn't an exact clone of a 8250 UART.
> In a particular, the clock divisor is calculated differently.
> A standard 8250 derives the baud rate as clock/(divisor16),
> whereas the BCM2835 mini UART uses clock/(divisor8). This means
> that if you want to use the standard driver then you need to lie
> about the clock frequency, providing a value is twice the real
> value, in order for a suitable divisor to be calculated.
> 
> Ref: https://github.com/raspberrypi/linux/pull/1008#issuecomment-139234607
> 
> So either we need a new uart1 driver or a doubled clock freq. somehow.

Found out the same thing and communicated it to Eric - not 
knowing about the different divider…

Martin--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [LINUX RFC v2 1/4] spi: add support of two chip selects & data stripe

[Martin Sperl]

> On 04.09.2015, at 17:37, Mark Brown  wrote:
> 
> On Fri, Sep 04, 2015 at 02:35:52PM +0200, Martin Sperl wrote:
>>> On 03.09.2015, at 14:12, Mark Brown  wrote:
> 
>>> This isn't saying that the controller supports more than one chip, it's
>>> saying that the controller supports asserting more than one chip select
>>> at once which isn't the same thing.  I'm also not entirely sure that
>>> this makes sense as a separate feature to the data striping one - I'm
>>> struggling to think of a way to use this sensibly separately to that.
> 
>> Well - there is one use-case that I can think of:
>> fbtft has the requirement for some devices to control a GPIO to
>> differentiate between command and data getting transferred
>> - sort of 9 bit.
> 
> That's another thing again, isn't it?  It's one device switching between
> two different control interfaces at runtime rather than two devices
> controlled in lockstep.

I agree, but there may be a solution that can handle both, so I wanted
to mention it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [LINUX RFC v2 1/4] spi: add support of two chip selects & data stripe

[Martin Sperl]

> On 03.09.2015, at 14:12, Mark Brown  wrote:
> 
> On Wed, Aug 26, 2015 at 11:56:04AM +0530, Ranjit Waghmode wrote:
> 
>> To support dual parallel mode operation of ZynqMP GQSPI controller
>> following API's are added inside the core:
> 
> As covered in SubmittingPatches please try to make each patch a single
> change rather than having multiple separate changes in one commit.
> 
>> +/* Controller may support more than one chip.
>> + * This flag will enable that feature.
>> + */
>> +#define SPI_MASTER_BOTH_CS  BIT(8)  /* enable both chips */
> 
> This isn't saying that the controller supports more than one chip, it's
> saying that the controller supports asserting more than one chip select
> at once which isn't the same thing.  I'm also not entirely sure that
> this makes sense as a separate feature to the data striping one - I'm
> struggling to think of a way to use this sensibly separately to that.

Well - there is one use-case that I can think of:
fbtft has the requirement for some devices to control a GPIO to
differentiate between command and data getting transferred
- sort of 9 bit.

Right now it is done outside of spi in the fbtft driver itself wrapping
spi_sync().

Similarly a “hold” line on an eeprom or similar could get (de)asserted
without requiring holding a spi-bus-lock.

But then the current patch would not allow this kind of “generic”
use-case.

Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/5] dt/bindings: bcm2835: add binding documentation for bcm2835-aux

[Martin Sperl]

> On 26.08.2015, at 03:44, Stephen Warren  wrote:
> 
> On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote:
> 
>> +Example:
>> +
>> +aux_enable: aux_enable@0x7e215004 {
>> +compatible = "bcrm,bcm2835-aux";
>> +reg = <0x7e215004 0x04>;
> 
> I'd expect that to be <0x7e215000 0x8>;

The reason is that we just handle enable with this driver,
which just requires access to the 0x7e215004 register.

The 0x7e215000 register (interrupt mask) could be used by a
cascaded interrupt-controller, but as the spi and uart drivers
can run with shared interrupts this is not a necessity.

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [LINUX RFC v2 1/4] spi: add support of two chip selects & data stripe

[Martin Sperl]

> On 03.09.2015, at 14:12, Mark Brown  wrote:
> 
> On Wed, Aug 26, 2015 at 11:56:04AM +0530, Ranjit Waghmode wrote:
> 
>> To support dual parallel mode operation of ZynqMP GQSPI controller
>> following API's are added inside the core:
> 
> As covered in SubmittingPatches please try to make each patch a single
> change rather than having multiple separate changes in one commit.
> 
>> +/* Controller may support more than one chip.
>> + * This flag will enable that feature.
>> + */
>> +#define SPI_MASTER_BOTH_CS  BIT(8)  /* enable both chips */
> 
> This isn't saying that the controller supports more than one chip, it's
> saying that the controller supports asserting more than one chip select
> at once which isn't the same thing.  I'm also not entirely sure that
> this makes sense as a separate feature to the data striping one - I'm
> struggling to think of a way to use this sensibly separately to that.

Well - there is one use-case that I can think of:
fbtft has the requirement for some devices to control a GPIO to
differentiate between command and data getting transferred
- sort of 9 bit.

Right now it is done outside of spi in the fbtft driver itself wrapping
spi_sync().

Similarly a “hold” line on an eeprom or similar could get (de)asserted
without requiring holding a spi-bus-lock.

But then the current patch would not allow this kind of “generic”
use-case.

Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [LINUX RFC v2 1/4] spi: add support of two chip selects & data stripe

[Martin Sperl]

> On 04.09.2015, at 17:37, Mark Brown <broo...@kernel.org> wrote:
> 
> On Fri, Sep 04, 2015 at 02:35:52PM +0200, Martin Sperl wrote:
>>> On 03.09.2015, at 14:12, Mark Brown <broo...@kernel.org> wrote:
> 
>>> This isn't saying that the controller supports more than one chip, it's
>>> saying that the controller supports asserting more than one chip select
>>> at once which isn't the same thing.  I'm also not entirely sure that
>>> this makes sense as a separate feature to the data striping one - I'm
>>> struggling to think of a way to use this sensibly separately to that.
> 
>> Well - there is one use-case that I can think of:
>> fbtft has the requirement for some devices to control a GPIO to
>> differentiate between command and data getting transferred
>> - sort of 9 bit.
> 
> That's another thing again, isn't it?  It's one device switching between
> two different control interfaces at runtime rather than two devices
> controlled in lockstep.

I agree, but there may be a solution that can handle both, so I wanted
to mention it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/5] dt/bindings: bcm2835: add binding documentation for bcm2835-aux

[Martin Sperl]

> On 26.08.2015, at 03:44, Stephen Warren  wrote:
> 
> On 08/24/2015 02:40 AM, ker...@martin.sperl.org wrote:
> 
>> +Example:
>> +
>> +aux_enable: aux_enable@0x7e215004 {
>> +compatible = "bcrm,bcm2835-aux";
>> +reg = <0x7e215004 0x04>;
> 
> I'd expect that to be <0x7e215000 0x8>;

The reason is that we just handle enable with this driver,
which just requires access to the 0x7e215004 register.

The 0x7e215000 register (interrupt mask) could be used by a
cascaded interrupt-controller, but as the spi and uart drivers
can run with shared interrupts this is not a necessity.

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Martin Sperl]

On 8/6/2015 23:33, Russell King - ARM Linux wrote:

On Thu, Aug 06, 2015 at 06:14:00PM +0200, Geert Uytterhoeven wrote:


Irrespective of the dummy bytes.
What if the spi device is not a FLASH ROM, but some other device,
which receives a data packet that accidentally looks like an m25p80 READ
command?

Well, for the most part it looks like it should still work, but there
could be a gotcha, but first, let's get rid of a myth there.

The QSPI is _not_ specific to the M25P80.  The manual says nothing
about being specific to that device.  What it says is that it's for
SPI NOR memory.  It will work with bus widths of 1, 2 or 4 data lines,
so it probably works with non-M25P80 SPI NOR devices too - and the fact
that the read and write commands are completely programmable suggests
that using it with SPI NOR devices which do not use the M25P80 read
command value is intended.


The SFI is a state machine based translator which sits behind the SPI
interface (look at the manual).  It sequence sthe SPI bus through a
series of standard SPI states which happen to be the states I detailed
above.

Now, the first byte of the SFI-generated SPI message can be programmed
to any 8 bit value.  So the first byte of the SPI message is totally
under software control.  The next one to four bytes which comprise the
"address" can be controlled to by deciding where in the memory map to
start reading from.  Hence, the value of those bytes is also totally
under software control.  The number of dummy bytes can be programmed
too.  So far so good.

So, if we know that we have a SPI message which says "send 0x01 0x20
0x30, send one dummy byte, read 32 bytes", if we program the SFI to
send a read command as 0x01, program an address length of 2 bytes
with one dummy byte, and then read the next 32 bytes at the appropriate
offset in the memory mapping to cause the next two bytes to be 0x20,
0x30, then what we end up with on the bus is:

send 0x01, 0x20, 0x30
send one dummy byte

That much is good, but now is the problem - how does the SFI know that
we're going to require to read 32 bytes?  I think the answer to that
is that it doesn't know, so it probably just reads the number of bytes
which the access on the SoC bus is asking for, which makes it
indeterminant from a software point of view to control how many bytes
will be read without provoking another "send 0x01, next address, dummy
byte" sequence.

So, I'm now on the side of not parsing commands in the SPI driver, and
back on the idea that this needs to be handled in some other manner
which doesn't involve polluting the SPI core with flag-hacks.


So I see 2 distinct options:

Have the nor driver modified to run SPI commands and then ask the
SPI framework (and driver) to switch into mmap mode:

Would probably look something like this inside the nor driver:
   /* lock spi bus for other activities */
   spi_bus_lock(spi);
   /* send the "configuration" for mmap */
   t[0].tx_buf = flash->command;
   t[0].len = m25p_cmdsz(nor);
   spi_message_add_tail([0], );
   t[1].tx_buf = dummy_buffer;
   t[1].len = dummy;
   spi_message_add_tail([1], );
   spi_sync(spi, );
   /* switch to mmap mode */
   spi->mode |= SPI_MMAP;
   spi_setup(spi);
   /* run the mmapped transfers bypassing the spi-layer */
   memcpy(...)
   /* open questions here: which address range
* and how to detect if transfer is done
*/
/* restore back to "normal" mode */
   spi->mode &= ~SPI_MMAP;
   spi_setup(spi);
   /* unlock spi bus for other activities */
   spi_bus_unlock(spi);

The downside is that it requires modification in several places
(nor-framework, spi-framework plus the driver) and it would not
be generic enough...

IMO such a situation is feasible if we only got a single device
on the spi-bus, but leaves a lot of questions open...
Alternatively we could create an additional api.

On the other end of spectrum could be a solution where the
spi-master driverwould have the opportunity to query the
device-tree for specific propertiesduring the spi_add_device
phase - in this case querying the followingproperty in the
device-tree:
  spi-master-XXX,use-mmap-cmd-mode = <0x08 0x38>;
to implement mmap-mode for commands 0x08 and 0x38.

Maybe we would want to also encode the number of address bytes
to send per command without hardcoding those values explicitly:
so maybe something like:
  spi-master-XXX,use-mmap-cmd-mode = <0x08 2> <0x38 3>;

Obviously these would need to get documented in the bindings
documentation of that driver.

Alternatively we could also introduce generic alternate modes
for the driver(similar to GPIO - ALT modes), but that would be
less transparent and more hard-coded...

In the end this would mean that from the nor framework side
therewould be no change at all - it still would be issuing
something like this:
   /* send the "normal" block read command */
   t[0].tx_buf = flash->command;
   /* note that the address would be encoded here */
   t[0].len = m25p_cmdsz(nor);
   

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Martin Sperl]

On 8/6/2015 23:33, Russell King - ARM Linux wrote:

On Thu, Aug 06, 2015 at 06:14:00PM +0200, Geert Uytterhoeven wrote:


Irrespective of the dummy bytes.
What if the spi device is not a FLASH ROM, but some other device,
which receives a data packet that accidentally looks like an m25p80 READ
command?

Well, for the most part it looks like it should still work, but there
could be a gotcha, but first, let's get rid of a myth there.

The QSPI is _not_ specific to the M25P80.  The manual says nothing
about being specific to that device.  What it says is that it's for
SPI NOR memory.  It will work with bus widths of 1, 2 or 4 data lines,
so it probably works with non-M25P80 SPI NOR devices too - and the fact
that the read and write commands are completely programmable suggests
that using it with SPI NOR devices which do not use the M25P80 read
command value is intended.


The SFI is a state machine based translator which sits behind the SPI
interface (look at the manual).  It sequence sthe SPI bus through a
series of standard SPI states which happen to be the states I detailed
above.

Now, the first byte of the SFI-generated SPI message can be programmed
to any 8 bit value.  So the first byte of the SPI message is totally
under software control.  The next one to four bytes which comprise the
address can be controlled to by deciding where in the memory map to
start reading from.  Hence, the value of those bytes is also totally
under software control.  The number of dummy bytes can be programmed
too.  So far so good.

So, if we know that we have a SPI message which says send 0x01 0x20
0x30, send one dummy byte, read 32 bytes, if we program the SFI to
send a read command as 0x01, program an address length of 2 bytes
with one dummy byte, and then read the next 32 bytes at the appropriate
offset in the memory mapping to cause the next two bytes to be 0x20,
0x30, then what we end up with on the bus is:

send 0x01, 0x20, 0x30
send one dummy byte

That much is good, but now is the problem - how does the SFI know that
we're going to require to read 32 bytes?  I think the answer to that
is that it doesn't know, so it probably just reads the number of bytes
which the access on the SoC bus is asking for, which makes it
indeterminant from a software point of view to control how many bytes
will be read without provoking another send 0x01, next address, dummy
byte sequence.

So, I'm now on the side of not parsing commands in the SPI driver, and
back on the idea that this needs to be handled in some other manner
which doesn't involve polluting the SPI core with flag-hacks.


So I see 2 distinct options:

Have the nor driver modified to run SPI commands and then ask the
SPI framework (and driver) to switch into mmap mode:

Would probably look something like this inside the nor driver:
   /* lock spi bus for other activities */
   spi_bus_lock(spi);
   /* send the configuration for mmap */
   t[0].tx_buf = flash-command;
   t[0].len = m25p_cmdsz(nor);
   spi_message_add_tail(t[0], m);
   t[1].tx_buf = dummy_buffer;
   t[1].len = dummy;
   spi_message_add_tail(t[1], m);
   spi_sync(spi, m);
   /* switch to mmap mode */
   spi-mode |= SPI_MMAP;
   spi_setup(spi);
   /* run the mmapped transfers bypassing the spi-layer */
   memcpy(...)
   /* open questions here: which address range
* and how to detect if transfer is done
*/
/* restore back to normal mode */
   spi-mode = ~SPI_MMAP;
   spi_setup(spi);
   /* unlock spi bus for other activities */
   spi_bus_unlock(spi);

The downside is that it requires modification in several places
(nor-framework, spi-framework plus the driver) and it would not
be generic enough...

IMO such a situation is feasible if we only got a single device
on the spi-bus, but leaves a lot of questions open...
Alternatively we could create an additional api.

On the other end of spectrum could be a solution where the
spi-master driverwould have the opportunity to query the
device-tree for specific propertiesduring the spi_add_device
phase - in this case querying the followingproperty in the
device-tree:
  spi-master-XXX,use-mmap-cmd-mode = 0x08 0x38;
to implement mmap-mode for commands 0x08 and 0x38.

Maybe we would want to also encode the number of address bytes
to send per command without hardcoding those values explicitly:
so maybe something like:
  spi-master-XXX,use-mmap-cmd-mode = 0x08 2 0x38 3;

Obviously these would need to get documented in the bindings
documentation of that driver.

Alternatively we could also introduce generic alternate modes
for the driver(similar to GPIO - ALT modes), but that would be
less transparent and more hard-coded...

In the end this would mean that from the nor framework side
therewould be no change at all - it still would be issuing
something like this:
   /* send the normal block read command */
   t[0].tx_buf = flash-command;
   /* note that the address would be encoded here */
   t[0].len = m25p_cmdsz(nor);
   spi_message_add_tail(t[0], m);
   

Re: [PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

> On 12.05.2015, at 17:58, Noralf Trønnes  wrote:
> 
> Is there something missing for this patch to get accepted?
> spi-bcm2835 has now DMA support that depends on this patch.

As the spi-bcm2835.c patch using DMA (relying on this) has gone into
spi/for-next (but so far without the DT changes to enable DMA mode)
can we also get this dmaengine patch merged into for-next?

Note that I played BigBuckBunny movie on my spi TFT for the last
24 hours using DMA only and it works without any hickups using
the V2 patch provided by Noralf - it transferred 24GB so far.
I also did test it with spi-mmc and enc28j60 - all of which use DMA
with different access pattern, so that test should be quite
comprehensive.

Thanks,
Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

 On 12.05.2015, at 17:58, Noralf Trønnes nor...@tronnes.org wrote:
 
 Is there something missing for this patch to get accepted?
 spi-bcm2835 has now DMA support that depends on this patch.

As the spi-bcm2835.c patch using DMA (relying on this) has gone into
spi/for-next (but so far without the DT changes to enable DMA mode)
can we also get this dmaengine patch merged into for-next?

Note that I played BigBuckBunny movie on my spi TFT for the last
24 hours using DMA only and it works without any hickups using
the V2 patch provided by Noralf - it transferred 24GB so far.
I also did test it with spi-mmc and enc28j60 - all of which use DMA
with different access pattern, so that test should be quite
comprehensive.

Thanks,
Martin


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

> On 08.05.2015, at 13:20, Jonathan Bell  wrote:
>> 
> I agree that the interrupt generated would be spurious - in the case where it 
> is not required.
> 
> However if you do && (flags & DMA_PREP_INTERRUPT) then all users of this 
> driver need to explicitly set interrupt flags when doing a scatter-gather 
> transfer. As I understand it, currently the only upstream client of this 
> driver is the I2S driver which only uses cyclic anyway.
> 
> Not requiring an interrupt on completion is a bit of an edge case - the 
> default among other dmaengine drivers appears to be to enable interrupts 
> unconditionally.

I have now submitted a patch for spi-bcm2835 to make use of dma,
so there is one candidate for this kind of behavior.
So please go forward with the merge.

Also note that with the spi-HW dma support of the bcm2835
it is necessary to do a RX transfer even if the data is not
used (similar for TX).

Right now we have to allocate a dummy buffer to run these kind
of “one-way” transfers where we need 2 DMA channels.

The bcm2835 dma hw supports such dummy-transfer modes via 
BCM2835_DMA_S_IGNORE and BCM2835_DMA_D_IGNORE.

So maybe we can add a “flag” to the dmaengine_prep_slave_sg
that will allow such kind of behavior to get implemented?

That is not a necessity, but would be a welcome improvement.

Tested-by: Martin Sperl 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

 On 08.05.2015, at 13:20, Jonathan Bell jonat...@raspberrypi.org wrote:
 
 I agree that the interrupt generated would be spurious - in the case where it 
 is not required.
 
 However if you do  (flags  DMA_PREP_INTERRUPT) then all users of this 
 driver need to explicitly set interrupt flags when doing a scatter-gather 
 transfer. As I understand it, currently the only upstream client of this 
 driver is the I2S driver which only uses cyclic anyway.
 
 Not requiring an interrupt on completion is a bit of an edge case - the 
 default among other dmaengine drivers appears to be to enable interrupts 
 unconditionally.

I have now submitted a patch for spi-bcm2835 to make use of dma,
so there is one candidate for this kind of behavior.
So please go forward with the merge.

Also note that with the spi-HW dma support of the bcm2835
it is necessary to do a RX transfer even if the data is not
used (similar for TX).

Right now we have to allocate a dummy buffer to run these kind
of “one-way” transfers where we need 2 DMA channels.

The bcm2835 dma hw supports such dummy-transfer modes via 
BCM2835_DMA_S_IGNORE and BCM2835_DMA_D_IGNORE.

So maybe we can add a “flag” to the dmaengine_prep_slave_sg
that will allow such kind of behavior to get implemented?

That is not a necessity, but would be a welcome improvement.

Tested-by: Martin Sperl ker...@martin.sperl.org


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

I did tests with the (almost) finished dma-support of spi-bcm2835
against a TFT device and it work fine there.

Upstreaming this spi-bcm2835 patch obviously requires that the DMA
support is in the dma-engine driver first...

Please also see prior note about unnecessary interrupts when none
are requested for possible improvements.

> On 18.04.2015, at 13:06, Noralf Trønnes  wrote:
> 
> Add slave transfer capability to BCM2835 dmaengine driver.
> This patch is pulled from the bcm2708-dmaengine driver in the
> Raspberry Pi repo. The work was done by Gellert Weisz.
> 
> Tested using the bcm2835-mmc driver from the same repo.
> 
> Signed-off-by: Noralf Trønnes 

Tested-by: Martin Sperl 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

I did tests with the (almost) finished dma-support of spi-bcm2835
against a TFT device and it work fine there.

Upstreaming this spi-bcm2835 patch obviously requires that the DMA
support is in the dma-engine driver first...

Please also see prior note about unnecessary interrupts when none
are requested for possible improvements.

 On 18.04.2015, at 13:06, Noralf Trønnes nor...@tronnes.org wrote:
 
 Add slave transfer capability to BCM2835 dmaengine driver.
 This patch is pulled from the bcm2708-dmaengine driver in the
 Raspberry Pi repo. The work was done by Gellert Weisz.
 
 Tested using the bcm2835-mmc driver from the same repo.
 
 Signed-off-by: Noralf Trønnes nor...@tronnes.org

Tested-by: Martin Sperl ker...@martin.sperl.org

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

> On 03.05.2015, at 11:59, Mark Brown  wrote:
> Hrm, yes - that should work.  I'd ask Greg, that's not something the bus
> implements.

It is still slightly more “complicated” from a distribution perspective,
but if that is what makes it a “clean” solution, then that is the way to
go forward.

I will investigate the fine details, but I fear we may need some
“compatibility” magic similar to “new_id” in USB to make it work,
because it seems as if you can ONLY force a driver to bind if it
_is_ compatible...

See also here: https://lwn.net/Articles/143397/

But from what I can tell this functionality (mentioned in this article
by Greg) has not been moved into driver-core and bus, so we would need
to run our own version of it.--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

> On 30.04.2015, at 21:58, Mark Brown  wrote:
> 
> A big reason for that is that it's not in my inbox for me to review,
> these messages I flagged as unhelpful aren't going to help with that if
> only because I don't want to create the impression that such behaviour
> achieves results.
> 

What about implementing it like this:
echo -n “spi32761.4” > /sys/bus/spi/drivers/spidev/bind

Would this be an acceptable solution?

This is actually mentioned in Documentation/spi/spidev as a
possible option for the future - quote:

> (Sysfs also supports userspace driven binding/unbinding of drivers to
> devices.  That mechanism might be supported here in the future.)

Not sure why it does not work right now (but it works for “real”
device-drivers), but I guess it has to do with compatibility checks.

Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

 On 30.04.2015, at 21:58, Mark Brown broo...@kernel.org wrote:
 
 A big reason for that is that it's not in my inbox for me to review,
 these messages I flagged as unhelpful aren't going to help with that if
 only because I don't want to create the impression that such behaviour
 achieves results.
 

What about implementing it like this:
echo -n “spi32761.4”  /sys/bus/spi/drivers/spidev/bind

Would this be an acceptable solution?

This is actually mentioned in Documentation/spi/spidev as a
possible option for the future - quote:

 (Sysfs also supports userspace driven binding/unbinding of drivers to
 devices.  That mechanism might be supported here in the future.)

Not sure why it does not work right now (but it works for “real”
device-drivers), but I guess it has to do with compatibility checks.

Martin


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

 On 03.05.2015, at 11:59, Mark Brown broo...@kernel.org wrote:
 Hrm, yes - that should work.  I'd ask Greg, that's not something the bus
 implements.

It is still slightly more “complicated” from a distribution perspective,
but if that is what makes it a “clean” solution, then that is the way to
go forward.

I will investigate the fine details, but I fear we may need some
“compatibility” magic similar to “new_id” in USB to make it work,
because it seems as if you can ONLY force a driver to bind if it
_is_ compatible...

See also here: https://lwn.net/Articles/143397/

But from what I can tell this functionality (mentioned in this article
by Greg) has not been moved into driver-core and bus, so we would need
to run our own version of it.--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

Tests with the initial (and incomplete) version of the spi-bcm2835 driver 
with DMA transfer support show that the dma-engine works as expected with 
this patch.

There is one one observation:

> On 18.04.2015, at 13:06, Noralf Trønnes  wrote:
> +static struct dma_async_tx_descriptor *
> +bcm2835_dma_prep_slave_sg(struct dma_chan *chan,
> +   struct scatterlist *sgl,
> +   unsigned int sg_len,
> +   enum dma_transfer_direction direction,
> +   unsigned long flags, void *context)
> +{
...
> + /* Enable */
> + if (i == sg_len - 1 && len - j <= max_size)
> + control_block->info |= BCM2835_DMA_INT_EN;

The observation is that an interrupt is always triggered - even in the case
where flags does NOT have DMA_PREP_INTERRUPT set.
This may not be necessary and avoid interrupts.

So maybe the above if clause should get extended by:
   && (flags & DMA_PREP_INTERRUPT)
to only trigger an interrupt when really requested.

I am not sure if there are any side-effects because of this besides having the
requirement on the client to run dmaengine_terminate_all() on that specific dma
channel without interrupts when the transfer is finished.

In the case of SPI we have TX feed the fifo - which finishes early - , but we
only need to the interrupt when RX finishes reading the fifo, which indicates
that the SPI-transfer is fully finished. 
So having an interrupt on TX is not necessary for the process.

The same observations may also apply to bcm2835_dma_prep_dma_cyclic (which is
outside of this patch provided by Noralf).

Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dmaengine: bcm2835: Add slave dma support

[Martin Sperl]

Tests with the initial (and incomplete) version of the spi-bcm2835 driver 
with DMA transfer support show that the dma-engine works as expected with 
this patch.

There is one one observation:

 On 18.04.2015, at 13:06, Noralf Trønnes nor...@tronnes.org wrote:
 +static struct dma_async_tx_descriptor *
 +bcm2835_dma_prep_slave_sg(struct dma_chan *chan,
 +   struct scatterlist *sgl,
 +   unsigned int sg_len,
 +   enum dma_transfer_direction direction,
 +   unsigned long flags, void *context)
 +{
...
 + /* Enable */
 + if (i == sg_len - 1  len - j = max_size)
 + control_block-info |= BCM2835_DMA_INT_EN;

The observation is that an interrupt is always triggered - even in the case
where flags does NOT have DMA_PREP_INTERRUPT set.
This may not be necessary and avoid interrupts.

So maybe the above if clause should get extended by:
(flags  DMA_PREP_INTERRUPT)
to only trigger an interrupt when really requested.

I am not sure if there are any side-effects because of this besides having the
requirement on the client to run dmaengine_terminate_all() on that specific dma
channel without interrupts when the transfer is finished.

In the case of SPI we have TX feed the fifo - which finishes early - , but we
only need to the interrupt when RX finishes reading the fifo, which indicates
that the SPI-transfer is fully finished. 
So having an interrupt on TX is not necessary for the process.

The same observations may also apply to bcm2835_dma_prep_dma_cyclic (which is
outside of this patch provided by Noralf).

Martin


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Martin Sperl]

> On 28.04.2015, at 03:21, Michael Welling  wrote:
> If I were to attempt to convert the driver to use the core chipselect support,
> how would I go about doing it?
> 
> Is there another driver that I can use for reference?
You may look into this patch: e34ff011c70e5f4ef219141711142d5111ae6ebb
for the spi-bcm2835 driver, which did the conversion to the new transfer_one
interface (and framework based GPIO chipselects).

For most parts all you have to do is take the contents of the loop over all the
spi_transfers inside your master->transfer_one_message method and create a new
method with it and assign it to master->transfer_one.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

> On 27.04.2015, at 17:27, Mark Brown  wrote:
> 
> 
> OK, so that is just a default overlay which is abusing the fact that we
> will bind to spidev without a DT compatible and when the binding is
> undocumented (which also applies to other devices and buses sadly).
> 
> Unfortunately nobody ever mentioned this upstream and the feedback
> upstream that listing spidev in a DT is a bad idea has been ignored.

Maybe it should also have been documented as such in
Documentation/spi/spidev or in Documentation/devicetree/bindings/spi/

> The whole reason we're doing this in the first place is that we got sick
> of telling people that using spidev in DTs like this was a bad idea.


The only reference I found in my history of the spi-list is this email:
> On 08.10.2014, at 22:05, Mark Brown  wrote:
> 
> On Wed, Oct 08, 2014 at 02:27:08PM -0500, ttha...@opensource.altera.com wrote:
> 
>> +spidev@0 {
>> +compatible = "spidev";
>> +reg = <0>;  /* chip select */
>> +spi-max-frequency = <1>;
>> +};
> 
> No, if you're putting spidev into the DT that's broken - describe the
> hardware, not the software you're using to control it.


And google found this patch from a little earlier:
http://comments.gmane.org/gmane.linux.ports.arm.kernel/231050

So finding this piece of information on the “use-policy” is quite hard
- google finds lots of links where it is described as working that way.

> It does sound like the people maintianing the u-boot fork for the Pi
> need to talk to both u-boot upstream (nothing here is specific to the
> Pi that I can see) and the kernel community a bit more.  I'd be a bit
> worried that they may be relying on other things that just happen to
> work without being intentional (and are therefore more vulnerable to
> issues) and it's a bit depressing to see things like this stuck in a
> fork where only a limited community can make use of them.
Actually this functionality is not in u-boot, but in the firmware
boot-loader itself, which can load the kernel (and the devicetree)
without u-boot, but which can also load u-boot as an additional
intermediate boot-stage.

>> The only thing that could possibly be better would be that
>> the user would define the "real" name of the device in the
>> device tree and spidev would bind to it if there is no driver
>> available (but that would require this "fallback" binding by
>> spidev in case of no driver).
> 
> Yes, that is exactly the solution I'd suggest - change the UI to provide
> a DT compatible to be used for the new device.  That would also have the
> benefit of meaning that users who have connected some device that does
> have a driver that works with a simple binding wouldn't need to write an
> overlay which seems like it should be useful.

Well then why did you just make the system complain loudly and bringing
problems to people instead of solving it in a usable manner that does not
require people to maintain an out of tree patch to work around that warning?

We still have the one-line warning about using the depreciated 
spi_master.transfer interface, but it is not such loud warning as this one.

I guess the time spent discussing this could have been better spent
implementing that solution instead.

All we need is a volunteer to get that implemented.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

On 2015-04-27 13:25, Mark Brown wrote:

On Mon, Apr 27, 2015 at 12:04:12PM +0200, Hans de Goede wrote:


Have you seen my mail about the raspberry pi use-case? Using dt-overlays
simply is not an acceptable answer there. There are legitimate use-cases
for a "generic spi bus" concept with the bus only being accessible via
spidev.



Blocking this use-case because you do not believe it is a valid use-case
is not going to help, this will just lead to the custom distros these
boards are shipping doing some ugly hack, which is not what we want
IMHO.


I don't think you've fully considered your use case here.  As I said in
my reply to your earlier e-mail I think what you're looking for here is
something like better UI around overlays.  Registering a SPI bus without
knowing what's connected to it doesn't allow generic maker style usage
of the board, it's just as likely to hinder a user as help them.  For
example, if someone wants to use the SPI pins for another function such
as GPIO they'll have to handle that (and may have problems with pin
conflicts causing electrical issues if they do load up the DT with
spidev in it).  If someone has a SPI device they want to bind an in
kernel driver to they'll have to handle that, if they want to use a GPIO
to provide an additional chip select they'll have to handle that too.

Note that the discussion here isn't about userspace drivers, it's about
how the hardware is described.


Mark, maybe you are missing something of how this can get done on the
raspberry pi with devicetree (and overlays).

So here how the raspberry-foundation describes the devices in their 
device-tree for spi:


dtsi:
spi0: spi@7e204000 {
compatible = "brcm,bcm2708-spi";
reg = <0x7e204000 0x1000>;
interrupts = <2 22>;
clocks = <_spi>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
};

dts:
 {
pinctrl-names = "default";
pinctrl-0 = <_pins>;

spidev@0{
compatible = "spidev";
reg = <0>;  /* CE0 */
#address-cells = <1>;
#size-cells = <0>;
spi-max-frequency = <50>;
};

spidev@1{
compatible = "spidev";
reg = <1>;  /* CE1 */
#address-cells = <1>;
#size-cells = <0>;
spi-max-frequency = <50>;
};
};
/ {
__overrides__ {
spi = <>,"status";
};
}

So you see that spi is disabled by default - the pins are
free to use for anything.

The firmware then integrates the overrrides into the device-tree
before loading the kernel.

So to enable spi in general you add the following to /boot/config.txt:
dtparam=spi=on

That gives you spi plus two "generic" spidev devices.

If you want to include a mcp2515 you add also the following:
dtoverlay=mcp2515-can0-overlay
and that loads also the overlay for the mcp2515 can module.

The corresponding mcp2515 overlay looks like this:
/ {
/* the spi config of the can-controller itself */
fragment@1 {
target = <>;
__overlay__ {
/* needed to avoid dtc warning */
#address-cells = <1>;
#size-cells = <0>;
can0: mcp2515@0 {
reg = <0>;
compatible = "microchip,mcp2515";
pinctrl-names = "default";
pinctrl-0 = <_pins>;
spi-max-frequency = <1000>;
interrupt-parent = <>;
interrupts = <25 0x2>;
clocks = <_osc>;
};
};
};
};

(left out the unimportant stuff like clocks,
interrupt GPIOs,...)

All this implements:
* easy means to enable spi if requested by user
* by default includes spidev as the default device
* but this can get just as easily get overridden by another
  devicetree to get specific devices onboarded using the
  in kernel drivers - there are now something like 25+
  overlays provided by the foundation that follow this
  pattern...

This is really about describing the hardware in the best possible
ways and keeping the interface with the users simple by having
him only to edit /boot/config.txt.

Adding your own overlays is just as simple and also quite well
defined.

So coming from this perspective I believe that there is some
concern in the raspberry pi community, because the description
they provide is now specific to the HW and their intent and so
the loud "croaking" of spidev will irritate people even when
they have done everything the best they can.

OK, I admit, the spi-devices could be separate overlays if
one really wants to have them, but they can get disabled just
as easily (by a specific overlay) if only a single device is
needed.

The only thing that could possibly be better would be that
the 

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

On 2015-04-27 13:25, Mark Brown wrote:

On Mon, Apr 27, 2015 at 12:04:12PM +0200, Hans de Goede wrote:


Have you seen my mail about the raspberry pi use-case? Using dt-overlays
simply is not an acceptable answer there. There are legitimate use-cases
for a generic spi bus concept with the bus only being accessible via
spidev.



Blocking this use-case because you do not believe it is a valid use-case
is not going to help, this will just lead to the custom distros these
boards are shipping doing some ugly hack, which is not what we want
IMHO.


I don't think you've fully considered your use case here.  As I said in
my reply to your earlier e-mail I think what you're looking for here is
something like better UI around overlays.  Registering a SPI bus without
knowing what's connected to it doesn't allow generic maker style usage
of the board, it's just as likely to hinder a user as help them.  For
example, if someone wants to use the SPI pins for another function such
as GPIO they'll have to handle that (and may have problems with pin
conflicts causing electrical issues if they do load up the DT with
spidev in it).  If someone has a SPI device they want to bind an in
kernel driver to they'll have to handle that, if they want to use a GPIO
to provide an additional chip select they'll have to handle that too.

Note that the discussion here isn't about userspace drivers, it's about
how the hardware is described.


Mark, maybe you are missing something of how this can get done on the
raspberry pi with devicetree (and overlays).

So here how the raspberry-foundation describes the devices in their 
device-tree for spi:


dtsi:
spi0: spi@7e204000 {
compatible = brcm,bcm2708-spi;
reg = 0x7e204000 0x1000;
interrupts = 2 22;
clocks = clk_spi;
#address-cells = 1;
#size-cells = 0;
status = disabled;
};

dts:
spi0 {
pinctrl-names = default;
pinctrl-0 = spi0_pins;

spidev@0{
compatible = spidev;
reg = 0;  /* CE0 */
#address-cells = 1;
#size-cells = 0;
spi-max-frequency = 50;
};

spidev@1{
compatible = spidev;
reg = 1;  /* CE1 */
#address-cells = 1;
#size-cells = 0;
spi-max-frequency = 50;
};
};
/ {
__overrides__ {
spi = spi0,status;
};
}

So you see that spi is disabled by default - the pins are
free to use for anything.

The firmware then integrates the overrrides into the device-tree
before loading the kernel.

So to enable spi in general you add the following to /boot/config.txt:
dtparam=spi=on

That gives you spi plus two generic spidev devices.

If you want to include a mcp2515 you add also the following:
dtoverlay=mcp2515-can0-overlay
and that loads also the overlay for the mcp2515 can module.

The corresponding mcp2515 overlay looks like this:
/ {
/* the spi config of the can-controller itself */
fragment@1 {
target = spi0;
__overlay__ {
/* needed to avoid dtc warning */
#address-cells = 1;
#size-cells = 0;
can0: mcp2515@0 {
reg = 0;
compatible = microchip,mcp2515;
pinctrl-names = default;
pinctrl-0 = can0_pins;
spi-max-frequency = 1000;
interrupt-parent = gpio;
interrupts = 25 0x2;
clocks = can0_osc;
};
};
};
};

(left out the unimportant stuff like clocks,
interrupt GPIOs,...)

All this implements:
* easy means to enable spi if requested by user
* by default includes spidev as the default device
* but this can get just as easily get overridden by another
  devicetree to get specific devices onboarded using the
  in kernel drivers - there are now something like 25+
  overlays provided by the foundation that follow this
  pattern...

This is really about describing the hardware in the best possible
ways and keeping the interface with the users simple by having
him only to edit /boot/config.txt.

Adding your own overlays is just as simple and also quite well
defined.

So coming from this perspective I believe that there is some
concern in the raspberry pi community, because the description
they provide is now specific to the HW and their intent and so
the loud croaking of spidev will irritate people even when
they have done everything the best they can.

OK, I admit, the spi-devices could be separate overlays if
one really wants to have them, but they can get disabled just
as easily (by a specific overlay) if only a single device is
needed.

The only thing that could possibly be better would be that
the user would define the real name of the 

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

 On 27.04.2015, at 17:27, Mark Brown broo...@kernel.org wrote:
 
 
 OK, so that is just a default overlay which is abusing the fact that we
 will bind to spidev without a DT compatible and when the binding is
 undocumented (which also applies to other devices and buses sadly).
 
 Unfortunately nobody ever mentioned this upstream and the feedback
 upstream that listing spidev in a DT is a bad idea has been ignored.

Maybe it should also have been documented as such in
Documentation/spi/spidev or in Documentation/devicetree/bindings/spi/

 The whole reason we're doing this in the first place is that we got sick
 of telling people that using spidev in DTs like this was a bad idea.


The only reference I found in my history of the spi-list is this email:
 On 08.10.2014, at 22:05, Mark Brown broo...@kernel.org wrote:
 
 On Wed, Oct 08, 2014 at 02:27:08PM -0500, ttha...@opensource.altera.com wrote:
 
 +spidev@0 {
 +compatible = spidev;
 +reg = 0;  /* chip select */
 +spi-max-frequency = 1;
 +};
 
 No, if you're putting spidev into the DT that's broken - describe the
 hardware, not the software you're using to control it.


And google found this patch from a little earlier:
http://comments.gmane.org/gmane.linux.ports.arm.kernel/231050

So finding this piece of information on the “use-policy” is quite hard
- google finds lots of links where it is described as working that way.

 It does sound like the people maintianing the u-boot fork for the Pi
 need to talk to both u-boot upstream (nothing here is specific to the
 Pi that I can see) and the kernel community a bit more.  I'd be a bit
 worried that they may be relying on other things that just happen to
 work without being intentional (and are therefore more vulnerable to
 issues) and it's a bit depressing to see things like this stuck in a
 fork where only a limited community can make use of them.
Actually this functionality is not in u-boot, but in the firmware
boot-loader itself, which can load the kernel (and the devicetree)
without u-boot, but which can also load u-boot as an additional
intermediate boot-stage.

 The only thing that could possibly be better would be that
 the user would define the real name of the device in the
 device tree and spidev would bind to it if there is no driver
 available (but that would require this fallback binding by
 spidev in case of no driver).
 
 Yes, that is exactly the solution I'd suggest - change the UI to provide
 a DT compatible to be used for the new device.  That would also have the
 benefit of meaning that users who have connected some device that does
 have a driver that works with a simple binding wouldn't need to write an
 overlay which seems like it should be useful.

Well then why did you just make the system complain loudly and bringing
problems to people instead of solving it in a usable manner that does not
require people to maintain an out of tree patch to work around that warning?

We still have the one-line warning about using the depreciated 
spi_master.transfer interface, but it is not such loud warning as this one.

I guess the time spent discussing this could have been better spent
implementing that solution instead.

All we need is a volunteer to get that implemented.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Martin Sperl]

 On 28.04.2015, at 03:21, Michael Welling mwell...@ieee.org wrote:
 If I were to attempt to convert the driver to use the core chipselect support,
 how would I go about doing it?
 
 Is there another driver that I can use for reference?
You may look into this patch: e34ff011c70e5f4ef219141711142d5111ae6ebb
for the spi-bcm2835 driver, which did the conversion to the new transfer_one
interface (and framework based GPIO chipselects).

For most parts all you have to do is take the contents of the loop over all the
spi_transfers inside your master-transfer_one_message method and create a new
method with it and assign it to master-transfer_one.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

> On 26.04.2015, at 13:23, Hans de Goede  wrote:
> I think there is actual a use for just binding spidev as spidev,
> think e.g. the spi pins on the raspberry pi.
> 
> How do you deal we suggest with such a situation ?

I actually asked the same question a few days ago on the spi list
(in thread: "spi: spidev: Warn loudly if instantiated from DT as “spidev”)
and the summary was:

You can still do as before, but you have to accept that long 
irritating warning.

Or you patch spidev.c to include your pattern of choice for compatiblity

Or you implement the following proposal (which needs a volunteer):
> On 23.04.2015, at 09:42, Geert Uytterhoeven  wrote:
> 
> So what you need is a way to handover from generic spidev to a device-specific
> driver, cfr. what graphics drivers do when the device has been bound to by
> vesafb or simplefb.
> 
> Could this be implemented in a generic way in the spi or DT code?

...
> On 23.04.2015, at 12:36, Mark Brown  wrote:
> On Thu, Apr 23, 2015 at 09:45:16AM +0200, Geert Uytterhoeven wrote:
> 
>> I guess this has been suggested before: the spi core could provide spidev
>> access to all spi client devices which are not bound by a driver?
> 
> I don't know if it's been suggested before, certainly nobody did the
> work to make it happen.  I don't think I have a massive objection in
> principal.


Martin



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Martin Sperl]

 On 26.04.2015, at 13:23, Hans de Goede hdego...@redhat.com wrote:
 I think there is actual a use for just binding spidev as spidev,
 think e.g. the spi pins on the raspberry pi.
 
 How do you deal we suggest with such a situation ?

I actually asked the same question a few days ago on the spi list
(in thread: spi: spidev: Warn loudly if instantiated from DT as “spidev”)
and the summary was:

You can still do as before, but you have to accept that long 
irritating warning.

Or you patch spidev.c to include your pattern of choice for compatiblity

Or you implement the following proposal (which needs a volunteer):
 On 23.04.2015, at 09:42, Geert Uytterhoeven ge...@linux-m68k.org wrote:
 
 So what you need is a way to handover from generic spidev to a device-specific
 driver, cfr. what graphics drivers do when the device has been bound to by
 vesafb or simplefb.
 
 Could this be implemented in a generic way in the spi or DT code?

...
 On 23.04.2015, at 12:36, Mark Brown broo...@kernel.org wrote:
 On Thu, Apr 23, 2015 at 09:45:16AM +0200, Geert Uytterhoeven wrote:
 
 I guess this has been suggested before: the spi core could provide spidev
 access to all spi client devices which are not bound by a driver?
 
 I don't know if it's been suggested before, certainly nobody did the
 work to make it happen.  I don't think I have a massive objection in
 principal.


Martin



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/