[OpenWrt-Devel] [PATCH] ath79: ar71xx create a separate driver for ar71xx pci interrupt controller.

[Chuanhong Guo]

Dmitry Tunin <hanipouspilot at gmail.com> 于2018年8月22日周三 上午12:26写道：
> I am still not very competent in the IRQ handling. I thought that if we set
> interrupts = <4>; and the new partent, your "case 4:" may work.
We have it there because it appears in datasheet. But we don't need it
to work unless we know what it's used for.
>
> > >
> > > +                       pci_intc: interrupt-controller at 18060018 {
> > > +                               compatible = "qca,ar7100-pci-intc";
> > > +                               reg = <0x18060018 0x4>;
> > > +                               interrupt-parent = <&cpuintc>;
> > > +                               interrupts = <2>;
> > > +                               interrupt-controller;
> > > +                               #interrupt-cells = <1>;
> > > +                       };
> > > +
> > For other chips this node is a subnode of reset-controller. But in
> > this case here we can just put this above reset-controller at 18060024.
> > Nodes in dts are supposed to be ordered by register address.
>
> No problem with that, I thought about a child node. But it doesn't
> make any difference IMHO.
> > > +
> > >                         pcie0: pcie-controller at 180c0000 {
> > >                                 compatible = "qca,ar7100-pci";
> > >                                 #address-cells = <3>;
> > > @@ -105,14 +115,16 @@
> > >                                 reg-names = "cfg_base";
> > >                                 ranges = <0x2000000 0 0x10000000 0x10000000 0 0x07000000        /* pci memory */
> > >                                           0x1000000 0 0x00000000 0x0000000 0 0x000001>;         /* io space */
> > > -                               interrupt-parent = <&cpuintc>;
> > > -                               interrupts = <2>;
> > > +                               interrupt-parent = <&pci_intc>;
> > > +                               interrupts = <4>;
> > Do we really need this? We don't even have a handler that actually do
> > anything for it. I think the above two lines can simply be dropped.
>
> See above. I thought the existing handler could work. I must be wrong.
All the job of the existing handler is now done by the added cascade
driver. And you have removed the existing handler in driver :)
>
> > > diff --git a/target/linux/ath79/patches-4.14/0020-MIPS-ath79-turn-pci-ar71xx-driver-into-a-pure-OF-dri.patch b/target/linux/ath79/patches-4.14/0020-MIPS-ath79-turn-pci-ar71xx-driver-into-a-pure-OF-dri.patch
> > > index ea3514a..95ca6d1 100644
> > > --- a/target/linux/ath79/patches-4.14/0020-MIPS-ath79-turn-pci-ar71xx-driver-into-a-pure-OF-dri.patch
> > > +++ b/target/linux/ath79/patches-4.14/0020-MIPS-ath79-turn-pci-ar71xx-driver-into-a-pure-OF-dri.patch
> > It's not a good idea to mix the removal of IRQ part into this patch. I
> > suggest using a separated patch to do the removing.
>
> It is a minor issue if everything else looks good. The idea was to
> keep number of patches smaller.
Keeping patches inside that directory less isn't useful at all.(If
it's useful why not squash all of them into one?)
Instead we need each patch did one job and that job is described in
file name/title in mbox header.
As @blogic put in the mbox header, this patch "turns the existing PCI
driver into a pure OF driver" but what you did is "Removing IRQ
dispatcher from PCI driver". Squashing them together only makes the
patch more confusing.
>
>
> > I haven't read the code thoroghly but it seemed that irq-ath79-misc
> > works in a similar way. Is it possible to use the misc intc driver for
> > PCI?
>
> They are similar. I used that code as an example, but the
> irq-ath79-pci has your mask/unmask for pci with reading the registers.
> It is possible to make one driver for both, but is it really needed?
It helps reducing code duplication.

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel