Re: [PATCH v2 3/7] OMAP4: hwmod data: add mailbox data
Hi Omar,
On 11/7/2010 10:07 AM, Ramirez Luna, Omar wrote:
On Sat, Nov 6, 2010 at 12:18 PM, Cousson, Benoitb-cous...@ti.com wrote:
I don't know why, but this patch has nothing to do with my original one.
Can you stick to the original code?
no, apart from the ordering of structure members, that I will change,
If you do want to change a patch already submitted to loml, the least
you can do is to comment on the mailing list, and afaik, you didn't
provide any comment on this code.
So do not change it without any explanation or any rational.
That's the most basic rule of the patch submission / review process in
Linux.
since keeping the order of the original structure doesn't fly, I don't
see anything that needs to be changed.
- The magic numbers replaced for the defines, afaik it gives more clarity.
No, since these are not magic numbers but physical address or dma number
or irq channel.
That does not give any more clarity to add a define, and in fact it adds
an extra level of indirection for nothing.
The hwmod file is the unique HW definition files. So all the
information that used to be scattered all over various header files will
have to be there.
You should considered this file as a global SoC HW definition file.
- mailbox irq has a name.
Which is in the structure anyway, so again no need to add a define.
All the structure that are populated hrer are all unique to every IP, so
having that kind of assignment does not seems very useful to me.
omap44xx_mailbox_irq = OMAP44XX_MAILBOX_IRQ;
whereas that:
omap44xx_mailbox_irq = 20;
Give you a little be more information, because you do not have have to
read the TRM or another header file to get the real number.
FYI, that was discussed at least 6 months ago during the submission of
the early hwmod series.
- overall defining block was improved:
class
ocp_if
slave ports
hwmod
If you see, each dependent reference is right before the structure
that is using it, which at least to me establishes some order, as of
today this ordering doesn't exists.
e.g. you are defining some hwmod and some how you are populating all
the members, if you are looking at your omap_hwmod struct and want to
see the irqs defined you need to scroll beyond the supposed first
reference in omap_hwmod (right now above ocp_if)
So what? If you have any issue with the original order, please feel free
to comment on the original patch.
In case you didn't notice, all the OMAP4 data are following the same
pattern. So any change to the structure should be applied everywhere.
I do not have any issue to improve the overall readability if that make
sense for everybody, but again, please comment first.
On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
+/* mailbox */
The original comment is missing.
quote
/*
* 'mailbox' class
* mailbox module allowing communication between the on-chip processors
* useusing a queued mailbox-interrupt mechanism.
*/
/quote
I don't think it adds anything to the patch, should we start
commenting on the functionality of the drivers for each hwmod?
In that case, it does not hurt since this file is the C file version of
the TRM. You might not care because you know what that module is doing,
but most people don't.
+
+static struct omap_hwmod omap44xx_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
+ {
+ .pa_start = OMAP44XX_MAILBOX_BASE,
If that physical address is not used elsewhere, and it should be the case,
there is no need to create a define for it. That's why the physical address
was directly used here.
There is no added value to create a define for that.
yes there is, apart from readability where '0x4a0f4000' doesn't say much
It says everything... when you use a debugger, what kind of information
OMAP44XX_MAILBOX_BASE will give you? Nothing, you will have to check
again the TRM or the define to get the real useful information.
for me at least, if reviewing I need to open the TRM check if that is
the address and move on, with the define you know that someone have
checked the address before (when creating the define)
And what if the define is wrong??? You have as well to check the TRM...
FYI, that file is automatically generated from the HW data, so it has a
much lower probably to contain wrong data. Especially compared to a
manually written header file done from a buggy TRM.
Regards,
Benoit
--
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 3/7] OMAP4: hwmod data: add mailbox data
Hi Benoit,
On Mon, Nov 8, 2010 at 2:56 AM, Cousson, Benoit b-cous...@ti.com wrote:
no, apart from the ordering of structure members, that I will change,
If you do want to change a patch already submitted to loml, the least you
can do is to comment on the mailing list, and afaik, you didn't provide any
comment on this code.
So do not change it without any explanation or any rational.
That's the most basic rule of the patch submission / review process in
Linux.
I'm submitting my changes to review, I accept that I should have put
loudly an clear the reasons why the ordering was changing. I guess you
are the maintainer of the code, right? so if you say the ordering
standard is omap4 then I'm fine with such triviality.
since keeping the order of the original structure doesn't fly, I don't
see anything that needs to be changed.
- The magic numbers replaced for the defines, afaik it gives more clarity.
No, since these are not magic numbers but physical address or dma number or
irq channel.
That does not give any more clarity to add a define, and in fact it adds an
extra level of indirection for nothing.
The hwmod file is the unique HW definition files. So all the information
that used to be scattered all over various header files will have to be
there.
You should considered this file as a global SoC HW definition file.
Ok, I don't mind changing this too, I'm glad you clarify/review this
before more hwmods make it to the tree, following the same style I'm
using which is the one already there.
- mailbox irq has a name.
Which is in the structure anyway, so again no need to add a define.
I didn't add a define for the irq name, I put the irq name because it
wasn't there.
All the structure that are populated hrer are all unique to every IP, so
having that kind of assignment does not seems very useful to me.
omap44xx_mailbox_irq = OMAP44XX_MAILBOX_IRQ;
whereas that:
omap44xx_mailbox_irq = 20;
Give you a little be more information, because you do not have have to read
the TRM or another header file to get the real number.
FYI, that was discussed at least 6 months ago during the submission of the
early hwmod series.
- overall defining block was improved:
class
ocp_if
slave ports
hwmod
If you see, each dependent reference is right before the structure
that is using it, which at least to me establishes some order, as of
today this ordering doesn't exists.
e.g. you are defining some hwmod and some how you are populating all
the members, if you are looking at your omap_hwmod struct and want to
see the irqs defined you need to scroll beyond the supposed first
reference in omap_hwmod (right now above ocp_if)
So what? If you have any issue with the original order, please feel free to
comment on the original patch.
In case you didn't notice, all the OMAP4 data are following the same
pattern. So any change to the structure should be applied everywhere.
I do not have any issue to improve the overall readability if that make
sense for everybody, but again, please comment first.
Again, I should have sent an RFC explaining such changes. will do.
I feel commenting in the original patch is useless because this code
is already in-tree, it might have already changed.
quote
/*
* 'mailbox' class
* mailbox module allowing communication between the on-chip processors
* useusing a queued mailbox-interrupt mechanism.
*/
/quote
I don't think it adds anything to the patch, should we start
commenting on the functionality of the drivers for each hwmod?
In that case, it does not hurt since this file is the C file version of the
TRM. You might not care because you know what that module is doing, but most
people don't.
Ok, then.
+static struct omap_hwmod omap44xx_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
+ {
+ .pa_start = OMAP44XX_MAILBOX_BASE,
If that physical address is not used elsewhere, and it should be the
case,
there is no need to create a define for it. That's why the physical
address
was directly used here.
There is no added value to create a define for that.
yes there is, apart from readability where '0x4a0f4000' doesn't say much
It says everything... when you use a debugger, what kind of information
OMAP44XX_MAILBOX_BASE will give you? Nothing, you will have to check again
the TRM or the define to get the real useful information.
Why is that? if you use a debugger you will see the address, if you
use an objdump you can easily traverse to the definition (with cscope,
ctags...). Again, I don't mind placing the hex number there, now that
there is an explanation for doing so.
Overall, I feel the lack of information made the code look as it is
right now for the existing LO hwmods, given that it is also a matter
of ordering stuff it should be pretty straight forward. I'll give you
my proposal offline, if you think it makes sense we can
Re: [PATCH v2 3/7] OMAP4: hwmod data: add mailbox data
On Sat, Nov 6, 2010 at 12:18 PM, Cousson, Benoit b-cous...@ti.com wrote:
I don't know why, but this patch has nothing to do with my original one.
Can you stick to the original code?
no, apart from the ordering of structure members, that I will change,
since keeping the order of the original structure doesn't fly, I don't
see anything that needs to be changed.
- The magic numbers replaced for the defines, afaik it gives more clarity.
- mailbox irq has a name.
- overall defining block was improved:
class
ocp_if
slave ports
hwmod
If you see, each dependent reference is right before the structure
that is using it, which at least to me establishes some order, as of
today this ordering doesn't exists.
e.g. you are defining some hwmod and some how you are populating all
the members, if you are looking at your omap_hwmod struct and want to
see the irqs defined you need to scroll beyond the supposed first
reference in omap_hwmod (right now above ocp_if)
On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
+/* mailbox */
The original comment is missing.
quote
/*
* 'mailbox' class
* mailbox module allowing communication between the on-chip processors
* useusing a queued mailbox-interrupt mechanism.
*/
/quote
I don't think it adds anything to the patch, should we start
commenting on the functionality of the drivers for each hwmod?
+
+static struct omap_hwmod omap44xx_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
+ {
+ .pa_start = OMAP44XX_MAILBOX_BASE,
If that physical address is not used elsewhere, and it should be the case,
there is no need to create a define for it. That's why the physical address
was directly used here.
There is no added value to create a define for that.
yes there is, apart from readability where '0x4a0f4000' doesn't say much
for me at least, if reviewing I need to open the TRM check if that is
the address and move on, with the define you know that someone have
checked the address before (when creating the define)
besides the define was already there
+static struct omap_hwmod omap44xx_mailbox_hwmod = {
+ .name = mailbox,
+ .class =omap44xx_mailbox_hwmod_class,
+ .prcm = {
+ .omap4 = {
+ .clkctrl_reg = OMAP4430_CM_L4CFG_MAILBOX_CLKCTRL,
+ },
+ },
+ .mpu_irqs = omap44xx_mailbox_irqs,
+ .mpu_irqs_cnt = ARRAY_SIZE(omap44xx_mailbox_irqs),
The order is different than the previous one.
as the order is given by omap4 data, and not the definition of the
structure, I'll keep the consistency with omap4 then.
Regards,
Omar
--
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 3/7] OMAP4: hwmod data: add mailbox data
I don't know why, but this patch has nothing to do with my original one.
Can you stick to the original code?
Of course, if you have valid comments or need to add extra data, you
can, but in this case, I do not see any needed change.
On 11/5/2010 9:17 PM, Ramirez Luna, Omar wrote:
From: Benoit Coussonb-cous...@ti.com
hwmod data for omap4 mailbox.
Signed-off-by: Benoit Coussonb-cous...@ti.com
Signed-off-by: Omar Ramirez Lunaomar.rami...@ti.com
---
arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 63
1 files changed, 63 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 0d5c6eb..f7525e3 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -1043,6 +1043,66 @@ static struct omap_hwmod omap44xx_uart4_hwmod = {
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
};
+/* mailbox */
The original comment is missing.
+
+static struct omap_hwmod_class_sysconfig omap44xx_mailbox_sysc = {
+ .rev_offs = 0x,
+ .sysc_offs = 0x0010,
+ .sysc_flags = (SYSC_HAS_RESET_STATUS | SYSC_HAS_SIDLEMODE |
+ SYSC_HAS_SOFTRESET),
+ .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+ .sysc_fields=omap_hwmod_sysc_type2,
+};
+
+static struct omap_hwmod_class omap44xx_mailbox_hwmod_class = {
+ .name = mailbox,
+ .sysc =omap44xx_mailbox_sysc,
+};
+
+static struct omap_hwmod omap44xx_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
+ {
+ .pa_start = OMAP44XX_MAILBOX_BASE,
If that physical address is not used elsewhere, and it should be the
case, there is no need to create a define for it. That's why the
physical address was directly used here.
There is no added value to create a define for that.
+ .pa_end = OMAP44XX_MAILBOX_BASE + SZ_4K - 1,
+ .flags = ADDR_TYPE_RT,
+ },
+};
+
+/* l4_cfg - mailbox */
+static struct omap_hwmod_ocp_if omap44xx_l4_cfg__mailbox = {
+ .master =omap44xx_l4_cfg_hwmod,
+ .slave =omap44xx_mailbox_hwmod,
+ .clk= l4_div_ck,
+ .addr = omap44xx_mailbox_addrs,
+ .addr_cnt = ARRAY_SIZE(omap44xx_mailbox_addrs),
+ .user = OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* mailbox slave ports */
+static struct omap_hwmod_ocp_if *omap44xx_mailbox_slaves[] = {
+ omap44xx_l4_cfg__mailbox,
+};
+
+static struct omap_hwmod_irq_info omap44xx_mailbox_irqs[] = {
+ { .name = mbox, .irq = 26 + OMAP44XX_IRQ_GIC_START, },
+};
+
+static struct omap_hwmod omap44xx_mailbox_hwmod = {
+ .name = mailbox,
+ .class =omap44xx_mailbox_hwmod_class,
+ .prcm = {
+ .omap4 = {
+ .clkctrl_reg = OMAP4430_CM_L4CFG_MAILBOX_CLKCTRL,
+ },
+ },
+ .mpu_irqs = omap44xx_mailbox_irqs,
+ .mpu_irqs_cnt = ARRAY_SIZE(omap44xx_mailbox_irqs),
The order is different than the previous one.
Benoit
+ .slaves = omap44xx_mailbox_slaves,
+ .slaves_cnt = ARRAY_SIZE(omap44xx_mailbox_slaves),
+ .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
+};
+
static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
/* dmm class */
omap44xx_dmm_hwmod,
@@ -1077,6 +1137,9 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
omap44xx_uart2_hwmod,
omap44xx_uart3_hwmod,
omap44xx_uart4_hwmod,
+
+ /* mailbox */
+ omap44xx_mailbox_hwmod,
NULL,
};
--
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
[PATCH v2 3/7] OMAP4: hwmod data: add mailbox data
From: Benoit Cousson b-cous...@ti.com
hwmod data for omap4 mailbox.
Signed-off-by: Benoit Cousson b-cous...@ti.com
Signed-off-by: Omar Ramirez Luna omar.rami...@ti.com
---
arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 63
1 files changed, 63 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 0d5c6eb..f7525e3 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -1043,6 +1043,66 @@ static struct omap_hwmod omap44xx_uart4_hwmod = {
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
};
+/* mailbox */
+
+static struct omap_hwmod_class_sysconfig omap44xx_mailbox_sysc = {
+ .rev_offs = 0x,
+ .sysc_offs = 0x0010,
+ .sysc_flags = (SYSC_HAS_RESET_STATUS | SYSC_HAS_SIDLEMODE |
+ SYSC_HAS_SOFTRESET),
+ .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+ .sysc_fields= omap_hwmod_sysc_type2,
+};
+
+static struct omap_hwmod_class omap44xx_mailbox_hwmod_class = {
+ .name = mailbox,
+ .sysc = omap44xx_mailbox_sysc,
+};
+
+static struct omap_hwmod omap44xx_mailbox_hwmod;
+
+static struct omap_hwmod_addr_space omap44xx_mailbox_addrs[] = {
+ {
+ .pa_start = OMAP44XX_MAILBOX_BASE,
+ .pa_end = OMAP44XX_MAILBOX_BASE + SZ_4K - 1,
+ .flags = ADDR_TYPE_RT,
+ },
+};
+
+/* l4_cfg - mailbox */
+static struct omap_hwmod_ocp_if omap44xx_l4_cfg__mailbox = {
+ .master = omap44xx_l4_cfg_hwmod,
+ .slave = omap44xx_mailbox_hwmod,
+ .clk= l4_div_ck,
+ .addr = omap44xx_mailbox_addrs,
+ .addr_cnt = ARRAY_SIZE(omap44xx_mailbox_addrs),
+ .user = OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* mailbox slave ports */
+static struct omap_hwmod_ocp_if *omap44xx_mailbox_slaves[] = {
+ omap44xx_l4_cfg__mailbox,
+};
+
+static struct omap_hwmod_irq_info omap44xx_mailbox_irqs[] = {
+ { .name = mbox, .irq = 26 + OMAP44XX_IRQ_GIC_START, },
+};
+
+static struct omap_hwmod omap44xx_mailbox_hwmod = {
+ .name = mailbox,
+ .class = omap44xx_mailbox_hwmod_class,
+ .prcm = {
+ .omap4 = {
+ .clkctrl_reg = OMAP4430_CM_L4CFG_MAILBOX_CLKCTRL,
+ },
+ },
+ .mpu_irqs = omap44xx_mailbox_irqs,
+ .mpu_irqs_cnt = ARRAY_SIZE(omap44xx_mailbox_irqs),
+ .slaves = omap44xx_mailbox_slaves,
+ .slaves_cnt = ARRAY_SIZE(omap44xx_mailbox_slaves),
+ .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
+};
+
static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
/* dmm class */
omap44xx_dmm_hwmod,
@@ -1077,6 +1137,9 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
omap44xx_uart2_hwmod,
omap44xx_uart3_hwmod,
omap44xx_uart4_hwmod,
+
+ /* mailbox */
+ omap44xx_mailbox_hwmod,
NULL,
};
--
1.7.1
--
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
