Freescale mpc8315 IRQ0 setup

Am Dienstag, 2. Mai 2017, 22:29:34 schrieb Scott Wood:
> On Tue, 2017-05-02 at 14:43 +0200, Juergen Schindele wrote:
> > Dear Scott,
> > sorry for the delay but i am not very familiar with the formating.
> > I passed the patch trough checkpatch.pl and there was no more error.
> > pease find patch in attached file.
> > Thanks
> 
> Documentation/process/submitting-patches.rst explains the way to format and
> submit kernel patches.
> 
> Also, why the unrelated change to a print statement in ipic_set_irq_type()?
> 
> -Scott
The second diff is not completely unrelated because when i was investigating 
the problem i saw only a message "edge sense not supported" but you dont
know on which interrupt he is complaining about. So i added this to find out 
who the suspect is.

Corrected patch

On Thu, 2017-05-04 at 17:06 +0200, Juergen Schindele wrote:
> Am Dienstag, 2. Mai 2017, 22:29:34 schrieb Scott Wood:
> > On Tue, 2017-05-02 at 14:43 +0200, Juergen Schindele wrote:
> > > Dear Scott,
> > > sorry for the delay but i am not very familiar with the formating.
> > > I passed the patch trough checkpatch.pl and there was no more error.
> > > pease find patch in attached file.
> > > Thanks
> > 
> > Documentation/process/submitting-patches.rst explains the way to format
> > and
> > submit kernel patches.
> > 
> > Also, why the unrelated change to a print statement in
> > ipic_set_irq_type()?
> > 
> > -Scott
> 
> The second diff is not completely unrelated because when i was
> investigating 
> the problem i saw only a message "edge sense not supported" but you dont
> know on which interrupt he is complaining about. So i added this to find
> out 
> who the suspect is.

That's fine but it's still fixing a different problem than "irq0 setup" and
should be a separate patch.

> Corrected patch

Again, please read Documentation/process/submitting-patches.rst.  Patches
should be inline, not attached.  The subject line should be something like
"powerpc/ipic: Configure "EDGE" capabilities for IRQ0 (like IRQ1-7)" and there
should be more description in the body of the changelog.

-Scott

Next try to submit two patches for Freescale mpc8315.

first one
-----------------------------------------------------------------------------------
powerpc/ipic: Configure "EDGE" capabilities for IRQ0 too (like IRQ1-7)
Signed-off-by: Jurgen Schindele <schindele@nentec.de>

The external IRQ0 has the same capabilities as the other IRQ1-7 and is
handeled by the same register IPIC_SEPNR. When this register for "ack"
is not setup in "ipic_info" you can not configure this IRQ for
IRQ_TYPE_EDGE_FALLING. This is probably due to the non-continued number
of IRQ0 in the Freescale hwirq number mapping.

--- linux-4.11/arch/powerpc/sysdev/ipic.c.orig  2017-05-11 13:40:43.874801534 
+0200
+++ linux-4.11/arch/powerpc/sysdev/ipic.c       2017-05-11 14:05:06.336610289 
+0200
@@ -315,6 +315,7 @@ static struct ipic_info ipic_info[] = {
                .prio_mask = 7,
        },
        [48] = {
+               .ack    = IPIC_SEPNR,
                .mask   = IPIC_SEMSR,
                .prio   = IPIC_SMPRR_A,
                .force  = IPIC_SEFCR,

second one
-----------------------------------------------------------------------------------------
powerpc/ipic: Precise ERR printk when "ipic_set_irq_type" fails
Signed-off-by: Jurgen Schindele <schindele@nentec.de>

When you setup an interrupt of a certain type on this interrupt-controler
which is not supported on this hardware you will get an error log but you
dont know which interrupt was the cause.
With this patch you are able to localize the source of failure.

--- linux-4.11/arch/powerpc/sysdev/ipic.c.orig  2017-05-11 14:08:07.098824986 
+0200
+++ linux-4.11/arch/powerpc/sysdev/ipic.c       2017-05-11 14:10:51.010934755 
+0200
@@ -611,14 +611,14 @@ static int ipic_set_irq_type(struct irq_
        /* ipic supports only low assertion and high-to-low change senses
         */
        if (!(flow_type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))) {
-               printk(KERN_ERR "ipic: sense type 0x%x not supported\n",
-                       flow_type);
+               printk(KERN_ERR "ipic: sense type 0x%x not supported on "
+                       "interrupt %d\n", flow_type, src);
                return -EINVAL;
        }
        /* ipic supports only edge mode on external interrupts */
        if ((flow_type & IRQ_TYPE_EDGE_FALLING) && !ipic_info[src].ack) {
                printk(KERN_ERR "ipic: edge sense not supported on internal "
-                               "interrupts\n");
+                               "interrupt %d\n", src);
                return -EINVAL;

        }

