| Message ID | 20210208140820.10410-1-Sergey.Semin@baikalelectronics.ru |
|---|---|
| Headers | show |
| Series | net: stmmac: Add DW MAC GPIOs and Baikal-T1 GMAC support | expand |
On Mon, Feb 08, 2021 at 05:08:04PM +0300, Serge Semin wrote: Hi Serge I suggest you split this patchset up. This uses the generic GPIO framework, which is great. But that also means you should be Cc: the GPIO subsystem maintainers and list. But you don't want to spam them with all the preparation work, which has little to do with the GPIO code. So please split the actual GPIO driver and DT binding patches from the rest. netdev can review the preparation work, with a comment in the 0/X patch about what the big picture is, and then afterwards review the GPIO patchset with a wider audience. And as Jakub pointed out, nobody is going to review 60 patches all at once. Please submit one series at a time, get it merged, and then move onto the next. Andrew
On Mon, Feb 08, 2021 at 05:08:05PM +0300, Serge Semin wrote: > Synopsys DesignWare Ethernet controllers can be synthesized with > General-Purpose IOs support. GPIOs can work either as inputs or as outputs > thus belong to the gpi_i and gpo_o ports respectively. The ports width > (number of possible inputs/outputs) and the configuration registers layout > depend on the IP-core version. For instance, DW GMAC can have from 0 to 4 > GPIs and from 0 to 4 GPOs, while DW xGMAC have a wider ports width up to > 16 pins of each one. > > So the DW MAC DT-node can be equipped with "ngpios" property, which can't > have a value greater than 32, standard GPIO-related properties like > "gpio-controller" and "#gpio-cells", and, if GPIs are supposed to be > detected, IRQ-controller related properties. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > .../devicetree/bindings/net/snps,dwmac.yaml | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > index bdc437b14878..fcca23d3727e 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > @@ -110,6 +110,23 @@ properties: > reset-names: > const: stmmaceth > > + ngpios: > + description: > + Total number of GPIOs the MAC supports. The property shall include both > + the GPI and GPO ports width. > + minimum: 1 > + maximum: 32 Does the driver actually need this? I'd omit it if just to validate consumers are in range. Are GPI and GPO counts independent? If so, this isn't really sufficient. Rob
On Tue, Feb 09, 2021 at 03:11:41PM +0100, Andrew Lunn wrote: > > Regarding splitting the series up. I don't see a problem in just > > sending the cover-letter patch and actual GPIO-related patches to > > the GPIO-maintainers with no need to have them added to Cc in the rest > > of the series. > > The Linux community has to handle a large number of patches. I don't > particularly want patches which are of no relevance to me landing in > my mailbox. It might take 4 or 5 rounds for the preparation patches to > be accepted. That is 4 or 5 times you are spamming the GPIO > maintainers with stuff which is not relevant to them. I don't really understand what you are arguing with. My suggestion didn't contradict to what you said. I exactly meant to omit sending the patches to GPIO maintainers, which they had no relevance to. So they wouldn't be spammed with unwanted patches. The GPIO maintainers can be Cc/To-ed only to the messages with GPIO-related patches. It's normal to have intermixed patchsets, but to post individual patches to the maintainers they might be interested in or they need to review. So splitting up isn't required in this case. Moreover doing as you suggest would extend the time of the patches review with no really much gain in the emailing activity optimization. > > One of the unfortunately things about the kernel process is, there are > a lot of developers, and not many maintainers. So the processes need > to make the life of maintainers easier, and not spamming them helps. Can't argue with that.) -Sergey > > Andrew
On Tue, Feb 09, 2021 at 05:13:52PM -0600, Rob Herring wrote: > On Mon, Feb 08, 2021 at 05:08:05PM +0300, Serge Semin wrote: > > Synopsys DesignWare Ethernet controllers can be synthesized with > > General-Purpose IOs support. GPIOs can work either as inputs or as outputs > > thus belong to the gpi_i and gpo_o ports respectively. The ports width > > (number of possible inputs/outputs) and the configuration registers layout > > depend on the IP-core version. For instance, DW GMAC can have from 0 to 4 > > GPIs and from 0 to 4 GPOs, while DW xGMAC have a wider ports width up to > > 16 pins of each one. > > > > So the DW MAC DT-node can be equipped with "ngpios" property, which can't > > have a value greater than 32, standard GPIO-related properties like > > "gpio-controller" and "#gpio-cells", and, if GPIs are supposed to be > > detected, IRQ-controller related properties. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > .../devicetree/bindings/net/snps,dwmac.yaml | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > index bdc437b14878..fcca23d3727e 100644 > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > @@ -110,6 +110,23 @@ properties: > > reset-names: > > const: stmmaceth > > > > + ngpios: > > + description: > > + Total number of GPIOs the MAC supports. The property shall include both > > + the GPI and GPO ports width. > > + minimum: 1 > > + maximum: 32 > > Does the driver actually need this? I'd omit it if just to validate > consumers are in range. I can't say for all possible DW MAC IP-cores (I've got manuals for GMAC and xGMAC only), but at least DW GMAC can't have more than four GPIs and four GPOs, while XGMACs can be synthesized with up to 16 each. That's why I've set the upper boundary here as 32. But the driver uses the ngpios property do determine the total number GPIOs the core has been synthesized. Th number of GPIs and GPOs will be auto-detected then (by writing-reading to-from the GPI type field of the GPIO control register). > > Are GPI and GPO counts independent? If so, this isn't really sufficient. Yeap, they are independent. What do you suggest then? Define some vendor-specific properties like snps,ngpis and snps,ngpos? If so then they seem more generic than vendor-specific, because the separated GPI and GPO space isn't an unique feature of the DW MAC GPIOs. Do we need to create a generic version of such properties then? (That much more changes then introduced here. We'd need to fix the dt-schema tool too then.) -Sergey > > Rob
On Thu, Feb 11, 2021 at 01:28:06AM +0300, Serge Semin wrote: > On Tue, Feb 09, 2021 at 05:13:52PM -0600, Rob Herring wrote: > > On Mon, Feb 08, 2021 at 05:08:05PM +0300, Serge Semin wrote: > > > Synopsys DesignWare Ethernet controllers can be synthesized with > > > General-Purpose IOs support. GPIOs can work either as inputs or as outputs > > > thus belong to the gpi_i and gpo_o ports respectively. The ports width > > > (number of possible inputs/outputs) and the configuration registers layout > > > depend on the IP-core version. For instance, DW GMAC can have from 0 to 4 > > > GPIs and from 0 to 4 GPOs, while DW xGMAC have a wider ports width up to > > > 16 pins of each one. > > > > > > So the DW MAC DT-node can be equipped with "ngpios" property, which can't > > > have a value greater than 32, standard GPIO-related properties like > > > "gpio-controller" and "#gpio-cells", and, if GPIs are supposed to be > > > detected, IRQ-controller related properties. > > > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > --- > > > .../devicetree/bindings/net/snps,dwmac.yaml | 17 +++++++++++++++++ > > > 1 file changed, 17 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > index bdc437b14878..fcca23d3727e 100644 > > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > @@ -110,6 +110,23 @@ properties: > > > reset-names: > > > const: stmmaceth > > > > > > + ngpios: > > > + description: > > > + Total number of GPIOs the MAC supports. The property shall include both > > > + the GPI and GPO ports width. > > > + minimum: 1 > > > + maximum: 32 > > > > > Does the driver actually need this? I'd omit it if just to validate > > consumers are in range. > > I can't say for all possible DW MAC IP-cores (I've got manuals for > GMAC and xGMAC only), but at least DW GMAC can't have more than four > GPIs and four GPOs, while XGMACs can be synthesized with up to 16 > each. That's why I've set the upper boundary here as 32. But the > driver uses the ngpios property do determine the total number GPIOs > the core has been synthesized. Th number of GPIs and GPOs will be > auto-detected then (by writing-reading to-from the GPI type field of > the GPIO control register). > > > > > Are GPI and GPO counts independent? If so, this isn't really sufficient. > > Yeap, they are independent. What do you suggest then? Define some > vendor-specific properties like snps,ngpis and snps,ngpos? If so then > they seem more generic than vendor-specific, because the separated > GPI and GPO space isn't an unique feature of the DW MAC GPIOs. Do we > need to create a generic version of such properties then? (That much > more changes then introduced here. We'd need to fix the dt-schema tool > too then.) > > -Sergey Rob, any comment on my questions above? As the kernel is in merge-window now, I just hope this part won't get pushed back in the emails log out of your sight. -Sergey > > > > > Rob
Baikal-T1 SoC is equipped with two Synopsys DesignWare GMAC v3.73a-based ethernet interfaces. There were no vendor-specific alterations provided for the synthesized core, but aside with standard configs which can be read from the DMA-capability registers the GPI/GPO ports were activated. Alas hardware designers have actively used them to implement the events detection and to reset PHYs attached to the MAC. So we had no choice, but to somehow fix the standard STMMAC driver so one would safely support these GPIOs usage. This series is mainly about introducing the GPI/GPO ports support into the STMMAC driver and finally adding the Baikal-T1 GMAC support. The later merely means having a corresponding compatible string added to the OF IDs list of the Generic DW MAC glue-driver and having the Baikal-T1 GMAC DT bindings defined in the kernel. The series starts with fixing the bindings as it was said above, because the bindings/doc-related patches are supposed to go before the changes they have been created for. Then the patchset proceeds with a set of preparation changes, so the driver would correctly register, work with and de-register the GPIO-chip in the kernel. The alterations are connected with two problems that made using the embedded into the MAC GPIOs very hard or even impossible: (1) MAC/DMA/etc software reset on each network device open. GPIO-chip is a device which state must be deterministic no matter whether some network device is opened or released. At least Linux kernel expect it to act like that. So after the DW MAC GPIO interface is registered in the kernel we can't let the GPI/GPO stochastically change. The situation is getting worse on the platforms (like Baikal-T1 SoC), which can't even temporarily detach the GPIO pins from the core while for instance the reset is in progress. To cut it short the only possible solution for such chips, which also don't have a vendor-specific GPIO-safe reset procedure, is to get rid of the resets. In the framework of this patchset we suggest to replace it with MAC and DMA registers manual cleanup in case if MAC GPIOs are intended to be used. Of course such solution isn't equivalent to the SW-reset because it doesn't imply the internal state machine reset. But at least the CSRs state will be as the rest of the code expect them to. (2) Main IRQ was requested and unmasked only while network device was opened. Indeed if we want to use MAC GPIs in the kernel, we must make sure the input state change is detected by the IRQ handler no matter whether the network device is opened or not. Of course we could just request the main IRQ in the probe method and mask all the network-related IRQs while the corresponding network device is closed. But turned out it wasn't that easy. Due to a very strange DW MAC IRQs subsystem design we can't disable/enable and clear/check all the device interrupts in a single CSR. MAC, DMA, MTL, Safety feature, GPIO subsystems each can generate IRQs and have got its own CSRs to set the interrupts up. Alas the STMMAC driver didn't have IP-core-specific callbacks to just disable/enable IRQs from each of them. Instead the interrupts were enabled in the framework of the subsystem functionality setup procedures. So in this patchset we've introduced a set of MAC, DMA, MTL and Safety feature IRQs enable/disable callbacks to be able to manually switch on and off all the network-related interrupts when it's required. After all of the denoted above problems are fixed we need to get the code a bit more prepared to adding the GPIOs functionality. Since the driver ISR will be used to handle both network- and GPIO-related events, it needs to have a way to determine a time when the network is up and running to permit the network-related IRQs handling. The best candidate for it is the STMMAC_DOWN flag which has already been defined in the STMMAC driver. But the flag semantics was a bit misleading and couldn't be used for our case out-of-boxi (see the patches log for details). We suggest to fix it by converting the flag to be STMMAC_UP and being set all the time the network device is opened. Finally the GPIOs functionality is introduced. First patch adds a generic DW MAC GPI/GPO driver, which can be used for any currently available types of DW *MAC GPIO interfaces. Please see the patch log and its body for the implementation details (like a reason of the configuration cache usage, a set of callbacks required to be defined for each GPIO-available DW MAC, etc). Secondly we've created a set of GPIO-related callbacks to access DW GMAC GPO/GPI ports so the STMMAC driver would be able to register the corresponding GPIO-chip for Baikal-T1 GMAC. Note the similar set of callbacks can be created for DW XGMAC GPIOs, since the GPIO driver has been designed to be independent from the GPI/GPO CSR implementation. But aside with them the MAC and DMA cleanup methods need to be introduced to work around the problem (1) described above in this text. The series is supposed to be applied on top of the last revision of the next patchset: Link: https://lore.kernel.org/netdev/20210208140341.9271-1-Sergey.Semin@baikalelectronics.ru otherwise a few patches won't get merged in cleanly. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> Cc: Vyacheslav Mitrofanov <Vyacheslav.Mitrofanov@baikalelectronics.ru> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: netdev@vger.kernel.org Cc: linux-stm32@st-md-mailman.stormreply.com Cc: linux-arm-kernel@lists.infradead.org Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Serge Semin (16): dt-bindings: net: dwmac: Add DW GMAC GPIOs properties dt-bindings: net: Add Baikal-T1 GMAC bindings net: stmmac: Introduce MAC core cleanup method net: stmmac: Introduce DMA core cleanup method net: stmmac: Introduce MAC IRQs enable/disable methods net: stmmac: Extend DMA IRQs enable/disable interface net: stmmac: Introduce MTL IRQs enable/disable methods net: stmmac: Introduce Safety Feature IRQs enable/disable methods net: stmmac: Disable MMC IRQs in the generic IRQs disable method net: stmmac: Convert STMMAC_DOWN flag to STMMAC_UP net: stmmac: Add STMMAC state getter net: stmmac: Introduce NIC software reset function net: stmmac: Request IRQs at device probe stage net: stmmac: Add Generic DW MAC GPIO port driver net: stmmac: Add DW GMAC GPIOs support net: stmmac: Add DW MAC IPs of 3.72a/3.73a/3.74a versions .../bindings/net/baikal,bt1-gmac.yaml | 150 +++++++ .../devicetree/bindings/net/snps,dwmac.yaml | 17 + .../ethernet/stmicro/stmmac.rst | 4 + drivers/net/ethernet/stmicro/stmmac/Kconfig | 2 + drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +- drivers/net/ethernet/stmicro/stmmac/common.h | 1 + .../ethernet/stmicro/stmmac/dwmac-generic.c | 3 + .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 33 +- .../net/ethernet/stmicro/stmmac/dwmac1000.h | 11 + .../ethernet/stmicro/stmmac/dwmac1000_core.c | 101 ++++- .../ethernet/stmicro/stmmac/dwmac1000_dma.c | 6 +- .../ethernet/stmicro/stmmac/dwmac100_dma.c | 5 +- .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 57 ++- .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 17 +- .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 11 +- .../net/ethernet/stmicro/stmmac/dwmac4_lib.c | 51 ++- drivers/net/ethernet/stmicro/stmmac/dwmac5.c | 36 +- drivers/net/ethernet/stmicro/stmmac/dwmac5.h | 2 + .../net/ethernet/stmicro/stmmac/dwmac_dma.h | 7 +- .../net/ethernet/stmicro/stmmac/dwmac_lib.c | 38 +- .../ethernet/stmicro/stmmac/dwxgmac2_core.c | 76 +++- .../ethernet/stmicro/stmmac/dwxgmac2_dma.c | 35 +- drivers/net/ethernet/stmicro/stmmac/hwif.c | 3 + drivers/net/ethernet/stmicro/stmmac/hwif.h | 55 ++- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 22 +- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 4 +- .../net/ethernet/stmicro/stmmac/stmmac_gpio.c | 400 ++++++++++++++++++ .../net/ethernet/stmicro/stmmac/stmmac_main.c | 323 ++++++++++---- .../ethernet/stmicro/stmmac/stmmac_platform.c | 5 + include/linux/stmmac.h | 1 + 30 files changed, 1271 insertions(+), 207 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/baikal,bt1-gmac.yaml create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_gpio.c