From: Paul Burton <hidden> Date: 2018-02-17 20:14:07
The Intel EG20T Platform Controller Hub is used on the MIPS Boston
development board to provide various peripherals including ethernet.
This series fixes some issues with the pch_gbe driver discovered whilst
in use on the Boston board, and implements support for device tree which
we use to provide the PHY reset GPIO.
Applies atop v4.16-rc1.
Hassan Naveed (1):
net: pch_gbe: Fix TX RX descriptor accesses for big endian systems
Paul Burton (13):
net: pch_gbe: Mark Minnow PHY reset GPIO active low
net: pch_gbe: Pull PHY GPIO handling out of Minnow code
dt-bindings: net: Document Intel pch_gbe binding
net: pch_gbe: Add device tree support
net: pch_gbe: Always reset PHY along with MAC
net: pch_gbe: Allow longer for resets
net: pch_gbe: Fix handling of TX padding
net: pch_gbe: Fold pch_gbe_setup_[rt]ctl into pch_gbe_configure_[rt]x
net: pch_gbe: Use pch_gbe_disable_dma_rx() in pch_gbe_configure_rx()
net: pch_gbe: Disable TX DMA whilst configuring descriptors
net: pch_gbe: Ensure DMA is ordered with descriptor writes
ptp: pch: Allow build on MIPS platforms
net: pch_gbe: Allow build on MIPS platforms
Documentation/devicetree/bindings/net/pch_gbe.txt | 25 ++
drivers/net/ethernet/oki-semi/pch_gbe/Kconfig | 2 +-
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 27 +-
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 283 ++++++++++++---------
drivers/ptp/Kconfig | 2 +-
5 files changed, 204 insertions(+), 135 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/pch_gbe.txt
--
2.16.1
From: Paul Burton <hidden> Date: 2018-02-17 20:10:26
Introduce documentation for a device tree binding for the Intel Platform
Controller Hub (PCH) GigaBit Ethernet (GBE) device. Although this is a
PCIe device & thus largely auto-detectable, this binding will be used to
provide the driver with the PHY reset GPIO.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- Use standard gpio & ethernet node names in example.
- Remove bus number from example unit addresses.
Changes in v4: None
Changes in v3:
- New patch.
Changes in v2: None
Documentation/devicetree/bindings/net/pch_gbe.txt | 25 +++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/pch_gbe.txt
@@ -0,0 +1,25 @@+Intel Platform Controller Hub (PCH) GigaBit Ethernet (GBE)++Required properties:+- compatible: Should be the PCI vendor & device ID, eg. "pci8086,8802".+- reg: Should be a PCI device number as specified by the PCI bus+ binding to IEEE Std 1275-1994.+- phy-reset-gpios: Should be a GPIO list containing a single GPIO that+ resets the attached PHY when active.++Example:++ ethernet@0,1 {+ compatible = "pci8086,8802";+ reg = <0x00020100 0 0 0 0>;+ phy-reset-gpios = <&eg20t_gpio 6+ GPIO_ACTIVE_LOW>;+ };++ eg20t_gpio: gpio@0,2 {+ compatible = "pci8086,8803";+ reg = <0x00020200 0 0 0 0>;++ gpio-controller;+ #gpio-cells = <2>;+ };
From: Paul Burton <hidden> Date: 2018-02-17 20:10:28
On the MIPS Boston development board, the EG20T MAC does not report
receiving the RX clock from the (RGMII) RTL8211E PHY unless the PHY is
reset at the same time as the MAC. Since the pch_gbe driver resets the
MAC a number of times - twice during probe, and when taking down the
network interface - we need to reset the PHY at all the same times. Do
that from pch_gbe_mac_reset_hw which is used to reset the MAC in all
cases.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 3 +++
1 file changed, 3 insertions(+)
@@ -380,10 +380,13 @@ static void pch_gbe_mac_reset_hw(struct pch_gbe_hw *hw){/* Read the MAC address. and store to the private data */pch_gbe_mac_read_mac_addr(hw);+pch_gbe_phy_set_reset(hw,1);iowrite32(PCH_GBE_ALL_RST,&hw->reg->RESET);#ifdef PCH_GBE_MAC_IFOP_RGMIIiowrite32(PCH_GBE_MODE_GMII_ETHER,&hw->reg->MODE);#endif+pch_gbe_phy_set_reset(hw,0);+usleep_range(1250,1500);pch_gbe_wait_clr_bit(&hw->reg->RESET,PCH_GBE_ALL_RST);/* Setup the receive addresses */pch_gbe_mac_mar_set(hw,hw->mac.addr,0);
From: Paul Burton <hidden> Date: 2018-02-17 20:10:30
The ethernet controller found in the Intel EG20T Platform Controller
Hub requires that we place 2 bytes of padding between the ethernet
header & the packet payload. Our pch_gbe driver handles this by copying
packets to be transmitted to a temporary struct skb with the padding
bytes inserted, however it sets the length of this temporary skb to
equal that of the original, without the 2 padding bytes, and then uses
this length as that of the memory to map for DMA.
This is problematic on systems that don't have cache-coherent DMA, since
if the length of the original buffer is either a multiple of the
system's cache line size or one less than such a multiple then the size
we provide to dma_map_single() will not cover the last byte or two of
the data when rounded up to a cache line boundary. This may result in us
transmitting corrupt data in the last one or two bytes of the packet,
depending upon its length.
Fix this by setting the length of tmp_skb to include the 2 padding
bytes, which is actually the length of the data it holds. This is then
assigned to buffer_info->length & provided to dma_map_single() which
will operate on all of the data as desired. The EG20T datasheet
specifies that the padding bytes should not be included in the length
stored in the TX descriptor, so we switch that to use the length of the
original skb.
Whilst modifying this code we switch to using PCH_GBE_DMA_PADDING rather
than the magic number 2 to specify the size of the padding, making it
clearer what the code is doing, and fix a typo in the comment indicating
that padding is inserted.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
From: Paul Burton <hidden> Date: 2018-02-17 20:10:34
On weakly ordered systems writes to the RX or TX descriptors may be
reordered with the write to the DMA control register that enables DMA.
If this happens then the device may see descriptors in an intermediate
& invalid state, leading to incorrect behaviour. Add barriers to ensure
that DMA is enabled only after all writes to the descriptors.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- New patch.
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 6 ++++++
1 file changed, 6 insertions(+)
From: Paul Burton <hidden> Date: 2018-02-17 20:10:36
Allow the ptp_pch driver to be built on MIPS platforms in preparation
for use on the MIPS Boston board.
Signed-off-by: Paul Burton <redacted>
Acked-by: Richard Cochran <richardcochran@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- Newly included in this series to satisfy Kconfig.
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/ptp/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Paul Burton <hidden> Date: 2018-02-17 20:14:08
The Minnow PHY reset GPIO is set to 0 to enter reset & 1 to leave reset
- that is, it is an active low GPIO. In order to allow for the code to
be made more generic by further patches, indicate to the GPIO subsystem
that the GPIO is active low & invert the values it is set to such that
they reflect logically whether the device is being reset or not.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
From: Paul Burton <hidden> Date: 2018-02-17 20:14:09
The MIPS Boston development board uses the Intel EG20T Platform
Controller Hub, including its gigabit ethernet controller, and requires
that its RTL8211E PHY be reset much like the Minnow platform. Pull the
PHY reset GPIO handling out of Minnow-specific code such that it can be
shared by later patches.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- Name struct pch_gbe_privdata's platform_init pdata arg, per checkpatch.
Changes in v4: None
Changes in v3:
- Use adapter->pdata as arg to platform_init, to fix bisectability.
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 5 +++-
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 33 +++++++++++++++-------
2 files changed, 27 insertions(+), 11 deletions(-)
@@ -2592,7 +2602,14 @@ static int pch_gbe_probe(struct pci_dev *pdev,adapter->hw.reg=pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];adapter->pdata=(structpch_gbe_privdata*)pci_id->driver_data;if(adapter->pdata&&adapter->pdata->platform_init)-adapter->pdata->platform_init(pdev);+adapter->pdata->platform_init(pdev,adapter->pdata);++if(adapter->pdata&&adapter->pdata->phy_reset_gpio){+pch_gbe_phy_set_reset(&adapter->hw,1);+usleep_range(1250,1500);+pch_gbe_phy_set_reset(&adapter->hw,0);+usleep_range(1250,1500);+}adapter->ptp_pdev=pci_get_domain_bus_and_slot(pci_domain_nr(adapter->pdev->bus),
@@ -2686,7 +2703,8 @@ static int pch_gbe_probe(struct pci_dev *pdev,/* The AR803X PHY on the MinnowBoard requires a physical pin to be toggled to*ensureitisawakeforprobeandinit.RequestthelineandresetthePHY.*/-staticintpch_gbe_minnow_platform_init(structpci_dev*pdev)+staticintpch_gbe_minnow_platform_init(structpci_dev*pdev,+structpch_gbe_privdata*pdata){unsignedlongflags=GPIOF_DIR_OUT|GPIOF_INIT_LOW|GPIOF_EXPORT|GPIOF_ACTIVE_LOW;
@@ -2695,16 +2713,11 @@ static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)ret=devm_gpio_request_one(&pdev->dev,gpio,flags,"minnow_phy_reset");-if(ret){+if(!ret)+pdata->phy_reset_gpio=gpio_to_desc(gpio);+elsedev_err(&pdev->dev,"ERR: Can't request PHY reset GPIO line '%d'\n",gpio);-returnret;-}--gpio_set_value(gpio,1);-usleep_range(1250,1500);-gpio_set_value(gpio,0);-usleep_range(1250,1500);returnret;}
From: Paul Burton <hidden> Date: 2018-02-17 20:14:17
Introduce support for retrieving the PHY reset GPIO from device tree,
which will be used on the MIPS Boston development board. This requires
support for probe deferral in order to work correctly, since the order
of device probe is not guaranteed & typically the EG20T GPIO controller
device will be probed after the ethernet MAC.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5: None
Changes in v4:
- Use ERR_CAST(), thanks kbuild test robot/Fengguang!
Changes in v3: None
Changes in v2:
- Tidy up handling of parsing private data, drop err_out.
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 31 +++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
From: Paul Burton <hidden> Date: 2018-02-17 20:14:22
Resets of the EG20T MAC on the MIPS Boston development board take longer
than the 1000 loops that pch_gbe_wait_clr_bit was performing. Rather
than simply increasing the number of loops, switch to using
readl_poll_timeout_atomic() from linux/iopoll.h in order to provide some
independence from the speed of the CPU.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- Bump up the timeout based on feedback from Marcin.
Changes in v4: None
Changes in v3:
- Switch to using readl_poll_timeout_atomic().
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Paul Burton <hidden> Date: 2018-02-17 20:14:23
The pch_gbe driver splits configuration of the receive path between
pch_gbe_setup_rctl() & pch_gbe_configure_rx(), which are always called
together and in that order. The split between the two functions seems
somewhat arbitrary, as both are configuring registers for the receive
path. Fold pch_gbe_setup_rctl() into pch_gbe_configure_rx() such that
callers only need to call one function to configure the receive path
registers.
Similarly configuration of transmit path registers is split between
pch_gbe_setup_tctl() & pch_gbe_configure_tx(), and we fold the former
into the latter in the same way.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- New patch.
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 52 ++++++----------------
1 file changed, 13 insertions(+), 39 deletions(-)
@@ -1954,9 +1931,7 @@ int pch_gbe_up(struct pch_gbe_adapter *adapter)/* hardware has been reset, we need to reload some things */pch_gbe_set_multi(netdev);-pch_gbe_setup_tctl(adapter);pch_gbe_configure_tx(adapter);-pch_gbe_setup_rctl(adapter);pch_gbe_configure_rx(adapter);err=pch_gbe_request_irq(adapter);
@@ -2486,7 +2461,6 @@ static int __pch_gbe_suspend(struct pci_dev *pdev)pch_gbe_down(adapter);if(wufc){pch_gbe_set_multi(netdev);-pch_gbe_setup_rctl(adapter);pch_gbe_configure_rx(adapter);pch_gbe_set_rgmii_ctrl(adapter,hw->mac.link_speed,hw->mac.link_duplex);
From: Paul Burton <hidden> Date: 2018-02-17 20:14:31
The pch_gbe_configure_rx() function open-codes the equivalent of
pch_gbe_disable_dma_rx(). Remove the duplication by moving
pch_gbe_disable_dma_rx(), and pch_gbe_enable_dma_rx() for consistency,
to be defined earlier than pch_gbe_configure_rx() and have
pch_gbe_configure_rx() call pch_gbe_disable_dma_rx() rather than
duplicate its functionality.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- New patch.
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 48 ++++++++++------------
1 file changed, 22 insertions(+), 26 deletions(-)
From: Paul Burton <hidden> Date: 2018-02-17 20:14:32
The pch_gbe driver enables TX DMA the first time we call
pch_gbe_configure_tx() and never disables it again, even if we
reconfigure the device & modify the transmit descriptor ring. This seems
unsafe, since the device may continue accessing descriptors whilst they
are in an unpredictable & possibly invalid state - especially on systems
where the CPUs writes to the descriptors is not coherent with DMA.
In the RX path pch_gbe_configure_rx() disables DMA before configuring
the descriptor pointers & before we set up the descriptors, then
pch_gbe_up() calls pch_gbe_enable_dma_rx() to enable DMA again after the
descriptors have been configured. Here we copy that same scheme for the
TX path - pch_gbe_configure_tx() calls pch_gbe_disable_dma_tx() to
disable DMA, and then after the descriptors have been configured
pch_gbe_up() calls pch_gbe_enable_dma_tx() to enable DMA. This should
ensure that the device doesn't begin reading descriptors before we have
configured them.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- New patch.
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 29 +++++++++++++++++-----
1 file changed, 23 insertions(+), 6 deletions(-)
From: Paul Burton <hidden> Date: 2018-02-17 20:14:35
From: Hassan Naveed <redacted>
Fix pch_gbe driver for ethernet operations for a big endian CPU.
Values written to and read from transmit and receive descriptors
in the pch_gbe driver are byte swapped from the perspective of a
big endian CPU, since the ethernet controller always operates in
little endian mode. Rectify this by appropriately byte swapping
these descriptor field values in the driver software.
Signed-off-by: Hassan Naveed <redacted>
Signed-off-by: Paul Burton <redacted>
Reviewed-by: Paul Burton <redacted>
Reviewed-by: Matt Redfearn <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5:
- Newly included in this series.
Changes in v4: None
Changes in v3: None
Changes in v2:
- Use __le{16,32} for field types, checked with sparse.
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 22 ++++----
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 66 ++++++++++++----------
2 files changed, 46 insertions(+), 42 deletions(-)
From: Paul Burton <hidden> Date: 2018-02-17 20:14:38
Allow the pch_gbe driver to be built on MIPS platforms, allowing its use
on the MIPS Boston development board.
Signed-off-by: Paul Burton <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-02-17 22:29:35
On Sat, Feb 17, 2018 at 12:10:25PM -0800, Paul Burton wrote:
The MIPS Boston development board uses the Intel EG20T Platform
Controller Hub, including its gigabit ethernet controller, and requires
that its RTL8211E PHY be reset much like the Minnow platform. Pull the
PHY reset GPIO handling out of Minnow-specific code such that it can be
shared by later patches.
Hi Paul
I'm i right in saying the driver currently supports the Atheros AT8031
PHY? The same phy which is supported in drivers/net/phy/at803x.c?
If so, i think you are doing this all wrong. You would be much better
off throwing away pch_gbe_phy.c and write a proper MDIO driver. You
then get the PHY driver for free, and the MDIO code could will handle
your GPIO for you, in the standardised way.
Andrew
From: Paul Burton <hidden> Date: 2018-02-17 22:53:47
Hi Andrew,
On Sat, Feb 17, 2018 at 11:29:33PM +0100, Andrew Lunn wrote:
On Sat, Feb 17, 2018 at 12:10:25PM -0800, Paul Burton wrote:
quoted
The MIPS Boston development board uses the Intel EG20T Platform
Controller Hub, including its gigabit ethernet controller, and requires
that its RTL8211E PHY be reset much like the Minnow platform. Pull the
PHY reset GPIO handling out of Minnow-specific code such that it can be
shared by later patches.
Hi Paul
I'm i right in saying the driver currently supports the Atheros AT8031
PHY? The same phy which is supported in drivers/net/phy/at803x.c?
It looks like the driver does contain some code relating to that PHY,
but it's not the one I'm using with the MIPS Boston board - there we
have a Realtek RTL8211E (as mentioned in the commit message) which is
working fine alongside this pch_gbe driver too.
If so, i think you are doing this all wrong.
Note that this is a driver which is already in mainline, and I didn't
write it. Claiming that *I* am doing this all wrong is a bit of a
stretch - all this patch does is make small changes to some existing
code, which only tangentially relates to a PHY driver, such that it
ceases to be specific to a single platform.
You would be much better off throwing away pch_gbe_phy.c and write a
proper MDIO driver. You then get the PHY driver for free, and the MDIO
code could will handle your GPIO for you, in the standardised way.
Even if that is true, rewriting the driver's PHY handling would be a
very separate change to the changes this series make which allow this
driver to work on a platform besides the Minnowboard. The *only* thing
this series does relating to the PHY is allow the reset GPIO to be
handled properly - rewriting the existing PHY handling is beyond it's
scope.
Note that I do have various cleanups to the driver beyond this series
which I intend to submit after it is functional for my system[1], so I
am not saying that I don't care about improving the driver. But please,
let's do one thing at a time.
Thanks,
Paul
[1] https://git.linux-mips.org/cgit/paul/linux.git/log/?h=up417-boston-eth-cleanup
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-02-17 23:34:46
Note that this is a driver which is already in mainline, and I didn't
write it. Claiming that *I* am doing this all wrong is a bit of a
stretch - all this patch does is make small changes to some existing
code, which only tangentially relates to a PHY driver, such that it
ceases to be specific to a single platform.
Hi Paul
I would so you are doing it all wrong for the reset GPIO.
Even if that is true, rewriting the driver's PHY handling would be a
very separate change to the changes this series make which allow this
driver to work on a platform besides the Minnowboard. The *only* thing
this series does relating to the PHY is allow the reset GPIO to be
handled properly - rewriting the existing PHY handling is beyond it's
scope.
Well, you are adding a device tree binding, which needs to be
supported forever. This is going to make things messy in the future
when you do such a cleanup that you follow the PHY binding, in that
you have to handle both what you add here, and the official PHY
binding.
I would prefer that for the moment, you drop the PHY binding patches
in this series. That is what i object to the most. Adding an MDIO
driver and using the standard PHY driver for this PHY is all
internal. You can change that anytime. But adding a binding means an
ABI.
Andrew
From: kbuild test robot <hidden> Date: 2018-02-18 15:30:18
Hi Paul,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on v4.16-rc1 next-20180216]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Paul-Burton/net-pch_gbe-Fixes-MIPS-support/20180218-213023
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:33: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned short uid_hi @@ got short uid_hi @@
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:33: expected unsigned short uid_hi
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:33: got restricted __be16 <noident>
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:45: sparse: incorrect type in argument 3 (different base types) @@ expected unsigned int uid_lo @@ got ed int uid_lo @@
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:45: expected unsigned int uid_lo
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:45: got restricted __be32 <noident>
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:56: sparse: incorrect type in argument 4 (different base types) @@ expected unsigned short seqid @@ got short seqid @@
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:56: expected unsigned short seqid
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:177:56: got restricted __be16 <noident>
quoted
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:325:15: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile @@ got @@
From: Paul Burton <hidden> Date: 2018-02-18 15:50:13
Hi Andrew,
On Sun, Feb 18, 2018 at 12:34:42AM +0100, Andrew Lunn wrote:
quoted
Even if that is true, rewriting the driver's PHY handling would be a
very separate change to the changes this series make which allow this
driver to work on a platform besides the Minnowboard. The *only* thing
this series does relating to the PHY is allow the reset GPIO to be
handled properly - rewriting the existing PHY handling is beyond it's
scope.
Well, you are adding a device tree binding, which needs to be
supported forever. This is going to make things messy in the future
when you do such a cleanup that you follow the PHY binding, in that
you have to handle both what you add here, and the official PHY
binding.
Thank you - it's useful to know what your concern actually is.
I would prefer that for the moment, you drop the PHY binding patches
in this series. That is what i object to the most. Adding an MDIO
driver and using the standard PHY driver for this PHY is all
internal. You can change that anytime. But adding a binding means an
ABI.
The problem is that the device in question doesn't actually work unless
we reset the PHY, so just removing the PHY reset GPIO handling would
break things.
How would you feel if I were to adjust the binding to match the standard
PHY binding, but internally leave the driver's PHY handling as-is for
now? That would:
1) Allow for the pch_gbe driver to move towards more standard PHY
handling in the future without DT changes.
2) Be fairly straightforward to implement in this patchset - the code
reading the DT would just follow the phandle to the PHY node to
find the reset GPIO - thereby not holding up the rest of the series.
3) Still function on our hardware.
Thanks,
Paul
From: Paul Burton <hidden> Date: 2018-02-18 17:01:46
Hi David,
On Sun, Feb 18, 2018 at 10:31:12AM -0500, David Miller wrote:
Nobody is going to see and apply these patches if you don't CC: the
Linux networking development list, netdev@vger.kernel.org
You're replying to mail that was "To: netdev@vger.kernel.org" and I see
the whole series in the archives[1] so it definitely reached the list.
I'm not sure I see the problem?
Thanks,
Paul
[1] https://www.spinics.net/lists/netdev/msg484102.html
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-02-18 17:56:13
On Sun, Feb 18, 2018 at 09:03:10AM -0800, Paul Burton wrote:
Hi David,
On Sun, Feb 18, 2018 at 10:31:12AM -0500, David Miller wrote:
quoted
Nobody is going to see and apply these patches if you don't CC: the
Linux networking development list, netdev@vger.kernel.org
You're replying to mail that was "To: netdev@vger.kernel.org" and I see
the whole series in the archives[1] so it definitely reached the list.
I'm not sure I see the problem?
Hi Paul
I'm guess that David is wondering about version 1-4 of this patchset?
As far as i can see, they were sent to the mips list, not the netdev
list.
Andrew
From: Paul Burton <hidden> Date: 2018-02-18 22:07:45
Hi Andrew,
On Sun, Feb 18, 2018 at 06:56:07PM +0100, Andrew Lunn wrote:
On Sun, Feb 18, 2018 at 09:03:10AM -0800, Paul Burton wrote:
quoted
Hi David,
On Sun, Feb 18, 2018 at 10:31:12AM -0500, David Miller wrote:
quoted
Nobody is going to see and apply these patches if you don't CC: the
Linux networking development list, netdev@vger.kernel.org
You're replying to mail that was "To: netdev@vger.kernel.org" and I see
the whole series in the archives[1] so it definitely reached the list.
I'm not sure I see the problem?
Hi Paul
I'm guess that David is wondering about version 1-4 of this patchset?
As far as i can see, they were sent to the mips list, not the netdev
list.
From: David Miller <davem@davemloft.net> Date: 2018-02-19 01:15:34
From: Paul Burton <redacted>
Date: Sun, 18 Feb 2018 09:03:10 -0800
Hi David,
On Sun, Feb 18, 2018 at 10:31:12AM -0500, David Miller wrote:
quoted
Nobody is going to see and apply these patches if you don't CC: the
Linux networking development list, netdev@vger.kernel.org
You're replying to mail that was "To: netdev@vger.kernel.org" and I see
the whole series in the archives[1] so it definitely reached the list.
I'm not sure I see the problem?
Sorry.
The issue is that your patch series didn't make it into patchwork
properly, I wonder what happened since you did send it to netdev.
Hmmm...
From: Paul Burton <redacted>
Date: Sun, 18 Feb 2018 09:03:10 -0800
quoted
Hi David,
On Sun, Feb 18, 2018 at 10:31:12AM -0500, David Miller wrote:
quoted
Nobody is going to see and apply these patches if you don't CC: the
Linux networking development list, netdev@vger.kernel.org
You're replying to mail that was "To: netdev@vger.kernel.org" and I see
the whole series in the archives[1] so it definitely reached the list.
I'm not sure I see the problem?
Sorry.
The issue is that your patch series didn't make it into patchwork
properly, I wonder what happened since you did send it to netdev.
Hmmm...
From: David Laight <hidden> Date: 2018-02-19 14:00:39
From: Paul Burton
Sent: 17 February 2018 20:11
The ethernet controller found in the Intel EG20T Platform Controller
Hub requires that we place 2 bytes of padding between the ethernet
header & the packet payload. Our pch_gbe driver handles this by copying
packets to be transmitted to a temporary struct skb with the padding
bytes inserted
...
Uggg WFT is the driver doing that for?
I'd guess that the two byte pad is there so that a 4 byte aligned
frame is still 4 byte aligned when the 14 byte ethernet header is added.
So instead of copying the entire frame the MAC header should be built
(or rebuilt?) two bytes further from the actual data.
David
From: Paul Burton <hidden> Date: 2018-02-19 16:41:06
Hi David,
On Mon, Feb 19, 2018 at 02:01:25PM +0000, David Laight wrote:
From: Paul Burton
quoted
Sent: 17 February 2018 20:11
The ethernet controller found in the Intel EG20T Platform Controller
Hub requires that we place 2 bytes of padding between the ethernet
header & the packet payload. Our pch_gbe driver handles this by copying
packets to be transmitted to a temporary struct skb with the padding
bytes inserted
...
Uggg WFT is the driver doing that for?
I'd guess that the two byte pad is there so that a 4 byte aligned
frame is still 4 byte aligned when the 14 byte ethernet header is added.
So instead of copying the entire frame the MAC header should be built
(or rebuilt?) two bytes further from the actual data.
I agree - the pch_gbe driver is pretty bad and does a lot of things
wrong. Frankly I'm amazed it's in tree, but it is & one patch series
isn't going to fix all of its shortcomings.
So whilst I totally agree that copying around the whole frame is awful,
it's a separate problem to the length used for DMA mapping being
incorrect which is what this patch addresses & I'd rather not start
adding more & more fixes or cleanups into this initial series before the
driver is even functional on my hardware.
Thanks,
Paul
From: David Miller <davem@davemloft.net> Date: 2018-02-19 16:41:46
From: Paul Burton <redacted>
Date: Mon, 19 Feb 2018 08:42:23 -0800
So whilst I totally agree that copying around the whole frame is awful,
it's a separate problem to the length used for DMA mapping being
incorrect which is what this patch addresses & I'd rather not start
adding more & more fixes or cleanups into this initial series before the
driver is even functional on my hardware.