From: "majun (F)" <majun258@huawei.com> To: Thomas Gleixner <tglx@linutronix.de> Cc: Catalin.Marinas@arm.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Will.Deacon@arm.com, mark.rutland@arm.com, marc.zyngier@arm.com, jason@lakedaemon.net, lizefan@huawei.com, huxinwei@huawei.com, dingtianhong <dingtianhong@huawei.com>, 吴云 <wuyun.wu@huawei.com>, 赵俊化 <zhaojunhua@hisilicon.com>, "Liguozhu (Kenneth)" <liguozhu@hisilicon.com>, 许威 <xuwei5@hisilicon.com>, chenwei <wei.chenwei@hisilicon.com> Subject: Re: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt Date: Mon, 15 Jun 2015 15:05:49 +0800 [thread overview] Message-ID: <557E794D.2080705@huawei.com> (raw) In-Reply-To: <alpine.DEB.2.11.1506121230320.3786@nanos> 在 2015/6/12 18:48, Thomas Gleixner 写道: > On Fri, 12 Jun 2015, Ma Jun wrote: > >> This patch is applied to support the mbigen interrupt. >> >> As a kind of MSI interrupt controller, the mbigen is used as a child >> domain of ITS domain just like PCI devices. >> So the arm-gic-v3-its and related files are changed. >> >> The chip.c is also changed to check irq_ach before it called. > > This patch wants to be split into several: > > 1) Changes to the core code > > 2) New functionality in the core code > > 2) Changes to gic-v3-its > > And all patches require proper changelogs which explain WHY these > changes are necessary. > > We can see which files are changed from the diffstat and the patch > ourself. So no point to mention this in the changelog. > > But we cannot figure out from looking at the code WHY you think that > your approach to solve the problem is the right one. > >> void irq_chip_ack_parent(struct irq_data *data) >> { >> data = data->parent_data; >> - data->chip->irq_ack(data); >> + if (data->chip->irq_ack) >> + data->chip->irq_ack(data); > > Why is this required? Just because? Again, you fail to provide a > rationale for the changes to the irq_chip*parent() functions. > > Why would you call irq_chip_ack_parent() if that parent does not > provide the required functionality in the first place? > Yes, this is not a necessary callback. I will remove this callback from mbigen driver. >> /* >> @@ -363,6 +364,9 @@ struct irq_chip { >> int (*irq_request_resources)(struct irq_data *data); >> void (*irq_release_resources)(struct irq_data *data); >> >> + void (*irq_compose_mbigen_msg)(struct irq_data *data, struct mbigen_msg *msg); >> + void (*irq_write_mbigen_msg)(struct irq_data *data, struct mbigen_msg *msg); >> + > > What's so special about mbigen to justify extra callbacks which just > bloat the data structure for everyone. Why are the msi callbacks not > sufficient? > > MBI is just another variant of MSI, right? > yes,MBI is a kind of MSI which used for non-pci devices. According to Marc's advice, the irq hierachy structure in my patch likes below: non-pci devices-->mbigen-->its-->gic pci devices -->msi __/ Eventhough the function *irq_compose_mbigen_msg does the same thing as *irq_chip_compose_msi_msg, I still added this function. Because I don't want mix the code used by msi(pci devices) with the code used by mbigen. > struct mbigen_msg { > u32 address_lo; > u32 address_hi; > u32 data; > }; > > struct mbigen_msg is just a mindless copy of struct msi_msg: > > struct msi_msg { > u32 address_lo; /* low 32 bits of msi message address */ > u32 address_hi; /* high 32 bits of msi message address */ > u32 data; /* 16 bits of msi message data */ > }; > > So what's the point of this? > Based on the same reason, I also added structure mbigen_msg for mbigen using. >> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg); >> void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg); >> > >> + >> +/** >> + * irq_chip_compose_mbigen_msg - Componse mbigen message for a mbigen irq chip >> + * @data: Pointer to interrupt specific data >> + * @msg: Pointer to the mbigen message >> + * >> + * For hierarchical domains we find the first chip in the hierarchy >> + * which implements the irq_compose_mbigen_msg callback. For non >> + * hierarchical we use the top level chip. >> + */ >> + >> +int irq_chip_compose_mbigen_msg(struct irq_data *data, struct mbigen_msg *msg) >> +{ >> + struct irq_data *pos = NULL; >> + >> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY >> + for (; data; data = data->parent_data) >> +#endif >> + if (data->chip && data->chip->irq_compose_mbigen_msg) >> + pos = data; >> + if (!pos) >> + return -ENOSYS; >> + >> + pos->chip->irq_compose_mbigen_msg(pos, msg); >> + >> + return 0; >> +} > > Again, this is a completely useless copy of irq_chip_compose_msi_msg(). > Why can't you just use the existing callbacks and use struct msi_msg > for your special chip? > As mentioned before, to avoid using the code of msi, i added this function.Because they are different domain. If you don't mind, I can use the irq_chip_compose_msi_msg function in mbigen driver instead of irq_chip_compose_mbigen_msg. > And w/o looking at the mbigen code in detail, I bet it's nothing else > than MSI for non PCI devices and contains tons of redundant and copied > code, right? > > Can you please provide a proper description of this mbigen chip and > explain WHY you think that it needs all this special hackery? > > Thanks, > > tglx > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > . >
WARNING: multiple messages have this Message-ID (diff)
From: majun258@huawei.com (majun (F)) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt Date: Mon, 15 Jun 2015 15:05:49 +0800 [thread overview] Message-ID: <557E794D.2080705@huawei.com> (raw) In-Reply-To: <alpine.DEB.2.11.1506121230320.3786@nanos> ? 2015/6/12 18:48, Thomas Gleixner ??: > On Fri, 12 Jun 2015, Ma Jun wrote: > >> This patch is applied to support the mbigen interrupt. >> >> As a kind of MSI interrupt controller, the mbigen is used as a child >> domain of ITS domain just like PCI devices. >> So the arm-gic-v3-its and related files are changed. >> >> The chip.c is also changed to check irq_ach before it called. > > This patch wants to be split into several: > > 1) Changes to the core code > > 2) New functionality in the core code > > 2) Changes to gic-v3-its > > And all patches require proper changelogs which explain WHY these > changes are necessary. > > We can see which files are changed from the diffstat and the patch > ourself. So no point to mention this in the changelog. > > But we cannot figure out from looking at the code WHY you think that > your approach to solve the problem is the right one. > >> void irq_chip_ack_parent(struct irq_data *data) >> { >> data = data->parent_data; >> - data->chip->irq_ack(data); >> + if (data->chip->irq_ack) >> + data->chip->irq_ack(data); > > Why is this required? Just because? Again, you fail to provide a > rationale for the changes to the irq_chip*parent() functions. > > Why would you call irq_chip_ack_parent() if that parent does not > provide the required functionality in the first place? > Yes, this is not a necessary callback. I will remove this callback from mbigen driver. >> /* >> @@ -363,6 +364,9 @@ struct irq_chip { >> int (*irq_request_resources)(struct irq_data *data); >> void (*irq_release_resources)(struct irq_data *data); >> >> + void (*irq_compose_mbigen_msg)(struct irq_data *data, struct mbigen_msg *msg); >> + void (*irq_write_mbigen_msg)(struct irq_data *data, struct mbigen_msg *msg); >> + > > What's so special about mbigen to justify extra callbacks which just > bloat the data structure for everyone. Why are the msi callbacks not > sufficient? > > MBI is just another variant of MSI, right? > yes,MBI is a kind of MSI which used for non-pci devices. According to Marc's advice, the irq hierachy structure in my patch likes below: non-pci devices-->mbigen-->its-->gic pci devices -->msi __/ Eventhough the function *irq_compose_mbigen_msg does the same thing as *irq_chip_compose_msi_msg, I still added this function. Because I don't want mix the code used by msi(pci devices) with the code used by mbigen. > struct mbigen_msg { > u32 address_lo; > u32 address_hi; > u32 data; > }; > > struct mbigen_msg is just a mindless copy of struct msi_msg: > > struct msi_msg { > u32 address_lo; /* low 32 bits of msi message address */ > u32 address_hi; /* high 32 bits of msi message address */ > u32 data; /* 16 bits of msi message data */ > }; > > So what's the point of this? > Based on the same reason, I also added structure mbigen_msg for mbigen using. >> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg); >> void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg); >> > >> + >> +/** >> + * irq_chip_compose_mbigen_msg - Componse mbigen message for a mbigen irq chip >> + * @data: Pointer to interrupt specific data >> + * @msg: Pointer to the mbigen message >> + * >> + * For hierarchical domains we find the first chip in the hierarchy >> + * which implements the irq_compose_mbigen_msg callback. For non >> + * hierarchical we use the top level chip. >> + */ >> + >> +int irq_chip_compose_mbigen_msg(struct irq_data *data, struct mbigen_msg *msg) >> +{ >> + struct irq_data *pos = NULL; >> + >> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY >> + for (; data; data = data->parent_data) >> +#endif >> + if (data->chip && data->chip->irq_compose_mbigen_msg) >> + pos = data; >> + if (!pos) >> + return -ENOSYS; >> + >> + pos->chip->irq_compose_mbigen_msg(pos, msg); >> + >> + return 0; >> +} > > Again, this is a completely useless copy of irq_chip_compose_msi_msg(). > Why can't you just use the existing callbacks and use struct msi_msg > for your special chip? > As mentioned before, to avoid using the code of msi, i added this function.Because they are different domain. If you don't mind, I can use the irq_chip_compose_msi_msg function in mbigen driver instead of irq_chip_compose_mbigen_msg. > And w/o looking at the mbigen code in detail, I bet it's nothing else > than MSI for non PCI devices and contains tons of redundant and copied > code, right? > > Can you please provide a proper description of this mbigen chip and > explain WHY you think that it needs all this special hackery? > > Thanks, > > tglx > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > . >
next prev parent reply other threads:[~2015-06-15 7:07 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-06-12 2:49 [PATCH v2 0/3] IRQ/Gic-V3:Support Mbigen interrupt controller Ma Jun 2015-06-12 2:49 ` Ma Jun 2015-06-12 2:49 ` [PATCH v2 1/3] IRQ/Gic-V3: Add mbigen driver to support mbigen " Ma Jun 2015-06-12 2:49 ` Ma Jun 2015-06-12 2:49 ` [PATCH v2 2/3] IRQ/Gic-V3: Change arm-gic-its to support the Mbigen interrupt Ma Jun 2015-06-12 2:49 ` Ma Jun 2015-06-12 10:48 ` Thomas Gleixner 2015-06-12 10:48 ` Thomas Gleixner 2015-06-15 7:05 ` majun (F) [this message] 2015-06-15 7:05 ` majun (F) 2015-06-18 23:52 ` Thomas Gleixner 2015-06-18 23:52 ` Thomas Gleixner 2015-06-23 9:03 ` majun (F) 2015-06-23 9:03 ` majun (F) 2015-06-23 9:29 ` Thomas Gleixner 2015-06-23 9:29 ` Thomas Gleixner 2015-06-26 8:45 ` Marc Zyngier 2015-06-26 8:45 ` Marc Zyngier 2015-06-26 6:31 ` majun (F) 2015-06-26 6:31 ` majun (F) 2015-06-26 8:44 ` Marc Zyngier 2015-06-26 8:44 ` Marc Zyngier 2015-06-26 10:28 ` majun (F) 2015-06-26 10:28 ` majun (F) 2015-06-26 10:40 ` Marc Zyngier 2015-06-26 10:40 ` Marc Zyngier 2015-06-26 12:04 ` majun (F) 2015-06-26 12:04 ` majun (F) 2015-06-26 13:14 ` Marc Zyngier 2015-06-26 13:14 ` Marc Zyngier 2015-06-12 2:49 ` [PATCH v2 3/3] dt-binding:Documents the mbigen bindings Ma Jun 2015-06-12 2:49 ` Ma Jun
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=557E794D.2080705@huawei.com \ --to=majun258@huawei.com \ --cc=Catalin.Marinas@arm.com \ --cc=Will.Deacon@arm.com \ --cc=dingtianhong@huawei.com \ --cc=huxinwei@huawei.com \ --cc=jason@lakedaemon.net \ --cc=liguozhu@hisilicon.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lizefan@huawei.com \ --cc=marc.zyngier@arm.com \ --cc=mark.rutland@arm.com \ --cc=tglx@linutronix.de \ --cc=wei.chenwei@hisilicon.com \ --cc=wuyun.wu@huawei.com \ --cc=xuwei5@hisilicon.com \ --cc=zhaojunhua@hisilicon.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.