| Message ID | 1430935194-7579-7-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wednesday 06 May 2015 10:59:50 Brian Norris wrote: > + /* > + * Some SoCs integrate this controller (e.g., its interrupt bits) in > + * interesting ways > + */ > + if (of_property_read_bool(dn, "brcm,nand-soc")) { > + struct device_node *soc_dn; > + > + soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0); > + if (!soc_dn) > + return -ENODEV; > + > + ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn); > + if (!ctrl->soc) { > + dev_err(dev, "could not probe SoC data\n"); > + of_node_put(soc_dn); > + return -ENODEV; > + } > + > + ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0, > + DRV_NAME, ctrl); > + > + /* Enable interrupt */ > + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); > + > + of_node_put(soc_dn); > + } else { > + /* Use standard interrupt infrastructure */ > + ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, > + DRV_NAME, ctrl); > + } > It looks to me like this should be handled as a nested irqchip, so the node you look up gets used as the "interrupt-parent" instead, making the behavior of this SoC transparent to the nand driver. We recently merged nested irqdomain support as well, which might help here, or might not be needed. Arnd
On Wed, May 06, 2015 at 09:12:43PM +0200, Arnd Bergmann wrote: > On Wednesday 06 May 2015 10:59:50 Brian Norris wrote: > > + /* > > + * Some SoCs integrate this controller (e.g., its interrupt bits) in > > + * interesting ways > > + */ > > + if (of_property_read_bool(dn, "brcm,nand-soc")) { > > + struct device_node *soc_dn; > > + > > + soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0); > > + if (!soc_dn) > > + return -ENODEV; > > + > > + ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn); > > + if (!ctrl->soc) { > > + dev_err(dev, "could not probe SoC data\n"); > > + of_node_put(soc_dn); > > + return -ENODEV; > > + } > > + > > + ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0, > > + DRV_NAME, ctrl); > > + > > + /* Enable interrupt */ > > + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); > > + > > + of_node_put(soc_dn); > > + } else { > > + /* Use standard interrupt infrastructure */ > > + ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, > > + DRV_NAME, ctrl); > > + } > > > > It looks to me like this should be handled as a nested irqchip, so the node > you look up gets used as the "interrupt-parent" instead, making the behavior > of this SoC transparent to the nand driver. You snipped the rest of the patch, which involves more than just IRQ handling. The same registers touch both interrupts and data bus endian configuration, so it can't possibly be done transparently to the NAND driver. > We recently merged nested irqdomain support as well, which might help here, > or might not be needed. I'm not familiar with nested irqdomains. Do they address anything like the above problem? Brian
On 2015-05-06 04:49 PM, Brian Norris wrote: > On Wed, May 06, 2015 at 09:12:43PM +0200, Arnd Bergmann wrote: >> On Wednesday 06 May 2015 10:59:50 Brian Norris wrote: >>> + /* >>> + * Some SoCs integrate this controller (e.g., its interrupt bits) in >>> + * interesting ways >>> + */ >>> + if (of_property_read_bool(dn, "brcm,nand-soc")) { >>> + struct device_node *soc_dn; >>> + >>> + soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0); >>> + if (!soc_dn) >>> + return -ENODEV; >>> + >>> + ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn); >>> + if (!ctrl->soc) { >>> + dev_err(dev, "could not probe SoC data\n"); >>> + of_node_put(soc_dn); >>> + return -ENODEV; >>> + } >>> + >>> + ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0, >>> + DRV_NAME, ctrl); >>> + >>> + /* Enable interrupt */ >>> + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); >>> + >>> + of_node_put(soc_dn); >>> + } else { >>> + /* Use standard interrupt infrastructure */ >>> + ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, >>> + DRV_NAME, ctrl); >>> + } >>> >> >> It looks to me like this should be handled as a nested irqchip, so the node >> you look up gets used as the "interrupt-parent" instead, making the behavior >> of this SoC transparent to the nand driver. > > You snipped the rest of the patch, which involves more than just IRQ > handling. The same registers touch both interrupts and data bus endian > configuration, so it can't possibly be done transparently to the NAND > driver. > >> We recently merged nested irqdomain support as well, which might help here, >> or might not be needed. > > I'm not familiar with nested irqdomains. Do they address anything like > the above problem? > > Brian > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > Brian, I don't known about them myself :). However always check the Documentation folder of the kernel root for things like this. Further more I already checked there and it appears there is a file explaining the basics entitled, IRQ-domain.txt. Hope this helps, Nick
On Wednesday 06 May 2015 13:49:10 Brian Norris wrote: > On Wed, May 06, 2015 at 09:12:43PM +0200, Arnd Bergmann wrote: > > On Wednesday 06 May 2015 10:59:50 Brian Norris wrote: > > > + /* > > > + * Some SoCs integrate this controller (e.g., its interrupt bits) in > > > + * interesting ways > > > + */ > > > + if (of_property_read_bool(dn, "brcm,nand-soc")) { > > > + struct device_node *soc_dn; > > > + > > > + soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0); > > > + if (!soc_dn) > > > + return -ENODEV; > > > + > > > + ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn); > > > + if (!ctrl->soc) { > > > + dev_err(dev, "could not probe SoC data\n"); > > > + of_node_put(soc_dn); > > > + return -ENODEV; > > > + } > > > + > > > + ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0, > > > + DRV_NAME, ctrl); > > > + > > > + /* Enable interrupt */ > > > + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); > > > + > > > + of_node_put(soc_dn); > > > + } else { > > > + /* Use standard interrupt infrastructure */ > > > + ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, > > > + DRV_NAME, ctrl); > > > + } > > > > > > > It looks to me like this should be handled as a nested irqchip, so the node > > you look up gets used as the "interrupt-parent" instead, making the behavior > > of this SoC transparent to the nand driver. > > You snipped the rest of the patch, which involves more than just IRQ > handling. The same registers touch both interrupts and data bus endian > configuration, so it can't possibly be done transparently to the NAND > driver. Anything else in there? The bus configuration would just involve writing a constant value in some register, right? Doing that in the irqchip is also a bit ugly, but may still be better overall than doing it the way you have above. > > We recently merged nested irqdomain support as well, which might help here, > > or might not be needed. > > I'm not familiar with nested irqdomains. Do they address anything like > the above problem? The problem that nested irqdomains address is when an interrupt is handled by two irqchips, in particular when one irqchip handles a virtual interrupt number that was claimed by another irqchip already. Arnd
On Thu, May 07, 2015 at 12:01:02PM +0200, Arnd Bergmann wrote: > On Wednesday 06 May 2015 13:49:10 Brian Norris wrote: > > On Wed, May 06, 2015 at 09:12:43PM +0200, Arnd Bergmann wrote: > > > On Wednesday 06 May 2015 10:59:50 Brian Norris wrote: > > > > + /* > > > > + * Some SoCs integrate this controller (e.g., its interrupt bits) in > > > > + * interesting ways > > > > + */ > > > > + if (of_property_read_bool(dn, "brcm,nand-soc")) { > > > > + struct device_node *soc_dn; > > > > + > > > > + soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0); > > > > + if (!soc_dn) > > > > + return -ENODEV; > > > > + > > > > + ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn); > > > > + if (!ctrl->soc) { > > > > + dev_err(dev, "could not probe SoC data\n"); > > > > + of_node_put(soc_dn); > > > > + return -ENODEV; > > > > + } > > > > + > > > > + ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0, > > > > + DRV_NAME, ctrl); > > > > + > > > > + /* Enable interrupt */ > > > > + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); > > > > + > > > > + of_node_put(soc_dn); > > > > + } else { > > > > + /* Use standard interrupt infrastructure */ > > > > + ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, > > > > + DRV_NAME, ctrl); > > > > + } > > > > > > > > > > It looks to me like this should be handled as a nested irqchip, so the node > > > you look up gets used as the "interrupt-parent" instead, making the behavior > > > of this SoC transparent to the nand driver. > > > > You snipped the rest of the patch, which involves more than just IRQ > > handling. The same registers touch both interrupts and data bus endian > > configuration, so it can't possibly be done transparently to the NAND > > driver. > > Anything else in there? Looks like miscellaneous NAND-related control bits. AXI and APB endian configuration; several interrupt-enable bits (we only use one for now); a clock-enable; and some timing test mode bits. > The bus configuration would just involve writing > a constant value in some register, right? I'm not an expert on the Cygnus/iProc chips, but I believe the answer is no: we *must* reconfigure the bus before and after each data transaction, because it affects the rest of the core too. Others on the CC list can probably elaborate. > Doing that in the irqchip > is also a bit ugly, but may still be better overall than doing it the > way you have above. Well, the Cygnus/iProc case is more complicated as I mention. But I agree that other cases could be nicer, like bcm63138 (which only has separate interrupt status/enable registers). Do you expect a new irqchip driver for every arrangement of registers like this? There are a few similar chips that have status/enable registers in different orders, and some that combine them into a single word. Do we really need a new irqchip driver for each one? I might have done that for bcm63138 and bcm3384, except that I thought I'd have to fall back to this extra per-SoC support driver for Cygnus anyway. > > > We recently merged nested irqdomain support as well, which might help here, > > > or might not be needed. > > > > I'm not familiar with nested irqdomains. Do they address anything like > > the above problem? > > The problem that nested irqdomains address is when an interrupt is handled > by two irqchips, in particular when one irqchip handles a virtual interrupt > number that was claimed by another irqchip already. I'll do some reading on that, but it definitely doesn't address the main problem here. Brian
On 5/7/2015 11:42 AM, Brian Norris wrote: > On Thu, May 07, 2015 at 12:01:02PM +0200, Arnd Bergmann wrote: >> On Wednesday 06 May 2015 13:49:10 Brian Norris wrote: >>> On Wed, May 06, 2015 at 09:12:43PM +0200, Arnd Bergmann wrote: >>>> On Wednesday 06 May 2015 10:59:50 Brian Norris wrote: >>>>> + /* >>>>> + * Some SoCs integrate this controller (e.g., its interrupt bits) in >>>>> + * interesting ways >>>>> + */ >>>>> + if (of_property_read_bool(dn, "brcm,nand-soc")) { >>>>> + struct device_node *soc_dn; >>>>> + >>>>> + soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0); >>>>> + if (!soc_dn) >>>>> + return -ENODEV; >>>>> + >>>>> + ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn); >>>>> + if (!ctrl->soc) { >>>>> + dev_err(dev, "could not probe SoC data\n"); >>>>> + of_node_put(soc_dn); >>>>> + return -ENODEV; >>>>> + } >>>>> + >>>>> + ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0, >>>>> + DRV_NAME, ctrl); >>>>> + >>>>> + /* Enable interrupt */ >>>>> + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); >>>>> + >>>>> + of_node_put(soc_dn); >>>>> + } else { >>>>> + /* Use standard interrupt infrastructure */ >>>>> + ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, >>>>> + DRV_NAME, ctrl); >>>>> + } >>>>> >>>> >>>> It looks to me like this should be handled as a nested irqchip, so the node >>>> you look up gets used as the "interrupt-parent" instead, making the behavior >>>> of this SoC transparent to the nand driver. >>> >>> You snipped the rest of the patch, which involves more than just IRQ >>> handling. The same registers touch both interrupts and data bus endian >>> configuration, so it can't possibly be done transparently to the NAND >>> driver. >> >> Anything else in there? > > Looks like miscellaneous NAND-related control bits. AXI and APB endian > configuration; several interrupt-enable bits (we only use one for now); > a clock-enable; and some timing test mode bits. > >> The bus configuration would just involve writing >> a constant value in some register, right? > > I'm not an expert on the Cygnus/iProc chips, but I believe the answer is > no: we *must* reconfigure the bus before and after each data > transaction, because it affects the rest of the core too. Others on the > CC list can probably elaborate. > Yes, we must configure the bus before the after each data/cache registers access, because it changes the APB bus endianess. Thanks, Ray
On 07/05/15 03:01, Arnd Bergmann wrote: > On Wednesday 06 May 2015 13:49:10 Brian Norris wrote: >> On Wed, May 06, 2015 at 09:12:43PM +0200, Arnd Bergmann wrote: >>> On Wednesday 06 May 2015 10:59:50 Brian Norris wrote: >>>> + /* >>>> + * Some SoCs integrate this controller (e.g., its interrupt bits) in >>>> + * interesting ways >>>> + */ >>>> + if (of_property_read_bool(dn, "brcm,nand-soc")) { >>>> + struct device_node *soc_dn; >>>> + >>>> + soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0); >>>> + if (!soc_dn) >>>> + return -ENODEV; >>>> + >>>> + ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn); >>>> + if (!ctrl->soc) { >>>> + dev_err(dev, "could not probe SoC data\n"); >>>> + of_node_put(soc_dn); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0, >>>> + DRV_NAME, ctrl); >>>> + >>>> + /* Enable interrupt */ >>>> + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); >>>> + >>>> + of_node_put(soc_dn); >>>> + } else { >>>> + /* Use standard interrupt infrastructure */ >>>> + ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, >>>> + DRV_NAME, ctrl); >>>> + } >>>> >>> >>> It looks to me like this should be handled as a nested irqchip, so the node >>> you look up gets used as the "interrupt-parent" instead, making the behavior >>> of this SoC transparent to the nand driver. >> >> You snipped the rest of the patch, which involves more than just IRQ >> handling. The same registers touch both interrupts and data bus endian >> configuration, so it can't possibly be done transparently to the NAND >> driver. > > Anything else in there? The bus configuration would just involve writing > a constant value in some register, right? Doing that in the irqchip > is also a bit ugly, but may still be better overall than doing it the > way you have above. IMHO this is uglier, having a pseudo interrupt controller driver that also takes care of preparing bus transfers, as opposed to an ad-hoc piece of code that does not pretend to be an interrupt controller at all sounds like the former is lying about what it is, while the latter is pretty straight forward even though it may duplicate an existing subsystem. I would definitively see some value in writing an irqchip driver if this was remotely useful outside of the NAND block, e.g: the interrupt bits would serve other peripherals than NAND, which is not the case for 63138 and 338x AFAICT, Cygnus is a special case. I could very well imagine a near future where this controller gets added more features in the DMA/data-path that may require bus/chip-level glue code to be interfaced properly between Broadcom's different internal buses. In which case, the interrupt controller portion of the code could be much smaller than the bus/data-path logic, would it still make sense to pretend this to be an interrupt controller?
On Thursday 07 May 2015 11:42:46 Brian Norris wrote: > On Thu, May 07, 2015 at 12:01:02PM +0200, Arnd Bergmann wrote: > > On Wednesday 06 May 2015 13:49:10 Brian Norris wrote: > > > On Wed, May 06, 2015 at 09:12:43PM +0200, Arnd Bergmann wrote: > > > > It looks to me like this should be handled as a nested irqchip, so the node > > > > you look up gets used as the "interrupt-parent" instead, making the behavior > > > > of this SoC transparent to the nand driver. > > > > > > You snipped the rest of the patch, which involves more than just IRQ > > > handling. The same registers touch both interrupts and data bus endian > > > configuration, so it can't possibly be done transparently to the NAND > > > driver. > > > > Anything else in there? > > Looks like miscellaneous NAND-related control bits. AXI and APB endian > configuration; several interrupt-enable bits (we only use one for now); > a clock-enable; and some timing test mode bits. I see. Describing that as an irqchip would indeed be too much of a stretch. > > The bus configuration would just involve writing > > a constant value in some register, right? > > I'm not an expert on the Cygnus/iProc chips, but I believe the answer is > no: we *must* reconfigure the bus before and after each data > transaction, because it affects the rest of the core too. Others on the > CC list can probably elaborate. That would not be a problem I think: the irqchip driver would always get initialized first, because the main driver would get an -EPROBE_DEFER when requesting the interrupt line otherwise. > > Doing that in the irqchip > > is also a bit ugly, but may still be better overall than doing it the > > way you have above. > > Well, the Cygnus/iProc case is more complicated as I mention. But I > agree that other cases could be nicer, like bcm63138 (which only has > separate interrupt status/enable registers). Do you expect a new irqchip > driver for every arrangement of registers like this? There are a few > similar chips that have status/enable registers in different orders, and > some that combine them into a single word. Do we really need a new > irqchip driver for each one? I might have done that for bcm63138 and > bcm3384, except that I thought I'd have to fall back to this extra > per-SoC support driver for Cygnus anyway. I assumed this one was the only odd one. > > > > We recently merged nested irqdomain support as well, which might help here, > > > > or might not be needed. > > > > > > I'm not familiar with nested irqdomains. Do they address anything like > > > the above problem? > > > > The problem that nested irqdomains address is when an interrupt is handled > > by two irqchips, in particular when one irqchip handles a virtual interrupt > > number that was claimed by another irqchip already. > > I'll do some reading on that, but it definitely doesn't address the main > problem here. Ok, back to the drawing board then: How about turning the probe order around and splitting the SoC-specific part out into its own platform_driver: Instead of bus_prepare/bus_unprepare for bcm63138, you'd get a bcm63138_nand_driver with its own probe() function that calls the common probe function. That would make the soc specific parts better contained and match how we normally do abstractions of similar drivers. Arnd
On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote: > On Thursday 07 May 2015 11:42:46 Brian Norris wrote: > > On Thu, May 07, 2015 at 12:01:02PM +0200, Arnd Bergmann wrote: > > > The bus configuration would just involve writing > > > a constant value in some register, right? > > > > I'm not an expert on the Cygnus/iProc chips, but I believe the answer is > > no: we *must* reconfigure the bus before and after each data > > transaction, because it affects the rest of the core too. Others on the > > CC list can probably elaborate. > > That would not be a problem I think: the irqchip driver would always > get initialized first, because the main driver would get an -EPROBE_DEFER > when requesting the interrupt line otherwise. Huh? I wasn't worried about initialization order. I was worried about the fact that the "NAND" and "SoC" portions are very integrated when handling the data path. And I think you agreed that this means we can't do a straight-up irqchip. FWIW, I agree that -EPROBE_DEFER can handle initialization ordering issues... > > > Doing that in the irqchip > > > is also a bit ugly, but may still be better overall than doing it the > > > way you have above. > > > > Well, the Cygnus/iProc case is more complicated as I mention. But I > > agree that other cases could be nicer, like bcm63138 (which only has > > separate interrupt status/enable registers). Do you expect a new irqchip > > driver for every arrangement of registers like this? There are a few > > similar chips that have status/enable registers in different orders, and > > some that combine them into a single word. Do we really need a new > > irqchip driver for each one? I might have done that for bcm63138 and > > bcm3384, except that I thought I'd have to fall back to this extra > > per-SoC support driver for Cygnus anyway. > > I assumed this one was the only odd one. Yes, the Cygnus/iProc are the oddest. The others (BCM33xx (not yet supported) and BCM63xxx) just have separate one-off interrupt register blocks. To be clear, since I'm not sure if you're confused below: * Cygnus is a family of chips using the IPROC architecture, coming from the Infrastructure/Networking Group; there are BCMxxxx numbers noted in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to the Cygnus family or the IPROC architecture. * BCM63xxx is a class of DSL chips from the Broadband/Connectivity Group. > > > > > We recently merged nested irqdomain support as well, which might help here, > > > > > or might not be needed. > > > > > > > > I'm not familiar with nested irqdomains. Do they address anything like > > > > the above problem? > > > > > > The problem that nested irqdomains address is when an interrupt is handled > > > by two irqchips, in particular when one irqchip handles a virtual interrupt > > > number that was claimed by another irqchip already. > > > > I'll do some reading on that, but it definitely doesn't address the main > > problem here. > > Ok, back to the drawing board then: How about turning the probe order around > and splitting the SoC-specific part out into its own platform_driver: > > Instead of bus_prepare/bus_unprepare for bcm63138, you'd get a bcm63138 does not need the bus_{,un}prepare. That's for IPROC/Cygnus. > bcm63138_nand_driver with its own probe() function that calls the > common probe function. That would make the soc specific parts > better contained and match how we normally do abstractions of > similar drivers. OK, so I can imagine this might require changing the DT binding a bit [1] (is that your goal?). But what's the intended software difference? [2] I'll still be passing around the same sorts of callbacks from the 'iproc_nand' probe to the common probe function. Brian [1] e.g.: nand: nand@18046000 { compatible = "brcm,iproc-nand", "brcm,brcmnand-v6.1", "brcm,brcmnand"; reg = <0x18046000 0x600>, <0xf8105408 0x600>, <0x18046f00 0x20>; reg-names = "nand", "iproc-idm", "iproc-ext"; interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>; #address-cells = <1>; #size-cells = <0>; brcm,nand-has-wp; }; This captures the extra "iproc-*" register ranges. Then we could have the iproc_nand driver bind against "brcm,iproc-nand", then call into the common probe, which would then accept/reject based on "brcm,brcmnand-vX.Y". [2] The DT structure from [1] could actually accommodate either driver structure just fine. So maybe that means it's a better hardware description?
On Friday 08 May 2015 12:38:50 Brian Norris wrote: > On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote: > > On Thursday 07 May 2015 11:42:46 Brian Norris wrote: > > > On Thu, May 07, 2015 at 12:01:02PM +0200, Arnd Bergmann wrote: > > > > The bus configuration would just involve writing > > > > a constant value in some register, right? > > > > > > I'm not an expert on the Cygnus/iProc chips, but I believe the answer is > > > no: we *must* reconfigure the bus before and after each data > > > transaction, because it affects the rest of the core too. Others on the > > > CC list can probably elaborate. > > > > That would not be a problem I think: the irqchip driver would always > > get initialized first, because the main driver would get an -EPROBE_DEFER > > when requesting the interrupt line otherwise. > > Huh? I wasn't worried about initialization order. I was worried about > the fact that the "NAND" and "SoC" portions are very integrated when > handling the data path. And I think you agreed that this means we can't > do a straight-up irqchip. Sorry, I missed the part about "each data transaction", got it now. > > > > Doing that in the irqchip > > > > is also a bit ugly, but may still be better overall than doing it the > > > > way you have above. > > > > > > Well, the Cygnus/iProc case is more complicated as I mention. But I > > > agree that other cases could be nicer, like bcm63138 (which only has > > > separate interrupt status/enable registers). Do you expect a new irqchip > > > driver for every arrangement of registers like this? There are a few > > > similar chips that have status/enable registers in different orders, and > > > some that combine them into a single word. Do we really need a new > > > irqchip driver for each one? I might have done that for bcm63138 and > > > bcm3384, except that I thought I'd have to fall back to this extra > > > per-SoC support driver for Cygnus anyway. > > > > I assumed this one was the only odd one. > > Yes, the Cygnus/iProc are the oddest. The others (BCM33xx (not yet > supported) and BCM63xxx) just have separate one-off interrupt register > blocks. > > To be clear, since I'm not sure if you're confused below: > > * Cygnus is a family of chips using the IPROC architecture, coming from > the Infrastructure/Networking Group; there are BCMxxxx numbers noted > in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to > the Cygnus family or the IPROC architecture. > > * BCM63xxx is a class of DSL chips from the Broadband/Connectivity > Group. Thanks for the clarification, I think that is roughly what I thought it was, but I'm still not sure about brcmstb. Is that related to bcm63xxx or separate? > > > > > > We recently merged nested irqdomain support as well, which might help here, > > > > > > or might not be needed. > > > > > > > > > > I'm not familiar with nested irqdomains. Do they address anything like > > > > > the above problem? > > > > > > > > The problem that nested irqdomains address is when an interrupt is handled > > > > by two irqchips, in particular when one irqchip handles a virtual interrupt > > > > number that was claimed by another irqchip already. > > > > > > I'll do some reading on that, but it definitely doesn't address the main > > > problem here. > > > > Ok, back to the drawing board then: How about turning the probe order around > > and splitting the SoC-specific part out into its own platform_driver: > > > > Instead of bus_prepare/bus_unprepare for bcm63138, you'd get a > > bcm63138 does not need the bus_{,un}prepare. That's for IPROC/Cygnus. Ok. > > bcm63138_nand_driver with its own probe() function that calls the > > common probe function. That would make the soc specific parts > > better contained and match how we normally do abstractions of > > similar drivers. > > OK, so I can imagine this might require changing the DT binding a bit [1] > (is that your goal?). But what's the intended software difference? [2] > I'll still be passing around the same sorts of callbacks from the > 'iproc_nand' probe to the common probe function. > > Brian > > [1] e.g.: > > nand: nand@18046000 { > compatible = "brcm,iproc-nand", "brcm,brcmnand-v6.1", "brcm,brcmnand"; > reg = <0x18046000 0x600>, <0xf8105408 0x600>, <0x18046f00 0x20>; > reg-names = "nand", "iproc-idm", "iproc-ext"; > interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>; > > #address-cells = <1>; > #size-cells = <0>; > > brcm,nand-has-wp; > }; > > This captures the extra "iproc-*" register ranges. Then we could have > the iproc_nand driver bind against "brcm,iproc-nand", then call into the > common probe, which would then accept/reject based on > "brcm,brcmnand-vX.Y". > > [2] The DT structure from [1] could actually accommodate either driver > structure just fine. So maybe that means it's a better hardware > description? Yes, I think this makes sense overall. Regarding the specific example, can you clarify how the register areas in iproc are structured? The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers of two, which often indicates that they are part of some other, larger, unit that might need to have a driver of its own, so before we specify a binding like the one you proposed above I'd like to make sure we're not getting ourselves into trouble later. Arnd
On Fri, May 08, 2015 at 09:49:02PM +0200, Arnd Bergmann wrote: > On Friday 08 May 2015 12:38:50 Brian Norris wrote: > > On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote: [...] > > To be clear, since I'm not sure if you're confused below: > > > > * Cygnus is a family of chips using the IPROC architecture, coming from > > the Infrastructure/Networking Group; there are BCMxxxx numbers noted > > in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to > > the Cygnus family or the IPROC architecture. > > > > * BCM63xxx is a class of DSL chips from the Broadband/Connectivity > > Group. > > Thanks for the clarification, I think that is roughly what I thought it was, > but I'm still not sure about brcmstb. Is that related to bcm63xxx or separate? I think arch/arm/mach-bcm/Kconfig has the best summary. brcmstb is separate; BCM7xxx is generally (always?) Set-Top Box. Another potentially confusing point: the main driver is named 'brcsmtb_nand' since the NAND core (and driver) originated from STB chips. But that core was applied to other non-STB chips, and so the driver has been extended. > > > bcm63138_nand_driver with its own probe() function that calls the > > > common probe function. That would make the soc specific parts > > > better contained and match how we normally do abstractions of > > > similar drivers. > > > > OK, so I can imagine this might require changing the DT binding a bit [1] > > (is that your goal?). But what's the intended software difference? [2] > > I'll still be passing around the same sorts of callbacks from the > > 'iproc_nand' probe to the common probe function. ^^ before getting bogged down on the DT details (which can be changed independently), I'd like to address this point. > > Brian > > > > [1] e.g.: > > > > nand: nand@18046000 { > > compatible = "brcm,iproc-nand", "brcm,brcmnand-v6.1", "brcm,brcmnand"; > > reg = <0x18046000 0x600>, <0xf8105408 0x600>, <0x18046f00 0x20>; > > reg-names = "nand", "iproc-idm", "iproc-ext"; > > interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>; > > > > #address-cells = <1>; > > #size-cells = <0>; > > > > brcm,nand-has-wp; > > }; > > > > This captures the extra "iproc-*" register ranges. Then we could have > > the iproc_nand driver bind against "brcm,iproc-nand", then call into the > > common probe, which would then accept/reject based on > > "brcm,brcmnand-vX.Y". > > > > [2] The DT structure from [1] could actually accommodate either driver > > structure just fine. So maybe that means it's a better hardware > > description? > > Yes, I think this makes sense overall. Regarding the specific example, can you > clarify how the register areas in iproc are structured? > > The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers > of two, which often indicates that they are part of some other, larger, > unit that might need to have a driver of its own, so before we specify > a binding like the one you proposed above I'd like to make sure we're not > getting ourselves into trouble later. I may want the Cygnus guys to speak up here, partly for technical expertise and partly to know how much they care to share... <0xf8105408 0x600>: covers a series of NAND_IDM registers. NAND has a few bits we don't care about (for debugging, logging, and resetting), as well as its interrupt enable bits. The adjacent blocks cover similar IDM blocks for other cores (SPI, PNOR, DDR), and they are similarly unaligned. Not sure why, exactly; probably just a compact layout. <0x18046f00 0x20>: a series of 8 NAND interrupt registers, each word containing a single bit representing status/clear. There is nothing between the "nand" range and this range, and the SPI core register range follows. So I think these are pretty clearly-delineated register ranges for NAND, and the alignment is not really missing anything. Adjacent hardware (e.g., SPI) is independent, though pieces look similar. For one, it has similar: * interrupt enable bits in the IDM range (0xf8106408 to 0xf8106a00); and * interrupt status/clear following the SPI block (0x180473a0 to 0x180473b8) Brian
On Friday 08 May 2015 13:47:25 Brian Norris wrote: > On Fri, May 08, 2015 at 09:49:02PM +0200, Arnd Bergmann wrote: > > On Friday 08 May 2015 12:38:50 Brian Norris wrote: > > > On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote: > [...] > > > > To be clear, since I'm not sure if you're confused below: > > > > > > * Cygnus is a family of chips using the IPROC architecture, coming from > > > the Infrastructure/Networking Group; there are BCMxxxx numbers noted > > > in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to > > > the Cygnus family or the IPROC architecture. > > > > > > * BCM63xxx is a class of DSL chips from the Broadband/Connectivity > > > Group. > > > > Thanks for the clarification, I think that is roughly what I thought it was, > > but I'm still not sure about brcmstb. Is that related to bcm63xxx or separate? > > I think arch/arm/mach-bcm/Kconfig has the best summary. brcmstb is > separate; BCM7xxx is generally (always?) Set-Top Box. > > Another potentially confusing point: the main driver is named > 'brcsmtb_nand' since the NAND core (and driver) originated from STB > chips. But that core was applied to other non-STB chips, and so the > driver has been extended. Ok, I see. > > > > bcm63138_nand_driver with its own probe() function that calls the > > > > common probe function. That would make the soc specific parts > > > > better contained and match how we normally do abstractions of > > > > similar drivers. > > > > > > OK, so I can imagine this might require changing the DT binding a bit [1] > > > (is that your goal?). But what's the intended software difference? [2] > > > I'll still be passing around the same sorts of callbacks from the > > > 'iproc_nand' probe to the common probe function. > > ^^ before getting bogged down on the DT details (which can be changed > independently), I'd like to address this point. The intended change is to make it work according to Documentation/driver-model/design-patterns.txt basically, by having all the shared code be a "library" module that gets called by the actual hardware specific drivers, rather than having the shared code be the central driver that fans out into all possible subdrivers. > > > > Yes, I think this makes sense overall. Regarding the specific example, can you > > clarify how the register areas in iproc are structured? > > > > The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers > > of two, which often indicates that they are part of some other, larger, > > unit that might need to have a driver of its own, so before we specify > > a binding like the one you proposed above I'd like to make sure we're not > > getting ourselves into trouble later. > > I may want the Cygnus guys to speak up here, partly for technical > expertise and partly to know how much they care to share... > > <0xf8105408 0x600>: covers a series of NAND_IDM registers. NAND has a > few bits we don't care about (for debugging, logging, and resetting), as > well as its interrupt enable bits. The adjacent blocks cover similar IDM > blocks for other cores (SPI, PNOR, DDR), and they are similarly > unaligned. Not sure why, exactly; probably just a compact layout. > > <0x18046f00 0x20>: a series of 8 NAND interrupt registers, each word > containing a single bit representing status/clear. There is nothing > between the "nand" range and this range, and the SPI core register range > follows. > > So I think these are pretty clearly-delineated register ranges for NAND, > and the alignment is not really missing anything. Adjacent hardware > (e.g., SPI) is independent, though pieces look similar. For one, it has > similar: > > * interrupt enable bits in the IDM range (0xf8106408 to 0xf8106a00); > and > * interrupt status/clear following the SPI block (0x180473a0 to > 0x180473b8) This would in turn indicate that we should treat these ranges as an irqchip that handles all sorts of devices, but it really depends on the particular register layout. Arnd
On Fri, May 08, 2015 at 11:38:11PM +0200, Arnd Bergmann wrote: > On Friday 08 May 2015 13:47:25 Brian Norris wrote: > > On Fri, May 08, 2015 at 09:49:02PM +0200, Arnd Bergmann wrote: > > > On Friday 08 May 2015 12:38:50 Brian Norris wrote: > > > > On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote: > > > > > bcm63138_nand_driver with its own probe() function that calls the > > > > > common probe function. That would make the soc specific parts > > > > > better contained and match how we normally do abstractions of > > > > > similar drivers. > > > > > > > > OK, so I can imagine this might require changing the DT binding a bit [1] > > > > (is that your goal?). But what's the intended software difference? [2] > > > > I'll still be passing around the same sorts of callbacks from the > > > > 'iproc_nand' probe to the common probe function. > > > > ^^ before getting bogged down on the DT details (which can be changed > > independently), I'd like to address this point. > > The intended change is to make it work according to > Documentation/driver-model/design-patterns.txt Huh? There are two bullet points in that file, and neither are particularly enlightening for this case. Maybe you're referring to your mental design patterns documentation? :) > basically, by having all the shared code be a "library" module that gets > called by the actual hardware specific drivers, rather than having the > shared code be the central driver that fans out into all possible subdrivers. OK, I'll see what I can do. It will be a fairly opaque "library" though, consisting largely of a single monolithic core driver. Might just move to a whole drivers/mtd/nand/brcmnand/ subdirectory at the same time... > > > Yes, I think this makes sense overall. Regarding the specific example, can you > > > clarify how the register areas in iproc are structured? > > > > > > The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers > > > of two, which often indicates that they are part of some other, larger, > > > unit that might need to have a driver of its own, so before we specify > > > a binding like the one you proposed above I'd like to make sure we're not > > > getting ourselves into trouble later. > > > > I may want the Cygnus guys to speak up here, partly for technical > > expertise and partly to know how much they care to share... > > > > <0xf8105408 0x600>: covers a series of NAND_IDM registers. NAND has a > > few bits we don't care about (for debugging, logging, and resetting), as > > well as its interrupt enable bits. The adjacent blocks cover similar IDM > > blocks for other cores (SPI, PNOR, DDR), and they are similarly > > unaligned. Not sure why, exactly; probably just a compact layout. > > > > <0x18046f00 0x20>: a series of 8 NAND interrupt registers, each word > > containing a single bit representing status/clear. There is nothing > > between the "nand" range and this range, and the SPI core register range > > follows. > > > > So I think these are pretty clearly-delineated register ranges for NAND, > > and the alignment is not really missing anything. Adjacent hardware > > (e.g., SPI) is independent, though pieces look similar. For one, it has > > similar: > > > > * interrupt enable bits in the IDM range (0xf8106408 to 0xf8106a00); > > and > > * interrupt status/clear following the SPI block (0x180473a0 to > > 0x180473b8) > > This would in turn indicate that we should treat these ranges as > an irqchip that handles all sorts of devices, but it really depends > on the particular register layout. OK, sure. But this has nothing to do with NAND (which we established cannot be an irqchip on Cygnus). I think SPI is coming through the pipeline soon, though, and that's a good point. Brian
On 5/8/2015 1:47 PM, Brian Norris wrote: > On Fri, May 08, 2015 at 09:49:02PM +0200, Arnd Bergmann wrote: >> On Friday 08 May 2015 12:38:50 Brian Norris wrote: >>> On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote: > [...] > >>> To be clear, since I'm not sure if you're confused below: >>> >>> * Cygnus is a family of chips using the IPROC architecture, coming from >>> the Infrastructure/Networking Group; there are BCMxxxx numbers noted >>> in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to >>> the Cygnus family or the IPROC architecture. >>> >>> * BCM63xxx is a class of DSL chips from the Broadband/Connectivity >>> Group. >> >> Thanks for the clarification, I think that is roughly what I thought it was, >> but I'm still not sure about brcmstb. Is that related to bcm63xxx or separate? > > I think arch/arm/mach-bcm/Kconfig has the best summary. brcmstb is > separate; BCM7xxx is generally (always?) Set-Top Box. > > Another potentially confusing point: the main driver is named > 'brcsmtb_nand' since the NAND core (and driver) originated from STB > chips. But that core was applied to other non-STB chips, and so the > driver has been extended. > >>>> bcm63138_nand_driver with its own probe() function that calls the >>>> common probe function. That would make the soc specific parts >>>> better contained and match how we normally do abstractions of >>>> similar drivers. >>> >>> OK, so I can imagine this might require changing the DT binding a bit [1] >>> (is that your goal?). But what's the intended software difference? [2] >>> I'll still be passing around the same sorts of callbacks from the >>> 'iproc_nand' probe to the common probe function. > > ^^ before getting bogged down on the DT details (which can be changed > independently), I'd like to address this point. > >>> Brian >>> >>> [1] e.g.: >>> >>> nand: nand@18046000 { >>> compatible = "brcm,iproc-nand", "brcm,brcmnand-v6.1", "brcm,brcmnand"; >>> reg = <0x18046000 0x600>, <0xf8105408 0x600>, <0x18046f00 0x20>; >>> reg-names = "nand", "iproc-idm", "iproc-ext"; >>> interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>; >>> >>> #address-cells = <1>; >>> #size-cells = <0>; >>> >>> brcm,nand-has-wp; >>> }; >>> >>> This captures the extra "iproc-*" register ranges. Then we could have >>> the iproc_nand driver bind against "brcm,iproc-nand", then call into the >>> common probe, which would then accept/reject based on >>> "brcm,brcmnand-vX.Y". >>> >>> [2] The DT structure from [1] could actually accommodate either driver >>> structure just fine. So maybe that means it's a better hardware >>> description? >> >> Yes, I think this makes sense overall. Regarding the specific example, can you >> clarify how the register areas in iproc are structured? >> >> The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers >> of two, which often indicates that they are part of some other, larger, >> unit that might need to have a driver of its own, so before we specify >> a binding like the one you proposed above I'd like to make sure we're not >> getting ourselves into trouble later. > > I may want the Cygnus guys to speak up here, partly for technical > expertise and partly to know how much they care to share... > > <0xf8105408 0x600>: covers a series of NAND_IDM registers. NAND has a > few bits we don't care about (for debugging, logging, and resetting), as > well as its interrupt enable bits. The adjacent blocks cover similar IDM > blocks for other cores (SPI, PNOR, DDR), and they are similarly > unaligned. Not sure why, exactly; probably just a compact layout. Yes, starting from 0xf8105408, within the range of 0x600, there are various NAND_IDM registers scattered, which is indeed a very weird register layout. Like Brian said, most of those NAND_IDM registers are for debugging, logging, or status reporting. As of today, we only care about the first register, that contains a bunch of bits that allow you to configure the endianness of the AXI/APB bus, enabling NAND interrupts and clocks. > > <0x18046f00 0x20>: a series of 8 NAND interrupt registers, each word > containing a single bit representing status/clear. There is nothing > between the "nand" range and this range, and the SPI core register range > follows. Correct. > > So I think these are pretty clearly-delineated register ranges for NAND, > and the alignment is not really missing anything. Adjacent hardware > (e.g., SPI) is independent, though pieces look similar. For one, it has > similar: > > * interrupt enable bits in the IDM range (0xf8106408 to 0xf8106a00); > and > * interrupt status/clear following the SPI block (0x180473a0 to > 0x180473b8) > > Brian >
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile index 3b1adddc83dd..806727d5a84b 100644 --- a/drivers/mtd/nand/Makefile +++ b/drivers/mtd/nand/Makefile @@ -52,6 +52,7 @@ obj-$(CONFIG_MTD_NAND_XWAY) += xway_nand.o obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/ obj-$(CONFIG_MTD_NAND_SUNXI) += sunxi_nand.o obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o -obj-$(CONFIG_MTD_NAND_BRCMSTB) += brcmstb_nand.o +obj-$(CONFIG_MTD_NAND_BRCMSTB) += brcmnand.o +brcmnand-objs := brcmstb_nand.o brcmstb_nand_soc.o nand-objs := nand_base.o nand_bbt.o nand_timings.o diff --git a/drivers/mtd/nand/brcmnand.h b/drivers/mtd/nand/brcmnand.h new file mode 100644 index 000000000000..59e0bfef2331 --- /dev/null +++ b/drivers/mtd/nand/brcmnand.h @@ -0,0 +1,56 @@ +/* + * Copyright © 2015 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * 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. + */ + +#ifndef __BRCMNAND_H__ +#define __BRCMNAND_H__ + +#include <linux/types.h> + +struct device; +struct device_node; + +struct brcmnand_soc { + struct device *dev; /* parent device */ + struct device_node *dn; + bool (*ctlrdy_ack)(struct brcmnand_soc *soc); + void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en); + void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare); + void *priv; +}; + +/** + * Probe for a custom Broadcom SoC + * + * @dev: device to bind devres objects to + * @dn: DT node for the custom SoC + * + * Return a new soc struct if successful. Should be freed with + * brcmnand_remove_soc(). + * Return NULL for all other errors + */ +struct brcmnand_soc *devm_brcmnand_probe_soc(struct device *dev, + struct device_node *dn); + +static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc) +{ + if (soc && soc->prepare_data_bus) + soc->prepare_data_bus(soc, true); +} + +static inline void brcmnand_soc_data_bus_unprepare(struct brcmnand_soc *soc) +{ + if (soc && soc->prepare_data_bus) + soc->prepare_data_bus(soc, false); +} + +#endif /* __BRCMNAND_H__ */ diff --git a/drivers/mtd/nand/brcmstb_nand.c b/drivers/mtd/nand/brcmstb_nand.c index ec65a48d2487..5abc88cfe702 100644 --- a/drivers/mtd/nand/brcmstb_nand.c +++ b/drivers/mtd/nand/brcmstb_nand.c @@ -37,6 +37,8 @@ #include <linux/list.h> #include <linux/log2.h> +#include "brcmnand.h" + /* * This flag controls if WP stays on between erase/write commands to mitigate * flash corruption due to power glitches. Values: @@ -117,6 +119,9 @@ struct brcmnand_controller { unsigned int dma_irq; int nand_version; + /* Some SoCs provide custom interrupt status register(s) */ + struct brcmnand_soc *soc; + int cmd_pending; bool dma_pending; struct completion done; @@ -963,6 +968,17 @@ static irqreturn_t brcmnand_ctlrdy_irq(int irq, void *data) return IRQ_HANDLED; } +/* Handle SoC-specific interrupt hardware */ +static irqreturn_t brcmnand_irq(int irq, void *data) +{ + struct brcmnand_controller *ctrl = data; + + if (ctrl->soc->ctlrdy_ack(ctrl->soc)) + return brcmnand_ctlrdy_irq(irq, data); + + return IRQ_NONE; +} + static irqreturn_t brcmnand_dma_irq(int irq, void *data) { struct brcmnand_controller *ctrl = data; @@ -1151,12 +1167,18 @@ static void brcmnand_cmdfunc(struct mtd_info *mtd, unsigned command, if (native_cmd == CMD_PARAMETER_READ || native_cmd == CMD_PARAMETER_CHANGE_COL) { int i; + + brcmnand_soc_data_bus_prepare(ctrl->soc); + /* * Must cache the FLASH_CACHE now, since changes in * SECTOR_SIZE_1K may invalidate it */ for (i = 0; i < FC_WORDS; i++) ctrl->flash_cache[i] = brcmnand_read_fc(ctrl, i); + + brcmnand_soc_data_bus_unprepare(ctrl->soc); + /* Cleanup from HW quirk: restore SECTOR_SIZE_1K */ if (host->hwcfg.sector_size_1k) brcmnand_set_sector_size_1k(host, @@ -1369,10 +1391,15 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, brcmnand_send_cmd(host, CMD_PAGE_READ); brcmnand_waitfunc(mtd, chip); - if (likely(buf)) + if (likely(buf)) { + brcmnand_soc_data_bus_prepare(ctrl->soc); + for (j = 0; j < FC_WORDS; j++, buf++) *buf = brcmnand_read_fc(ctrl, j); + brcmnand_soc_data_bus_unprepare(ctrl->soc); + } + if (oob) oob += read_oob_from_regs(ctrl, i, oob, mtd->oobsize / trans, @@ -1544,12 +1571,17 @@ static int brcmnand_write(struct mtd_info *mtd, struct nand_chip *chip, lower_32_bits(addr)); (void)brcmnand_read_reg(ctrl, BRCMNAND_CMD_ADDRESS); - if (buf) + if (buf) { + brcmnand_soc_data_bus_prepare(ctrl->soc); + for (j = 0; j < FC_WORDS; j++, buf++) brcmnand_write_fc(ctrl, j, *buf); - else if (oob) + + brcmnand_soc_data_bus_unprepare(ctrl->soc); + } else if (oob) { for (j = 0; j < FC_WORDS; j++) brcmnand_write_fc(ctrl, j, 0xffffffff); + } if (oob) { oob += write_oob_to_regs(ctrl, i, oob, @@ -1993,6 +2025,11 @@ static int brcmnand_resume(struct device *dev) brcmnand_write_reg(ctrl, BRCMNAND_CS_XOR, ctrl->nand_cs_nand_xor); brcmnand_write_reg(ctrl, BRCMNAND_CORR_THRESHOLD, ctrl->corr_stat_threshold); + if (ctrl->soc) { + /* Clear/re-enable interrupt */ + ctrl->soc->ctlrdy_ack(ctrl->soc); + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); + } list_for_each_entry(host, &ctrl->host_list, node) { struct mtd_info *mtd = &host->mtd; @@ -2122,8 +2159,36 @@ static int brcmnand_probe(struct platform_device *pdev) return -ENODEV; } - ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, - DRV_NAME, ctrl); + /* + * Some SoCs integrate this controller (e.g., its interrupt bits) in + * interesting ways + */ + if (of_property_read_bool(dn, "brcm,nand-soc")) { + struct device_node *soc_dn; + + soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0); + if (!soc_dn) + return -ENODEV; + + ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn); + if (!ctrl->soc) { + dev_err(dev, "could not probe SoC data\n"); + of_node_put(soc_dn); + return -ENODEV; + } + + ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0, + DRV_NAME, ctrl); + + /* Enable interrupt */ + ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true); + + of_node_put(soc_dn); + } else { + /* Use standard interrupt infrastructure */ + ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0, + DRV_NAME, ctrl); + } if (ret < 0) { dev_err(dev, "can't allocate IRQ %d: error %d\n", ctrl->irq, ret); diff --git a/drivers/mtd/nand/brcmstb_nand_soc.c b/drivers/mtd/nand/brcmstb_nand_soc.c new file mode 100644 index 000000000000..970912c690a7 --- /dev/null +++ b/drivers/mtd/nand/brcmstb_nand_soc.c @@ -0,0 +1,65 @@ +/* + * Copyright © 2015 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * 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/device.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/slab.h> + +#include "brcmnand.h" + +#define DRV_NAME "brcmnand-soc" + +struct brcmnand_soc_ofdata { + int (*init)(struct brcmnand_soc *soc); + bool (*ctlrdy_ack)(struct brcmnand_soc *soc); + void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en); + void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare); +}; + +static const struct of_device_id brcmnand_soc_ofmatch[] = { + {}, +}; + +struct brcmnand_soc *devm_brcmnand_probe_soc(struct device *dev, + struct device_node *dn) +{ + const struct brcmnand_soc_ofdata *soc_data; + const struct of_device_id *match; + struct brcmnand_soc *soc; + int ret; + + match = of_match_node(brcmnand_soc_ofmatch, dn); + if (!match) + return NULL; + + soc_data = match->data; + + soc = devm_kzalloc(dev, sizeof(*soc), GFP_KERNEL); + if (!soc) + return NULL; + + soc->dev = dev; + soc->dn = dn; + soc->ctlrdy_ack = soc_data->ctlrdy_ack; + soc->ctlrdy_set_enabled = soc_data->ctlrdy_set_enabled; + soc->prepare_data_bus = soc_data->prepare_data_bus; + if (soc_data->init) { + ret = soc_data->init(soc); + if (ret) + return NULL; + } + + return soc; +}
The STB NAND controller is integrated into various non-STB SoCs, and those SoCs integrate things like interrupts and busing differently. Add support for masking/clearing interrupts via a custom hook, as well as preparing/unpreparing the data bus for data transfers. Does not support any new SoCs yet; support will be added incrementally. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/nand/Makefile | 3 +- drivers/mtd/nand/brcmnand.h | 56 +++++++++++++++++++++++++++ drivers/mtd/nand/brcmstb_nand.c | 75 ++++++++++++++++++++++++++++++++++--- drivers/mtd/nand/brcmstb_nand_soc.c | 65 ++++++++++++++++++++++++++++++++ 4 files changed, 193 insertions(+), 6 deletions(-) create mode 100644 drivers/mtd/nand/brcmnand.h create mode 100644 drivers/mtd/nand/brcmstb_nand_soc.c