Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3
* Varadarajan, Charu Latha [091118 00:58]: > > > >-Original Message- > >From: Tony Lindgren [mailto:t...@atomide.com] > >Sent: Friday, November 13, 2009 2:41 AM > >To: Varadarajan, Charu Latha > >Cc: linux-omap@vger.kernel.org; Syed, Rafiuddin > >Subject: Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3 > > > >* ch...@ti.com [091019 23:41]: > >> omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access > >> McBSP registers as 16-bit registers. > >> > >> In OMAP3, McBSP registers (DRR_REG and DXR_REG) are limited to > >> 32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is > >> not allowed and can corrupt register content. > >> > >> This patch adds omap3_mcbsp_pollwrite and > >> omap3_mcbsp_pollread functions for OMAP3 cpu, inorder to > >> perform 32 bit access of above mentioned McBSP registers. > > > >How about making the functions generic, I don't think we want > >to have separate omap1_mcbsp_pollread, omap2_mcbsp_pollread, > >omap3_mcbsp_pollread.. > > > >If it's not obvious how to implement it for some omap, just > >return an error based on the CPU type. > > > My v1 patch handles this in a single generic API. > [http://www.spinics.net/lists/linux-omap/msg19074.html and > http://patchwork.kernel.org/patch/53631/] Well first of all we don't have any references to this function in our tree. Second, we have: #define OMAP_MCBSP_REG_DRR2 0x00 #define OMAP_MCBSP_REG_DRR1 0x02 #define OMAP_MCBSP_REG_DXR2 0x04 #define OMAP_MCBSP_REG_DXR1 0x06 Meaning that both DRR and DXR are 32-bits in length. And based on that it seems that at least omap_mcbsp_pollwrite is broken if it supports only one 16-bit write instead of 16 + 16 bits that the hardware seems to support. > As per the review comments received for v1 patch, v2 patch is > modified to have new API "omap3_mcbsp_pollread/write" in > addition to "omap_mcbsp_pollread/write" APIs. That does not make much sense based on the fact that the hardware has 32-bits for DRR and DXR. > Review comments for V1 mentioned that API signartures should not be > changed from 16bit to 32 bit input parameters. > If the API needs to be made generic, API signature has to be > modified as given below (as in patch v1): > -int omap_mcbsp_pollread(unsigned int id, u16 *buf) > +int omap_mcbsp_pollread(unsigned int id, u32 *buf) > > If agreed for this generic API change, I shall post v3 patch > which would handle cputypes dynamically inside the API To me it smells like the all drivers using the the omap_mcbsp_pollread/write are broken legacy drivers. So maybe we should just remove these two functions? If we really want to keep these around, we should review the drivers using these functions. And if we end up keeping these functions around, I like the first patch better, except it does not seem to support DRR2 and DXR2 on older hardware, so that's broken too. IMHO, we should rather set up this driver as a proper bus driver as suggested by Paul in a related thread. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3
>-Original Message- >From: Tony Lindgren [mailto:t...@atomide.com] >Sent: Friday, November 13, 2009 2:41 AM >To: Varadarajan, Charu Latha >Cc: linux-omap@vger.kernel.org; Syed, Rafiuddin >Subject: Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3 > >* ch...@ti.com [091019 23:41]: >> omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access >> McBSP registers as 16-bit registers. >> >> In OMAP3, McBSP registers (DRR_REG and DXR_REG) are limited to >> 32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is >> not allowed and can corrupt register content. >> >> This patch adds omap3_mcbsp_pollwrite and >> omap3_mcbsp_pollread functions for OMAP3 cpu, inorder to >> perform 32 bit access of above mentioned McBSP registers. > >How about making the functions generic, I don't think we want >to have separate omap1_mcbsp_pollread, omap2_mcbsp_pollread, >omap3_mcbsp_pollread.. > >If it's not obvious how to implement it for some omap, just >return an error based on the CPU type. My v1 patch handles this in a single generic API. [http://www.spinics.net/lists/linux-omap/msg19074.html and http://patchwork.kernel.org/patch/53631/] As per the review comments received for v1 patch, v2 patch is modified to have new API "omap3_mcbsp_pollread/write" in addition to "omap_mcbsp_pollread/write" APIs. Review comments for V1 mentioned that API signartures should not be changed from 16bit to 32 bit input parameters. If the API needs to be made generic, API signature has to be modified as given below (as in patch v1): -int omap_mcbsp_pollread(unsigned int id, u16 *buf) +int omap_mcbsp_pollread(unsigned int id, u32 *buf) If agreed for this generic API change, I shall post v3 patch which would handle cputypes dynamically inside the API > >Regards, > >Tony > >> Signed-off-by: Charulatha V >> Signed-off-by: Syed Rafiuddin >> --- >> arch/arm/plat-omap/include/mach/mcbsp.h |4 + >> arch/arm/plat-omap/mcbsp.c | 92 >++- >> 2 files changed, 95 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h >b/arch/arm/plat-omap/include/mach/mcbsp.h >> index 0c5f937..b0f9260 100644 >> --- a/arch/arm/plat-omap/include/mach/mcbsp.h >> +++ b/arch/arm/plat-omap/include/mach/mcbsp.h >> @@ -406,6 +406,10 @@ void omap_mcbsp_set_spi_mode(unsigned >int id, const struct omap_mcbsp_spi_cfg * >> /* Polled read/write functions */ >> int omap_mcbsp_pollread(unsigned int id, u16 * buf); >> int omap_mcbsp_pollwrite(unsigned int id, u16 buf); >> + >> +int omap3_mcbsp_pollread(unsigned int id, u32 *buf); >> +int omap3_mcbsp_pollwrite(unsigned int id, u32 buf); >> + >> int omap_mcbsp_set_io_type(unsigned int id, >omap_mcbsp_io_type_t io_type); >> >> #endif -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3
* ch...@ti.com [091019 23:41]:
> omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access
> McBSP registers as 16-bit registers.
>
> In OMAP3, McBSP registers (DRR_REG and DXR_REG) are limited to
> 32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is
> not allowed and can corrupt register content.
>
> This patch adds omap3_mcbsp_pollwrite and
> omap3_mcbsp_pollread functions for OMAP3 cpu, inorder to
> perform 32 bit access of above mentioned McBSP registers.
How about making the functions generic, I don't think we want
to have separate omap1_mcbsp_pollread, omap2_mcbsp_pollread,
omap3_mcbsp_pollread..
If it's not obvious how to implement it for some omap, just
return an error based on the CPU type.
Regards,
Tony
> Signed-off-by: Charulatha V
> Signed-off-by: Syed Rafiuddin
> ---
> arch/arm/plat-omap/include/mach/mcbsp.h |4 +
> arch/arm/plat-omap/mcbsp.c | 92
> ++-
> 2 files changed, 95 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h
> b/arch/arm/plat-omap/include/mach/mcbsp.h
> index 0c5f937..b0f9260 100644
> --- a/arch/arm/plat-omap/include/mach/mcbsp.h
> +++ b/arch/arm/plat-omap/include/mach/mcbsp.h
> @@ -406,6 +406,10 @@ void omap_mcbsp_set_spi_mode(unsigned int id, const
> struct omap_mcbsp_spi_cfg *
> /* Polled read/write functions */
> int omap_mcbsp_pollread(unsigned int id, u16 * buf);
> int omap_mcbsp_pollwrite(unsigned int id, u16 buf);
> +
> +int omap3_mcbsp_pollread(unsigned int id, u32 *buf);
> +int omap3_mcbsp_pollwrite(unsigned int id, u32 buf);
> +
> int omap_mcbsp_set_io_type(unsigned int id, omap_mcbsp_io_type_t io_type);
>
> #endif
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index b6b520d..1212d11 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -425,7 +425,7 @@ void omap_mcbsp_stop(unsigned int id)
> }
> EXPORT_SYMBOL(omap_mcbsp_stop);
>
> -/* polled mcbsp i/o operations */
> +/* polled mcbsp i/o operations for omap1 and omap2420 */
> int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
> {
> struct omap_mcbsp *mcbsp;
> @@ -515,6 +515,96 @@ int omap_mcbsp_pollread(unsigned int id, u16 *buf)
> }
> EXPORT_SYMBOL(omap_mcbsp_pollread);
>
> +/* polled mcbsp i/o operations for omap3 */
> +int omap3_mcbsp_pollwrite(unsigned int id, u32 buf)
> +{
> + struct omap_mcbsp *mcbsp;
> + void __iomem *base;
> +
> + if (!omap_mcbsp_check_valid_id(id)) {
> + printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> + return -ENODEV;
> + }
> +
> + mcbsp = id_to_mcbsp_ptr(id);
> + base = mcbsp->io_base;
> +
> + OMAP_MCBSP_WRITE(base, DXR, buf);
> + /* if frame sync error - clear the error */
> + if (OMAP_MCBSP_READ(base, SPCR2) & XSYNC_ERR) {
> + /* clear error */
> + OMAP_MCBSP_WRITE(base, SPCR2, OMAP_MCBSP_READ(base, SPCR2)
> + & (~XSYNC_ERR));
> + /* resend */
> + return -EPERM;
> + } else {
> + /* wait for transmit confirmation */
> + int attemps = 0;
> + while (!(OMAP_MCBSP_READ(base, SPCR2) & XRDY)) {
> + if (attemps++ > 1000) {
> + OMAP_MCBSP_WRITE(base, SPCR2,
> + OMAP_MCBSP_READ(base, SPCR2) &
> + (~XRST));
> + udelay(10);
> + OMAP_MCBSP_WRITE(base, SPCR2,
> + OMAP_MCBSP_READ(base, SPCR2) |
> + (XRST));
> + udelay(10);
> + dev_err(mcbsp->dev, "Could not write to"
> + " McBSP%d Register\n", mcbsp->id);
> + return -ENOENT;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(omap3_mcbsp_pollwrite);
> +
> +int omap3_mcbsp_pollread(unsigned int id, u32 *buf)
> +{
> + struct omap_mcbsp *mcbsp;
> + void __iomem *base;
> +
> + if (!omap_mcbsp_check_valid_id(id)) {
> + printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> + return -ENODEV;
> + }
> + mcbsp = id_to_mcbsp_ptr(id);
> +
> + base = mcbsp->io_base;
> + /* if frame sync error - clear the error */
> + if (OMAP_MCBSP_READ(base, SPCR1) & RSYNC_ERR) {
> + /* clear error */
> + OMAP_MCBSP_WRITE(base, SPCR1, OMAP_MCBSP_READ(base, SPCR1)
> + & (~RSYNC_ERR));
> + /* resend */
> + return -EPERM;
> + } else {
> + /* wait for recieve confirmation */
> + int attemps = 0;
> + while (!(OMAP_MCBSP_READ(base, SPCR1) & RRDY)) {
> + if (attemps++ > 1000) {
> +
