| Message ID | 1326838190-13746-3-git-send-email-eric.nelson@boundarydevices.com |
|---|---|
| State | Accepted |
| Commit | d5c37c9cc45ee400e0f53ba0e11edc88d6bd630b |
| Headers | show |
> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com> > --- > arch/arm/include/asm/arch-mx6/imx-regs.h | 44 > ++++++++++++++++++++++++++++++ 1 files changed, 44 insertions(+), 0 > deletions(-) > > diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h > b/arch/arm/include/asm/arch-mx6/imx-regs.h index 7650cb9..00040c4 100644 > --- a/arch/arm/include/asm/arch-mx6/imx-regs.h > +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h > @@ -190,6 +190,50 @@ struct src { > u32 gpr10; > }; > > +/* ECSPI registers */ > +struct cspi_regs { > + u32 rxdata; > + u32 txdata; > + u32 ctrl; > + u32 cfg; > + u32 intr; > + u32 dma; > + u32 stat; > + u32 period; > +}; Sigh ... it's no fun I can have only one remark :-) Is this part common for all imx-es ? > + > +/* > + * CSPI register definitions > + */ > +#define MXC_ECSPI > +#define MXC_CSPICTRL_EN (1 << 0) > +#define MXC_CSPICTRL_MODE (1 << 1) > +#define MXC_CSPICTRL_XCH (1 << 2) > +#define MXC_CSPICTRL_CHIPSELECT(x) (((x) & 0x3) << 12) > +#define MXC_CSPICTRL_BITCOUNT(x) (((x) & 0xfff) << 20) > +#define MXC_CSPICTRL_PREDIV(x) (((x) & 0xF) << 12) > +#define MXC_CSPICTRL_POSTDIV(x) (((x) & 0xF) << 8) > +#define MXC_CSPICTRL_SELCHAN(x) (((x) & 0x3) << 18) > +#define MXC_CSPICTRL_MAXBITS 0xfff > +#define MXC_CSPICTRL_TC (1 << 7) > +#define MXC_CSPICTRL_RXOVF (1 << 6) > +#define MXC_CSPIPERIOD_32KHZ (1 << 15) > +#define MAX_SPI_BYTES 32 > + > +/* Bit position inside CTRL register to be associated with SS */ > +#define MXC_CSPICTRL_CHAN 18 > + > +/* Bit position inside CON register to be associated with SS */ > +#define MXC_CSPICON_POL 4 > +#define MXC_CSPICON_PHA 0 > +#define MXC_CSPICON_SSPOL 12 > +#define MXC_SPI_BASE_ADDRESSES \ > + ECSPI1_BASE_ADDR, \ > + ECSPI2_BASE_ADDR, \ > + ECSPI3_BASE_ADDR, \ > + ECSPI4_BASE_ADDR, \ > + ECSPI5_BASE_ADDR > + > struct iim_regs { > u32 ctrl; > u32 ctrl_set;
On 01/17/2012 04:19 PM, Marek Vasut wrote: >> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com> >> --- >> arch/arm/include/asm/arch-mx6/imx-regs.h | 44 >> ++++++++++++++++++++++++++++++ 1 files changed, 44 insertions(+), 0 >> deletions(-) >> >> diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h >> b/arch/arm/include/asm/arch-mx6/imx-regs.h index 7650cb9..00040c4 100644 >> --- a/arch/arm/include/asm/arch-mx6/imx-regs.h >> +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h >> @@ -190,6 +190,50 @@ struct src { >> u32 gpr10; >> }; >> >> +/* ECSPI registers */ >> +struct cspi_regs { >> + u32 rxdata; >> + u32 txdata; >> + u32 ctrl; >> + u32 cfg; >> + u32 intr; >> + u32 dma; >> + u32 stat; >> + u32 period; >> +}; > > Sigh ... it's no fun I can have only one remark :-) > > Is this part common for all imx-es ? > All i.MX6's This is a cut & paste from MX51. I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' for i.MX31 and i.MX35 that share the CSPI peripheral.
> On 01/17/2012 04:19 PM, Marek Vasut wrote: > >> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com> > >> --- > >> > >> arch/arm/include/asm/arch-mx6/imx-regs.h | 44 > >> > >> ++++++++++++++++++++++++++++++ 1 files changed, 44 insertions(+), 0 > >> deletions(-) > >> > >> diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h > >> b/arch/arm/include/asm/arch-mx6/imx-regs.h index 7650cb9..00040c4 100644 > >> --- a/arch/arm/include/asm/arch-mx6/imx-regs.h > >> +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h > >> @@ -190,6 +190,50 @@ struct src { > >> > >> u32 gpr10; > >> > >> }; > >> > >> +/* ECSPI registers */ > >> +struct cspi_regs { > >> + u32 rxdata; > >> + u32 txdata; > >> + u32 ctrl; > >> + u32 cfg; > >> + u32 intr; > >> + u32 dma; > >> + u32 stat; > >> + u32 period; > >> +}; > > > > Sigh ... it's no fun I can have only one remark :-) > > > > Is this part common for all imx-es ? > > All i.MX6's > > This is a cut & paste from MX51. > > I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration > for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' > for i.MX31 and i.MX35 that share the CSPI peripheral. But you don't even need this outside of the spi driver so just put it into the spi driver and be done with it. That'll solve your duplication issue. M
On 01/17/2012 06:27 PM, Marek Vasut wrote: >> On 01/17/2012 04:19 PM, Marek Vasut wrote: >>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com> >>>> +/* ECSPI registers */ >>>> +struct cspi_regs { >>>> + u32 rxdata; >>>> + u32 txdata; >>>> + u32 ctrl; >>>> + u32 cfg; >>>> + u32 intr; >>>> + u32 dma; >>>> + u32 stat; >>>> + u32 period; >>>> +}; >>> >>> Sigh ... it's no fun I can have only one remark :-) >>> >>> Is this part common for all imx-es ? >> >> All i.MX6's >> >> This is a cut& paste from MX51. >> >> I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration >> for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' >> for i.MX31 and i.MX35 that share the CSPI peripheral. > > But you don't even need this outside of the spi driver so just put it into the > spi driver and be done with it. That'll solve your duplication issue. > > M > I'll defer to Stefano on this one, since I did this in response to his request: > Right - and we already discussed in the past how to avoid to put > specific SOC code inside the driver. In fact, the cspi_regs structure > was already moved into the specific SOC header (imx-regs.h) - but the > definitions of the single bits of the registers are still inside the > driver, as well as the base address of the (e)cspi controllers. > > They should also be moved - take into acoount by implementing your > changes for i.mx6 The struct cspi_regs is already present in mx31, mx35, and mx51 headers, so I'm not breaking new ground here, only in the bitfield declarations. http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mx31/imx-regs.h;h=6a517ddd931ca0d1e598bd7456c4c611741602eb;hb=HEAD#l60 http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mx35/imx-regs.h;h=df74508a93ee87ae986f7c2f48f6c5fb36626070;hb=HEAD#l279 http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mx5/imx-regs.h;h=0ee88d25b7800ae9e6aed809d02dd19d9cac9c82;hb=HEAD#l423 My interpretation of Stefano's intent is to clean up the driver at the expense of extra defines in the arch-specific headers. Regards, Eric
> On 01/17/2012 06:27 PM, Marek Vasut wrote: > >> On 01/17/2012 04:19 PM, Marek Vasut wrote: > >>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com> > >>>> +/* ECSPI registers */ > >>>> +struct cspi_regs { > >>>> + u32 rxdata; > >>>> + u32 txdata; > >>>> + u32 ctrl; > >>>> + u32 cfg; > >>>> + u32 intr; > >>>> + u32 dma; > >>>> + u32 stat; > >>>> + u32 period; > >>>> +}; > >>> > >>> Sigh ... it's no fun I can have only one remark :-) > >>> > >>> Is this part common for all imx-es ? > >> > >> All i.MX6's > >> > >> This is a cut& paste from MX51. > >> > >> I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration > >> for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' > >> for i.MX31 and i.MX35 that share the CSPI peripheral. > > > > But you don't even need this outside of the spi driver so just put it > > into the spi driver and be done with it. That'll solve your duplication > > issue. > > > > M > > I'll defer to Stefano on this one, since I did this in response > > to his request: > > Right - and we already discussed in the past how to avoid to put > > specific SOC code inside the driver. In fact, the cspi_regs structure > > was already moved into the specific SOC header (imx-regs.h) - but the > > definitions of the single bits of the registers are still inside the > > driver, as well as the base address of the (e)cspi controllers. > > > > They should also be moved - take into acoount by implementing your > > changes for i.mx6 > > The struct cspi_regs is already present in mx31, mx35, and mx51 headers, > so I'm not breaking new ground here, only in the bitfield declarations. > > http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch- mx31/i > mx-regs.h;h=6a517ddd931ca0d1e598bd7456c4c611741602eb;hb=HEAD#l60 > http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mx35/i > mx-regs.h;h=df74508a93ee87ae986f7c2f48f6c5fb36626070;hb=HEAD#l279 > http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/arch-mx5/im > x-regs.h;h=0ee88d25b7800ae9e6aed809d02dd19d9cac9c82;hb=HEAD#l423 > > My interpretation of Stefano's intent is to clean up the driver at the > expense of extra defines in the arch-specific headers. But they're all the same, right? So we have now the same structure defined thrice? M > > Regards, > > > Eric
On 01/17/2012 06:47 PM, Marek Vasut wrote: >> On 01/17/2012 06:27 PM, Marek Vasut wrote: >> >> I'll defer to Stefano on this one, since I did this in response >> to his request: >> >>> Right - and we already discussed in the past how to avoid to put >>> specific SOC code inside the driver. In fact, the cspi_regs structure >>> was already moved into the specific SOC header (imx-regs.h) - but the >>> definitions of the single bits of the registers are still inside the >>> driver, as well as the base address of the (e)cspi controllers. >>> >>> They should also be moved - take into acoount by implementing your >>> changes for i.mx6 >> >> The struct cspi_regs is already present in mx31, mx35, and mx51 headers, >> so I'm not breaking new ground here, only in the bitfield declarations. >> <snip> >> >> My interpretation of Stefano's intent is to clean up the driver at the >> expense of extra defines in the arch-specific headers. > > But they're all the same, right? So we have now the same structure defined > thrice? > Almost, but not quite: mx31 and mx35 both use the CSPI peripheral and have one layout. mx5 and mx6 have the ECSPI peripheral, which has an extra register "cfg" to control the polarity and phase of the signals. Actually, that comment is wrong. The MX51 and MX53 have **both** CSPI and ECSPI peripherals, but the existing code in mxc_spi only supports ECSPI. The bitfields that my patches move into the imx-regs.h files are also almost identical.
On 18/01/2012 02:44, Eric Nelson wrote: > On 01/17/2012 06:27 PM, Marek Vasut wrote: >>> On 01/17/2012 04:19 PM, Marek Vasut wrote: >>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com> >>>>> +/* ECSPI registers */ >>>>> +struct cspi_regs { >>>>> + u32 rxdata; >>>>> + u32 txdata; >>>>> + u32 ctrl; >>>>> + u32 cfg; >>>>> + u32 intr; >>>>> + u32 dma; >>>>> + u32 stat; >>>>> + u32 period; >>>>> +}; >>>> >>>> Sigh ... it's no fun I can have only one remark :-) >>>> >>>> Is this part common for all imx-es ? >>> >>> All i.MX6's >>> >>> This is a cut& paste from MX51. >>> >>> I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration >>> for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' >>> for i.MX31 and i.MX35 that share the CSPI peripheral. >> >> But you don't even need this outside of the spi driver so just put it >> into the >> spi driver and be done with it. That'll solve your duplication issue. >> >> M >> > > I'll defer to Stefano on this one, since I did this in response > to his request: Yes, I admit I am guilty about this ! The layout of the CSPI registers is not exactly the same for all SOCs. For example, the MXC_CSPICTRL_TC has a different position, as well as the BITCOUNT (because the MX31 can send less bits in one shot) and MXC_CSPICTRL_CHIPSELECT. So they are similar, but not exactly the same. Then we have the MX5 registers. Even if we check the CSPI (not eCSPI) controller, the layout of the registers is different compared to the MX3 SOCs. > The struct cspi_regs is already present in mx31, mx35, and mx51 headers, > so I'm not breaking new ground here, only in the bitfield declarations. Right, I see the same. The cspi_regs structure is already defined into the imx_regs.h, only the bit layout was moved. And as the bit layout is SOC dependent, I think the right place for it is inside the imx-regs.h registers. > My interpretation of Stefano's intent is to clean up the driver at the > expense > of extra defines in the arch-specific headers. Yes, you're right - of course, I am open also to other solutions if they are proofed to be better ;-). Best regards, Stefano
> On 18/01/2012 02:44, Eric Nelson wrote: > > On 01/17/2012 06:27 PM, Marek Vasut wrote: > >>> On 01/17/2012 04:19 PM, Marek Vasut wrote: > >>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com> > >>>>> +/* ECSPI registers */ > >>>>> +struct cspi_regs { > >>>>> + u32 rxdata; > >>>>> + u32 txdata; > >>>>> + u32 ctrl; > >>>>> + u32 cfg; > >>>>> + u32 intr; > >>>>> + u32 dma; > >>>>> + u32 stat; > >>>>> + u32 period; > >>>>> +}; > >>>> > >>>> Sigh ... it's no fun I can have only one remark :-) > >>>> > >>>> Is this part common for all imx-es ? > >>> > >>> All i.MX6's > >>> > >>> This is a cut& paste from MX51. > >>> > >>> I was tempted to introduce an 'mxc_ecspi.h' to merge the declaration > >>> for i.MX5x and i.MX6 which share the ECSPI peripheral and 'mxc_cspi.h' > >>> for i.MX31 and i.MX35 that share the CSPI peripheral. > >> > >> But you don't even need this outside of the spi driver so just put it > >> into the > >> spi driver and be done with it. That'll solve your duplication issue. > >> > >> M > > > > I'll defer to Stefano on this one, since I did this in response > > > to his request: > Yes, I admit I am guilty about this ! > > The layout of the CSPI registers is not exactly the same for all SOCs. > For example, the MXC_CSPICTRL_TC has a different position, as well as > the BITCOUNT (because the MX31 can send less bits in one shot) and > MXC_CSPICTRL_CHIPSELECT. > > So they are similar, but not exactly the same. > > Then we have the MX5 registers. Even if we check the CSPI (not eCSPI) > controller, the layout of the registers is different compared to the MX3 > SOCs. > > > The struct cspi_regs is already present in mx31, mx35, and mx51 headers, > > so I'm not breaking new ground here, only in the bitfield declarations. > > Right, I see the same. The cspi_regs structure is already defined into > the imx_regs.h, only the bit layout was moved. And as the bit layout is > SOC dependent, I think the right place for it is inside the imx-regs.h > registers. > > > My interpretation of Stefano's intent is to clean up the driver at the > > expense > > of extra defines in the arch-specific headers. > > Yes, you're right - of course, I am open also to other solutions if they > are proofed to be better ;-). > > Best regards, > Stefano Ok guys, I see ... Stefano, you're ok with putting the reg structures into these header files? M
On 18/01/2012 17:08, Marek Vasut wrote: > Ok guys, I see ... Stefano, you're ok with putting the reg structures into these > header files? The reg structures are already into these header files - the patch moves only the bit meaning inside the imx-regs.h files. We can discuss if we should move them again into the driver, making the decision on which structure to be used not on a SOC level (#ifdef CONFIG_MXxx), but on basis of the controller type (CSPI or ECSPI). This makes sense if we run (we had until now no use case with the currently supported boards) a MX5 board using a CSPI instead of a ECSPI. In this case, we should also transform MXC_CSPI / MXC_ECSPI in a CONFIG_ define to be put in the board configuration file. However, as usual in u-boot, dead code or code that has no use case and cannot be tested is not allowed. Until we have not such a board (MX5 board requiring CSPI instead of ECSPI), we should avoid to introduce not tested code and let those changes for a next patch. Stefano
On 01/18/2012 01:39 AM, Stefano Babic wrote: > On 18/01/2012 02:44, Eric Nelson wrote: >> I'll defer to Stefano on this one, since I did this in response >> to his request: > > Yes, I admit I am guilty about this ! > > The layout of the CSPI registers is not exactly the same for all SOCs. > For example, the MXC_CSPICTRL_TC has a different position, as well as > the BITCOUNT (because the MX31 can send less bits in one shot) and > MXC_CSPICTRL_CHIPSELECT. > > So they are similar, but not exactly the same. > > Then we have the MX5 registers. Even if we check the CSPI (not eCSPI) > controller, the layout of the registers is different compared to the MX3 > SOCs. > >> The struct cspi_regs is already present in mx31, mx35, and mx51 headers, >> so I'm not breaking new ground here, only in the bitfield declarations. > > Right, I see the same. The cspi_regs structure is already defined into > the imx_regs.h, only the bit layout was moved. And as the bit layout is > SOC dependent, I think the right place for it is inside the imx-regs.h > registers. > >> My interpretation of Stefano's intent is to clean up the driver at the >> expense of extra defines in the arch-specific headers. > > Yes, you're right - of course, I am open also to other solutions if they > are proofed to be better ;-). > I think this is about as good as things get with the current code base. I would argue that the driver would be better if it explicitly supported ECSPI and CSPI at the same time since the mx5x CPUs support it. Implememting that would likely require a de-structuring (removing the use of structs to represent the register set). IOW, a re-write. That's probably not worth the effort unless someone's built hardware that needs it (I'm not aware of any). On our boards that use more than one channel of SPI (for PMIC and SF), we're using ECSPI on both. I think the same was true on the MX51 EVK.
On 18/01/2012 21:05, Eric Nelson wrote: >> >> Yes, you're right - of course, I am open also to other solutions if they >> are proofed to be better ;-). >> > > I think this is about as good as things get with the current code base. > I would argue that the driver would be better if it explicitly supported > ECSPI and CSPI at the same time since the mx5x CPUs support it. This means that the driver goes to support multiple interfaces at the same time, independently if they are CSPI or ECSPI. At the moment, there is no use case for it. > > Implememting that would likely require a de-structuring (removing the > use of structs to represent the register set). IOW, a re-write. Yes, this is also for most drivers in u-boot to support multiple interface and not only one. > > That's probably not worth the effort unless someone's built hardware > that needs it (I'm not aware of any). Agree. > > On our boards that use more than one channel of SPI (for PMIC and SF), > we're using ECSPI on both. I think the same was true on the MX51 EVK. Yes, it is the same. Best regards, Stefano Babic
diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h b/arch/arm/include/asm/arch-mx6/imx-regs.h index 7650cb9..00040c4 100644 --- a/arch/arm/include/asm/arch-mx6/imx-regs.h +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h @@ -190,6 +190,50 @@ struct src { u32 gpr10; }; +/* ECSPI registers */ +struct cspi_regs { + u32 rxdata; + u32 txdata; + u32 ctrl; + u32 cfg; + u32 intr; + u32 dma; + u32 stat; + u32 period; +}; + +/* + * CSPI register definitions + */ +#define MXC_ECSPI +#define MXC_CSPICTRL_EN (1 << 0) +#define MXC_CSPICTRL_MODE (1 << 1) +#define MXC_CSPICTRL_XCH (1 << 2) +#define MXC_CSPICTRL_CHIPSELECT(x) (((x) & 0x3) << 12) +#define MXC_CSPICTRL_BITCOUNT(x) (((x) & 0xfff) << 20) +#define MXC_CSPICTRL_PREDIV(x) (((x) & 0xF) << 12) +#define MXC_CSPICTRL_POSTDIV(x) (((x) & 0xF) << 8) +#define MXC_CSPICTRL_SELCHAN(x) (((x) & 0x3) << 18) +#define MXC_CSPICTRL_MAXBITS 0xfff +#define MXC_CSPICTRL_TC (1 << 7) +#define MXC_CSPICTRL_RXOVF (1 << 6) +#define MXC_CSPIPERIOD_32KHZ (1 << 15) +#define MAX_SPI_BYTES 32 + +/* Bit position inside CTRL register to be associated with SS */ +#define MXC_CSPICTRL_CHAN 18 + +/* Bit position inside CON register to be associated with SS */ +#define MXC_CSPICON_POL 4 +#define MXC_CSPICON_PHA 0 +#define MXC_CSPICON_SSPOL 12 +#define MXC_SPI_BASE_ADDRESSES \ + ECSPI1_BASE_ADDR, \ + ECSPI2_BASE_ADDR, \ + ECSPI3_BASE_ADDR, \ + ECSPI4_BASE_ADDR, \ + ECSPI5_BASE_ADDR + struct iim_regs { u32 ctrl; u32 ctrl_set;
Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com> --- arch/arm/include/asm/arch-mx6/imx-regs.h | 44 ++++++++++++++++++++++++++++++ 1 files changed, 44 insertions(+), 0 deletions(-)