| Message ID | 20150828082638.GA3407@verge.net.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Simon, Thanks for the update! On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote: > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > @@ -6,8 +6,12 @@ interface contains. > Required properties: > - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > - reg: offset and length of (1) the register block and (2) the stream buffer. > -- interrupts: interrupt specifier for the sole interrupt. > +- interrupts: interrupt specifiers. > + One data and one emac interrupt for the R8A7795 SoC; > + these interrupts must be named. > + One named or unnamed data interrupt otherwise. > - phy-mode: see ethernet.txt file in the same directory. > - phy-handle: see ethernet.txt file in the same directory. > - #address-cells: number of address cells for the MDIO bus, must be equal to 1. > @@ -18,6 +22,12 @@ Required properties: > Optional properties: > - interrupt-parent: the phandle for the interrupt controller that services > interrupts for this device. > +- interrupt-names: Names of named interrupts. > + If the property is present "data" is required. > + "emac" is also required for the R8A7795 SoC; > + it is prohibited otherwise. > + This property is mandatory for the R8A7795 SoC; > + optional otherwise. > - pinctrl-names: pin configuration state name ("default"). > - renesas,no-ether-link: boolean, specify when a board does not provide a proper > AVB_LINK signal. What about the 25 channel interrupts? "data" and "emac" seem to use ch22 resp. ch 24 on Gen3. I'm afraid this will bite us one day. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 28, 2015 at 10:35:56AM +0200, Geert Uytterhoeven wrote: > Hi Simon, > > Thanks for the update! > > On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote: > > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > > @@ -6,8 +6,12 @@ interface contains. > > Required properties: > > - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > > "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > > + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > > - reg: offset and length of (1) the register block and (2) the stream buffer. > > -- interrupts: interrupt specifier for the sole interrupt. > > +- interrupts: interrupt specifiers. > > + One data and one emac interrupt for the R8A7795 SoC; > > + these interrupts must be named. > > + One named or unnamed data interrupt otherwise. > > - phy-mode: see ethernet.txt file in the same directory. > > - phy-handle: see ethernet.txt file in the same directory. > > - #address-cells: number of address cells for the MDIO bus, must be equal to 1. > > @@ -18,6 +22,12 @@ Required properties: > > Optional properties: > > - interrupt-parent: the phandle for the interrupt controller that services > > interrupts for this device. > > +- interrupt-names: Names of named interrupts. > > + If the property is present "data" is required. > > + "emac" is also required for the R8A7795 SoC; > > + it is prohibited otherwise. > > + This property is mandatory for the R8A7795 SoC; > > + optional otherwise. > > - pinctrl-names: pin configuration state name ("default"). > > - renesas,no-ether-link: boolean, specify when a board does not provide a proper > > AVB_LINK signal. > > What about the 25 channel interrupts? > "data" and "emac" seem to use ch22 resp. ch 24 on Gen3. > > I'm afraid this will bite us one day. Yes, I worry about that too. But perhaps we can disallow the use of "data" and "emac" if all (any of) the chXX interrupts are named. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/28/2015 11:35 AM, Geert Uytterhoeven wrote: > Thanks for the update! > On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote: >> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt >> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt >> @@ -6,8 +6,12 @@ interface contains. >> Required properties: >> - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. >> "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. >> + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. >> - reg: offset and length of (1) the register block and (2) the stream buffer. >> -- interrupts: interrupt specifier for the sole interrupt. >> +- interrupts: interrupt specifiers. >> + One data and one emac interrupt for the R8A7795 SoC; Data?! What the heck does it mean? :-/ >> + these interrupts must be named. >> + One named or unnamed data interrupt otherwise. >> - phy-mode: see ethernet.txt file in the same directory. >> - phy-handle: see ethernet.txt file in the same directory. >> - #address-cells: number of address cells for the MDIO bus, must be equal to 1. >> @@ -18,6 +22,12 @@ Required properties: >> Optional properties: >> - interrupt-parent: the phandle for the interrupt controller that services >> interrupts for this device. >> +- interrupt-names: Names of named interrupts. >> + If the property is present "data" is required. >> + "emac" is also required for the R8A7795 SoC; >> + it is prohibited otherwise. >> + This property is mandatory for the R8A7795 SoC; >> + optional otherwise. >> - pinctrl-names: pin configuration state name ("default"). >> - renesas,no-ether-link: boolean, specify when a board does not provide a proper >> AVB_LINK signal. > > What about the 25 channel interrupts? > "data" and "emac" seem to use ch22 resp. ch 24 on Gen3. > > I'm afraid this will bite us one day. Me too. We should describe the real hardware, not how the driver uses it. Where does configuring the AVB interrupt mode happen? > Gr{oetje,eeting}s, > Geert MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/28/2015 11:27 AM, Simon Horman wrote: >>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt >>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt >>>> @@ -6,8 +6,12 @@ interface contains. >>>> Required properties: >>>> - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. >>>> "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. >>>> + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. >>>> - reg: offset and length of (1) the register block and (2) the stream buffer. >>>> -- interrupts: interrupt specifier for the sole interrupt. >>>> +- interrupts: if the device is a part of R8A7790/R8A7794 SoC >>>> + interrupt specifier for the sole interrupt. >>>> + if the device is a part of R8A7795 SoC >>>> + interrupt specifier for the two interrupts. >>> >>> If there are multiple interrupts, you best make them named interrupts, >>> i.e. requiring "interrupt-names", too. >> >> Thanks, I will look into that. >> >>> Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24. >> >> Thanks for bringing that up. I think it has also come up elsewhere in this >> thread. I'd like to focus on addressing it here rather than spreading the >> discussion around any further. >> >> My understanding is that on the R-Car Gen3 r8a7795 SoC >> the EthernetAVB hardware may function in one of two modes. >> >> 1. A mode which is "mostly" compatible with R-Car Gen2 >> (e.g. r8a7790, r8a7790). >> >> In this mode only ch22 and ch24 are used. But this is *not* really compatible! >> I note that the emac interrupt also appears to be documented for Gen-2. Where? I'm looking at the common h/w manual rev1.02 and only seeing IRQ 162 for the normal Ether and IRQ163 for the EtherAVB... >> Its not clear to me at this time why it isn't appropriate to use it >> on those SoCs. I just never saw it in the manual. > I have confirmed with that the emac interrupt shouldn't be used > on Gen2 SoCs. Hm... It seems to me from re-reading the driver code, that the current driver only uses the *E-MAC* interrupts and doesn't use the AVB-DMAC interrupts. Maybe I'm over-simplifying though... >> 2. A mode which is new in Gen 3. >> >> In this mode ch0 - ch24 are used. >> I believe that in this mode there are per DMA queue interrupts. > > With regards to named interrupts, and indeed most of the other feedback I > have received for this patch, how about the following? > > I also confirmed with Mizuguchi-san that it is intentional that the same > handler is used for both interrupts. I don't doubt it was intentional, I just very much doubt that it's correct. > From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > Subject: [PATCH/RFC v1.1] ravb: Add support for r8a7795 SoC > > This patch supports the r8a7795 SoC by: > - Adding a compat string for the new hardware > - Support named-interrupts > - Support the E-DMAC interrupt on the new hardware. > Although also present on Gen2 SoCs my understanding is > that it can't be used on those SoCs. No, I don't agree with this description, I don't know where you got this info. > Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > [horms: updated changelog and cleaned up] > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > v0 [Kazuya Mizuguchi] > > v1 [Simon Horman] > * Updated patch subject > > v1.1 [Simon Horman] > * As suggested by Sergei Shtylyov > - Updated changelog > - Corrected capitalisation in documentation > - Removed pdev variable from ravb_open() > - Corrected indentation > - Leave initialisation and freeing of existing interrupt in common code > * As suggested by Geert Uytterhoeven and Sergei Shtylyov > - support named interrupts > > TODO: > * Consider propagating error for both new and existing call to > platform_get_irq(). > --- > .../devicetree/bindings/net/renesas,ravb.txt | 12 ++++++- > drivers/net/ethernet/renesas/ravb.h | 1 + > drivers/net/ethernet/renesas/ravb_main.c | 38 +++++++++++++++++++--- > 3 files changed, 46 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt > index 1fd8831437bf..e2b2a551813a 100644 > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > @@ -6,8 +6,12 @@ interface contains. > Required properties: > - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > - reg: offset and length of (1) the register block and (2) the stream buffer. > -- interrupts: interrupt specifier for the sole interrupt. > +- interrupts: interrupt specifiers. > + One data and one emac interrupt for the R8A7795 SoC; No, the "data" doesn't seem a correct name... > + these interrupts must be named. > + One named or unnamed data interrupt otherwise. > - phy-mode: see ethernet.txt file in the same directory. > - phy-handle: see ethernet.txt file in the same directory. > - #address-cells: number of address cells for the MDIO bus, must be equal to 1. [...] > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index a157aaaaff6a..1832737063f3 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -809,6 +809,7 @@ struct ravb_private { > > unsigned no_avb_link:1; > unsigned avb_link_active_low:1; > + int emac_irq; Please place this somewhere before the bit fields. [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 026d98435d87..0276707089a5 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c [...] > @@ -1197,6 +1198,16 @@ static int ravb_open(struct net_device *ndev) > goto out_napi_off; > } > > + if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) { > + error = request_irq(priv->emac_irq, ravb_interrupt, No, I can't agree to that. You need to clearly separate the interrupt handlers. > + IRQF_SHARED, ndev->name, ndev); > + if (error) { > + netdev_err(ndev, "cannot request IRQ\n"); > + free_irq(ndev->irq, ndev); No, please just add a new label below instead, we already have free_irq() call there. [...] > @@ -1220,6 +1231,8 @@ out_ptp_stop: > ravb_ptp_stop(ndev); > out_free_irq: > free_irq(ndev->irq, ndev); > + if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) Repeating this check all over again doesn't look good to me... can you see about adding some data to the 'struct of_device_id' initalizers? [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2015 01:51 PM, Sergei Shtylyov wrote: >>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt >>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt >>> @@ -6,8 +6,12 @@ interface contains. >>> Required properties: >>> - compatible: "renesas,etheravb-r8a7790" if the device is a part of >>> R8A7790 SoC. >>> "renesas,etheravb-r8a7794" if the device is a part of >>> R8A7794 SoC. >>> + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 >>> SoC. >>> - reg: offset and length of (1) the register block and (2) the stream >>> buffer. >>> -- interrupts: interrupt specifier for the sole interrupt. >>> +- interrupts: interrupt specifiers. >>> + One data and one emac interrupt for the R8A7795 SoC; > > Data?! What the heck does it mean? :-/ Oh, and the manual consistently spells "emac" as E-MAC. MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 28, 2015 at 02:46:30PM +0300, Sergei Shtylyov wrote: > On 8/28/2015 11:27 AM, Simon Horman wrote: > > >>>>--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > >>>>+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > >>>>@@ -6,8 +6,12 @@ interface contains. > >>>> Required properties: > >>>> - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > >>>> "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > >>>>+ "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > >>>> - reg: offset and length of (1) the register block and (2) the stream buffer. > >>>>-- interrupts: interrupt specifier for the sole interrupt. > >>>>+- interrupts: if the device is a part of R8A7790/R8A7794 SoC > >>>>+ interrupt specifier for the sole interrupt. > >>>>+ if the device is a part of R8A7795 SoC > >>>>+ interrupt specifier for the two interrupts. > >>> > >>>If there are multiple interrupts, you best make them named interrupts, > >>>i.e. requiring "interrupt-names", too. > >> > >>Thanks, I will look into that. > >> > >>>Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24. > >> > >>Thanks for bringing that up. I think it has also come up elsewhere in this > >>thread. I'd like to focus on addressing it here rather than spreading the > >>discussion around any further. > >> > >>My understanding is that on the R-Car Gen3 r8a7795 SoC > >>the EthernetAVB hardware may function in one of two modes. > >> > >>1. A mode which is "mostly" compatible with R-Car Gen2 > >> (e.g. r8a7790, r8a7790). > >> > >> In this mode only ch22 and ch24 are used. > > But this is *not* really compatible! Compatible may not have been the wisest choice of wording. Regardless, the point is that there are two modes. I will describe them more fully in a separate sub-thread. > >> I note that the emac interrupt also appears to be documented for Gen-2. > > Where? I'm looking at the common h/w manual rev1.02 and only seeing IRQ > 162 for the normal Ether and IRQ163 for the EtherAVB... > > >> Its not clear to me at this time why it isn't appropriate to use it > >> on those SoCs. > > I just never saw it in the manual. Sorry, I was mistaken. On closer inspection I don't see it either. > >I have confirmed with that the emac interrupt shouldn't be used > >on Gen2 SoCs. > > Hm... It seems to me from re-reading the driver code, that the current > driver only uses the *E-MAC* interrupts and doesn't use the AVB-DMAC > interrupts. Maybe I'm over-simplifying though... I'm not sure that I understand the distinction that you are making there. > >>2. A mode which is new in Gen 3. > >> > >> In this mode ch0 - ch24 are used. > >> I believe that in this mode there are per DMA queue interrupts. > > > >With regards to named interrupts, and indeed most of the other feedback I > >have received for this patch, how about the following? > > > >I also confirmed with Mizuguchi-san that it is intentional that the same > >handler is used for both interrupts. > > I don't doubt it was intentional, I just very much doubt that it's correct. As mentioned above, I have will describe more fully the interrupts in another sub-thread. I hope that helps clear things up. My position on using a shared handler is that it is a simple step to create a correct implementation. I believe it is correct because the existing handler handles a superset of the interrupts from each of the two lines hooked up to it will generate. I believe that it in the longer term it will make for a nicer implementation to have specific handlers for each interrupt. But it that it makes sense to handle that as incremental updates as the driver matures. > > >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > >Subject: [PATCH/RFC v1.1] ravb: Add support for r8a7795 SoC > > > >This patch supports the r8a7795 SoC by: > >- Adding a compat string for the new hardware > >- Support named-interrupts > >- Support the E-DMAC interrupt on the new hardware. > > Although also present on Gen2 SoCs my understanding is > > that it can't be used on those SoCs. > > No, I don't agree with this description, I don't know where you got this info. Thanks, I withdraw that description. > >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > >[horms: updated changelog and cleaned up] > >Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > > >--- > > > >v0 [Kazuya Mizuguchi] > > > >v1 [Simon Horman] > >* Updated patch subject > > > >v1.1 [Simon Horman] > >* As suggested by Sergei Shtylyov > > - Updated changelog > > - Corrected capitalisation in documentation > > - Removed pdev variable from ravb_open() > > - Corrected indentation > > - Leave initialisation and freeing of existing interrupt in common code > >* As suggested by Geert Uytterhoeven and Sergei Shtylyov > > - support named interrupts > > > >TODO: > >* Consider propagating error for both new and existing call to > > platform_get_irq(). > >--- > > .../devicetree/bindings/net/renesas,ravb.txt | 12 ++++++- > > drivers/net/ethernet/renesas/ravb.h | 1 + > > drivers/net/ethernet/renesas/ravb_main.c | 38 +++++++++++++++++++--- > > 3 files changed, 46 insertions(+), 5 deletions(-) > > > >diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt > >index 1fd8831437bf..e2b2a551813a 100644 > >--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > >+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > >@@ -6,8 +6,12 @@ interface contains. > > Required properties: > > - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > > "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > >+ "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > > - reg: offset and length of (1) the register block and (2) the stream buffer. > >-- interrupts: interrupt specifier for the sole interrupt. > >+- interrupts: interrupt specifiers. > >+ One data and one emac interrupt for the R8A7795 SoC; > > No, the "data" doesn't seem a correct name... I will respond more fully with regards to the bindings in another sub-thread. > >+ these interrupts must be named. > >+ One named or unnamed data interrupt otherwise. > > - phy-mode: see ethernet.txt file in the same directory. > > - phy-handle: see ethernet.txt file in the same directory. > > - #address-cells: number of address cells for the MDIO bus, must be equal to 1. > [...] > >diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > >index a157aaaaff6a..1832737063f3 100644 > >--- a/drivers/net/ethernet/renesas/ravb.h > >+++ b/drivers/net/ethernet/renesas/ravb.h > >@@ -809,6 +809,7 @@ struct ravb_private { > > > > unsigned no_avb_link:1; > > unsigned avb_link_active_low:1; > >+ int emac_irq; > > Please place this somewhere before the bit fields. Will do. > [...] > >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > >index 026d98435d87..0276707089a5 100644 > >--- a/drivers/net/ethernet/renesas/ravb_main.c > >+++ b/drivers/net/ethernet/renesas/ravb_main.c > [...] > >@@ -1197,6 +1198,16 @@ static int ravb_open(struct net_device *ndev) > > goto out_napi_off; > > } > > > >+ if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) { > >+ error = request_irq(priv->emac_irq, ravb_interrupt, > > No, I can't agree to that. You need to clearly separate the interrupt > handlers. > > >+ IRQF_SHARED, ndev->name, ndev); > >+ if (error) { > >+ netdev_err(ndev, "cannot request IRQ\n"); > >+ free_irq(ndev->irq, ndev); > > No, please just add a new label below instead, we already have free_irq() > call there. Sure, will do. > [...] > >@@ -1220,6 +1231,8 @@ out_ptp_stop: > > ravb_ptp_stop(ndev); > > out_free_irq: > > free_irq(ndev->irq, ndev); > >+ if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) > > Repeating this check all over again doesn't look good to me... can you > see about adding some data to the 'struct of_device_id' initalizers? Good idea, I'll look into it. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 28, 2015 at 01:51:20PM +0300, Sergei Shtylyov wrote: > On 8/28/2015 11:35 AM, Geert Uytterhoeven wrote: > > >Thanks for the update! > > >On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote: > >>--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > >>+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > >>@@ -6,8 +6,12 @@ interface contains. > >> Required properties: > >> - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > >> "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > >>+ "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > >> - reg: offset and length of (1) the register block and (2) the stream buffer. > >>-- interrupts: interrupt specifier for the sole interrupt. > >>+- interrupts: interrupt specifiers. > >>+ One data and one emac interrupt for the R8A7795 SoC; > > Data?! What the heck does it mean? :-/ Perhaps it would be better to refer to them as dmac interrupts. The documentation makes reference to "merged data interrupt", which seems to refer to tx and rx interrupts. But this interrupt is also used for error and management related interrupts. I'm all ears with regards to a good name. With regards to the documentation consistently referring to "E-MAC", which you raised elsewhere. Yes I see that too but I'm not sure where you are going there. If you would like to tweak the documentation or name of the interrupt somehow please spell out your ideas a little more clearly. > >>+ these interrupts must be named. > >>+ One named or unnamed data interrupt otherwise. > >> - phy-mode: see ethernet.txt file in the same directory. > >> - phy-handle: see ethernet.txt file in the same directory. > >> - #address-cells: number of address cells for the MDIO bus, must be equal to 1. > >>@@ -18,6 +22,12 @@ Required properties: > >> Optional properties: > >> - interrupt-parent: the phandle for the interrupt controller that services > >> interrupts for this device. > >>+- interrupt-names: Names of named interrupts. > >>+ If the property is present "data" is required. > >>+ "emac" is also required for the R8A7795 SoC; > >>+ it is prohibited otherwise. > >>+ This property is mandatory for the R8A7795 SoC; > >>+ optional otherwise. > >> - pinctrl-names: pin configuration state name ("default"). > >> - renesas,no-ether-link: boolean, specify when a board does not provide a proper > >> AVB_LINK signal. > > > >What about the 25 channel interrupts? > >"data" and "emac" seem to use ch22 resp. ch 24 on Gen3. > > > >I'm afraid this will bite us one day. > > Me too. We should describe the real hardware, not how the driver uses it. > Where does configuring the AVB interrupt mode happen? I believe that we all want that. Lets see about making it so :) As I mentioned above, broadly speaking the interrupts may be configured in one of two modes. And the default configuration is to use what I earlier described as "compatible". I will now just refer to it as mode 1 with the other mode being mode 2. Mode 1. ch22 or ch23 is used for: * Merged Data Interrupts (TX and RX) * Error related interrupts * Management related interrupts ch24 is used for: * E-MAC interrupts It seems to me that the above basically splits the single interrupt in Gen2 into two. This is the default mode for the hardware and the mode it ends up in using the current driver implementation (+ this series which doesn't alter the initialisation of the hardware). Mode 2. ch0 - ch17 are used for: * per-queue Rx interrupts ch18 - ch21 are used for: * per-queue Tx interrupts ch22 or ch23 are used for: * Other DMA descriptor interrupts (if enabled) * Error related interrupts * Management related interrupts ch24 is used for * E-MAC interrupts I think it is instructive to examine differences between the modes for Each interrupt. And here is how I see that: * ch0 - ch21: mode 1: unused mode 2: per queue rx or tx interrupts * ch22 or ch23: both mode 1 and 2: + Error related interrupts + Management related interrupts mode 1 only: + Merged Data Interrupts (TX and RX) mode 2 only: + Other DMA descriptor interrupts (if enabled) * ch24: mode 1 and 2: E-MAC interrupts So it seems to me that one possibility would be to: * Have names for all of the channels, say rx0 - 18, tx0 - 4, data, dataB, emac. - Better names could be found for data and dataB. * Allow for properties to describe the mode and possibly which od dataA or B should be used. But for now: * Only describe the interrupts that are used and default to mode 1 with the data interrupt and; * Defer defining properties to switch to combinations I am all ears for other ideas. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon, On Wed, Sep 2, 2015 at 4:13 AM, Simon Horman <horms@verge.net.au> wrote: > On Fri, Aug 28, 2015 at 01:51:20PM +0300, Sergei Shtylyov wrote: >> On 8/28/2015 11:35 AM, Geert Uytterhoeven wrote: >> >On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote: >> >>--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt >> >>+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt >> >>@@ -6,8 +6,12 @@ interface contains. >> >> Required properties: >> >> - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. >> >> "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. >> >>+ "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. >> >> - reg: offset and length of (1) the register block and (2) the stream buffer. >> >>-- interrupts: interrupt specifier for the sole interrupt. >> >>+- interrupts: interrupt specifiers. >> >>+ One data and one emac interrupt for the R8A7795 SoC; >> >> Data?! What the heck does it mean? :-/ > > Perhaps it would be better to refer to them as dmac interrupts. > > The documentation makes reference to "merged data interrupt", which > seems to refer to tx and rx interrupts. But this interrupt is also > used for error and management related interrupts. > > I'm all ears with regards to a good name. > > With regards to the documentation consistently referring to "E-MAC", > which you raised elsewhere. Yes I see that too but I'm not sure where > you are going there. If you would like to tweak the documentation > or name of the interrupt somehow please spell out your ideas a little > more clearly. > >> >>+ these interrupts must be named. >> >>+ One named or unnamed data interrupt otherwise. >> >> - phy-mode: see ethernet.txt file in the same directory. >> >> - phy-handle: see ethernet.txt file in the same directory. >> >> - #address-cells: number of address cells for the MDIO bus, must be equal to 1. >> >>@@ -18,6 +22,12 @@ Required properties: >> >> Optional properties: >> >> - interrupt-parent: the phandle for the interrupt controller that services >> >> interrupts for this device. >> >>+- interrupt-names: Names of named interrupts. >> >>+ If the property is present "data" is required. >> >>+ "emac" is also required for the R8A7795 SoC; >> >>+ it is prohibited otherwise. >> >>+ This property is mandatory for the R8A7795 SoC; >> >>+ optional otherwise. >> >> - pinctrl-names: pin configuration state name ("default"). >> >> - renesas,no-ether-link: boolean, specify when a board does not provide a proper >> >> AVB_LINK signal. >> > >> >What about the 25 channel interrupts? >> >"data" and "emac" seem to use ch22 resp. ch 24 on Gen3. >> > >> >I'm afraid this will bite us one day. >> >> Me too. We should describe the real hardware, not how the driver uses it. >> Where does configuring the AVB interrupt mode happen? > > I believe that we all want that. Lets see about making it so :) > > As I mentioned above, broadly speaking the interrupts may be configured in > one of two modes. And the default configuration is to use what I earlier > described as "compatible". I will now just refer to it as mode 1 with > the other mode being mode 2. > > Mode 1. > > ch22 or ch23 is used for: > * Merged Data Interrupts (TX and RX) > * Error related interrupts > * Management related interrupts > > ch24 is used for: > * E-MAC interrupts > > It seems to me that the above basically splits the single interrupt > in Gen2 into two. > > This is the default mode for the hardware and the mode it ends up > in using the current driver implementation (+ this series which doesn't > alter the initialisation of the hardware). > > Mode 2. > > ch0 - ch17 are used for: > * per-queue Rx interrupts > > ch18 - ch21 are used for: > * per-queue Tx interrupts > > ch22 or ch23 are used for: > * Other DMA descriptor interrupts (if enabled) > * Error related interrupts > * Management related interrupts > > ch24 is used for > * E-MAC interrupts > > > I think it is instructive to examine differences between the modes for > Each interrupt. And here is how I see that: > > * ch0 - ch21: > mode 1: unused > mode 2: per queue rx or tx interrupts > > * ch22 or ch23: > both mode 1 and 2: > + Error related interrupts > + Management related interrupts > mode 1 only: > + Merged Data Interrupts (TX and RX) > mode 2 only: > + Other DMA descriptor interrupts (if enabled) > > * ch24: > mode 1 and 2: E-MAC interrupts Thanks for the analysis and the explanation! > So it seems to me that one possibility would be to: > * Have names for all of the channels, say rx0 - 18, tx0 - 4, data, dataB, emac. > - Better names could be found for data and dataB. That's one possibility, but it looks to me like the rx* and tx* naming would be purely a configuration, which doesn't describe the hardware, and perhaps could even change for future parts? At least having 18 RX channels and 4 TX channels looks arbitrary to me. > * Allow for properties to describe the mode and possibly which od dataA or B > should be used. Also configuration, not description of the hardware. > But for now: > * Only describe the interrupts that are used and default to mode 1 with the > data interrupt and; > * Defer defining properties to switch to combinations > > I am all ears for other ideas. I think there should be either a single multiplexed interrupt (for Gen2), or a list of named interrupts ("ch%u"), of which ch22/ch23 and ch24 are mandatory (for Gen3). Other SoCs may reuse the RAVB IP core without wiring up all channels. For Gen3, the driver can then choose itself between mode1 and mode2 (if implemented in the driver, and all needed channels are available). Does this sound sane? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Wed, Sep 02, 2015 at 09:26:32AM +0200, Geert Uytterhoeven wrote: > Hi Simon, [snip] > Thanks for the analysis and the explanation! > > > So it seems to me that one possibility would be to: > > * Have names for all of the channels, say rx0 - 18, tx0 - 4, data, dataB, emac. > > - Better names could be found for data and dataB. > > That's one possibility, but it looks to me like the rx* and tx* naming would > be purely a configuration, which doesn't describe the hardware, and perhaps > could even change for future parts? At least having 18 RX channels and 4 > TX channels looks arbitrary to me. > > > * Allow for properties to describe the mode and possibly which od dataA or B > > should be used. > > Also configuration, not description of the hardware. > > > But for now: > > * Only describe the interrupts that are used and default to mode 1 with the > > data interrupt and; > > * Defer defining properties to switch to combinations > > > > I am all ears for other ideas. > > I think there should be either a single multiplexed interrupt (for Gen2), > or a list of named interrupts ("ch%u"), of which ch22/ch23 and ch24 are > mandatory (for Gen3). Other SoCs may reuse the RAVB IP core without > wiring up all channels. > > For Gen3, the driver can then choose itself between mode1 and mode2 > (if implemented in the driver, and all needed channels are available). > > Does this sound sane? Sure, that sounds sane to me. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 09/02/2015 10:26 AM, Geert Uytterhoeven wrote: >>>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt >>>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt >>>>> @@ -6,8 +6,12 @@ interface contains. >>>>> Required properties: >>>>> - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. >>>>> "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. >>>>> + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. >>>>> - reg: offset and length of (1) the register block and (2) the stream buffer. >>>>> -- interrupts: interrupt specifier for the sole interrupt. >>>>> +- interrupts: interrupt specifiers. >>>>> + One data and one emac interrupt for the R8A7795 SoC; >>> >>> Data?! What the heck does it mean? :-/ >> >> Perhaps it would be better to refer to them as dmac interrupts. >> >> The documentation makes reference to "merged data interrupt", which >> seems to refer to tx and rx interrupts. But this interrupt is also >> used for error and management related interrupts. >> >> I'm all ears with regards to a good name. >> >> With regards to the documentation consistently referring to "E-MAC", >> which you raised elsewhere. Yes I see that too but I'm not sure where >> you are going there. If you would like to tweak the documentation >> or name of the interrupt somehow please spell out your ideas a little >> more clearly. >> >>>>> + these interrupts must be named. >>>>> + One named or unnamed data interrupt otherwise. >>>>> - phy-mode: see ethernet.txt file in the same directory. >>>>> - phy-handle: see ethernet.txt file in the same directory. >>>>> - #address-cells: number of address cells for the MDIO bus, must be equal to 1. >>>>> @@ -18,6 +22,12 @@ Required properties: >>>>> Optional properties: >>>>> - interrupt-parent: the phandle for the interrupt controller that services >>>>> interrupts for this device. >>>>> +- interrupt-names: Names of named interrupts. >>>>> + If the property is present "data" is required. >>>>> + "emac" is also required for the R8A7795 SoC; >>>>> + it is prohibited otherwise. >>>>> + This property is mandatory for the R8A7795 SoC; >>>>> + optional otherwise. >>>>> - pinctrl-names: pin configuration state name ("default"). >>>>> - renesas,no-ether-link: boolean, specify when a board does not provide a proper >>>>> AVB_LINK signal. >>>> >>>> What about the 25 channel interrupts? >>>> "data" and "emac" seem to use ch22 resp. ch 24 on Gen3. >>>> >>>> I'm afraid this will bite us one day. >>> >>> Me too. We should describe the real hardware, not how the driver uses it. >>> Where does configuring the AVB interrupt mode happen? Answering my own question: it's done by the new registers of the EtherAVB controller. I now have the gen3 manual. >> I believe that we all want that. Lets see about making it so :) >> >> As I mentioned above, broadly speaking the interrupts may be configured in >> one of two modes. And the default configuration is to use what I earlier >> described as "compatible". I will now just refer to it as mode 1 with >> the other mode being mode 2. >> >> Mode 1. >> >> ch22 or ch23 is used for: >> * Merged Data Interrupts (TX and RX) No, only ch22 can be used fo those. >> * Error related interrupts >> * Management related interrupts >> ch24 is used for: >> * E-MAC interrupts >> It seems to me that the above basically splits the single interrupt >> in Gen2 into two. >> >> This is the default mode for the hardware and the mode it ends up >> in using the current driver implementation (+ this series which doesn't >> alter the initialisation of the hardware). This is also assuming that the bootloader doesn't change it. Are we sure it doesn't? >> Mode 2. >> >> ch0 - ch17 are used for: >> * per-queue Rx interrupts >> >> ch18 - ch21 are used for: >> * per-queue Tx interrupts >> >> ch22 or ch23 are used for: >> * Other DMA descriptor interrupts (if enabled) >> * Error related interrupts >> * Management related interrupts >> >> ch24 is used for >> * E-MAC interrupts >> >> >> I think it is instructive to examine differences between the modes for >> Each interrupt. And here is how I see that: >> >> * ch0 - ch21: >> mode 1: unused >> mode 2: per queue rx or tx interrupts >> >> * ch22 or ch23: >> both mode 1 and 2: >> + Error related interrupts >> + Management related interrupts >> mode 1 only: >> + Merged Data Interrupts (TX and RX) >> mode 2 only: >> + Other DMA descriptor interrupts (if enabled) >> >> * ch24: >> mode 1 and 2: E-MAC interrupts > Thanks for the analysis and the explanation! Yes, although it doesn't quite agree with the manual I have (rev 0.5E). >> So it seems to me that one possibility would be to: >> * Have names for all of the channels, say rx0 - 18, tx0 - 4, data, dataB, emac. >> - Better names could be found for data and dataB. > That's one possibility, but it looks to me like the rx* and tx* naming would > be purely a configuration, which doesn't describe the hardware, No, it does, according to the table 50.19 in the manual. > and perhaps > could even change for future parts? At least having 18 RX channels and 4 > TX channels looks arbitrary to me. These #s are the same across the known EtherAVB implementations. >> * Allow for properties to describe the mode and possibly which od dataA or B >> should be used. > Also configuration, not description of the hardware. Agreed here. >> But for now: >> * Only describe the interrupts that are used and default to mode 1 with the >> data interrupt and; >> * Defer defining properties to switch to combinations >> >> I am all ears for other ideas. > > I think there should be either a single multiplexed interrupt (for Gen2), > or a list of named interrupts ("ch%u"), of which ch22/ch23 and ch24 are > mandatory (for Gen3). No, all IRQs should be mandatory. Otherwise we're into describing the config. again, not the real hardware. > Other SoCs may reuse the RAVB IP core without > wiring up all channels. > For Gen3, the driver can then choose itself between mode1 and mode2 > (if implemented in the driver, and all needed channels are available). Agreed. > Does this sound sane? Mostly. :-) > Gr{oetje,eeting}s, > Geert MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 09/02/2015 05:13 AM, Simon Horman wrote: >>> Thanks for the update! >> >>> On Fri, Aug 28, 2015 at 10:27 AM, Simon Horman <horms@verge.net.au> wrote: >>>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt >>>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt >>>> @@ -6,8 +6,12 @@ interface contains. >>>> Required properties: >>>> - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. >>>> "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. >>>> + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. >>>> - reg: offset and length of (1) the register block and (2) the stream buffer. >>>> -- interrupts: interrupt specifier for the sole interrupt. >>>> +- interrupts: interrupt specifiers. >>>> + One data and one emac interrupt for the R8A7795 SoC; >> >> Data?! What the heck does it mean? :-/ > > Perhaps it would be better to refer to them as dmac interrupts. No, they do not seem to be limited to AVB-DMAC. > The documentation makes reference to "merged data interrupt", which > seems to refer to tx and rx interrupts. But this interrupt is also > used for error and management related interrupts. Yes. > I'm all ears with regards to a good name. AFAICS from the manual, there are many reasons for these, perhaps calling them "a" for ch22 (and "b" for ch23) would make more sense. > With regards to the documentation consistently referring to "E-MAC", > which you raised elsewhere. Yes I see that too but I'm not sure where > you are going there. If you would like to tweak the documentation > or name of the interrupt somehow please spell out your ideas a little > more clearly. Please just don't call it emac in the prop description, call it E-MAC, that's all. >>>> + these interrupts must be named. >>>> + One named or unnamed data interrupt otherwise. >>>> - phy-mode: see ethernet.txt file in the same directory. >>>> - phy-handle: see ethernet.txt file in the same directory. >>>> - #address-cells: number of address cells for the MDIO bus, must be equal to 1. >>>> @@ -18,6 +22,12 @@ Required properties: >>>> Optional properties: >>>> - interrupt-parent: the phandle for the interrupt controller that services >>>> interrupts for this device. >>>> +- interrupt-names: Names of named interrupts. >>>> + If the property is present "data" is required. >>>> + "emac" is also required for the R8A7795 SoC; >>>> + it is prohibited otherwise. >>>> + This property is mandatory for the R8A7795 SoC; >>>> + optional otherwise. >>>> - pinctrl-names: pin configuration state name ("default"). >>>> - renesas,no-ether-link: boolean, specify when a board does not provide a proper >>>> AVB_LINK signal. >>> >>> What about the 25 channel interrupts? >>> "data" and "emac" seem to use ch22 resp. ch 24 on Gen3. >>> >>> I'm afraid this will bite us one day. >> >> Me too. We should describe the real hardware, not how the driver uses it. >> Where does configuring the AVB interrupt mode happen? Now I'm seeing it happens in the controller itself. > I believe that we all want that. Lets see about making it so :) > > As I mentioned above, broadly speaking the interrupts may be configured in > one of two modes. There are many bits controlling the interrupt routing, so not sure we're limited to just 2 modes. > And the default configuration is to use what I earlier > described as "compatible". I will now just refer to it as mode 1 with > the other mode being mode 2. I believe I have replied to the text below in a mail to Geert. [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Sep 08, 2015 at 02:06:17AM +0300, Sergei Shtylyov wrote: > Hello. > > On 09/02/2015 10:26 AM, Geert Uytterhoeven wrote: [snip] > >> It seems to me that the above basically splits the single interrupt > >> in Gen2 into two. > >> > >> This is the default mode for the hardware and the mode it ends up > >> in using the current driver implementation (+ this series which doesn't > >> alter the initialisation of the hardware). > > This is also assuming that the bootloader doesn't change it. Are we sure > it doesn't? If it is important I can check with regards to the current uboot (plans). From my point of view it is a reasonable assumption to make. [snip] > >>But for now: > >>* Only describe the interrupts that are used and default to mode 1 with the > >> data interrupt and; > >>* Defer defining properties to switch to combinations > >> > >>I am all ears for other ideas. > > > >I think there should be either a single multiplexed interrupt (for Gen2), > >or a list of named interrupts ("ch%u"), of which ch22/ch23 and ch24 are > >mandatory (for Gen3). > > No, all IRQs should be mandatory. Otherwise we're into describing the > config. again, not the real hardware. That is fine by me. [snip] -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt index 1fd8831437bf..e2b2a551813a 100644 --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt @@ -6,8 +6,12 @@ interface contains. Required properties: - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. - reg: offset and length of (1) the register block and (2) the stream buffer. -- interrupts: interrupt specifier for the sole interrupt. +- interrupts: interrupt specifiers. + One data and one emac interrupt for the R8A7795 SoC; + these interrupts must be named. + One named or unnamed data interrupt otherwise. - phy-mode: see ethernet.txt file in the same directory. - phy-handle: see ethernet.txt file in the same directory. - #address-cells: number of address cells for the MDIO bus, must be equal to 1. @@ -18,6 +22,12 @@ Required properties: Optional properties: - interrupt-parent: the phandle for the interrupt controller that services interrupts for this device. +- interrupt-names: Names of named interrupts. + If the property is present "data" is required. + "emac" is also required for the R8A7795 SoC; + it is prohibited otherwise. + This property is mandatory for the R8A7795 SoC; + optional otherwise. - pinctrl-names: pin configuration state name ("default"). - renesas,no-ether-link: boolean, specify when a board does not provide a proper AVB_LINK signal. diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index a157aaaaff6a..1832737063f3 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -809,6 +809,7 @@ struct ravb_private { unsigned no_avb_link:1; unsigned avb_link_active_low:1; + int emac_irq; }; static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg) diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 026d98435d87..0276707089a5 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1185,6 +1185,7 @@ static const struct ethtool_ops ravb_ethtool_ops = { static int ravb_open(struct net_device *ndev) { struct ravb_private *priv = netdev_priv(ndev); + struct device *dev = &priv->pdev->dev; int error; napi_enable(&priv->napi[RAVB_BE]); @@ -1197,6 +1198,16 @@ static int ravb_open(struct net_device *ndev) goto out_napi_off; } + if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) { + error = request_irq(priv->emac_irq, ravb_interrupt, + IRQF_SHARED, ndev->name, ndev); + if (error) { + netdev_err(ndev, "cannot request IRQ\n"); + free_irq(ndev->irq, ndev); + goto out_napi_off; + } + } + /* Device init */ error = ravb_dmac_init(ndev); if (error) @@ -1220,6 +1231,8 @@ out_ptp_stop: ravb_ptp_stop(ndev); out_free_irq: free_irq(ndev->irq, ndev); + if (of_device_is_compatible(dev->of_node, "renesas,etheravb-r8a7795")) + free_irq(priv->emac_irq, ndev); out_napi_off: napi_disable(&priv->napi[RAVB_NC]); napi_disable(&priv->napi[RAVB_BE]); @@ -1628,9 +1641,9 @@ static int ravb_mdio_release(struct ravb_private *priv) static int ravb_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; + int error, irq, q, is_r8a7795; struct ravb_private *priv; struct net_device *ndev; - int error, irq, q; struct resource *res; if (!np) { @@ -1657,10 +1670,17 @@ static int ravb_probe(struct platform_device *pdev) /* The Ether-specific entries in the device structure. */ ndev->base_addr = res->start; ndev->dma = -1; - irq = platform_get_irq(pdev, 0); + + is_r8a7795 = of_device_is_compatible(np, "renesas,etheravb-r8a7795"); + + irq = platform_get_irq_byname(pdev, "data"); if (irq < 0) { - error = -ENODEV; - goto out_release; + if (!is_r8a7795) + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + error = -ENODEV; + goto out_release; + } } ndev->irq = irq; @@ -1688,6 +1708,15 @@ static int ravb_probe(struct platform_device *pdev) priv->avb_link_active_low = of_property_read_bool(np, "renesas,ether-link-active-low"); + if (is_r8a7795) { + irq = platform_get_irq_byname(pdev, "emac"); + if (irq < 0) { + error = -ENODEV; + goto out_release; + } + priv->emac_irq = irq; + } + /* Set function */ ndev->netdev_ops = &ravb_netdev_ops; ndev->ethtool_ops = &ravb_ethtool_ops; @@ -1821,6 +1850,7 @@ static const struct dev_pm_ops ravb_dev_pm_ops = { static const struct of_device_id ravb_match_table[] = { { .compatible = "renesas,etheravb-r8a7790" }, { .compatible = "renesas,etheravb-r8a7794" }, + { .compatible = "renesas,etheravb-r8a7795" }, { } }; MODULE_DEVICE_TABLE(of, ravb_match_table);