[meta-ti] [PATCH] ipc: IPC for communication between multiple processors

Wed Nov 20 07:18:43 PST 2013

See my comments below.

On Wed, Nov 20, 2013 at 01:02:30PM +0000, Maupin, Chase wrote:
> >-----Original Message-----
> >From: meta-ti-bounces at yoctoproject.org [mailto:meta-ti-
> >bounces at yoctoproject.org] On Behalf Of Hingolikar, Mrinmayee
> >Sent: Wednesday, November 20, 2013 5:06 AM
> >To: meta-ti at yoctoproject.org
> >Subject: [meta-ti] [PATCH] ipc: IPC for communication between
> >multiple processors
> 
> I would recommend using --numbered in your patches and creating them as a 
> series to make it easier to determine the order in which they should be 
> applied.  For example this patch should come before you other patch for 
> libdce.

If you extract more than one patch with git-format-patch, then --numbered is 
used by default.

> >Signed-off-by: Mrinmayee Hingolikar <mrinmayee at ti.com>
> >---
> > ...nstallation-prefix-feature-to-products.ma.patch |   38
> >++++++++++++++++++++
> > recipes-ti/ipc/ti-ipc_3.10.00.08.bb                |   30
> >++++++++++++++++
> > 2 files changed, 68 insertions(+)
> > create mode 100644 recipes-ti/ipc/0001-ipc-Added-installation-
> >prefix-feature-to-products.ma.patch
> > create mode 100644 recipes-ti/ipc/ti-ipc_3.10.00.08.bb
> >
> >diff --git a/recipes-ti/ipc/0001-ipc-Added-installation-prefix-
> >feature-to-products.ma.patch b/recipes-ti/ipc/0001-ipc-Added-
> >installation-prefix-feature-to-products.ma.patch
> >new file mode 100644
> >index 0000000..7d5ff94
> >--- /dev/null
> >+++ b/recipes-ti/ipc/0001-ipc-Added-installation-prefix-feature-
> >to-products.ma.patch
> >@@ -0,0 +1,38 @@
> >+From 26d09063063593aec760151393226b96bc7ab9f8 Mon Sep 17 00:00:00
> >2001
> >+From: Mrinmayee Hingolikar <mrinmayee at ti.com>
> >+Date: Thu, 5 Sep 2013 17:01:15 +0530
> >+Subject: [PATCH] ipc: Added installation prefix feature to
> >products.mak
> 
> Can this be driven back into the IPC team to add to their makefile?
> 
> >+
> >+Signed-off-by: Mrinmayee Hingolikar <mrinmayee at ti.com>
> >+---
> >+ ipc-linux.mak |    1 +
> >+ products.mak  |    1 +
> >+ 2 files changed, 2 insertions(+)
> >+
> >+diff --git a/ipc-linux.mak b/ipc-linux.mak
> >+index 788a5a2..01579e4 100644
> >+--- a/ipc-linux.mak
> >++++ b/ipc-linux.mak
> >+@@ -38,6 +38,7 @@ include products.mak
> >+ config:
> >+ 	@echo "Configuring Linux Ipc ..."
> >+ 	./configure --host=$(TOOLCHAIN_LONGNAME) \
> >++            --prefix=$(PREFIX) \
> >+             CC=$(TOOLCHAIN_PREFIX)gcc \
> >+             PLATFORM=$(PLATFORM) \
> >+             CMEM_INSTALL_DIR=$(CMEM_INSTALL_DIR) \
> 
> This line makes me think there is a dependency missing.  In the past 
> something like ti-paths.inc would pass this value along.  I'm not saying 
> that should be replicated as is, but likely you want to se this to 
> STAGING_DIR_TARGET or something similar and the cmem recipe should stage 
> appropriately.
> 
> I think it would be good if you walked through your software stack to make 
> sure you are sending your patches in order.  Likewise, there was some 
> discussion with Sam about cmem recipes and basically about aligning recipes 
> between your groups.  I thin the approach of breaking linux-utils up into a 
> recipe per component is going to be the cleanest approach.  I'll ping on 
> Sam's patches as well to get an update posted to meta-ti instead of 
> meta-arago list.
> 
> Any pointers you can provide to documentation about the configuration and 
> building of these components you are pushing up would also be useful to help 
> in reviewing so we can understand what you are trying to accomplish.

