Re: [PATCH RESEND] ARM: dts: Fix Makefile target for sun4i-a10-itead-iteaduino-plus
On Sun, 2015-09-06 at 13:20 +0200, Maxime Ripard wrote: > On Fri, Sep 04, 2015 at 08:49:34AM -0400, Josh Boyer wrote: > > Commit 79ae3e66f8d (ARM: dts: sun4i: Add Iteaduino Plus A10) added a new > > make target for the sun4i-a10-itead-iteaduino-plus dts file, but mistakenly > > used .dts instead of the correct .dtb suffix. This resulted in a build > > error > > like: > > > > scripts/Makefile.dtbinst:42: target > > 'sun4i-a10-itead-iteaduino-plus.dts' doesn't match the target pattern > > > > when doing a make dtbs_install. > > > > Fix it to use the proper file name. > > > > Signed-off-by: Josh Boyer > > Queued, thanks! This still isn't in Linus's tree; please make sure it gets to him before 4.3. Ben. -- Ben Hutchings compatible: Gracefully accepts erroneous data from any source signature.asc Description: This is a digitally signed message part
Re: [Linux-kernel] [RFC PATCH 5/7] mmc: sh_mobile_sdhi: Add UHS-I mode support
On Tue, 2015-05-05 at 10:47 +0200, Ulf Hansson wrote: > On 5 May 2015 at 10:35, Ben Dooks wrote: > > On 05/05/15 10:56, Ulf Hansson wrote: > >> On 30 April 2015 at 14:32, Ben Hutchings > >> wrote: > >>> Implement voltage switch, supporting modes up to SDR-50. > >>> > >>> Based on work by Shinobu Uehara, Rob Taylor, William Towle and Ian Molton. > >>> > >>> This uses two voltage regulators, one external and one on the pfc. > >> > >> Why two? If there is a parent child relation ship, that should be > >> handled through the regulator tree, right!? Please elaborate. > > > > The card main power is separate from the IO line voltages. > > > > To get to the high-speed, card power is left at 3.3V and the IO > > voltage is changed to 1.8V. > > > > In the systems we have the power gate is separate from the controls > > for the IO but not integrated into the MMC controller itself. In this case, there are *three* regulators: 1. External regulator for card power (VDD pin): "vmmc" 2. External regulator for pull-up voltage for the data pins: "vqmmc" 3. Internal regulator for input(?) level on the data pins: "vqmmc_ref" (I'm open to suggestions of a better name) > Okay, that's what I was expecting and hoping for :-) > > Then you need to rework $subject patch according to below. > > 1) Use mmc_regulator_get_supply() to fetch both the I/O voltage > regulator (vqmmc) and the main power regulator (vmmc). We already do that. > Your "vqmmc_ref" regulator should thus be renamed to "vmmc". No, we have one of those already. > 2) The vmmc regulator should not be handled from the > ->start_signal_voltage_switch() callback, since it's only vqmmc > voltage levels that should be changed from there. It isn't. > 3) The voltage levels changes for vmmc shall be handled via the > ->set_ios() callback. We don't support UHS-II so we never change the voltage of this regulator. Ben. > I suggest you go and have a look in drivers/mmc/host/mmci.c, that > should provide you with a good inspiration. > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/7] pinctrl: sh-pfc: r8a7790: Add regulators for SD voltage switch
On Tue, 2015-05-05 at 09:52 +0200, Ulf Hansson wrote: > On 30 April 2015 at 14:31, Ben Hutchings > wrote: > > Model the choice of 1.8V or 3.3V signalling for each SD interface as a > > regulator. > > > > Signed-off-by: Ben Hutchings > > You need also to send this to the pinctrl maintainer and the corresponding > list. [...] OK, will do next time. Ben. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 7/7] ARM: shmobile: r8a7790-lager.dts: Assert UHS-I SDR-50 capability
From: William Towle
[bwh: Also enable the 'regulators' on the pfc that we need]
Signed-off-by: Ben Hutchings
---
arch/arm/boot/dts/r8a7790-lager.dts | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7790-lager.dts
b/arch/arm/boot/dts/r8a7790-lager.dts
index 343ec0ccc8df..8bb4507f3112 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -384,6 +384,14 @@
renesas,groups = "audio_clk_a";
renesas,function = "audio_clk";
};
+
+ sd-regulator@0 {
+ status = "ok";
+ };
+
+ sd-regulator@2 {
+ status = "ok";
+ };
};
ðer {
@@ -499,6 +507,7 @@
vmmc-supply = <&vcc_sdhi0>;
vqmmc-supply = <&vccq_sdhi0>;
cd-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
+ sd-uhs-sdr50;
status = "okay";
};
@@ -512,6 +521,7 @@
vmmc-supply = <&vcc_sdhi2>;
vqmmc-supply = <&vccq_sdhi2>;
cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
+ sd-uhs-sdr50;
status = "okay";
};
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 5/7] mmc: sh_mobile_sdhi: Add UHS-I mode support
Implement voltage switch, supporting modes up to SDR-50.
Based on work by Shinobu Uehara, Rob Taylor, William Towle and Ian Molton.
This uses two voltage regulators, one external and one on the pfc.
Signed-off-by: Ben Hutchings
---
drivers/mmc/host/sh_mobile_sdhi.c | 48 +
1 file changed, 48 insertions(+)
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c
b/drivers/mmc/host/sh_mobile_sdhi.c
index 92a58c6007fe..c8538a256e22 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include "tmio_mmc.h"
@@ -84,6 +85,7 @@ MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match);
struct sh_mobile_sdhi {
struct clk *clk;
+ struct regulator *vqmmc_ref;
struct tmio_mmc_data mmc_data;
struct tmio_mmc_dma dma_priv;
};
@@ -148,6 +150,41 @@ static void sh_mobile_sdhi_set_clk_div(struct
platform_device *pdev, int clk)
}
}
+static int sh_mobile_sdhi_start_signal_voltage_switch(
+ struct tmio_mmc_host *host, unsigned char signal_voltage)
+{
+ struct sh_mobile_sdhi *priv = host_to_priv(host);
+ int min_uV, max_uV;
+ int ret;
+
+ if (signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
+ min_uV = 270;
+ max_uV = 360;
+ } else if (signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
+ min_uV = 170;
+ max_uV = 195;
+ } else {
+ return -EINVAL;
+ }
+
+ if (!IS_ERR(host->mmc->supply.vqmmc)) {
+ ret = regulator_set_voltage(host->mmc->supply.vqmmc,
+ min_uV, max_uV);
+ if (ret)
+ return ret;
+ }
+
+ if (!IS_ERR(priv->vqmmc_ref)) {
+ ret = regulator_set_voltage(priv->vqmmc_ref,
+ min_uV, max_uV);
+ if (ret)
+ return ret;
+ }
+
+ usleep_range(5000, 5500);
+ return 0;
+}
+
static int sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host)
{
int timeout = 1000;
@@ -240,6 +277,13 @@ static int sh_mobile_sdhi_probe(struct platform_device
*pdev)
goto eprobe;
}
+ priv->vqmmc_ref = devm_regulator_get_optional(&pdev->dev, "vqmmc-ref");
+ if (IS_ERR(priv->vqmmc_ref)) {
+ if (PTR_ERR(priv->vqmmc_ref) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ dev_info(&pdev->dev, "No vqmmc reference regulator found\n");
+ }
+
host = tmio_mmc_host_alloc(pdev);
if (!host) {
ret = -ENOMEM;
@@ -253,6 +297,10 @@ static int sh_mobile_sdhi_probe(struct platform_device
*pdev)
host->clk_enable= sh_mobile_sdhi_clk_enable;
host->clk_disable = sh_mobile_sdhi_clk_disable;
host->multi_io_quirk= sh_mobile_sdhi_multi_io_quirk;
+
+ host->start_signal_voltage_switch
+ = sh_mobile_sdhi_start_signal_voltage_switch;
+
/* SD control register space size is 0x100, 0x200 for bus_shift=1 */
if (resource_size(res) > 0x100)
host->bus_shift = 1;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 4/7] ARM: shmobile: r8a7790: Add nodes for pfc SD voltage regulators
Signed-off-by: Ben Hutchings
---
arch/arm/boot/dts/r8a7790.dtsi | 21 +
1 file changed, 21 insertions(+)
diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index 4bb2f4c17321..23e826153a9d 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -483,6 +483,23 @@
pfc: pfc@e606 {
compatible = "renesas,pfc-r8a7790";
reg = <0 0xe606 0 0x250>;
+
+ vccq_ref_sdhi0: sd-regulator@0 {
+ compatible = "renesas,pfc-r8a7790-sd-regulator";
+ status = "disabled";
+ };
+ vccq_ref_sdhi1: sd-regulator@1 {
+ compatible = "renesas,pfc-r8a7790-sd-regulator";
+ status = "disabled";
+ };
+ vccq_ref_sdhi2: sd-regulator@2 {
+ compatible = "renesas,pfc-r8a7790-sd-regulator";
+ status = "disabled";
+ };
+ vccq_ref_sdhi3: sd-regulator@3 {
+ compatible = "renesas,pfc-r8a7790-sd-regulator";
+ status = "disabled";
+ };
};
sdhi0: sd@ee10 {
@@ -490,6 +507,7 @@
reg = <0 0xee10 0 0x328>;
interrupts = <0 165 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&mstp3_clks R8A7790_CLK_SDHI0>;
+ vqmmc-ref-supply = <&vccq_ref_sdhi0>;
dmas = <&dmac1 0xcd>, <&dmac1 0xce>;
dma-names = "tx", "rx";
status = "disabled";
@@ -500,6 +518,7 @@
reg = <0 0xee12 0 0x328>;
interrupts = <0 166 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&mstp3_clks R8A7790_CLK_SDHI1>;
+ vqmmc-ref-supply = <&vccq_ref_sdhi1>;
dmas = <&dmac1 0xc9>, <&dmac1 0xca>;
dma-names = "tx", "rx";
status = "disabled";
@@ -510,6 +529,7 @@
reg = <0 0xee14 0 0x100>;
interrupts = <0 167 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&mstp3_clks R8A7790_CLK_SDHI2>;
+ vqmmc-ref-supply = <&vccq_ref_sdhi2>;
dmas = <&dmac1 0xc1>, <&dmac1 0xc2>;
dma-names = "tx", "rx";
status = "disabled";
@@ -520,6 +540,7 @@
reg = <0 0xee16 0 0x100>;
interrupts = <0 168 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&mstp3_clks R8A7790_CLK_SDHI3>;
+ vqmmc-ref-supply = <&vccq_ref_sdhi3>;
dmas = <&dmac1 0xd3>, <&dmac1 0xd4>;
dma-names = "tx", "rx";
status = "disabled";
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 6/7] ARM: shmobile: r8a7790-lager.dts: Set sdhi and mmcif clock rates
From: Ben Dooks [bwh: Fold in fix from Ian Molton] Signed-off-by: Ben Hutchings --- arch/arm/boot/dts/r8a7790-lager.dts | 11 +++ 1 file changed, 11 insertions(+) diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts index aaa4f258e279..343ec0ccc8df 100644 --- a/arch/arm/boot/dts/r8a7790-lager.dts +++ b/arch/arm/boot/dts/r8a7790-lager.dts @@ -413,6 +413,11 @@ vmmc-supply = <&fixedregulator3v3>; bus-width = <8>; non-removable; + + assigned-clocks = <&mstp3_clks R8A7790_CLK_MMCIF1>; + assigned-clock-rates = <9750>; + max-frequency = <5000>; + status = "okay"; }; @@ -488,6 +493,9 @@ pinctrl-0 = <&sdhi0_pins>; pinctrl-names = "default"; + assigned-clocks = <&mstp3_clks R8A7790_CLK_SDHI0>; + assigned-clock-rates = <15600>; + vmmc-supply = <&vcc_sdhi0>; vqmmc-supply = <&vccq_sdhi0>; cd-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; @@ -498,6 +506,9 @@ pinctrl-0 = <&sdhi2_pins>; pinctrl-names = "default"; + assigned-clocks = <&mstp3_clks R8A7790_CLK_SDHI2>; + assigned-clock-rates = <9750>; + vmmc-supply = <&vcc_sdhi2>; vqmmc-supply = <&vccq_sdhi2>; cd-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>; -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 0/7] UHS-I support for sh_mobile_sdhi
This series adds support for UHS-I in sh_mobile_sdhi, partly implemented in tmio_mmc. This does not yet include tuning for SDR-104, but SDR-50 works. The pfc block needs to be configured for 1.8V or 3.3V signalling on the SDHI, separately from the external voltage regulator. I've treated this as an additional regulator that sh_mobile_sdhi has to adjust, but I'm not entirely happy with this or the way it's represented in the DT. I would appreciate advice on how to improve that. Ben. Ben Dooks (1): ARM: shmobile: r8a7790-lager.dts: Set sdhi and mmcif clock rates Ben Hutchings (4): mmc: tmio: Add UHS-I mode support pinctrl: sh-pfc: r8a7790: Add regulators for SD voltage switch ARM: shmobile: r8a7790: Add nodes for pfc SD voltage regulators mmc: sh_mobile_sdhi: Add UHS-I mode support Shinobu Uehara (1): mmc: sh_mobile_sdhi: Add actual clock rate support William Towle (1): ARM: shmobile: r8a7790-lager.dts: Assert UHS-I SDR-50 capability arch/arm/boot/dts/r8a7790-lager.dts | 21 arch/arm/boot/dts/r8a7790.dtsi | 21 drivers/mmc/host/sh_mobile_sdhi.c| 62 +++ drivers/mmc/host/tmio_mmc.h |3 + drivers/mmc/host/tmio_mmc_pio.c | 31 ++ drivers/pinctrl/sh-pfc/Kconfig |1 + drivers/pinctrl/sh-pfc/core.c|2 +- drivers/pinctrl/sh-pfc/core.h|1 + drivers/pinctrl/sh-pfc/pfc-r8a7790.c | 189 ++ 9 files changed, 330 insertions(+), 1 deletion(-) -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] Renesas Ethernet AVB driver
On Fri, 2015-04-24 at 21:53 +0300, Sergei Shtylyov wrote:
> On 04/23/2015 02:22 AM, Florian Fainelli wrote:
>
> [...]
>
> +if (ecmd->duplex == DUPLEX_FULL)
> +priv->duplex = 1;
> +else
> +priv->duplex = 0;
>
> >>> Why not use what priv->phydev->duplex has cached for you?
>
> >> Because we compare 'priv->duplex' with 'priv->phydev->duplex' in
> >> ravb_adjust_link(). Or what did you mean?
>
> > Oh I see how you are using this now, but it does not look like it is
> > necessary, since you use phy_ethtool_sset() using phydev->duplex
>
>It only writes to it, doesn't use it AFAICS...
>
> > directly ought to be enough anywhere in your driver?
>
> 'priv->phydev' is NULL when the device is closed, so I just can't call
> phy_ethtool_sset().
>
> > Unless there is a
> > mode where you are running PHY-less, and not using a fixed PHY to
> > emulate a PHY...
>
> No such mode.
>
> >> [...]
>
> +static int ravb_nway_reset(struct net_device *ndev)
> +{
> +struct ravb_private *priv = netdev_priv(ndev);
> +int error = -ENODEV;
> +unsigned long flags;
> +
> +if (priv->phydev) {
>
> >>> Is checking against priv->phydev really necessary, it does not look like
> >>> the driver will work or accept an invalid PHY device at all anyway?
>
> This check was copied from sh_eth that was fixed by Ben ot to crash due to
> 'ethtool' being called on closed device, see:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/renesas/sh_eth.c?id=4f9dce230b32eec45cec8c28cae61efdfa2f7d57
>
> That commit refers to a dangling pointer, not sure what does this mean...
> The PHy device doesn't seem to be freed by phy_disconnect(). Ben?
[...]
In practice the phy_device is unlikely to be freed immediately. Bt it
is certainly not valid for a net driver to pass a phy_device pointer to
phylib functions after calling phy_disconnect() on it.
Ben.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH resend] Renesas Ethernet AVB driver
On Tue, 2015-04-14 at 00:53 +0200, Lino Sanfilippo wrote:
> On 14.04.2015 00:31, Ben Hutchings wrote:
>
> >> >> This driver looks somewhat similar to sh-eth, but lacks some of the
> >> >> recent bug fixes made to that. At least commit 283e38db65e7 ("sh_eth:
> >> >> Fix serialisation of interrupt disable with interrupt & NAPI handler")
> >> >> appears to be applicable, but there are probably others.
> >> >
> >> > I suspect this issue applies to many drivers...
> >> > I couldn't reproduce the bug that patch was fixing, so left this fix
> >> > out
> >> > for the time being. Others cases were fixed (if applicable).
> >>
> >> Maybe its just harder to trigger but it indeed looks similar to what Ben
> >> has fixed for sh-eth. I wonder if that shutdown flag in the fix is
> >> really needed though. IMHO it should be save if we simply call
> >> napi_disable first, then disable irqs on hardware and finally
> >> synchronize_irq...
> >
> > In sh_eth: if we call napi_disable() first, EESR_RX_CHECK can still be
> > set and nothing will clear it. If only one CPU is online this can hard
> > hang the system. Please trust that I did consider and rule out the
> > simpler approaches first.
> >
>
> The idea was to check the return value from napi_schedule_prep() and in
> case it returns "false" use this as an indication for a shutdown. In
> case of sh_eth this would be sh_eth_write(ndev, 0, EESIPR) for example.
Might work - but please give it a thorough test.
Ben.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH resend] Renesas Ethernet AVB driver
On Tue, 2015-04-14 at 00:13 +0200, Lino Sanfilippo wrote:
> Hi,
>
> On 13.04.2015 22:34, Sergei Shtylyov wrote:
> > Hello.
> >
> > On 04/02/2015 04:56 PM, Ben Hutchings wrote:
> >
> >> This driver looks somewhat similar to sh-eth, but lacks some of the
> >> recent bug fixes made to that. At least commit 283e38db65e7 ("sh_eth:
> >> Fix serialisation of interrupt disable with interrupt & NAPI handler")
> >> appears to be applicable, but there are probably others.
> >
> > I suspect this issue applies to many drivers...
> > I couldn't reproduce the bug that patch was fixing, so left this fix
> > out
> > for the time being. Others cases were fixed (if applicable).
>
> Maybe its just harder to trigger but it indeed looks similar to what Ben
> has fixed for sh-eth. I wonder if that shutdown flag in the fix is
> really needed though. IMHO it should be save if we simply call
> napi_disable first, then disable irqs on hardware and finally
> synchronize_irq...
In sh_eth: if we call napi_disable() first, EESR_RX_CHECK can still be
set and nothing will clear it. If only one CPU is online this can hard
hang the system. Please trust that I did consider and rule out the
simpler approaches first.
I don't have the full documentation for the EtherAVB block, so I don't
know how its programming model differs from the other Ethernet DMA
engines. It's possible that a simpler approach will work in ravb.
Ben.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH resend] Renesas Ethernet AVB driver
This driver looks somewhat similar to sh-eth, but lacks some of the
recent bug fixes made to that. At least commit 283e38db65e7 ("sh_eth:
Fix serialisation of interrupt disable with interrupt & NAPI handler")
appears to be applicable, but there are probably others.
One feature request:
On Sat, 2015-03-28 at 02:13 +0300, Sergei Shtylyov wrote:
[...]
> +/* ioctl to device function */
> +static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
> +{
> + struct ravb_private *priv = netdev_priv(ndev);
> + struct phy_device *phydev = priv->phydev;
> +
> + if (!netif_running(ndev))
> + return -EINVAL;
> +
> + if (!phydev)
> + return -ENODEV;
> +
> + if (cmd == SIOCSHWTSTAMP)
> + return ravb_hwtstamp_ioctl(ndev, req, cmd);
[...]
The driver should also handle SIOCGHWTSTAMP.
Ben.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linux-kernel] [PATCH 2/9] ARM: shmobile: r8a7790: add dma defines for sys and audio dmacs
On Mon, 2014-04-07 at 21:07 +0100, Ben Dooks wrote: > Add the DMA resource IDs for the R8A7790 Audio and SYS DMA controllers > for use when specifying DMA handles. > > Signed-off-by: Ben Dooks > --- > include/dt-bindings/dma/r8a7790-dma.h | 223 > ++ > 1 file changed, 223 insertions(+) > create mode 100644 include/dt-bindings/dma/r8a7790-dma.h > > diff --git a/include/dt-bindings/dma/r8a7790-dma.h > b/include/dt-bindings/dma/r8a7790-dma.h > new file mode 100644 > index 000..7c52132 > --- /dev/null > +++ b/include/dt-bindings/dma/r8a7790-dma.h > @@ -0,0 +1,223 @@ > +/* > + * R8A7790 System and Audio DMA channel resource identifiers > + * > + * Copyirght (c) 2014 Codethink Ltd. [...] Typo: 'Copyirght' Ben. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V12 6/7] net: sxgbe: add ethtool related functions support
; + case AH_V6_FLOW: > > > + case ESP_V6_FLOW: > > > + case SCTP_V6_FLOW: > > > + case IPV4_FLOW: > > > + case IPV6_FLOW: > > > + if (!(cmd->data & RXH_IP_SRC) || > > > + !(cmd->data & RXH_IP_DST) || > > > + (cmd->data & RXH_L4_B_0_1) || > > > + (cmd->data & RXH_L4_B_2_3)) > > > + return -EINVAL; > > > + reg_val = SXGBE_CORE_RSS_CTL_IP2TE; > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > > Unless I'm missing something, this only accepts the default values, > so > > it's not actually possible to change the configuration. Why are you > > implementing this operation at all? > It is possible to change the configuration. There is a register write > to the > SXGBE_CORE_RSS_CTL_REG at the end of the function. I think you might > have missed > it. [...] I see, but I'm not sure I understand. Does this mean that only one hash function can be implemented at a time, i.e. do you have to choose: (IP packets hashed by address OR TCP packets hashed by address+port OR UDP packets hashed by address+port) AND all other packets go to single queue ? Because that's not what this operation is supposed to do at all. It's supposed to select *independently* for each flow type, how flows of that type are hashed. Most multiqueue controllers can do RX flow hashing like this: (TCP packets hashed by port+address OR by address) AND (UDP packets hashed by port+address OR by address) AND all other IP packets hashed by address AND all non-IP packets go to single queue Then this operation would let you select between the alternative hash functions for TCP and UDP, while not affecting other flow types. (Some hardware is more flexible, hence the large number of flow types defined.) Ben. -- Ben Hutchings Always try to do things in chronological order; it's less confusing that way. signature.asc Description: This is a digitally signed message part
Re: [PATCH V12 6/7] net: sxgbe: add ethtool related functions support Samsung sxgbe
case SCTP_V4_FLOW:
> + case AH_ESP_V4_FLOW:
> + case AH_V4_FLOW:
> + case ESP_V4_FLOW:
> + case AH_ESP_V6_FLOW:
> + case AH_V6_FLOW:
> + case ESP_V6_FLOW:
> + case SCTP_V6_FLOW:
> + case IPV4_FLOW:
> + case IPV6_FLOW:
> + if (!(cmd->data & RXH_IP_SRC) ||
> + !(cmd->data & RXH_IP_DST) ||
> + (cmd->data & RXH_L4_B_0_1) ||
> + (cmd->data & RXH_L4_B_2_3))
> + return -EINVAL;
> + reg_val = SXGBE_CORE_RSS_CTL_IP2TE;
> + break;
> + default:
> + return -EINVAL;
> + }
Unless I'm missing something, this only accepts the default values, so
it's not actually possible to change the configuration. Why are you
implementing this operation at all?
[...]
> +static void sxgbe_get_regs(struct net_device *dev,
> +struct ethtool_regs *regs, void *space)
> +{
> + struct sxgbe_priv_data *priv = netdev_priv(dev);
> + u32 *reg_space = (u32 *)space;
> + int reg_offset;
> + int reg_ix = 0;
> + void __iomem *ioaddr = priv->ioaddr;
> +
> + memset(reg_space, 0x0, REG_SPACE_SIZE);
> +
> + /* MAC registers */
> + for (reg_offset = START_MAC_REG_OFFSET;
> + reg_offset <= MAX_MAC_REG_OFFSET; reg_offset += 4) {
> + reg_space[reg_ix] = readl(ioaddr + reg_offset);
> + reg_ix++;
> + }
> +
> + /* MTL registers */
> + for (reg_offset = START_MTL_REG_OFFSET;
> + reg_offset <= MAX_MTL_REG_OFFSET; reg_offset += 4) {
> + reg_space[reg_ix] = readl(ioaddr + reg_offset);
> + reg_ix++;
> + }
> +
> + /* DMA registers */
> + for (reg_offset = START_DMA_REG_OFFSET;
> + reg_offset <= MAX_DMA_REG_OFFSET; reg_offset += 4) {
> + reg_space[reg_ix] = readl(ioaddr + reg_offset);
> + reg_ix++;
> + }
[...]
Consider adding an assertion at the end:
BUG_ON(reg_ix * 4 > REG_SPACE_SIZE);
Ben.
--
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
- Albert Einstein
signature.asc
Description: This is a digitally signed message part
Re: [PATCH net-next v3 5/9] Altera TSE: Add Miscellaneous Files for Altera Ethernet Driver
On Wed, 2014-03-12 at 15:26 -0700, Joe Perches wrote: > On Wed, 2014-03-12 at 21:14 +0000, Ben Hutchings wrote: > > These look like the statistic names specified in IEEE 802.3. I would > > support a general move to using standard names for MAC stats in Ethernet > > drivers, because they are quite clearly defined and widely implemented > > in hardware. However, that is not the current practice in most Linux > > drivers. > > Hey Ben. > > Maybe something like this would be a start? > > Change the actual #defines/strings to taste. > > Maybe this shouldn't be in uapi or maybe in if_ether.h [...] This is pointless without also nailing down the definition of each statistic. That could be done with reference to standards/RFCs, of course. Ben. -- Ben Hutchings Experience is directly proportional to the value of equipment destroyed. - Carolyn Scheppner signature.asc Description: This is a digitally signed message part
Re: [PATCH net-next v3 5/9] Altera TSE: Add Miscellaneous Files for Altera Ethernet Driver
On Tue, 2014-03-11 at 19:28 -0500, Vince Bridgers wrote:
> Hi Joe,
>
> On Tue, Mar 11, 2014 at 5:59 PM, Joe Perches wrote:
> > On Tue, 2014-03-11 at 17:43 -0500, Vince Bridgers wrote:
> >> This patch adds miscellaneous files for the Altera Ethernet Driver,
> >> including ethtool support.
> >
> > trivial notes:
> >
> >> diff --git a/drivers/net/ethernet/altera/altera_tse_ethtool.c
> >> b/drivers/net/ethernet/altera/altera_tse_ethtool.c
> > []
> >> +static char const stat_gstrings[][ETH_GSTRING_LEN] = {
> >
> > static const char
> >
> >> + "aFramesTransmittedOK",
> >
> > Why the prefix with a?
>
> These names follow the names of the statistics found in the databook
> for the Altera Triple Speed Ethernet (TSE) soft IP. I'm not
> emotionally attached to these names, so will review the statistics
> names and follow common usage instead. Thank you for noticing and
> commenting.
[...]
These look like the statistic names specified in IEEE 802.3. I would
support a general move to using standard names for MAC stats in Ethernet
drivers, because they are quite clearly defined and widely implemented
in hardware. However, that is not the current practice in most Linux
drivers.
Ben.
--
Ben Hutchings
Any sufficiently advanced bug is indistinguishable from a feature.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH RFC 3/3] Altera TSE: Add Altera Triple Speed Ethernet (TSE) Driver
t; + if (len < 4)
> + return -EINVAL;
> +
> + if (!strncmp((char *)prop, "mii", 3)) {
> + *iface = PHY_INTERFACE_MODE_MII;
> + return 0;
> + } else if (!strncmp((char *)prop, "gmii", 4)) {
> + *iface = PHY_INTERFACE_MODE_GMII;
> + return 0;
> + } else if (!strncmp((char *)prop, "rgmii-id", 8)) {
> + *iface = PHY_INTERFACE_MODE_RGMII_ID;
> + return 0;
> + } else if (!strncmp((char *)prop, "rgmii", 5)) {
> + *iface = PHY_INTERFACE_MODE_RGMII;
> + return 0;
> + } else if (!strncmp((char *)prop, "sgmii", 5)) {
> + *iface = PHY_INTERFACE_MODE_SGMII;
> + return 0;
> + }
Is there really no common function for this? Maybe you should add one.
> + return -EINVAL;
> +}
> +
> +static int request_and_map(struct platform_device *pdev, const char *name,
> +struct resource **res, void __iomem **ptr)
The fact that you return the MMIO pointer by reference is problematic,
because the callers actually want to initialise pointers to specific
structure types. They have to cast, and in fact they do the cast
wrongly as they don't include the __iomem qualifier.
Perhaps you could change this function to return the MMIO pointer
directly and use ERR_PTR for errors?
[...]
> + ndev->mem_start = control_port->start;
> + ndev->mem_end = control_port->end;
Not necessary, these fields are for ancient ISA crap where the user
might have to tell the driver which addresses to use.
[...]
> + altera_tse_netdev_ops.ndo_set_rx_mode = tse_set_rx_mode;
> +
> + if (priv->hash_filter)
> + altera_tse_netdev_ops.ndo_set_rx_mode =
> + tse_set_rx_mode_hashfilter;
Now imagine someone uses two instances of this IP block with different
capabilities.
[...]
> +/* Remove Altera TSE MAC device
> + */
> +static int altera_tse_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> +
> + platform_set_drvdata(pdev, NULL);
Redundant, driver core does this.
> + if (ndev) {
> + altera_tse_mdio_destroy(ndev);
I think this needs to be done after unregister_netdev(), but I'm not
sure.
> + netif_carrier_off(ndev);
Redundant.
> + unregister_netdev(ndev);
> + free_netdev(ndev);
> + }
> +
> + return 0;
> +}
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/altera/altera_tse_ethtool.c
[...]
> +#define TSE_STATS_LEN 31
> +
> +static char stat_gstrings[][ETH_GSTRING_LEN] = {
Please either use TSE_STATS_LEN as the outer dimension, or define
TSE_STATS_LEN using ARRAY_SIZE().
[...]
> +static void tse_get_drvinfo(struct net_device *dev,
> + struct ethtool_drvinfo *info)
> +{
> + struct altera_tse_private *priv = netdev_priv(dev);
> + u32 rev = ioread32(&priv->mac_dev->megacore_revision);
> +
> + strcpy(info->driver, "Altera TSE MAC IP Driver");
This should match the driver name elsewhere, i.e.
ALTERA_TSE_RESOURCE_NAME.
> + strcpy(info->version, "v8.0");
> + snprintf(info->fw_version, ETHTOOL_FWVERS_LEN, "v%d.%d",
> + rev & 0x, (rev & 0x) >> 16);
That appears to be a hardware version, not a firmware version.
> + sprintf(info->bus_info, "AVALON");
I don't think that's helpful. Either put some sort of address here or
leave it blank.
> +}
[...]
> +static int tse_sset_count(struct net_device *dev, int sset)
> +{
> + switch (sset) {
> + case ETH_SS_STATS:
> + return TSE_STATS_LEN;
> + default:
> + return -EOPNOTSUPP;
EINVAL
> + }
> +}
> +
[...]
> +static int tse_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> + struct altera_tse_private *priv = netdev_priv(dev);
> + struct phy_device *phydev = priv->phydev;
> +
> + if (phydev == NULL)
> + return -ENODEV;
EOPNOTSUPP
> + return phy_ethtool_gset(phydev, cmd);
> +}
> +
> +static int tse_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> + struct altera_tse_private *priv = netdev_priv(dev);
> + struct phy_device *phydev = priv->phydev;
> +
> + if (phydev == NULL)
> + return -ENODEV;
EOPNOTSUPP
> + return phy_ethtool_sset(phydev, cmd);
> +}
[...]
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree
On Tue, 2014-02-04 at 12:39 -0800, Florian Fainelli wrote: > 2014-01-17 Matthew Garrett : > > Some hardware may be broken in interesting and board-specific ways, such > > that various bits of functionality don't work. This patch provides a > > mechanism for overriding mii registers during init based on the contents of > > the device tree data, allowing board-specific fixups without having to > > pollute generic code. > > It would be good to explain exactly how your hardware is broken > exactly. I really do not think that such a fine-grained setting where > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to > remain usable makes that much sense. In general, Gigabit might be > badly broken, but 100 and 10Mbits/sec should work fine. How about the > MASTER-SLAVE bit, is overriding it really required? Yes, it is entirely possible that one or other of the clock modes (locally generated vs recovered) is not reliable. > Is not a PHY fixup registered for a specific OUI the solution you are > looking for? [...] The fault is in the board, not the PHY. Ben. -- Ben Hutchings One of the nice things about standards is that there are so many of them. signature.asc Description: This is a digitally signed message part
Re: [PATCH 3/3] net: via-rhine: add OF bus binding
On Tue, 2014-01-28 at 22:31 +0400, Alexey Charkov wrote: > 2014/1/28 Ben Hutchings : > > On Mon, 2014-01-27 at 19:34 +0400, Alexey Charkov wrote: > >> 2014/1/27 Ben Hutchings : > >> > On Mon, 2014-01-27 at 15:51 +0400, Alexey Charkov wrote: > >> >> This should make the driver usable with VIA/WonderMedia ARM-based > >> >> Systems-on-Chip integrated Rhine III adapters. Note that these > >> >> are always in MMIO mode, and don't have any known EEPROM. > >> > [...] > >> >> --- a/drivers/net/ethernet/via/Kconfig > >> >> +++ b/drivers/net/ethernet/via/Kconfig > >> >> @@ -19,7 +19,7 @@ if NET_VENDOR_VIA > >> >> > >> >> config VIA_RHINE > >> >> tristate "VIA Rhine support" > >> >> - depends on PCI > >> >> + depends on (PCI || USE_OF) > >> >> select CRC32 > >> >> select MII > >> >> ---help--- > >> > > >> > This seems like the right thing to do, but it means you need to add > >> > #ifdef CONFIG_PCI and #ifdef CONFIG_USE_OF around the driver structures > >> > and related functions. > >> > >> Frankly, I would like to avoid that if possible (as pointed out in the > >> cover email), as I believe we would get a cleaner driver without > >> #ifdef. This is also the way it was done in via-velocity, and it works > >> just fine. > > > > OK, I'm surprised that all the PCI functions have dummy definitions. > > > >> > You should compile-test in configurations that have just one of those > >> > dependencies enabled. > >> > >> This has been compile-tested and runtime-tested in OF-only > >> configuration on WM8950, and Roger also tested it in PCI-only > >> configuration, so it seems to work fine. > > [...] > > > > Good, then I have no objection. > > Thanks Ben! Would it be fine to add your Reviewed-by at the next > iteration, once I fix indentation of function arguments and > dev_is_pci()? Sorry, I don't think I know enough to claim that I've reviewed the whole thing properly. Ben. -- Ben Hutchings It is easier to write an incorrect program than to understand a correct one. signature.asc Description: This is a digitally signed message part
Re: [PATCH 3/3] net: via-rhine: add OF bus binding
On Mon, 2014-01-27 at 19:34 +0400, Alexey Charkov wrote: > 2014/1/27 Ben Hutchings : > > On Mon, 2014-01-27 at 15:51 +0400, Alexey Charkov wrote: > >> This should make the driver usable with VIA/WonderMedia ARM-based > >> Systems-on-Chip integrated Rhine III adapters. Note that these > >> are always in MMIO mode, and don't have any known EEPROM. > > [...] > >> --- a/drivers/net/ethernet/via/Kconfig > >> +++ b/drivers/net/ethernet/via/Kconfig > >> @@ -19,7 +19,7 @@ if NET_VENDOR_VIA > >> > >> config VIA_RHINE > >> tristate "VIA Rhine support" > >> - depends on PCI > >> + depends on (PCI || USE_OF) > >> select CRC32 > >> select MII > >> ---help--- > > > > This seems like the right thing to do, but it means you need to add > > #ifdef CONFIG_PCI and #ifdef CONFIG_USE_OF around the driver structures > > and related functions. > > Frankly, I would like to avoid that if possible (as pointed out in the > cover email), as I believe we would get a cleaner driver without > #ifdef. This is also the way it was done in via-velocity, and it works > just fine. OK, I'm surprised that all the PCI functions have dummy definitions. > > You should compile-test in configurations that have just one of those > > dependencies enabled. > > This has been compile-tested and runtime-tested in OF-only > configuration on WM8950, and Roger also tested it in PCI-only > configuration, so it seems to work fine. [...] Good, then I have no objection. Ben. -- Ben Hutchings If at first you don't succeed, you're doing about average. signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/3] net: via-rhine: switch to generic DMA functions
On Mon, 2014-01-27 at 19:26 +0400, Alexey Charkov wrote: > 2014/1/27 Ben Hutchings : > > On Mon, 2014-01-27 at 15:51 +0400, Alexey Charkov wrote: [...] > >> @@ -1094,20 +1094,22 @@ static int alloc_ring(struct net_device* dev) > >> void *ring; > >> dma_addr_t ring_dma; > >> > >> - ring = pci_alloc_consistent(rp->pdev, > >> + ring = dma_alloc_coherent(&rp->pdev->dev, > >> RX_RING_SIZE * sizeof(struct rx_desc) + > >> TX_RING_SIZE * sizeof(struct tx_desc), > >> - &ring_dma); > >> + &ring_dma, > >> + GFP_ATOMIC); > > [...] > > > > Indentation is messed up here (and in several other function calls > > you're changing). You should align the function arguments so each line > > begins in the column after the opening parenthesis. > > Ben, thanks for pointing out. I actually just tried to follow the > style of surrounding code, but happy to adjust if that's the preferred > option. From what I can see, these lines should still fit in below 80 > cols even with increased indents... > > Should we then also adjust other function calls within the driver with > similar indentation (if any), that are currently not touched by this > patch series? There is no need to do that at the same time, but it would be a nice bit of cleanup. Ben. -- Ben Hutchings If at first you don't succeed, you're doing about average. signature.asc Description: This is a digitally signed message part
Re: [PATCH 3/3] net: via-rhine: add OF bus binding
On Mon, 2014-01-27 at 15:51 +0400, Alexey Charkov wrote: > This should make the driver usable with VIA/WonderMedia ARM-based > Systems-on-Chip integrated Rhine III adapters. Note that these > are always in MMIO mode, and don't have any known EEPROM. [...] > --- a/drivers/net/ethernet/via/Kconfig > +++ b/drivers/net/ethernet/via/Kconfig > @@ -19,7 +19,7 @@ if NET_VENDOR_VIA > > config VIA_RHINE > tristate "VIA Rhine support" > - depends on PCI > + depends on (PCI || USE_OF) > select CRC32 > select MII > ---help--- This seems like the right thing to do, but it means you need to add #ifdef CONFIG_PCI and #ifdef CONFIG_USE_OF around the driver structures and related functions. You should compile-test in configurations that have just one of those dependencies enabled. [...] > --- a/drivers/net/ethernet/via/via-rhine.c > +++ b/drivers/net/ethernet/via/via-rhine.c [...] > @@ -847,7 +856,8 @@ static void rhine_hw_init(struct net_device *dev, long > pioaddr) > msleep(5); > > /* Reload EEPROM controlled bytes cleared by soft reset */ > - rhine_reload_eeprom(pioaddr, dev); > + if (!strncmp(dev->dev.parent->bus->name, "pci", 3)) > + rhine_reload_eeprom(pioaddr, dev); [...] Ew. I think you should use dev_is_pci(), although you might also need to guard that with #ifdef CONFIG_PCI. Ben. -- Ben Hutchings If at first you don't succeed, you're doing about average. signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/3] net: via-rhine: switch to generic DMA functions
On Mon, 2014-01-27 at 15:51 +0400, Alexey Charkov wrote:
> Remove legacy PCI DMA wrappers and instead use generic DMA functions
> directly in preparation for OF bus binding
>
> Signed-off-by: Alexey Charkov
> Signed-off-by: Roger Luethi
> ---
> drivers/net/ethernet/via/via-rhine.c | 56
> +++-
> 1 file changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/ethernet/via/via-rhine.c
> b/drivers/net/ethernet/via/via-rhine.c
> index ef312bc..fee8732 100644
> --- a/drivers/net/ethernet/via/via-rhine.c
> +++ b/drivers/net/ethernet/via/via-rhine.c
> @@ -919,10 +919,10 @@ static int rhine_init_one(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> goto err_out;
>
> /* this should always be supported */
> - rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> + rc = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> if (rc) {
> dev_err(&pdev->dev,
> - "32-bit PCI DMA addresses not supported by the
> card!?\n");
> + "32-bit DMA addresses not supported by the card!?\n");
> goto err_out;
> }
>
> @@ -1094,20 +1094,22 @@ static int alloc_ring(struct net_device* dev)
> void *ring;
> dma_addr_t ring_dma;
>
> - ring = pci_alloc_consistent(rp->pdev,
> + ring = dma_alloc_coherent(&rp->pdev->dev,
> RX_RING_SIZE * sizeof(struct rx_desc) +
> TX_RING_SIZE * sizeof(struct tx_desc),
> - &ring_dma);
> + &ring_dma,
> + GFP_ATOMIC);
[...]
Indentation is messed up here (and in several other function calls
you're changing). You should align the function arguments so each line
begins in the column after the opening parenthesis.
Ben.
--
Ben Hutchings
If at first you don't succeed, you're doing about average.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/3] net: ethoc: don't advertise gigabit speed on attached PHY
On Mon, 2014-01-27 at 07:59 +0400, Max Filippov wrote: > OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does > not disable advertisement when PHY supports them. This results in > non-functioning network when the MAC is connected to a gigabit PHY connected > to a gigabit switch. > > The fix is to disable gigabit speed advertisement on attached PHY > unconditionally. > > Signed-off-by: Max Filippov > --- > drivers/net/ethernet/ethoc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c > index 4de8cfd..0aa1a05 100644 > --- a/drivers/net/ethernet/ethoc.c > +++ b/drivers/net/ethernet/ethoc.c > @@ -712,6 +712,8 @@ static int ethoc_open(struct net_device *dev) > netif_start_queue(dev); > } > > + priv->phy->advertising &= ~(ADVERTISED_1000baseT_Full | > + ADVERTISED_1000baseT_Half); > phy_start(priv->phy); > napi_enable(&priv->napi); > This is not sufficient to disable gigabit speeds; the supported mask should also be limited. And it should be done even before the net device is registered. Rather than poking into the phy_device structure directly from this driver, I think you should add a function in phylib for this purpose. Ben. -- Ben Hutchings If at first you don't succeed, you're doing about average. signature.asc Description: This is a digitally signed message part
Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree
On Fri, 2014-01-17 at 17:57 -0500, Matthew Garrett wrote: > Some hardware may be broken in interesting and board-specific ways, such > that various bits of functionality don't work. This patch provides a > mechanism for overriding mii registers during init based on the contents of > the device tree data, allowing board-specific fixups without having to > pollute generic code. [...] > --- a/Documentation/devicetree/bindings/net/phy.txt > +++ b/Documentation/devicetree/bindings/net/phy.txt > @@ -23,6 +23,21 @@ Optional Properties: >assume clause 22. The compatible list may also contain other >elements. > > +The following properties may be added to either the phy node or the parent > +ethernet device. If not present, the hardware defaults will be used. > + > +- phy-mii-advertise-10half: 1 to advertise half-duplex 10MBit, 0 to disable > +- phy-mii-advertise-10full: 1 to advertise full-duplex 10MBit, 0 to disable > +- phy-mii-advertise-100half: 1 to advertise half-duplex 100MBit, 0 to disable > +- phy-mii-advertise-100full: 1 to advertise full-duplex 100MBit, 0 to disable > +- phy-mii-advertise-100base4: 1 to advertise 100base4, 0 to disable > +- phy-mii-advertise-1000half: 1 to advertise half-duplex 1000MBit, 0 to > disable > +- phy-mii-advertise-1000full: 1 to advertise full-duplex 1000MBit, 0 to > disable Are these really all needed? Apparently there is a standard 'max-speed' property on Ethernet devices already, which I think is probably sufficient to express the actual restrictions of some boards. > +- phy-mii-manual-master: 1 to enable manual master/slave configuration, 0 > + to disable manual master/slave configuration [...] Although the standard calls this 'manual', if it's set in the DT it won't really be a manual setting. The description should probably clarify that. I think the name should include 'master-slave' or 'clock-role' rather than just 'master', as currently it suggests only the master role can be forced. Ben. -- Ben Hutchings friends: People who know you well, but like you anyway. signature.asc Description: This is a digitally signed message part
Re: [PATCH net-next v4 1/2] dts: Add a binding for Synopsys emac max-frame-size
On Thu, 2014-01-16 at 13:29 -0600, Rob Herring wrote: > On Thu, Jan 16, 2014 at 12:48 PM, Vince Bridgers > wrote: > > On Thu, Jan 16, 2014 at 11:50 AM, Ben Hutchings > > wrote: > >> On Thu, 2014-01-16 at 08:05 -0600, Vince Bridgers wrote: > >>> This change adds a parameter for the Synopsys 10/100/1000 > >>> stmmac Ethernet driver to configure the maximum frame > >>> size supported by the EMAC driver. Synopsys allows the FIFO > >>> sizes to be configured when the cores are built for a particular > >>> device, but do not provide a way for the driver to read > >>> information from the device about the maximum MTU size > >>> supported as limited by the device's FIFO size. > >>> > >>> Signed-off-by: Vince Bridgers > >>> --- > >>> V4: add comments to explain use of max-frame-size with respect > >>> to inconsistent definition and use in the ePAPR v1.1 spec > >> > >> Well, ePAPR does not seem to be consistent with itself. :-) > >> > >>> V3: change snps,max-frame-size to max-frame-size > >>> V2: change snps,max-mtu to snps,max-frame-size > >>> --- > >>> Documentation/devicetree/bindings/net/stmmac.txt | 13 - > >>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt > >>> b/Documentation/devicetree/bindings/net/stmmac.txt > >>> index eba0e5e..d553be2 100644 > >>> --- a/Documentation/devicetree/bindings/net/stmmac.txt > >>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt > >>> @@ -29,7 +29,17 @@ Required properties: > >>> ignored if force_thresh_dma_mode is set. > >>> > >>> Optional properties: > >>> -- mac-address: 6 bytes, mac address > >>> +- mac-address: 6 bytes, mac address > >>> +- max-frame-size:Maximum frame size permitted. This parameter is > >>> useful > >>> + since different implementations of the Synopsys MAC > >>> may > >>> + have different FIFO sizes depending on the > >>> selections > >>> + made in Synopsys Core Consultant. Note that the > >>> usage > >>> + is inconsistent with the definition in the ePAPR > >>> v1.1 > >>> + specification, as it defines max-frame-size > >>> inclusive > >>> + of the MAC DA, SA, Ethertype and CRC while usage is > >>> + consistent with the IEEE definition of MAC Client > >>> + Data, which is sans the MAC DA, SA, Ethertype and > >>> + CRC. > >> [...] > >> > >> While this is very precise, I fear that it is now so verbose that it > >> actually becomes confusing. Can this not be condensed to 'the maximum > >> MTU and MRU, rather than the maximum Ethernet frame size'? > >> > >> Ben. > > > > Sure, I'll cut this down and resubmit as V5. How about > > > > "The Maximum Transfer Unit (IEEE defined MTU), rather than the maximum > > frame size." > > If you are not following the ePAPR definition, then call the property > something else rather than relying on the documentation to explain the > different meaning. Why can't the driver adjust the size from the ePAPR > definition? Otherwise, I would go back to the v2 name of > "snps,max-mtu". ePAPR *doesn't have* a clear definition: http://article.gmane.org/gmane.linux.network/300098 Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v4 1/2] dts: Add a binding for Synopsys emac max-frame-size
On Thu, 2014-01-16 at 12:48 -0600, Vince Bridgers wrote: > On Thu, Jan 16, 2014 at 11:50 AM, Ben Hutchings > wrote: > > On Thu, 2014-01-16 at 08:05 -0600, Vince Bridgers wrote: > >> This change adds a parameter for the Synopsys 10/100/1000 > >> stmmac Ethernet driver to configure the maximum frame > >> size supported by the EMAC driver. Synopsys allows the FIFO > >> sizes to be configured when the cores are built for a particular > >> device, but do not provide a way for the driver to read > >> information from the device about the maximum MTU size > >> supported as limited by the device's FIFO size. > >> > >> Signed-off-by: Vince Bridgers > >> --- > >> V4: add comments to explain use of max-frame-size with respect > >> to inconsistent definition and use in the ePAPR v1.1 spec > > > > Well, ePAPR does not seem to be consistent with itself. :-) > > > >> V3: change snps,max-frame-size to max-frame-size > >> V2: change snps,max-mtu to snps,max-frame-size > >> --- > >> Documentation/devicetree/bindings/net/stmmac.txt | 13 - > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt > >> b/Documentation/devicetree/bindings/net/stmmac.txt > >> index eba0e5e..d553be2 100644 > >> --- a/Documentation/devicetree/bindings/net/stmmac.txt > >> +++ b/Documentation/devicetree/bindings/net/stmmac.txt > >> @@ -29,7 +29,17 @@ Required properties: > >> ignored if force_thresh_dma_mode is set. > >> > >> Optional properties: > >> -- mac-address: 6 bytes, mac address > >> +- mac-address: 6 bytes, mac address > >> +- max-frame-size:Maximum frame size permitted. This parameter is > >> useful > >> + since different implementations of the Synopsys MAC > >> may > >> + have different FIFO sizes depending on the selections > >> + made in Synopsys Core Consultant. Note that the usage > >> + is inconsistent with the definition in the ePAPR v1.1 > >> + specification, as it defines max-frame-size inclusive > >> + of the MAC DA, SA, Ethertype and CRC while usage is > >> + consistent with the IEEE definition of MAC Client > >> + Data, which is sans the MAC DA, SA, Ethertype and > >> + CRC. > > [...] > > > > While this is very precise, I fear that it is now so verbose that it > > actually becomes confusing. Can this not be condensed to 'the maximum > > MTU and MRU, rather than the maximum Ethernet frame size'? > > > > Ben. > > Sure, I'll cut this down and resubmit as V5. How about > > "The Maximum Transfer Unit (IEEE defined MTU), rather than the maximum > frame size." Sounds good. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v4 1/2] dts: Add a binding for Synopsys emac max-frame-size
On Thu, 2014-01-16 at 08:05 -0600, Vince Bridgers wrote: > This change adds a parameter for the Synopsys 10/100/1000 > stmmac Ethernet driver to configure the maximum frame > size supported by the EMAC driver. Synopsys allows the FIFO > sizes to be configured when the cores are built for a particular > device, but do not provide a way for the driver to read > information from the device about the maximum MTU size > supported as limited by the device's FIFO size. > > Signed-off-by: Vince Bridgers > --- > V4: add comments to explain use of max-frame-size with respect > to inconsistent definition and use in the ePAPR v1.1 spec Well, ePAPR does not seem to be consistent with itself. :-) > V3: change snps,max-frame-size to max-frame-size > V2: change snps,max-mtu to snps,max-frame-size > --- > Documentation/devicetree/bindings/net/stmmac.txt | 13 - > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/stmmac.txt > b/Documentation/devicetree/bindings/net/stmmac.txt > index eba0e5e..d553be2 100644 > --- a/Documentation/devicetree/bindings/net/stmmac.txt > +++ b/Documentation/devicetree/bindings/net/stmmac.txt > @@ -29,7 +29,17 @@ Required properties: > ignored if force_thresh_dma_mode is set. > > Optional properties: > -- mac-address: 6 bytes, mac address > +- mac-address: 6 bytes, mac address > +- max-frame-size:Maximum frame size permitted. This parameter is useful > + since different implementations of the Synopsys MAC may > + have different FIFO sizes depending on the selections > + made in Synopsys Core Consultant. Note that the usage > + is inconsistent with the definition in the ePAPR v1.1 > + specification, as it defines max-frame-size inclusive > + of the MAC DA, SA, Ethertype and CRC while usage is > + consistent with the IEEE definition of MAC Client > + Data, which is sans the MAC DA, SA, Ethertype and > + CRC. [...] While this is very precise, I fear that it is now so verbose that it actually becomes confusing. Can this not be condensed to 'the maximum MTU and MRU, rather than the maximum Ethernet frame size'? Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 2/2] stmmac: Fix kernel crashes for jumbo frames
On Tue, 2014-01-14 at 14:44 -0600, Vince Bridgers wrote:
> On Tue, Jan 14, 2014 at 1:53 PM, Ben Hutchings
[...]
> >>
> >> /*
> >>* Currently only the properties needed on SPEAr600
> >> @@ -60,6 +64,7 @@ static int stmmac_probe_config_dt(struct platform_device
> >> *pdev,
> >> if (of_device_is_compatible(np, "st,spear600-gmac") ||
> >> of_device_is_compatible(np, "snps,dwmac-3.70a") ||
> >> of_device_is_compatible(np, "snps,dwmac")) {
> >> + of_property_read_u32(np, "max-frame-size", &plat->maxmtu);
> > [...]
> >
> > Is it the maximum frame size, including Ethernet header? (And then, is
> > the CRC or any VLAN header included in the frame size?)
> > Or is it the MTU, excluding all of those?
> > You really need to be very clear about what this number represents.
>
> max-frame-size as read from the device tree is defined in the ePAPR
> v1.1 specification. I originally used a name of "max-mtu", but was
> asked to change that since this attribute was already defined in the
> ePAPR v1.1 specification.
[...]
Oh dear, this specification is just as confused as I am. The example
value in §6.3.1.4 is 1518, implying that it is really the frame size,
including the Ethernet header, any VLAN tag, and maybe the CRC. However
the full example in Appendix B1 has 0x5dc (== 1500) implying that it is
the MTU.
I suppose you should follow whatever interpretation is already used by
other drivers. :-/
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 2/2] stmmac: Fix kernel crashes for jumbo frames
On Tue, 2014-01-14 at 11:17 -0600, Vince Bridgers wrote:
> These changes correct the following issues with jumbo frames on the
> stmmac driver:
>
> 1) The Synopsys EMAC can be configured to support different FIFO
> sizes at core configuration time. There's no way to query the
> controller and know the FIFO size, so the driver needs to get this
> information from the device tree in order to know how to correctly
> handle MTU changes and setting up dma buffers. The default
> max-frame-size is as currently used, which is the size of a jumbo
> frame.
>
> 2) The driver was enabling Jumbo frames by default, but was not allocating
> dma buffers of sufficient size to handle the maximum possible packet
> size that could be received. This led to memory corruption since DMAs were
> occurring beyond the extent of the allocated receive buffers for certain types
> of network traffic.
[...]
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -293,6 +293,8 @@ struct dma_features {
> #define STMMAC_CHAIN_MODE0x1
> #define STMMAC_RING_MODE 0x2
>
> +#define JUMBO_LEN9000
> +
> struct stmmac_desc_ops {
> /* DMA RX descriptor ring initialization */
> void (*init_rx_desc) (struct dma_desc *p, int disable_rx_ic, int mode,
[...]
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -51,6 +51,10 @@ static int stmmac_probe_config_dt(struct platform_device
> *pdev,
> plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> sizeof(struct stmmac_mdio_bus_data),
> GFP_KERNEL);
> + /* Set the maxmtu to a default of 1500 in case the
> + * parameter is not present in the device tree
> + */
> + plat->maxmtu = JUMBO_LEN;
The comment disagrees with the definition of JUMBO_LEN.
>
> /*
>* Currently only the properties needed on SPEAr600
> @@ -60,6 +64,7 @@ static int stmmac_probe_config_dt(struct platform_device
> *pdev,
> if (of_device_is_compatible(np, "st,spear600-gmac") ||
> of_device_is_compatible(np, "snps,dwmac-3.70a") ||
> of_device_is_compatible(np, "snps,dwmac")) {
> + of_property_read_u32(np, "max-frame-size", &plat->maxmtu);
[...]
Is it the maximum frame size, including Ethernet header? (And then, is
the CRC or any VLAN header included in the frame size?)
Or is it the MTU, excluding all of those?
You really need to be very clear about what this number represents.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] misc: xgene: Add support for APM X-Gene SoC Queue Manager/Traffic Manager
> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, > is for the sole use of the intended recipient(s) and contains information > that is confidential and proprietary to Applied Micro Circuits Corporation or > its subsidiaries. > It is to be used solely for the purpose of furthering the parties' business > relationship. > All unauthorized review, use, disclosure or distribution is prohibited. > If you are not the intended recipient, please contact the sender by reply > e-mail > and destroy all copies of the original message. Well this definitely has nothing to do with any business relationship that may exist between AMCC and Solarflare, so please don't send any more of these messages either directly or via netdev. David, you'd better let all the netdev archives know to delete these messages... ;-) Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Ksummit-2013-discuss] ARM topic: Is DT on ARM the solution, or is there something better?
On Thu, 2013-10-24 at 09:32 +0200, Richard Cochran wrote: > On Thu, Oct 24, 2013 at 12:29:23AM +0100, Ben Hutchings wrote: > > > > It starts by looking up the machine ID (in procfs, but may be overridden > > because some board vendors don't set it correctly). If the machine is > > supported using FDT then there is an associated DTB file which will have > > been installed with the kernel. The script copies/appends this to the > > appropriate place for the boot loader. > > And if the ID is not recognized, then what? Then we have no idea even where to install the kernel, or what wrapper might be needed, so the boot loader will load it. Lack of a DTB file is a small problem; it would be easy to add an option to provide one outside the kernel package. Ben. -- Ben Hutchings Teamwork is essential - it allows you to blame someone else. signature.asc Description: This is a digitally signed message part
Re: [Ksummit-2013-discuss] ARM topic: Is DT on ARM the solution, or is there something better?
On Wed, 2013-10-23 at 20:30 +0200, Richard Cochran wrote: > On Wed, Oct 23, 2013 at 12:25:02PM -0600, Jason Gunthorpe wrote: > > > What they want is one vmlinux/z, and a package of 'stuff' that they > > can assemble a boot image for all hardware out of. > > ... > > > On ARM the package of 'stuff' can very reasonably include dtb. Distro > > scripts can package modules+DTB+vmlinuz into something the bootloader > > can understand. (The next pain point will be to standardize that) > > > > The DTB doesn't have to be 'outside' the distro/kernel to give users a > > seamless upgrade experience. > > How can a distro possibly provide me a DTB? > > They don't know what hardware I am using. Only I know that. This is what Debian runs when installing a kernel on most ARM systems: http://sources.debian.net/src/flash-kernel/3.11/functions It starts by looking up the machine ID (in procfs, but may be overridden because some board vendors don't set it correctly). If the machine is supported using FDT then there is an associated DTB file which will have been installed with the kernel. The script copies/appends this to the appropriate place for the boot loader. Ben. -- Ben Hutchings Teamwork is essential - it allows you to blame someone else. signature.asc Description: This is a digitally signed message part
