diff mbox

[1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2

Message ID 1434980408-4086-1-git-send-email-kernel@martin.sperl.org (mailing list archive)
State New, archived
Headers show

Commit Message

Martin Sperl June 22, 2015, 1:40 p.m. UTC
From: Martin Sperl <kernel@martin.sperl.org>

This driver does NOT make use of native chip-selects but uses the
generic cs-gpios facilities provided by the framework allowing for
more than 3 chip-selects.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

---
 drivers/spi/Kconfig          |   10 +
 drivers/spi/Makefile         |    1 +
 drivers/spi/spi-bcm2835aux.c |  549 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 560 insertions(+)
 create mode 100644 drivers/spi/spi-bcm2835aux.c

Only tested spi1, as no Compute Module is available to me to test spi2.

Also note that the driver currently implements locking of the auxiliar
device enable bits inside the driver. This is shared with the uart1,
for which we have so far no driver. Ideally this should get moved out
as soon as we get the uart1 driver enabled.

Uart1 is also only accessible on the Compute Module.

Comments

Noralf Trønnes June 22, 2015, 2:08 p.m. UTC | #1
Den 22.06.2015 15:40, skrev kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
>
> This driver does NOT make use of native chip-selects but uses the
> generic cs-gpios facilities provided by the framework allowing for
> more than 3 chip-selects.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>
> ---
>   drivers/spi/Kconfig          |   10 +
>   drivers/spi/Makefile         |    1 +
>   drivers/spi/spi-bcm2835aux.c |  549 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 560 insertions(+)
>   create mode 100644 drivers/spi/spi-bcm2835aux.c
>
> Only tested spi1, as no Compute Module is available to me to test spi2.
>
> Also note that the driver currently implements locking of the auxiliar
> device enable bits inside the driver. This is shared with the uart1,
> for which we have so far no driver. Ideally this should get moved out
> as soon as we get the uart1 driver enabled.
>
> Uart1 is also only accessible on the Compute Module.

Uart1 is available on the uart0 pins (alt5) and the 8250 driver can be 
used (ns16550 compatible).
See: https://github.com/raspberrypi/linux/pull/1008

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
Jakub Kici?ski June 22, 2015, 2:55 p.m. UTC | #2
On Mon, 22 Jun 2015 13:40:06 +0000, kernel@martin.sperl.org wrote:
> Also note that the driver currently implements locking of the auxiliar
> device enable bits inside the driver. This is shared with the uart1,
> for which we have so far no driver. Ideally this should get moved out
> as soon as we get the uart1 driver enabled.

As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
Proper driver - perhaps modelled as a bus - seems like a prerequisite
for this work.  You are also not using IRQ mux in DT binding example
which is very misleading.

Do we have any indication weather this AUX hardware is available on
RPi2 as well?  IIRC bcm2836 differs only in CPU cores, peripherals
remain identical.  Perhaps this driver could be used on RPi2 and
naming it 283x would be more appropriate?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
Martin Sperl June 22, 2015, 3:26 p.m. UTC | #3
> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
> Proper driver - perhaps modelled as a bus - seems like a prerequisite
> for this work.  You are also not using IRQ mux in DT binding example
> which is very misleading.

Well - there is no support far for uart1 in upstream as of now.
And I am not even sure if the compute module is supported as there
is no device tree available in upstream for that...

So I have taken the simple route of using shared interrupts and providing
a minimal framework to enable the spi1/spi2 hw blocks with proper locking.
And I have mentioned that it would need to get modified when uart1 support
comes along.

If you know of a better solution how to control this and that also
incorporates a patch to enable uart1, then please share it!

Just modify the bcm2835aux_spi_enable/disable to use a different framework
than just bcm2835_aux_modify_enable.

The patch Noralf mentions only applies downstream and only sets the
enable-register during the boot sequence, so it would not impact the
solution as is.

> Do we have any indication weather this AUX hardware is available on
> RPi2 as well?  IIRC bcm2836 differs only in CPU cores, peripherals
> remain identical.  Perhaps this driver could be used on RPi2 and
> naming it 283x would be more appropriate?

I have not checked, but I would guess that it exists.

As for naming: I have asked around and bcm2835aux seemed fine.
Also if we go that route we should start renaming ALL driver instances that
contain 2835.--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
Martin Sperl June 25, 2015, 2:19 p.m. UTC | #4
So what does it require to get this merged?

Rename of the module?

Thanks

> On 22.06.2015, at 17:26, Martin Sperl <kernel@martin.sperl.org> wrote:
> 
> 
>> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
>> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
>> Proper driver - perhaps modelled as a bus - seems like a prerequisite
>> for this work.  You are also not using IRQ mux in DT binding example
>> which is very misleading.
> 
> Well - there is no support far for uart1 in upstream as of now.
> And I am not even sure if the compute module is supported as there
> is no device tree available in upstream for that...
> 
> So I have taken the simple route of using shared interrupts and providing
> a minimal framework to enable the spi1/spi2 hw blocks with proper locking.
> And I have mentioned that it would need to get modified when uart1 support
> comes along.
> 
> If you know of a better solution how to control this and that also
> incorporates a patch to enable uart1, then please share it!
> 
> Just modify the bcm2835aux_spi_enable/disable to use a different framework
> than just bcm2835_aux_modify_enable.
> 
> The patch Noralf mentions only applies downstream and only sets the
> enable-register during the boot sequence, so it would not impact the
> solution as is.
> 
>> Do we have any indication weather this AUX hardware is available on
>> RPi2 as well?  IIRC bcm2836 differs only in CPU cores, peripherals
>> remain identical.  Perhaps this driver could be used on RPi2 and
>> naming it 283x would be more appropriate?
> 
> I have not checked, but I would guess that it exists.
> 
> As for naming: I have asked around and bcm2835aux seemed fine.
> Also if we go that route we should start renaming ALL driver instances that
> contain 2835.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 30, 2015, 9:42 a.m. UTC | #5
On Thu, Jun 25, 2015 at 04:19:17PM +0200, Martin Sperl wrote:
> So what does it require to get this merged?

Don't top post.

This looks relevant:

> >> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
> >> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
> >> Proper driver - perhaps modelled as a bus - seems like a prerequisite
> >> for this work.  You are also not using IRQ mux in DT binding example
> >> which is very misleading.
Martin Sperl July 1, 2015, 7:39 p.m. UTC | #6
> On 30.06.2015, at 19:42, Mark Brown <broonie@kernel.org> wrote:
> 
> This looks relevant:
> 
>>>> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
>>>> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
>>>> Proper driver - perhaps modelled as a bus - seems like a prerequisite
>>>> for this work.  You are also not using IRQ mux in DT binding example
>>>> which is very misleading.
Well, we are using shared interrupts, which is typical for other drivers on
the rpi as well - i2c and usb are examples for multiple handlers on
a single irq line on the rpi - excerpt from /proc/interrupts:
          CPU0
33: 3547607286  ARMCTRL-level  41 Edge      20980000.usb, dwc2_hsotg:usb1
77:          0  ARMCTRL-level  85 Edge      20205000.i2c, 20804000.i2c


The compute module is not supported upstream and nobody
created any patches for a device tree for a cm...

On top there is no direct support for uart1 in the upstream kernel.

Support only exists in the foundation kernel where the implementation
enables the uart1 in the board-config directly, so that uart1
can also get used as the console during boot.

This means (as far as I understand) that there can be no 
race-condition, as spi would get configured later during boot
and the access to the enable register is then properly
protected against concurrent access when enabling both
auxiliar spi devices (and spi2 is only usable on the compute
Module - see above for upstream support of the cm)

Finally asking for a recommendation with regards to using a bus
to arbitrate access to the enable register there was no feedback
how this could be get implemented...

This last piece is actually one of the last reasons why I have not
posted a patch for spi1 and spi2 to the default device-trees for
the rpi models yet.
(That and the fact that assigning pins in multiple pinctrl groups
- i2s and spi1 - gives warnings or errors in dmesg, because i2s is
also requiring Gpio 18, 19, 20 and 21 on the model-b-plus
device tree - so it can only be either/or not both...)

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Noralf Trønnes July 1, 2015, 8:57 p.m. UTC | #7
Den 01.07.2015 21:39, skrev Martin Sperl:
>> On 30.06.2015, at 19:42, Mark Brown <broonie@kernel.org> wrote:
>>
>> This looks relevant:
>>
>>>>> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
>>>>> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
>>>>> Proper driver - perhaps modelled as a bus - seems like a prerequisite
>>>>> for this work.  You are also not using IRQ mux in DT binding example
>>>>> which is very misleading.

[...]

> Finally asking for a recommendation with regards to using a bus
> to arbitrate access to the enable register there was no feedback
> how this could be get implemented...

Maybe you can use drivers/mfd/syscon.c to enable shared access to the
aux enable register. Then the spi driver could get the regmap with:
aux_regmap = syscon_regmap_lookup_by_phandle(np, "syscon");

aux_syscon: aux_syscon@7e215000 {
   compatible = "syscon";
   reg = <0x7e215000 0x08>;
};

spi2@7e2150c0 {
   compatible = "brcm,bcm2835-aux-spi";
   reg = <0x7e2150c0 0x40>;
   syscon = <&aux_syscon>;
};

So if someone later will add a Pi extension to the 8250 serial driver or
enable the mini uart by some other means, they can use the syscon regmap.


About sharing the aux interrupt, could this be implemented in irq-bcm2835
as a Bank 3?

spi1@7e215080 {
  compatible = "brcm,bcm2835-aux-spi";
  reg = <0x7e215080 0x40>, <0x7e215000 0x08>;
  interrupts = <3 1>;
};

spi2@7e2150c0 {
  compatible = "brcm,bcm2835-aux-spi";
  reg = <0x7e2150c0 0x40>, <0x7e215000 0x08>;
  interrupts = <3 2>;
};

These are just a suggestions, I don't know if it is "the correct" way to 
do it.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 5, 2015, 1:14 a.m. UTC | #8
> On 02.07.2015, at 06:57, Noralf Trønnes <noralf@tronnes.org> wrote:
> 
> 
> Den 01.07.2015 21:39, skrev Martin Sperl:
>>> On 30.06.2015, at 19:42, Mark Brown <broonie@kernel.org> wrote:
>>> 
>>> This looks relevant:
>>> 
>>>>>> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
>>>>>> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
>>>>>> Proper driver - perhaps modelled as a bus - seems like a prerequisite
>>>>>> for this work.  You are also not using IRQ mux in DT binding example
>>>>>> which is very misleading.
> 
> [...]
> 
>> Finally asking for a recommendation with regards to using a bus
>> to arbitrate access to the enable register there was no feedback
>> how this could be get implemented...
> 
> Maybe you can use drivers/mfd/syscon.c to enable shared access to the
> aux enable register. Then the spi driver could get the regmap with:
> aux_regmap = syscon_regmap_lookup_by_phandle(np, "syscon");

Seems as if you found an answer to my original question of if
there is a framework to handle this.

> About sharing the aux interrupt, could this be implemented in irq-bcm2835
> as a Bank 3?
Obviously it could, but the question is if it is not more overhead than using
a shared interrupt (requesting an interrupt with the shared flag set) in the
first place (like this driver currently does or i2c and USB do).

I am working on a new version to make use of syscon.

Thanks, Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Noralf Trønnes July 5, 2015, 10:37 a.m. UTC | #9
Den 05.07.2015 03:14, skrev Martin Sperl:
>> On 02.07.2015, at 06:57, Noralf Trønnes <noralf@tronnes.org> wrote:
>>
>>
>> Den 01.07.2015 21:39, skrev Martin Sperl:
>>>> On 30.06.2015, at 19:42, Mark Brown <broonie@kernel.org> wrote:
>>>>
>>>> This looks relevant:
>>>>
>>>>>>> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
>>>>>>> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
>>>>>>> Proper driver - perhaps modelled as a bus - seems like a prerequisite
>>>>>>> for this work.  You are also not using IRQ mux in DT binding example
>>>>>>> which is very misleading.

[...]

>> About sharing the aux interrupt, could this be implemented in irq-bcm2835
>> as a Bank 3?
> Obviously it could, but the question is if it is not more overhead than using
> a shared interrupt (requesting an interrupt with the shared flag set) in the
> first place (like this driver currently does or i2c and USB do).

I have looked closer at the 8250 serial driver, and in fact it has a
framework for writing 8250 like drivers. This makes it easy to extend
it to include support for the mini uart.
drivers/tty/serial/8250/8250_dw.c is an example of a driver that
overrides the interrupt handler. So a 8250_bcm2835.c can easily be
made with interrupt sharing as you suggest (and aux enabling). But
this means that the irq status register has to be included in the
syscon regmap as well.


Noralf.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 5, 2015, 1:11 p.m. UTC | #10
> On 05.07.2015, at 20:37, Noralf Trønnes <noralf@tronnes.org> wrote:
> 
> I have looked closer at the 8250 serial driver, and in fact it has a
> framework for writing 8250 like drivers. This makes it easy to extend
> it to include support for the mini uart.
> drivers/tty/serial/8250/8250_dw.c is an example of a driver that
> overrides the interrupt handler. So a 8250_bcm2835.c can easily be
> made with interrupt sharing as you suggest (and aux enabling). But
> this means that the irq status register has to be included in the
> syscon regmap as well.

With the Module parameter share_irqs=1 the stock 8250 driver will
request a shared irq, but you can also control it via:
CONFIG_SERIAL_8250_SHARE_IRQ

As for regmap support - the driver only uses the status register of
the uart so it does not need access to the shared interrupt flag register.
(If no irq condition is detected then IRQ_NoNE is returned and the
Shared irq system will call the next irq handler in the list)

So all that is left to do is enabling the uart device in the aux-enable
registers using regmap and then the uart can get used.

Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 11, 2015, 4:37 a.m. UTC | #11
On 06/22/2015 09:26 AM, Martin Sperl wrote:
>> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
>> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
>> Proper driver - perhaps modelled as a bus - seems like a prerequisite
>> for this work.  You are also not using IRQ mux in DT binding example
>> which is very misleading.
> 
> Well - there is no support far for uart1 in upstream as of now.
> And I am not even sure if the compute module is supported as there
> is no device tree available in upstream for that...

IIRC I've run the CM with the RPi B DT. I've been meaning to upstream
explicit DTs for all the various RPi models, but haven't manage to get
around to that yet.

> So I have taken the simple route of using shared interrupts and providing
> a minimal framework to enable the spi1/spi2 hw blocks with proper locking.
> And I have mentioned that it would need to get modified when uart1 support
> comes along.
> 
> If you know of a better solution how to control this and that also
> incorporates a patch to enable uart1, then please share it!

I haven't looked at the registers to be sure of the structure and hence
if this is good advice, but creating an IRQ controller driver for the
aux registers (as Mark intimated) sounds like a good idea.

> Just modify the bcm2835aux_spi_enable/disable to use a different framework
> than just bcm2835_aux_modify_enable.
> 
> The patch Noralf mentions only applies downstream and only sets the
> enable-register during the boot sequence, so it would not impact the
> solution as is.
> 
>> Do we have any indication weather this AUX hardware is available on
>> RPi2 as well?  IIRC bcm2836 differs only in CPU cores, peripherals
>> remain identical.  Perhaps this driver could be used on RPi2 and
>> naming it 283x would be more appropriate?
> 
> I have not checked, but I would guess that it exists.
> 
> As for naming: I have asked around and bcm2835aux seemed fine.
> Also if we go that route we should start renaming ALL driver instances that
> contain 2835.

I'd suggest leaving named after the "first" chip the driver supports, in
a similar fashion to how DT compatible values work. There's zero benefit
from renaming existing drivers, and having new drivers continue with the
existing naming convention would leave everything nice and consistent.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 11, 2015, 4:41 a.m. UTC | #12
On 07/04/2015 07:14 PM, Martin Sperl wrote:
> 
>> On 02.07.2015, at 06:57, Noralf Trønnes <noralf@tronnes.org> wrote:
>>
>>
>> Den 01.07.2015 21:39, skrev Martin Sperl:
>>>> On 30.06.2015, at 19:42, Mark Brown <broonie@kernel.org> wrote:
>>>>
>>>> This looks relevant:
>>>>
>>>>>>> On 22.06.2015, at 16:55, Jakub Kici?ski <moorray3@wp.pl> wrote:
>>>>>>> As mentioned by Noralf UART1 is quite commonly used on Compute Modules.
>>>>>>> Proper driver - perhaps modelled as a bus - seems like a prerequisite
>>>>>>> for this work.  You are also not using IRQ mux in DT binding example
>>>>>>> which is very misleading.
>>
>> [...]
>>
>>> Finally asking for a recommendation with regards to using a bus
>>> to arbitrate access to the enable register there was no feedback
>>> how this could be get implemented...
>>
>> Maybe you can use drivers/mfd/syscon.c to enable shared access to the
>> aux enable register. Then the spi driver could get the regmap with:
>> aux_regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
> 
> Seems as if you found an answer to my original question of if
> there is a framework to handle this.

I'd suggest avoiding syscon. IIUC, it just exposes raw register IO for
the shared register region, which is very "non-semantic", hence
completely hides what the reason the registers are being touched. I'd
much prefer a custom driver with use-case-specific APIs. If the
registers implement an IRQ mux/demux, an explicit IRQ controller driver
would be much better.

>> About sharing the aux interrupt, could this be implemented in irq-bcm2835
>> as a Bank 3?
>
> Obviously it could, but the question is if it is not more overhead than using
> a shared interrupt (requesting an interrupt with the shared flag set) in the
> first place (like this driver currently does or i2c and USB do).

IIUC, these registers aren't part of the main IRQ controller's register
set, and hence shouldn't be touched by the main IRQ controller driver.
Create a separate driver for entirely separate HW blocks/functionality/...

> I am working on a new version to make use of syscon.

:-(

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 11, 2015, 4:53 a.m. UTC | #13
On 06/22/2015 07:40 AM, kernel@martin.sperl.org wrote:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> This driver does NOT make use of native chip-selects but uses the
> generic cs-gpios facilities provided by the framework allowing for
> more than 3 chip-selects.

> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c

> +/*
> + * spi register defines
> + *
> + * note there is garbage in the "official" documentation,
> + * so somedata taken from the file:
> + *   brcm_usrlib/dag/vmcsx/vcinclude/bcm2708_chip/aux_io.h
> + * inside of:
> + *   http://www.broadcom.com/docs/support/videocore/Brcm_Android_ICS_Graphics_Stack.tar.gz
> + */

Hopefully the license of that tar file allows for that. I didn't look.

> +DEFINE_SPINLOCK(__bcm2835_aux_lock);
> +static void bcm2835_aux_modify_enable(struct bcm2835aux_spi *bs,
> +				      u32 mask, u32 val)
> +{
> +	unsigned long flags;
> +	u32 r;
> +
> +	spin_lock_irqsave(&__bcm2835_aux_lock, flags);
> +
> +	r = readl(bs->aux_regs + BCM2835_AUX_ENABLE);
> +	r &= mask;
> +	r |= val;
> +	writel(r, bs->aux_regs + BCM2835_AUX_ENABLE);
> +
> +	spin_unlock_irqrestore(&__bcm2835_aux_lock, flags);
> +}

As mentioned elsewhere, I'd hope all the shared aux register
manipulations can be pushed into a shared driver.

> +static void bcm2835aux_spi_enable(struct bcm2835aux_spi *bs)
> +{
> +	/* identify the spi device - needed to activate the right HW-block */
> +	u32 mask = (size_t)bs->regs & 0x00000040 ?
> +		   BCM2835_AUX_BIT_SPI2 : BCM2835_AUX_BIT_SPI1;
> +
> +	bcm2835_aux_modify_enable(bs, ~mask, mask);
> +}

I expect that function will go away when my previous comment is
resolved. If not, it'd be nice to encode the "ID" of the device into DT,
so that the driver has no hard-coded idea of what register addresses
exist; what happens when (a hypothetical) bcm2837 comes along with 3
instances of this HW block?

> +static int bcm2835aux_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *master = platform_get_drvdata(pdev);
> +	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
> +
> +	/* Clear FIFOs, and disable the HW block */
> +	clk_disable_unprepare(bs->clk);

You probably want remove() to do things in the reverse order of probe().
In particular, disable the clock after anything that could touch
registers in the HW.

> +	bcm2835aux_spi_reset_hw(bs);
> +
> +	/* disable HW block */
> +	bcm2835aux_spi_disable(bs);

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 12, 2015, 2:10 a.m. UTC | #14
I am on a business-trip right now, so I can not check the mailing list
that often and my access to a rpi to develop is also limited hence
the V3 patch set took some time to get out of the door and crossed
the responses you had sent.

> On 11.07.2015, at 14:53, Stephen Warren <swarren@wwwdotorg.org> wrote:
> Hopefully the license of that tar file allows for that. I didn't look.
Well, I just took the values from the released video driver for those values
where doe documentation is obviously wrong - essentially clarifying
the SPI_STAT register on page 25.

> As mentioned elsewhere, I'd hope all the shared aux register
> manipulations can be pushed into a shared driver.

I just found this email after having sent the latest version of the patch
that makes use of syscon/regmap to serialize access to the
 bcm2835_AUX_ENABLE register - that is all that it is really needed for.

From my perspective it seems that a new driver just would produce
more code (to maintain) for something that the syscon driver already
provides. If someone starts to enable aux-uart, then it could go with
a new framework, but that is still more code that needs to get maintained
than just using syscon as that "shared" driver.

As for hardcoded ids, that may be the case, but I just wanted to keep
Device tree as minimal as necessary...
If we get to that situation then we can easily move that to some
other logic...

Finally with regards to interrupts: these are shared anyway so I
wonder why we would want to write an extra irq chip driver when
it works as is anyway.

If someone thinks that is of a performance advantage then this
extra layer of indirection could get written and we would only need
to change the device-tree to use the new irq vectors instead.

But we are really only talking about 2 drivers and 3 interrupt sources.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 14, 2015, 4:56 a.m. UTC | #15
On 07/11/2015 08:10 PM, Martin Sperl wrote:
> I am on a business-trip right now, so I can not check the mailing list
> that often and my access to a rpi to develop is also limited hence
> the V3 patch set took some time to get out of the door and crossed
> the responses you had sent.
> 
>> On 11.07.2015, at 14:53, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> Hopefully the license of that tar file allows for that. I didn't look.
> Well, I just took the values from the released video driver for those values
> where doe documentation is obviously wrong - essentially clarifying
> the SPI_STAT register on page 25.
> 
>> As mentioned elsewhere, I'd hope all the shared aux register
>> manipulations can be pushed into a shared driver.
> 
> I just found this email after having sent the latest version of the patch
> that makes use of syscon/regmap to serialize access to the
>  bcm2835_AUX_ENABLE register - that is all that it is really needed for.
> 
> From my perspective it seems that a new driver just would produce
> more code (to maintain) for something that the syscon driver already
> provides. If someone starts to enable aux-uart, then it could go with
> a new framework, but that is still more code that needs to get maintained
> than just using syscon as that "shared" driver.
> 
> As for hardcoded ids, that may be the case, but I just wanted to keep
> Device tree as minimal as necessary...
> If we get to that situation then we can easily move that to some
> other logic...
> 
> Finally with regards to interrupts: these are shared anyway so I
> wonder why we would want to write an extra irq chip driver when
> it works as is anyway.
> 
> If someone thinks that is of a performance advantage then this
> extra layer of indirection could get written and we would only need
> to change the device-tree to use the new irq vectors instead.
> 
> But we are really only talking about 2 drivers and 3 interrupt sources.

I don't care so much about the internal code details; anything there can
be trivially changed. But please do make sure the DT correctly
represents the HW by:

* Having a separate DT node for each HW block (SPI controller, UART, and
any shared interrupt mux/demux/controller/...)

* If e.g. the SPI controller driver is going to need to manipulate the
"shared interrupt mux/demux/controller/...", there should be a phandle
from the SPI controller node to the "shared interrupt
mux/demux/controller/..." node, rather than listing the shared registers
in each "client"'s reg property.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 14, 2015, 12:39 p.m. UTC | #16
> On 14.07.2015, at 14:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
> I don't care so much about the internal code details; anything there can
> be trivially changed. But please do make sure the DT correctly
> represents the HW by:
> 
> * Having a separate DT node for each HW block (SPI controller, UART, and
> any shared interrupt mux/demux/controller/...)

That is what we have with the v3 patch:
* spi1
* spi2
* syscon controlling the aux-enable register
  (only used to enable/disable the hw block)

> * If e.g. the SPI controller driver is going to need to manipulate the
> "shared interrupt mux/demux/controller/...", there should be a phandle
> from the SPI controller node to the "shared interrupt
> mux/demux/controller/..." node, rather than listing the shared registers
> in each "client"'s reg property.
That is not what is in the v3 release of the patch any longer.

Each of the above dt-nodes has their own (non-overlapping) register
ranges and interrupts are enabled/disabled in the respective ranges
for spi1/spi2. Also the status register of spi1/2 contain the
corresponding flags for the reason of an interrupt - so no need
to access any of the shared aux registers for that - hence the use
of IRQF_SHARED.


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 22, 2015, 1:55 a.m. UTC | #17
On 07/14/2015 06:39 AM, Martin Sperl wrote:
> 
>> On 14.07.2015, at 14:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>> I don't care so much about the internal code details; anything there can
>> be trivially changed. But please do make sure the DT correctly
>> represents the HW by:
>>
>> * Having a separate DT node for each HW block (SPI controller, UART, and
>> any shared interrupt mux/demux/controller/...)
> 
> That is what we have with the v3 patch:
> * spi1
> * spi2
> * syscon controlling the aux-enable register
>   (only used to enable/disable the hw block)

OK, having separate nodes is great.

However, I'd like to see a "semantic" driver for the shared register
region rather than a "syscon". IIUC, "syscon" simply provides a stylized
way for one driver to touch some shared registers directly without any
semantics. I'd strongly prefer to see a real driver inside Linux rather
than something that just lets drivers fiddle with the shared registers
willy nilly. Still, this aspect is an internal implementation detail
inside the kernel that we can change without external impact later if we
need.

More concerning: The bcm283x HW doesn't implement a "syscon" module, but
some semantic IP block. The DT should contain a real compatible value
that describes what the HW block really is, not just "syscon". We could
bind the syscon driver to this compatible value if we have to for now.

>> * If e.g. the SPI controller driver is going to need to manipulate the
>> "shared interrupt mux/demux/controller/...", there should be a phandle
>> from the SPI controller node to the "shared interrupt
>> mux/demux/controller/..." node, rather than listing the shared registers
>> in each "client"'s reg property.
> That is not what is in the v3 release of the patch any longer.
> 
> Each of the above dt-nodes has their own (non-overlapping) register
> ranges and interrupts are enabled/disabled in the respective ranges
> for spi1/spi2. Also the status register of spi1/2 contain the
> corresponding flags for the reason of an interrupt - so no need
> to access any of the shared aux registers for that - hence the use
> of IRQF_SHARED.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 22, 2015, 4:28 p.m. UTC | #18
> On 22.07.2015, at 03:55, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
> However, I'd like to see a "semantic" driver for the shared register
> region rather than a "syscon". IIUC, "syscon" simply provides a stylized
> way for one driver to touch some shared registers directly without any
> semantics. I'd strongly prefer to see a real driver inside Linux rather
> than something that just lets drivers fiddle with the shared registers
> willy nilly. Still, this aspect is an internal implementation detail
> inside the kernel that we can change without external impact later if we
> need.
> 
> More concerning: The bcm283x HW doesn't implement a "syscon" module, but
> some semantic IP block. The DT should contain a real compatible value
> that describes what the HW block really is, not just "syscon". We could
> bind the syscon driver to this compatible value if we have to for

So, do I understand you correctly that if we would call the node 
bcm2835aux_enable as syscon with only the enable bit field register in it 
and add a enable_reg (pointing to the above) and enable_reg_mask=2 or 4
to the spi1/2 nodes then it would be acceptable? 

Would look like this:
spi2@7e2150c0 {
+    compatible = "brcm,bcm2835-aux-spi";
+    reg = <0x7e2150c0 0x40>;
+    interrupts = <1 29>;
+    clocks = <&clk_spi>;
+    #address-cells = <1>;
+    #size-cells = <0>;
+    cs-gpios = <&gpio 43>, <&gpio 44>, <&gpio 45>;
+    enable_reg = <&bcm2835aux_enable>;
+    enable_reg_mask = 4;
+};
+
+/* the necessary syscon config referenced above */
+bcm2835aux_enable: bcm2835aux_enable@0x7e215004 {
+    compatible = "syscon";
+    reg = <0x7e215004 0x04>;
+};


The uart aux driver would use:
+    enable_reg = <&bcm2835aux_enable>;
+    enable_reg_mask = 1;

Or maybe you would prefer a different naming?

The only use-case would be using:
regmap_update_bits(enable_reg, 0, enable_reg_mask, enable_reg_mask); /*enable */
regmap_update_bits(enable_reg, 0, enable_reg_mask, 0); /* disable */


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 24, 2015, 4:09 a.m. UTC | #19
On 07/22/2015 10:28 AM, Martin Sperl wrote:
> 
> 
>> On 22.07.2015, at 03:55, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>> However, I'd like to see a "semantic" driver for the shared register
>> region rather than a "syscon". IIUC, "syscon" simply provides a stylized
>> way for one driver to touch some shared registers directly without any
>> semantics. I'd strongly prefer to see a real driver inside Linux rather
>> than something that just lets drivers fiddle with the shared registers
>> willy nilly. Still, this aspect is an internal implementation detail
>> inside the kernel that we can change without external impact later if we
>> need.
>>
>> More concerning: The bcm283x HW doesn't implement a "syscon" module, but
>> some semantic IP block. The DT should contain a real compatible value
>> that describes what the HW block really is, not just "syscon". We could
>> bind the syscon driver to this compatible value if we have to for
> 
> So, do I understand you correctly that if we would call the node 
> bcm2835aux_enable as syscon with only the enable bit field register in it 
> and add a enable_reg (pointing to the above) and enable_reg_mask=2 or 4
> to the spi1/2 nodes then it would be acceptable? 
> 
> Would look like this:
> spi2@7e2150c0 {
> +    compatible = "brcm,bcm2835-aux-spi";
> +    reg = <0x7e2150c0 0x40>;
> +    interrupts = <1 29>;
> +    clocks = <&clk_spi>;
> +    #address-cells = <1>;
> +    #size-cells = <0>;
> +    cs-gpios = <&gpio 43>, <&gpio 44>, <&gpio 45>;
> +    enable_reg = <&bcm2835aux_enable>;
> +    enable_reg_mask = 4;
> +};
> +
> +/* the necessary syscon config referenced above */
> +bcm2835aux_enable: bcm2835aux_enable@0x7e215004 {
> +    compatible = "syscon";
> +    reg = <0x7e215004 0x04>;
> +};
> 
> 
> The uart aux driver would use:
> +    enable_reg = <&bcm2835aux_enable>;
> +    enable_reg_mask = 1;

I think I'd expect the shared registers to be:

bcm2835aux: misc@0x7e215000 {
    compatible = "brcm,bcm2835-aux";
    reg = <0x7e215000 0x08>;
};

There are two 4-byte registers outside the UART/SPI blocks, and the
compatible value should reflect what the block is, not that Linux may
use a syscon driver to implement the driver for it.

In the client, I'd expect a more semantic naming for the reference;
something like:

	brcm,aux = <&bcm2835aux 4>;

brcm, since it's a custom/vendor-specific property. aux is the name of
the object that's referenced. Packing the phandle and data together into
a single property reduces the number of separate properties, and is a
typical thing to do in a client of a service in DT.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 24, 2015, 6:47 a.m. UTC | #20
> On 24.07.2015, at 06:09, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
> I think I'd expect the shared registers to be:
> 
> bcm2835aux: misc@0x7e215000 {
>    compatible = "brcm,bcm2835-aux";
>    reg = <0x7e215000 0x08>;
> };
> 
> There are two 4-byte registers outside the UART/SPI blocks, and the
> compatible value should reflect what the block is, not that Linux may
> use a syscon driver to implement the driver for it.
> 
> In the client, I'd expect a more semantic naming for the reference;
> something like:
> 
> 	brcm,aux = <&bcm2835aux 4>;
> 
> brcm, since it's a custom/vendor-specific property. aux is the name of
> the object that's referenced. Packing the phandle and data together into
> a single property reduces the number of separate properties, and is a
> typical thing to do in a client of a service in DT.

So in the end you are saying we need a separate driver to get written
(because of ‘compatible = "brcm,bcm2835-aux”;’)

That is unless you would allow:
compatible = compatible = "brcm,bcm2835-aux”, “syscon”;

If this is not acceptable, then where should such a driver go in the
kernel tree?

It would essentially implement the following:
bcm2835aux_enable(dev, device-id);
bcm2835aux_disable(dev, device-id);

Which would just set/clean the bits in the register while holding a lock.

As an alternative: maybe we could implement it as an IRQ driver
and when the irq is requested for that device, then the HW-block gets
enabled automatically (and disabled when released).

That way we may not need to have a separate driver that would enable
the uart1, but it would be sufficient to configure it as follows:

uart1: uart@7e215040 {
	compatible = "brcm,bcm2835-aux-uart", "ns16550";
	reg = <0x7e215040 0x40>;
	interrupts = <X 0>;
	clock-frequency = <500000000>;
	reg-shift = <2>;
	no-loopback-test;
	status = "disabled";
};

(taken from the foundation device-tree)

Please clarify

Thanks,
Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 28, 2015, 2:51 a.m. UTC | #21
On 07/24/2015 12:47 AM, Martin Sperl wrote:
> 
>> On 24.07.2015, at 06:09, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>> I think I'd expect the shared registers to be:
>>
>> bcm2835aux: misc@0x7e215000 {
>>    compatible = "brcm,bcm2835-aux";
>>    reg = <0x7e215000 0x08>;
>> };
>>
>> There are two 4-byte registers outside the UART/SPI blocks, and the
>> compatible value should reflect what the block is, not that Linux may
>> use a syscon driver to implement the driver for it.
>>
>> In the client, I'd expect a more semantic naming for the reference;
>> something like:
>>
>> 	brcm,aux = <&bcm2835aux 4>;
>>
>> brcm, since it's a custom/vendor-specific property. aux is the name of
>> the object that's referenced. Packing the phandle and data together into
>> a single property reduces the number of separate properties, and is a
>> typical thing to do in a client of a service in DT.
> 
> So in the end you are saying we need a separate driver to get written
> (because of ‘compatible = "brcm,bcm2835-aux”;’)

It's fine if some existing driver matches that compatible value.

> That is unless you would allow:
> compatible = compatible = "brcm,bcm2835-aux”, “syscon”;

"syscon" definitely shouldn't be in a DT.

> If this is not acceptable, then where should such a driver go in the
> kernel tree?
> 
> It would essentially implement the following:
> bcm2835aux_enable(dev, device-id);
> bcm2835aux_disable(dev, device-id);
> 
> Which would just set/clean the bits in the register while holding a lock.

That sounds reasonable. I'd also expect a function that the client
drivers could call during probe() to look up the device (and implement
deferred probe) and another to release the reference during the client's
remove().

> As an alternative: maybe we could implement it as an IRQ driver
> and when the irq is requested for that device, then the HW-block gets
> enabled automatically (and disabled when released).

That seems a bit too implicit; there's no strict requirement that a
driver for e.g. the SPI block has to use an interrupt; it's merely
extremely likely.

> That way we may not need to have a separate driver that would enable
> the uart1, but it would be sufficient to configure it as follows:

Surely the IRQ driver would be a separate driver either way? The only
difference is whether it exposes custom APIs, or does things as a
side-effect of the existing IRQ API.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 28, 2015, 6:18 a.m. UTC | #22
Hi Stephen!

> On 28.07.2015, at 04:51, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
> 
>> If this is not acceptable, then where should such a driver go in the
>> kernel tree?
>> 
>> It would essentially implement the following:
>> bcm2835aux_enable(dev, device-id);
>> bcm2835aux_disable(dev, device-id);
>> 
>> Which would just set/clean the bits in the register while holding a lock.
> 
> That sounds reasonable. I'd also expect a function that the client
> drivers could call during probe() to look up the device (and implement
> deferred probe) and another to release the reference during the client's
> remove().
But the bigger question you have not answered is: “where should such an
auxiliar driver go in the kernel tree?” i.e. which directory?

I really do not want to implement it and then get told: “that should not
go here” - been thru too many iterations already…

Also I am not sure I understood the “defer” thingy.
I thought of actually implementing only 2 functions to use during probe
and removal:
* bcm2835aux_enable(dev)
* bcm2835aux_disable(dev)

Both would pick up the “bcrm,aux” property from the device tree (as per 
“bcrm,aux = <&bcm2835aux 4>”) and set the enable register accordingly
holding a lock.

I still could leave those 2 functions in the spi driver for now and
when someone wants to implement the uart1, then they would need to pull
that out into a separate driver.

As for interrupt-handler: it was a simple idea that would work fine for
the bcm2835aux case to avoid configuring things for the uart, but then
you do not want the uart1 to get configured as 'compatible=“ns16650”;’
inside the device-tree, so this is not acceptable anyway (unless there
were some way to define “additional” compatibility for a driver without
modifying the driver itself).
This means we will need to implement a minimal driver for uart1 that
wraps ns16650.

But as all the drivers (spi-bcm2835aux as well as ns16650) can use
FIRQ_SHARED (and do not need to read/write the shared aux_irg registers)
the need for a interrupt controller is not there.



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 28, 2015, 10:48 a.m. UTC | #23
On 28.07.2015 08:18, Martin Sperl wrote:
> Hi Stephen!
> But the bigger question you have not answered is: “where should such an
> auxiliar driver go in the kernel tree?” i.e. which directory?
One thing: could the "module" be a regulator?

So drivers/regulators/bcm2835-aux-regulator.c

The devicetree would look something like this:

regulators {
     bcm2835aux_reg {
         compatible = "bcrm,bcrm-2835-aux-enable";
         reg = <0x7e215004 0x4>; /* the AUX enable register */

         bcm2835aux_uart1: {
             regulator-name = "bcm2835_aux_uart1";
             reg = <0>; /* bit 0 in enable register */
         };
         bcm2835aux_spi1: {
             regulator-name = "bcm2835_aux_spi1";
             reg = <1>; /* bit 1 in enable register */
         };
         bcm2835aux_spi2: {
             regulator-name = "bcm2835_aux_spi2";
             reg = <2>; /* bit 2 in enable register */
         };
     }
};

uart1: uart1@7e215040 {
compatible = "brcm,bcm2835-aux-uart";
     reg = <0x7e215040 0x40>;
     ...
     vcc = <&bcm2835aux_uart1>
};

spi1: spi1@7e215080 {
compatible = "brcm,bcm2835-aux-spi";
     reg = <0x7e215080 0x40>;
     ...
     vcc = <&bcm2835aux_spi1>
};

spi2: spi2@7e2150C0 {
compatible = "brcm,bcm2835-aux-spi";
     reg = <0x7e2150C0 0x40>;
     ...
     vcc = <&bcm2835aux_spi2>
};

And the necessary driver-side code for bcm2835aux_spi and
bcm2835aux_uart would be along those lines:

probe:
   /* get the "power-supply" */
   bs->power = devm_regulator_get(dev, "vcc");
   if ((PTR_ERR(priv->power) == -EPROBE_DEFER)
     return -EPROBE_DEFER
   /* enable the power-supply */
   regulator_enable(bs->power);

remove:
   regulator_disable(bs->power);

Would that look like an acceptable solution?

Thanks,
	Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Wahren July 29, 2015, 4:37 p.m. UTC | #24
Hi Martin,

Am 28.07.2015 um 12:48 schrieb Martin Sperl:
> On 28.07.2015 08:18, Martin Sperl wrote:
>> Hi Stephen!
>> But the bigger question you have not answered is: “where should such an
>> auxiliar driver go in the kernel tree?” i.e. which directory?
> One thing: could the "module" be a regulator?
>

IMHO that won't be acceptable.

How about a multifunction device driver (drivers/mfd)?

Other candidates could be drivers/soc or drivers/misc.

Regards Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Noralf Trønnes July 29, 2015, 5:25 p.m. UTC | #25
Den 29.07.2015 18:37, skrev Stefan Wahren:
> Hi Martin,
>
> Am 28.07.2015 um 12:48 schrieb Martin Sperl:
>> On 28.07.2015 08:18, Martin Sperl wrote:
>>> Hi Stephen!
>>> But the bigger question you have not answered is: “where should such an
>>> auxiliar driver go in the kernel tree?” i.e. which directory?
>> One thing: could the "module" be a regulator?
>>
>
> IMHO that won't be acceptable.
>
> How about a multifunction device driver (drivers/mfd)?
>
> Other candidates could be drivers/soc or drivers/misc.
>

Could it be a power domain driver?
Documentation/devicetree/bindings/power/power_domain.txt

There are currently a deferred probing issue that might be a problem
depending on the module linking order:
https://lkml.org/lkml/2015/3/11/483


Noralf.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 29, 2015, 9:16 p.m. UTC | #26
> On 29.07.2015, at 18:37, Stefan Wahren <info@lategoodbye.de> wrote:
> 
> Hi Martin,
> 
>> Am 28.07.2015 um 12:48 schrieb Martin Sperl:
>>> On 28.07.2015 08:18, Martin Sperl wrote:
>>> Hi Stephen!
>>> But the bigger question you have not answered is: “where should such an
>>> auxiliar driver go in the kernel tree?” i.e. which directory?
>> One thing: could the "module" be a regulator?
> 
> IMHO that won't be acceptable.
Why would it not be acceptable?

It provides all sorts of methods and you do not have to implement all of them.
Enable and disable would be sufficient.
On top it would be a generic interface.

Even the mcp251x can driver uses 2 of those to enable/disable  the controller chip 
and the transceiver (and 2 dummy regulators if none are defined) 
There are also gpio regulators to switch a gpio to enable/disable a power-supply.

So with all those features it should be fairly standard to be used for
such things.

See here for a bit of documentation:
http://lxr.free-electrons.com/source/Documentation/power/regulator/design.txt
http://lxr.free-electrons.com/source/Documentation/power/regulator/consumer.txt
(Especially point 2 about enable/disable)

> How about a multifunction device driver (drivers/mfd)?
> Other candidates could be drivers/soc or drivers/misc.
Also possible...

Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Wahren July 30, 2015, 5:36 a.m. UTC | #27
Hi Martin,

Am 29.07.2015 um 23:16 schrieb Martin Sperl:
>
>
>> On 29.07.2015, at 18:37, Stefan Wahren <info@lategoodbye.de> wrote:
>>
>> Hi Martin,
>>
>>> Am 28.07.2015 um 12:48 schrieb Martin Sperl:
>>>> On 28.07.2015 08:18, Martin Sperl wrote:
>>>> Hi Stephen!
>>>> But the bigger question you have not answered is: “where should such an
>>>> auxiliar driver go in the kernel tree?” i.e. which directory?
>>> One thing: could the "module" be a regulator?
>>
>> IMHO that won't be acceptable.
> Why would it not be acceptable?
>

first of all, sorry i didn't follow this thread in every detail. My 
intention was to give you some ideas. I'm a not maintainer.

IMU a regulator is a hardware part who controls voltage or current.

Does it apply in your case?

Any question: would a user expect this function in the regulator framework?

Regards Stefan



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Sperl July 30, 2015, 6:45 a.m. UTC | #28
> On 30.07.2015, at 07:36, Stefan Wahren <info@lategoodbye.de> wrote:
> 
> Hi Martin,
> 
> Am 29.07.2015 um 23:16 schrieb Martin Sperl:
>> 
>> 
>>> On 29.07.2015, at 18:37, Stefan Wahren <info@lategoodbye.de> wrote:
>>> 
>>> Hi Martin,
>>> 
>>>> Am 28.07.2015 um 12:48 schrieb Martin Sperl:
>>>>> On 28.07.2015 08:18, Martin Sperl wrote:
>>>>> Hi Stephen!
>>>>> But the bigger question you have not answered is: “where should such an
>>>>> auxiliar driver go in the kernel tree?” i.e. which directory?
>>>> One thing: could the "module" be a regulator?
>>> 
>>> IMHO that won't be acceptable.
>> Why would it not be acceptable?
>> 
> 
> first of all, sorry i didn't follow this thread in every detail. My intention was to give you some ideas. I'm a not maintainer.
> 
> IMU a regulator is a hardware part who controls voltage or current.
> 
> Does it apply in your case?
> 
> Any question: would a user expect this function in the regulator framework?

I am just trying to find a solution that will get accepted with the minimum
number of reposts/rewrites to avoid frustration, but nobody has an answer
which api we really should use.

The problem is that we have DT and we have code and as we only want HW
being represented in the device tree we need something “sensible”.

What about adding “bcrm,bcm2835-aux-enable” to drivers/mfd/syscon.c
compatibility?

That way we:
* have a clean dt that only represents hardware
* reuse existing code with minimal modifications
* minimal effort

That would be a minimal patch to enable this, so we can ask if that is
acceptable and if it is not then we can still think of something else,
which would be mostly replicating the bit-management portion of syscon.

Sometimes I wish there was a way to “extend” compatibility of a driver
by just adding something to the device-tree (say compatibility) or define 
it in the platform config c-files without having to touch the actual
drivers to add that compatibility string...

Martin








--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Aug. 1, 2015, 2:51 a.m. UTC | #29
On 07/28/2015 12:18 AM, Martin Sperl wrote:
> Hi Stephen!
> 
>> On 28.07.2015, at 04:51, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>>
>>> If this is not acceptable, then where should such a driver go in the
>>> kernel tree?
>>>
>>> It would essentially implement the following:
>>> bcm2835aux_enable(dev, device-id);
>>> bcm2835aux_disable(dev, device-id);
>>>
>>> Which would just set/clean the bits in the register while holding a lock.
>>
>> That sounds reasonable. I'd also expect a function that the client
>> drivers could call during probe() to look up the device (and implement
>> deferred probe) and another to release the reference during the client's
>> remove().
>
> But the bigger question you have not answered is: “where should such an
> auxiliar driver go in the kernel tree?” i.e. which directory?

drivers/soc seems made for this.

> I really do not want to implement it and then get told: “that should not
> go here” - been thru too many iterations already…
>
> Also I am not sure I understood the “defer” thingy.
> I thought of actually implementing only 2 functions to use during probe
> and removal:
> * bcm2835aux_enable(dev)
> * bcm2835aux_disable(dev)
> 
> Both would pick up the “bcrm,aux” property from the device tree (as per 
> “bcrm,aux = <&bcm2835aux 4>”) and set the enable register accordingly
> holding a lock.

That'd probably be fine. The important thing is to get the DT right
since that's an ABI. The implementation can be refactored/rewritten at
will later provided the right info is in the DT.

If you go this route, deferred probe isn't relevant.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Aug. 1, 2015, 2:55 a.m. UTC | #30
On 07/28/2015 04:48 AM, Martin Sperl wrote:
> On 28.07.2015 08:18, Martin Sperl wrote:
>> Hi Stephen!
>> But the bigger question you have not answered is: “where should such an
>> auxiliar driver go in the kernel tree?” i.e. which directory?
>
> One thing: could the "module" be a regulator?

The HW docs aren't very detailed on this point, but they certainly don't
state that these bits control a regulator (or power domain as mentioned
later in the thread). I guess it's possible, but it feels like an
assumption to me. The HW modules seem small enough that it doesn't feel
likely anyone would have bothered with power-gating them.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 0cae169..270445c 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -88,6 +88,16 @@  config SPI_BCM2835
 	  is for the regular SPI controller. Slave mode operation is not also
 	  not supported.
 
+config SPI_BCM2835AUX
+	tristate "BCM2835 SPI auxiliar controller"
+	depends on ARCH_BCM2835 || COMPILE_TEST
+	help
+	  This selects a driver for the Broadcom BCM2835 SPI aux master.
+
+	  The BCM2835 contains two types of SPI master controller; the
+	  "universal SPI master", and the regular SPI controller. This driver
+	  is for the universal/auxiliar SPI controller.
+
 config SPI_BFIN5XX
 	tristate "SPI controller driver for ADI Blackfin5xx"
 	depends on BLACKFIN && !BF60x
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 1154dba..eed9614 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -15,6 +15,7 @@  obj-$(CONFIG_SPI_ATMEL)			+= spi-atmel.o
 obj-$(CONFIG_SPI_ATH79)			+= spi-ath79.o
 obj-$(CONFIG_SPI_AU1550)		+= spi-au1550.o
 obj-$(CONFIG_SPI_BCM2835)		+= spi-bcm2835.o
+obj-$(CONFIG_SPI_BCM2835AUX)		+= spi-bcm2835aux.o
 obj-$(CONFIG_SPI_BCM53XX)		+= spi-bcm53xx.o
 obj-$(CONFIG_SPI_BCM63XX)		+= spi-bcm63xx.o
 obj-$(CONFIG_SPI_BCM63XX_HSSPI)		+= spi-bcm63xx-hsspi.o
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
new file mode 100644
index 0000000..0e48ec5
--- /dev/null
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -0,0 +1,549 @@ 
+/*
+ * Driver for Broadcom BCM2835 SPI Controllers
+ *
+ * the driver does not rely on the native chipselects at all
+ * but only uses the gpio type chipselects
+ *
+ * Based on: spi-bcm2835.c
+ *
+ * Copyright (C) 2015 Martin Sperl
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
+#include <linux/spi/spi.h>
+#include <linux/spinlock.h>
+
+/*
+ * shared aux registers between spi1/spi2 and uart1
+ *
+ * these defines could go to a separate module if needed
+ * so that it can also get used with the uart1 implementation
+ * when it materializes.
+ */
+
+/* the AUX register offsets */
+#define BCM2835_AUX_IRQ		0x00
+#define BCM2835_AUX_ENABLE	0x04
+
+/* the AUX Bitfield identical for both register */
+#define BCM2835_AUX_BIT_UART	0x00000001
+#define BCM2835_AUX_BIT_SPI1	0x00000002
+#define BCM2835_AUX_BIT_SPI2	0x00000004
+
+/*
+ * spi register defines
+ *
+ * note there is garbage in the "official" documentation,
+ * so somedata taken from the file:
+ *   brcm_usrlib/dag/vmcsx/vcinclude/bcm2708_chip/aux_io.h
+ * inside of:
+ *   http://www.broadcom.com/docs/support/videocore/Brcm_Android_ICS_Graphics_Stack.tar.gz
+ */
+
+/* SPI register offsets */
+#define BCM2835_AUX_SPI_CNTL0	0x00
+#define BCM2835_AUX_SPI_CNTL1	0x04
+#define BCM2835_AUX_SPI_STAT	0x08
+#define BCM2835_AUX_SPI_PEEK	0x0C
+#define BCM2835_AUX_SPI_IO	0x20
+#define BCM2835_AUX_SPI_TXHOLD	0x30
+
+/* Bitfields in CNTL0 */
+#define BCM2835_AUX_SPI_CNTL0_SPEED	0xFFF00000
+#define BCM2835_AUX_SPI_CNTL0_SPEED_MAX	0xFFF
+#define BCM2835_AUX_SPI_CNTL0_SPEED_SHIFT	20
+#define BCM2835_AUX_SPI_CNTL0_CS	0x000E0000
+#define BCM2835_AUX_SPI_CNTL0_POSTINPUT	0x00010000
+#define BCM2835_AUX_SPI_CNTL0_VAR_CS	0x00008000
+#define BCM2835_AUX_SPI_CNTL0_VAR_WIDTH	0x00004000
+#define BCM2835_AUX_SPI_CNTL0_DOUTHOLD	0x00003000
+#define BCM2835_AUX_SPI_CNTL0_ENABLE	0x00000800
+#define BCM2835_AUX_SPI_CNTL0_CPHA_IN	0x00000400
+#define BCM2835_AUX_SPI_CNTL0_CLEARFIFO	0x00000200
+#define BCM2835_AUX_SPI_CNTL0_CPHA_OUT	0x00000100
+#define BCM2835_AUX_SPI_CNTL0_CPOL	0x00000080
+#define BCM2835_AUX_SPI_CNTL0_MSBF_OUT	0x00000040
+#define BCM2835_AUX_SPI_CNTL0_SHIFTLEN	0x0000003F
+
+/* Bitfields in CNTL1 */
+#define BCM2835_AUX_SPI_CNTL1_CSHIGH	0x00000700
+#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000080
+#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000040
+#define BCM2835_AUX_SPI_CNTL1_MSBF_IN	0x00000002
+#define BCM2835_AUX_SPI_CNTL1_KEEP_IN	0x00000001
+
+/* Bitfields in STAT */
+#define BCM2835_AUX_SPI_STAT_TX_LVL	0xFF000000
+#define BCM2835_AUX_SPI_STAT_RX_LVL	0x00FF0000
+#define BCM2835_AUX_SPI_STAT_TX_FULL	0x00000400
+#define BCM2835_AUX_SPI_STAT_TX_EMPTY	0x00000200
+#define BCM2835_AUX_SPI_STAT_RX_FULL	0x00000100
+#define BCM2835_AUX_SPI_STAT_RX_EMPTY	0x00000080
+#define BCM2835_AUX_SPI_STAT_BUSY	0x00000040
+#define BCM2835_AUX_SPI_STAT_BITCOUNT	0x0000003F
+
+/* timeout values */
+#define BCM2835_AUX_SPI_POLLING_LIMIT_US	30
+#define BCM2835_AUX_SPI_POLLING_JIFFIES		2
+
+#define BCM2835_AUX_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | SPI_CS_HIGH \
+				  | SPI_NO_CS)
+
+#define DRV_NAME	"spi-bcm2835aux"
+
+struct bcm2835aux_spi {
+	void __iomem *regs;
+	void __iomem *aux_regs;
+	struct clk *clk;
+	int irq;
+	u32 cntl[2];
+	const u8 *tx_buf;
+	u8 *rx_buf;
+	int tx_len;
+	int rx_len;
+};
+
+/* this function could go to a separate module if needed
+ * so that it can also get used with the uart1 implementation
+ */
+
+DEFINE_SPINLOCK(__bcm2835_aux_lock);
+static void bcm2835_aux_modify_enable(struct bcm2835aux_spi *bs,
+				      u32 mask, u32 val)
+{
+	unsigned long flags;
+	u32 r;
+
+	spin_lock_irqsave(&__bcm2835_aux_lock, flags);
+
+	r = readl(bs->aux_regs + BCM2835_AUX_ENABLE);
+	r &= mask;
+	r |= val;
+	writel(r, bs->aux_regs + BCM2835_AUX_ENABLE);
+
+	spin_unlock_irqrestore(&__bcm2835_aux_lock, flags);
+}
+
+static void bcm2835aux_spi_enable(struct bcm2835aux_spi *bs)
+{
+	/* identify the spi device - needed to activate the right HW-block */
+	u32 mask = (size_t)bs->regs & 0x00000040 ?
+		   BCM2835_AUX_BIT_SPI2 : BCM2835_AUX_BIT_SPI1;
+
+	bcm2835_aux_modify_enable(bs, ~mask, mask);
+}
+
+static void bcm2835aux_spi_disable(struct bcm2835aux_spi *bs)
+{
+	/* identify the spi device - needed to activate the right HW-block */
+	u32 mask = (size_t)bs->regs & 0x00000040 ?
+		   BCM2835_AUX_BIT_SPI2 : BCM2835_AUX_BIT_SPI1;
+
+	bcm2835_aux_modify_enable(bs, ~mask, 0);
+}
+
+static inline u32 bcm2835aux_rd(struct bcm2835aux_spi *bs, unsigned reg)
+{
+	return readl(bs->regs + reg);
+}
+
+static inline void bcm2835aux_wr(struct bcm2835aux_spi *bs, unsigned reg,
+				 u32 val)
+{
+	writel(val, bs->regs + reg);
+}
+
+static inline void bcm2835aux_rd_fifo(struct bcm2835aux_spi *bs)
+{
+	u32 data;
+	int i;
+	int count = min(bs->rx_len, 3);
+
+	data = bcm2835aux_rd(bs, BCM2835_AUX_SPI_IO);
+	if (bs->rx_buf) {
+		for (i = 0; i < count; i++)
+			*bs->rx_buf++ = (data >> (8 * (2 - i))) & 0xff;
+	}
+	bs->rx_len -= count;
+}
+
+static inline void bcm2835aux_wr_fifo(struct bcm2835aux_spi *bs)
+{
+	u32 data;
+	u8 byte;
+	int count;
+	int i;
+
+	/* gather up to 3 bytes to write to the FIFO */
+	count = min(bs->tx_len, 3);
+	data = 0;
+	for (i = 0; i < count; i++) {
+		byte = bs->tx_buf ? *bs->tx_buf++ : 0;
+		data |= byte << (8 * (2 - i));
+	}
+
+	/* and set the variable bit-length */
+	data |= (count * 8) << 24;
+
+	/* and decrement length */
+	bs->tx_len -= count;
+
+	/* write to the correct TX-register */
+	if (bs->tx_len)
+		bcm2835aux_wr(bs, BCM2835_AUX_SPI_TXHOLD, data);
+	else
+		bcm2835aux_wr(bs, BCM2835_AUX_SPI_IO, data);
+}
+
+static void bcm2835aux_spi_reset_hw(struct bcm2835aux_spi *bs)
+{
+	/* disable spi clearing fifo and interrupts */
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, 0);
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0,
+		      BCM2835_AUX_SPI_CNTL0_CLEARFIFO);
+}
+
+static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
+{
+	struct spi_master *master = dev_id;
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+	irqreturn_t ret = IRQ_NONE;
+
+	/* check if we have data to read */
+	while (bs->rx_len &&
+	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
+		  BCM2835_AUX_SPI_STAT_RX_EMPTY))) {
+		bcm2835aux_rd_fifo(bs);
+		ret = IRQ_HANDLED;
+	}
+
+	/* check if we have data to write */
+	while (bs->tx_len &&
+	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
+		  BCM2835_AUX_SPI_STAT_TX_FULL))) {
+		bcm2835aux_wr_fifo(bs);
+		ret = IRQ_HANDLED;
+	}
+
+	/* and check if we have reached "done" */
+	while (bs->rx_len &&
+	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
+		  BCM2835_AUX_SPI_STAT_BUSY))) {
+		bcm2835aux_rd_fifo(bs);
+		ret = IRQ_HANDLED;
+	}
+
+	/* and if rx_len is 0 then wake up completion and disable spi */
+	if (!bs->rx_len) {
+		bcm2835aux_spi_reset_hw(bs);
+		complete(&master->xfer_completion);
+	}
+
+	/* and return */
+	return ret;
+}
+
+static int __bcm2835aux_spi_transfer_one_irq(struct spi_master *master,
+					     struct spi_device *spi,
+					     struct spi_transfer *tfr)
+{
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	/* enable interrupts */
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1] |
+		BCM2835_AUX_SPI_CNTL1_TXEMPTY |
+		BCM2835_AUX_SPI_CNTL1_IDLE);
+
+	/* and wait for finish... */
+	return 1;
+}
+
+static int bcm2835aux_spi_transfer_one_irq(struct spi_master *master,
+					   struct spi_device *spi,
+					   struct spi_transfer *tfr)
+{
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	/* fill in registers and fifos before enabling interrupts */
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);
+
+	/* fill in tx fifo with data before enabling interrupts */
+	while ((bs->tx_len) &&
+	       (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) &
+		  BCM2835_AUX_SPI_STAT_TX_FULL))) {
+		bcm2835aux_wr_fifo(bs);
+	}
+
+	/* now run the interrupt mode */
+	return __bcm2835aux_spi_transfer_one_irq(master, spi, tfr);
+}
+
+static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
+					    struct spi_device *spi,
+					    struct spi_transfer *tfr,
+					    unsigned long xfer_time_us)
+{
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+	unsigned long timeout;
+	u32 stat;
+
+	/* configure spi */
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);
+
+	/* set the timeout */
+	timeout = jiffies + BCM2835_AUX_SPI_POLLING_JIFFIES;
+
+	/* loop until finished the transfer */
+	while (bs->rx_len) {
+		/* read status */
+		stat = bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT);
+
+		/* fill in tx fifo with remaining data */
+		if ((bs->tx_len) && (!(stat & BCM2835_AUX_SPI_STAT_TX_FULL))) {
+			bcm2835aux_wr_fifo(bs);
+			continue;
+		}
+
+		/* read data from fifo for both cases */
+		if (!(stat & BCM2835_AUX_SPI_STAT_RX_EMPTY)) {
+			bcm2835aux_rd_fifo(bs);
+			continue;
+		}
+		if (!(stat & BCM2835_AUX_SPI_STAT_BUSY)) {
+			bcm2835aux_rd_fifo(bs);
+			continue;
+		}
+
+		/* there is still data pending to read check the timeout */
+		if (bs->rx_len && time_after(jiffies, timeout)) {
+			dev_dbg_ratelimited(&spi->dev,
+					    "timeout period reached: jiffies: %lu remaining tx/rx: %d/%d - falling back to interrupt mode\n",
+					    jiffies - timeout,
+					    bs->tx_len, bs->rx_len);
+			/* forward to interrupt handler */
+			return __bcm2835aux_spi_transfer_one_irq(master,
+							       spi, tfr);
+		}
+	}
+
+	/* Transfer complete - reset SPI HW */
+	bcm2835aux_spi_reset_hw(bs);
+
+	/* and return without waiting for completion */
+	return 0;
+}
+
+static int bcm2835aux_spi_transfer_one(struct spi_master *master,
+				       struct spi_device *spi,
+				       struct spi_transfer *tfr)
+{
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+	unsigned long spi_hz, clk_hz, speed;
+	unsigned long spi_used_hz, xfer_time_us;
+
+	/* calculate the registers to handle
+	 *
+	 * note that we use the variable data mode, which
+	 * is not optimal for longer transfers as we waste registers
+	 * resulting (potentially) in more interrupts when transferring
+	 * more than 12 bytes
+	 */
+	bs->cntl[0] = BCM2835_AUX_SPI_CNTL0_ENABLE |
+		      BCM2835_AUX_SPI_CNTL0_VAR_WIDTH |
+		      BCM2835_AUX_SPI_CNTL0_MSBF_OUT;
+	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
+
+	/* set clock */
+	spi_hz = tfr->speed_hz;
+	clk_hz = clk_get_rate(bs->clk);
+
+	if (spi_hz >= clk_hz / 2) {
+		speed = 0;
+	} else if (spi_hz) {
+		speed = DIV_ROUND_UP(clk_hz, 2 * spi_hz) - 1;
+		if (speed >  BCM2835_AUX_SPI_CNTL0_SPEED_MAX)
+			speed = BCM2835_AUX_SPI_CNTL0_SPEED_MAX;
+	} else { /* the slowest we can go */
+		speed = BCM2835_AUX_SPI_CNTL0_SPEED_MAX;
+	}
+	bs->cntl[0] |= speed << BCM2835_AUX_SPI_CNTL0_SPEED_SHIFT;
+	spi_used_hz = clk_hz / (2 * (speed + 1));
+
+	/* handle all the modes */
+	if (spi->mode & SPI_CPOL)
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
+	if (spi->mode & SPI_CPHA)
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
+			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+
+	/* set transmit buffers and length */
+	bs->tx_buf = tfr->tx_buf;
+	bs->rx_buf = tfr->rx_buf;
+	bs->tx_len = tfr->len;
+	bs->rx_len = tfr->len;
+
+	/* calculate the estimated time in us the transfer runs */
+	xfer_time_us = tfr->len
+		* 9 /* clocks/byte - SPI-HW waits 1 clock after each byte */
+		* 1000000 / spi_used_hz;
+
+	/* run in polling mode for short transfers */
+	if (xfer_time_us < BCM2835_AUX_SPI_POLLING_LIMIT_US)
+		return bcm2835aux_spi_transfer_one_poll(master, spi, tfr,
+							xfer_time_us);
+
+	/* run in interrupt mode for all others */
+	return bcm2835aux_spi_transfer_one_irq(master, spi, tfr);
+}
+
+static void bcm2835aux_spi_handle_err(struct spi_master *master,
+				      struct spi_message *msg)
+{
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	bcm2835aux_spi_reset_hw(bs);
+}
+
+static int bcm2835aux_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct bcm2835aux_spi *bs;
+	struct resource *res;
+	int err;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*bs));
+	if (!master) {
+		dev_err(&pdev->dev, "spi_alloc_master() failed\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, master);
+	master->mode_bits = BCM2835_AUX_SPI_MODE_BITS;
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->num_chipselect = -1;
+	master->transfer_one = bcm2835aux_spi_transfer_one;
+	master->handle_err = bcm2835aux_spi_handle_err;
+	master->dev.of_node = pdev->dev.of_node;
+
+	bs = spi_master_get_devdata(master);
+
+	/* the main area */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	bs->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(bs->regs)) {
+		err = PTR_ERR(bs->regs);
+		goto out_master_put;
+	}
+	/* the aux area - slight hack...*/
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	bs->aux_regs = devm_ioremap(&pdev->dev, res->start,
+				    resource_size(res));
+	if (IS_ERR(bs->aux_regs)) {
+		err = PTR_ERR(bs->aux_regs);
+		goto out_master_put;
+	}
+
+	bs->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(bs->clk)) {
+		err = PTR_ERR(bs->clk);
+		dev_err(&pdev->dev, "could not get clk: %d\n", err);
+		goto out_master_put;
+	}
+	bs->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (bs->irq <= 0) {
+		dev_err(&pdev->dev, "could not get IRQ: %d\n", bs->irq);
+		err = bs->irq ? bs->irq : -ENODEV;
+		goto out_master_put;
+	}
+
+	clk_prepare_enable(bs->clk);
+
+	err = devm_request_irq(&pdev->dev, bs->irq,
+			       bcm2835aux_spi_interrupt,
+			       IRQF_SHARED,
+			       dev_name(&pdev->dev), master);
+	if (err) {
+		dev_err(&pdev->dev, "could not request IRQ: %d\n", err);
+		goto out_clk_disable;
+	}
+
+	/* enable HW block and reset it */
+	bcm2835aux_spi_enable(bs);
+	bcm2835aux_spi_reset_hw(bs);
+
+	err = devm_spi_register_master(&pdev->dev, master);
+	if (err) {
+		dev_err(&pdev->dev, "could not register SPI master: %d\n", err);
+		goto out_hw_disable;
+	}
+
+	return 0;
+
+out_hw_disable:
+	bcm2835aux_spi_disable(bs);
+out_clk_disable:
+	clk_disable_unprepare(bs->clk);
+out_master_put:
+	spi_master_put(master);
+	return err;
+}
+
+static int bcm2835aux_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	/* Clear FIFOs, and disable the HW block */
+	clk_disable_unprepare(bs->clk);
+
+	bcm2835aux_spi_reset_hw(bs);
+
+	/* disable HW block */
+	bcm2835aux_spi_disable(bs);
+
+	return 0;
+}
+
+static const struct of_device_id bcm2835aux_spi_match[] = {
+	{ .compatible = "brcm,bcm2835-aux-spi", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bcm2835aux_spi_match);
+
+static struct platform_driver bcm2835aux_spi_driver = {
+	.driver		= {
+		.name		= DRV_NAME,
+		.of_match_table	= bcm2835aux_spi_match,
+	},
+	.probe		= bcm2835aux_spi_probe,
+	.remove		= bcm2835aux_spi_remove,
+};
+module_platform_driver(bcm2835aux_spi_driver);
+
+MODULE_DESCRIPTION("SPI controller driver for Broadcom BCM2835 aux");
+MODULE_AUTHOR("Martin Sperl <kernel@martin.sperl.org>");
+MODULE_LICENSE("GPL v2");