From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out.google.com ([74.125.121.35]:27017 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752740Ab0DLIX1 convert rfc822-to-8bit (ORCPT ); Mon, 12 Apr 2010 04:23:27 -0400 Received: from hpaq5.eem.corp.google.com (hpaq5.eem.corp.google.com [10.3.21.5]) by smtp-out.google.com with ESMTP id o3C8NOel024781 for ; Mon, 12 Apr 2010 10:23:24 +0200 Received: from wyi11 (wyi11.prod.google.com [10.241.227.11]) by hpaq5.eem.corp.google.com with ESMTP id o3C8NNvv030078 for ; Mon, 12 Apr 2010 10:23:23 +0200 Received: by wyi11 with SMTP id 11so699339wyi.37 for ; Mon, 12 Apr 2010 01:23:23 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1270805833-1466-1-git-send-email-dwalker@codeaurora.org> References: <1270805833-1466-1-git-send-email-dwalker@codeaurora.org> Date: Mon, 12 Apr 2010 01:23:23 -0700 Message-ID: Subject: Re: [PATCH] arm: msm: smd: use either package v3 or v4 not both From: Dima Zavin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-arm-msm-owner@vger.kernel.org List-ID: To: Daniel Walker Cc: linux-arm-msm@vger.kernel.org, Daniel Walker inline. On Fri, Apr 9, 2010 at 2:37 AM, Daniel Walker wrote: > From: Daniel Walker > > > Dima could you review this patch please? > > -- > > This modifies SMD to use either the package v3 or package v4, > but not both. The current code tries to allocate as v4 on all > system which can produce a scary looking error message on boot up, > > smem_find(16, 40): wrong size 16424 > smd_alloc_channel() cid=02 size=08192 'SMD_RPCCALL' > > With this error the code then falls back on the package v3 allocation > method. This method is inefficient because it causes a slow down > on some systems even when the allocation method can be determined > at compile time. It also causes a kernel size increase that effects > all system and is not needed. > > This change corrects the allocation to use one method or the other > and not both. > > Signed-off-by: Daniel Walker > --- >  arch/arm/mach-msm/Kconfig       |    4 +++ >  arch/arm/mach-msm/smd.c         |   44 +----------------------------- >  arch/arm/mach-msm/smd_private.h |   58 +++++++++++++++++++++++++++++++++++++- >  3 files changed, 61 insertions(+), 45 deletions(-) > > diff --git a/arch/arm/mach-msm/Kconfig b/arch/arm/mach-msm/Kconfig > index b548e42..381e817 100644 > --- a/arch/arm/mach-msm/Kconfig > +++ b/arch/arm/mach-msm/Kconfig > @@ -7,6 +7,7 @@ choice >  config ARCH_MSM7X00A >        bool "MSM7x00A / MSM7x01A" >        select ARCH_MSM_ARM11 > +       select MSM_SMD_PKG3 >        select CPU_V6 > >  config ARCH_QSD8X50 > @@ -325,6 +326,9 @@ config MSM_SERIAL_DEBUGGER_CONSOLE >          Enables a console so that printk messages are displayed on >          the debugger serial port as the occur. > > +config MSM_SMD_PKG3 What is PKG3/PKG4? How does it relate to smd structure versions? > +       bool > + >  config MSM_SMD >        default y >        bool "MSM Shared Memory Driver (SMD)" > diff --git a/arch/arm/mach-msm/smd.c b/arch/arm/mach-msm/smd.c > index 42b59c9..1b4ecce 100644 > --- a/arch/arm/mach-msm/smd.c > +++ b/arch/arm/mach-msm/smd.c > @@ -600,48 +600,6 @@ static int smd_packet_read(smd_channel_t *ch, void *data, int len) >        return r; >  } > > -static int smd_alloc_v2(struct smd_channel *ch) > -{ > -       struct smd_shared_v2 *shared2; > -       void *buffer; > -       unsigned buffer_sz; > - > -       shared2 = smem_alloc(SMEM_SMD_BASE_ID + ch->n, sizeof(*shared2)); > -       buffer = smem_item(SMEM_SMD_FIFO_BASE_ID + ch->n, &buffer_sz); > - > -       if (!buffer) > -               return -1; > - > -       /* buffer must be a power-of-two size */ > -       if (buffer_sz & (buffer_sz - 1)) > -               return -1; > - > -       buffer_sz /= 2; > -       ch->send = &shared2->ch0; > -       ch->recv = &shared2->ch1; > -       ch->send_data = buffer; > -       ch->recv_data = buffer + buffer_sz; > -       ch->fifo_size = buffer_sz; > -       return 0; > -} > - > -static int smd_alloc_v1(struct smd_channel *ch) > -{ > -       struct smd_shared_v1 *shared1; > -       shared1 = smem_alloc(ID_SMD_CHANNELS + ch->n, sizeof(*shared1)); > -       if (!shared1) { > -               pr_err("smd_alloc_channel() cid %d does not exist\n", ch->n); > -               return -1; > -       } > -       ch->send = &shared1->ch0; > -       ch->recv = &shared1->ch1; > -       ch->send_data = shared1->data0; > -       ch->recv_data = shared1->data1; > -       ch->fifo_size = SMD_BUF_SIZE; > -       return 0; > -} > - > - >  static int smd_alloc_channel(const char *name, uint32_t cid, uint32_t type) >  { >        struct smd_channel *ch; > @@ -653,7 +611,7 @@ static int smd_alloc_channel(const char *name, uint32_t cid, uint32_t type) >        } >        ch->n = cid; > > -       if (smd_alloc_v2(ch) && smd_alloc_v1(ch)) { > +       if (_smd_alloc_channel(ch)) { >                kfree(ch); >                return -1; >        } > diff --git a/arch/arm/mach-msm/smd_private.h b/arch/arm/mach-msm/smd_private.h > index 33a33f1..a403424 100644 > --- a/arch/arm/mach-msm/smd_private.h > +++ b/arch/arm/mach-msm/smd_private.h > @@ -61,7 +61,7 @@ struct smem_shared { >  #define SMSM_V1_SIZE           (sizeof(unsigned) * 8) >  #define SMSM_V2_SIZE           (sizeof(unsigned) * 4) > > -#ifndef CONFIG_ARCH_MSM_SCORPION > +#ifdef CONFIG_MSM_SMD_PKG3 >  struct smsm_interrupt_info { >        uint32_t interrupt_mask; >        uint32_t pending_interrupts; > @@ -123,7 +123,7 @@ struct msm_dem_slave_data { >  #define SMSM_WKUP_REASON_ALARM 0x00000010 >  #define SMSM_WKUP_REASON_RESET 0x00000020 > > -#ifndef CONFIG_ARCH_MSM_SCORPION > +#ifdef CONFIG_ARCH_MSM7X00A >  enum smsm_state_item { >        SMSM_STATE_APPS = 1, >        SMSM_STATE_MODEM = 3, > @@ -265,6 +265,7 @@ struct smd_half_channel { >        unsigned head; >  } __attribute__(( aligned(4), packed )); > > +/* Only used on SMD package v3 on msm7201a */ >  struct smd_shared_v1 { >        struct smd_half_channel ch0; >        unsigned char data0[SMD_BUF_SIZE]; > @@ -272,6 +273,7 @@ struct smd_shared_v1 { >        unsigned char data1[SMD_BUF_SIZE]; >  }; > > +/* Used on SMD package v4 */ >  struct smd_shared_v2 { >        struct smd_half_channel ch0; >        struct smd_half_channel ch1; > @@ -330,4 +332,56 @@ uint32_t raw_smsm_get_state(enum smsm_state_item item); > >  extern void msm_init_last_radio_log(struct module *); > > +#ifdef CONFIG_MSM_SMD_PKG3 > +/* > + * This allocator assumes an SMD Package v3 which only exists on > + * MSM7x00 SoC's. > + */ > +static inline int _smd_alloc_channel(struct smd_channel *ch) > +{ > +       struct smd_shared_v1 *shared1; > + > +       shared1 = smem_alloc(ID_SMD_CHANNELS + ch->n, sizeof(*shared1)); > +       if (!shared1) { > +               pr_err("smd_alloc_channel() cid %d does not exist\n", ch->n); > +               return -1; > +       } > +       ch->send = &shared1->ch0; > +       ch->recv = &shared1->ch1; > +       ch->send_data = shared1->data0; > +       ch->recv_data = shared1->data1; > +       ch->fifo_size = SMD_BUF_SIZE; > +       return 0; > +} > +#else > +/* > + * This allocator assumes an SMD Package v4, the most common > + * and the default. > + */ > +static inline int _smd_alloc_channel(struct smd_channel *ch) > +{ > +       struct smd_shared_v2 *shared2; > +       void *buffer; > +       unsigned buffer_sz; > + > +       shared2 = smem_alloc(SMEM_SMD_BASE_ID + ch->n, sizeof(*shared2)); > +       buffer = smem_item(SMEM_SMD_FIFO_BASE_ID + ch->n, &buffer_sz); > + > +       if (!buffer) > +               return -1; > + > +       /* buffer must be a power-of-two size */ > +       if (buffer_sz & (buffer_sz - 1)) > +               return -1; > + > +       buffer_sz /= 2; > +       ch->send = &shared2->ch0; > +       ch->recv = &shared2->ch1; > +       ch->send_data = buffer; > +       ch->recv_data = buffer + buffer_sz; > +       ch->fifo_size = buffer_sz; > +       return 0; > +} > +#endif /* CONFIG_ARCH_MSM7X00A */ get rid of the comment Also, I'm not sure I see the benefit of putting these functions the way they are as inlines into this header file. There's only a single call site to this new alloc_channel function, and moving the code out of the smd.c and putting them here doesn't make anything cleaner. --Dima > + >  #endif > -- > 1.6.2.3 > >