Message ID | 1434980408-4086-1-git-send-email-kernel@martin.sperl.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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
> 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
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
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.
> 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
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
> 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
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
> 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
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
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
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
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
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
> 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
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
> 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
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
> 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
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
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
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
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
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
> 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
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
> 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
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
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 --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");