Nice catch. The first question - does it depend and require cmem?

> >+diff --git a/products.mak b/products.mak
> >+index e418d2f..4578a1e 100644
> >+--- a/products.mak
> >++++ b/products.mak
> >+@@ -55,6 +55,7 @@ PLATFORM ?=
> >+ TOOLCHAIN_LONGNAME ?= arm-none-linux-gnueabi
> >+ TOOLCHAIN_INSTALL_DIR ?= $(DEPOT)/_your_arm_code_gen_install_
> >+ TOOLCHAIN_PREFIX ?=
> >$(TOOLCHAIN_INSTALL_DIR)/bin/$(TOOLCHAIN_LONGNAME)-
> >++PREFIX ?= /usr/local
> >+
> >+ # Optional: Path to Linux Kernel - needed to build the MmRpc
> >user libraries
> >+ # (for devices that support it)
> >+--
> >+1.7.9.5
> >+
> >diff --git a/recipes-ti/ipc/ti-ipc_3.10.00.08.bb b/recipes-
> >ti/ipc/ti-ipc_3.10.00.08.bb
> >new file mode 100644
> >index 0000000..645061d
> >--- /dev/null
> >+++ b/recipes-ti/ipc/ti-ipc_3.10.00.08.bb
> >@@ -0,0 +1,30 @@
> >+DESCRIPTION = "TI Inter Process Communication (IPC) Mechanisms
> >(for Uni- and Multi- Processor Configurations)"
> >+HOMEPAGE = "https://git.ti.com/ipc/pages/Home"
> >+LICENSE = "BSD"
> >+
> >+PV = "3_10_00_08"
> 
> I don't think you want to do this.  Why not leave the version . separated?

This is actually more critical than that. Don't use underscores in the 
recipe/package version! If the component requires undersocred version, pass it 
separately.

> >+PR = "r0"
> >+
> >+LIC_FILES_CHKSUM = "file://${S}/ipc-
> >linux.mak;beginline=1;endline=30;md5=f2518e421e230f06fe6d449718d02
> >edc"
> >+
> >+PLATFORM_omap5-evm = "omap54xx_smp"
> >+PLATFORM_dra7xx-evm = "dra7xx"
> >+
> >+inherit autotools pkgconfig
> >+
> >+SRC_URI = "git://git.ti.com/ipc/ipcdev.git;protocol=git \
> >+           file://0001-ipc-Added-installation-prefix-feature-to-
> >products.ma.patch \
> >+           "
> >+SRCREV = "b11251f705f84f32740cd288afe9281e653bd8eb"
> 
> Any need for a BRANCH setting?

Well, it's not a required variables - as a matter of fact, I invented it 
years ago and since it's useful in many cases, it just stuck around... :)

> >+S = "${WORKDIR}/git"
> >+
> >+do_configure() {
> >+	sed -i -e "s#^KERNEL_INSTALL_DIR ?=.*#KERNEL_INSTALL_DIR =
> >${STAGING_KERNEL_DIR}#" ${S}/products.mak
> >+	sed -i -e "s#^TOOLCHAIN_INSTALL_DIR
> >?=.*#TOOLCHAIN_INSTALL_DIR = ${TOOLCHAIN_PATH}#" ${S}/products.mak
> >+	sed -i -e "s#^TOOLCHAIN_LONGNAME ?=.*#TOOLCHAIN_LONGNAME =
> >arm-linux-gnueabihf#" ${S}/products.mak
> >+	sed -i -e "s#^PLATFORM ?=.*#PLATFORM = ${PLATFORM}#"
> >${S}/products.mak
> >+	sed -i -e "s#^PREFIX ?=.*#PREFIX = /usr#" ${S}/products.mak
> 
> Is there a way to drive this back into the IPC team to allow you to pass 
> these values and have them picked up from the command line, rather than 
> using a lot of sed commands?  History has shown in the past that this gets 
> ugly to maintain.

>From what I can see, the above variables are being assigned conditionally in 
the Makefile (uses ?=), meaning that IPC team is already provinding a way to 
cleanly pass those variables from outside. So, there's no need to sed those 
vars! You are meant to pass them on the command line to oe_runmake...

-- 
Denys