Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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 = ;
>>>
>>>#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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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 = ;
> >
> >#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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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 = ;
>
>#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?
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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 = ;
#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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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,
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 BCM 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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?
--
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
---
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
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 ..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
+
+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
#include
+#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 intdma_irq;
int nand_version;
+ /* Some SoCs provide custom interrupt status register(s) */
+ struct brcmnand_soc *soc;
+
int cmd_pending;
booldma_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
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 06/10] mtd: brcmstb_nand: add SoC-specific support
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 computersforpe...@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
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 ..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 intdma_irq;
int nand_version;
+ /* Some SoCs provide custom interrupt status register(s) */
+ struct brcmnand_soc *soc;
+
int cmd_pending;
booldma_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);
+