I hope i respected all patch requirements ;-)
Am Donnerstag, 4. Mai 2017, 15:55:28 schrieb Scott Wood:
> On Thu, 2017-05-04 at 17:06 +0200, Juergen Schindele wrote:
> > Am Dienstag, 2. Mai 2017, 22:29:34 schrieb Scott Wood:
> > > On Tue, 2017-05-02 at 14:43 +0200, Juergen Schindele wrote:
> > > > Dear Scott,
> > > > sorry for the delay but i am not very familiar with the formating.
> > > > I passed the patch trough checkpatch.pl and there was no more error.
> > > > pease find patch in attached file.
> > > > Thanks
> > > 
> > > Documentation/process/submitting-patches.rst explains the way to format
> > > and
> > > submit kernel patches.
> > > 
> > > Also, why the unrelated change to a print statement in
> > > ipic_set_irq_type()?
> > > 
> > > -Scott
> > 
> > The second diff is not completely unrelated because when i was
> > investigating 
> > the problem i saw only a message "edge sense not supported" but you dont
> > know on which interrupt he is complaining about. So i added this to find
> > out 
> > who the suspect is.
> 
> That's fine but it's still fixing a different problem than "irq0 setup" and
> should be a separate patch.
> 
> > Corrected patch
> 
> Again, please read Documentation/process/submitting-patches.rst.  Patches
> should be inline, not attached.  The subject line should be something like
> "powerpc/ipic: Configure "EDGE" capabilities for IRQ0 (like IRQ1-7)" and
> there should be more description in the body of the changelog.
> 
> -Scott
Thanks for your hints

On Thu, May 11, 2017 at 10:34 PM, Juergen Schindele <schindele@nentec.de> wrote:
> Next try to submit two patches for Freescale mpc8315.
>
> first one
> -----------------------------------------------------------------------------------
> powerpc/ipic: Configure "EDGE" capabilities for IRQ0 too (like IRQ1-7)
> Signed-off-by: Jurgen Schindele <schindele@nentec.de>
>
> The external IRQ0 has the same capabilities as the other IRQ1-7 and is
> handeled by the same register IPIC_SEPNR. When this register for "ack"
> is not setup in "ipic_info" you can not configure this IRQ for
> IRQ_TYPE_EDGE_FALLING. This is probably due to the non-continued number
> of IRQ0 in the Freescale hwirq number mapping.
>
> --- linux-4.11/arch/powerpc/sysdev/ipic.c.orig  2017-05-11 13:40:43.874801534
> +0200
> +++ linux-4.11/arch/powerpc/sysdev/ipic.c       2017-05-11 14:05:06.336610289
> +0200
> @@ -315,6 +315,7 @@ static struct ipic_info ipic_info[] = {
>                 .prio_mask = 7,
>         },
>         [48] = {
> +               .ack    = IPIC_SEPNR,
>                 .mask   = IPIC_SEMSR,
>                 .prio   = IPIC_SMPRR_A,
>                 .force  = IPIC_SEFCR,
>
> second one
> -----------------------------------------------------------------------------------------

> powerpc/ipic: Precise ERR printk when "ipic_set_irq_type" fails
> Signed-off-by: Jurgen Schindele <schindele@nentec.de>

The Signed-off-by: goes after the commit message. So rather than being here:
| >
| > When you setup an interrupt of a certain type on this interrupt-controler
| > which is not supported on this hardware you will get an error log but you
| > dont know which interrupt was the cause.
| > With this patch you are able to localize the source of failure.
|
\---> you want it down here

>
> --- linux-4.11/arch/powerpc/sysdev/ipic.c.orig  2017-05-11 14:08:07.098824986
> +0200
> +++ linux-4.11/arch/powerpc/sysdev/ipic.c       2017-05-11 14:10:51.010934755
> +0200
> @@ -611,14 +611,14 @@ static int ipic_set_irq_type(struct irq_
>         /* ipic supports only low assertion and high-to-low change senses
>          */
>         if (!(flow_type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))) {
> -               printk(KERN_ERR "ipic: sense type 0x%x not supported\n",
> -                       flow_type);
> +               printk(KERN_ERR "ipic: sense type 0x%x not supported on "
> +                       "interrupt %d\n", flow_type, src);
>                 return -EINVAL;
>         }
>         /* ipic supports only edge mode on external interrupts */
>         if ((flow_type & IRQ_TYPE_EDGE_FALLING) && !ipic_info[src].ack) {
>                 printk(KERN_ERR "ipic: edge sense not supported on internal "
> -                               "interrupts\n");
> +                               "interrupt %d\n", src);

While you're here might as well change these from the old style
"printk(KERN_ERR" to the new style "pr_err()"

>                 return -EINVAL;
>
>         }
>

> I hope i respected all patch requirements ;-)

It's an interesting take on the rules, but it needs a bit of work.
We're pedantic about patch formatting, etc because we have tools for
handling patches that require strict formatting in order to function
(e.g https://patchwork.ozlabs.org/project/linuxppc-dev/list/). The
easiest way to ensure you get the formatting right is to use git to
generate the patch files for you, so:

First of all grab a kernel git tree (warning: it's big):
$ git clone https://github.com/torvalds/linux.git

Once it's done, make your changes inside of the git tree. You can run
"git diff" to get a diff between what has been commited in the tree
and what's in the filesystem. To start turning your changes into
patches use:
$ git add -p

NB: you can see what's been staged so fair with:
 $ git diff --staged

When you're happy with the staged changes commit them with:
$ git commit

This will give you a text editor where you can enter the commit
message. You can skip the signed-off-by for now. You'll probably get
yelled at by git for not having a name or an email. You should do what
it says to set those up so your name appears correctly in the patch.

Once you've done all that you should see two commits in:
$ git log

To finish generate the actual patch files with:
$ git format-patch -s HEAD~2

You can (in theory) use any email client to send patches. In practice
most clients are bad at handling whitespace and will do things like
turn tabs into spaces which breaks the diffs. The easiest way get it
right is to use git's email sending plugin. send-email isn't part of
the standard git package so you'll probably need to install and
configure it to use your local SMTP server. It's well worth the hassle
though.
$ git send-email --to=linuxppc-dev@lists.ozlabs.org <patch files go here>

Hope all that helps. There are much better git tutorials out there
than some idiot of the linuxppc-dev mailing list, but that should be
enough to get you started.
Oliver