RE: [EXT] Re: [PATCH V5 0/2] Change vring space from nomal memory to dma coherent memory
From: Greg KH Sent: Wednesday, October 28, 2020 3:07 PM > On Wed, Oct 28, 2020 at 06:05:28AM +, Sherry Sun wrote: > > Hi Greg, > > > > > Subject: Re: [PATCH V5 0/2] Change vring space from nomal memory to > > > dma coherent memory > > > > > > On Wed, Oct 28, 2020 at 10:03:03AM +0800, Sherry Sun wrote: > > > > Changes in V5: > > > > 1. Reorganize the vop_mmap function code in patch 1, which is done > > > > by > > > Christoph. > > > > 2. Completely remove the unnecessary code related to reassign the > > > > used ring for card in patch 2. > > > > > > > > The original vop driver only supports dma coherent device, as it > > > > allocates and maps vring by _get_free_pages and dma_map_single, > > > > but not use dma_sync_single_for_cpu/device to sync the updates of > > > > device_page/vring between EP and RC, which will cause memory > > > > synchronization problem for device don't support hardware dma coherent. > > > > > > > > And allocate vrings use dma_alloc_coherent is a common way in > > > > kernel, as the memory interacted between two systems should use > > > > consistent memory to avoid caching effects. So here add > > > > noncoherent platform > > > support for vop driver. > > > > Also add some related dma changes to make sure noncoherent > > > > platform works well. > > > > > > > > Sherry Sun (2): > > > > misc: vop: change the way of allocating vrings and device page > > > > misc: vop: do not allocate and reassign the used ring > > > > > > > > drivers/misc/mic/bus/vop_bus.h | 2 + > > > > drivers/misc/mic/host/mic_boot.c | 9 ++ > > > > drivers/misc/mic/host/mic_main.c | 43 ++-- > > > > drivers/misc/mic/vop/vop_debugfs.c | 4 - > > > > drivers/misc/mic/vop/vop_main.c| 70 +--- > > > > drivers/misc/mic/vop/vop_vringh.c | 166 ++--- > > > > include/uapi/linux/mic_common.h| 9 +- > > > > 7 files changed, 85 insertions(+), 218 deletions(-) > > > > > > Have you all seen: > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%25 > > > > 2Flore.kernel.org%2Fr%2F8c1443136563de34699d2c084df478181c205db4.16 > > > > 03854416.git.sudeep.dutt%40intel.com&data=04%7C01%7Csherry.sun% > > > > 40nxp.com%7Cc19c987667434969847e08d87b0685e8%7C686ea1d3bc2b4c6f > > > > a92cd99c5c301635%7C0%7C0%7C637394615238940323%7CUnknown%7CTW > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > VCI6Mn0%3D%7C1000&sdata=Zq%2FtHWTq%2BuIVBYXFGoeBmq0JJzYd > > > 9zDyv4NVN4TpC%2FU%3D&reserved=0 > > > > > > Looks like this code is asking to just be deleted, is that ok with you? > > > > Yes, I saw that patch. I'm ok with it. > > Great, can you please provide a "Reviewed-by:" or "Acked-by:" for it? > > thanks, > > greg k-h Sherry took much effort on the features support on i.MX series like i.MX8QM/i.MX8QXP/i.MX8MM. Now it is a pity to delete the vop code. One question, can we resubmit vop code by clean up, now only for i.MX series as Dutt's suggestion ? Or we have to drop the design and switch to select other solutions ? Thanks, Andy
RE: [EXT] Re: [PATCH V5 0/2] Change vring space from nomal memory to dma coherent memory
From: Greg KH Sent: Wednesday, October 28, 2020 7:14 PM > On Wed, Oct 28, 2020 at 10:17:39AM +0000, Andy Duan wrote: > > From: Greg KH Sent: Wednesday, October > > 28, 2020 3:07 PM > > > On Wed, Oct 28, 2020 at 06:05:28AM +, Sherry Sun wrote: > > > > Hi Greg, > > > > > > > > > Subject: Re: [PATCH V5 0/2] Change vring space from nomal memory > > > > > to dma coherent memory > > > > > > > > > > On Wed, Oct 28, 2020 at 10:03:03AM +0800, Sherry Sun wrote: > > > > > > Changes in V5: > > > > > > 1. Reorganize the vop_mmap function code in patch 1, which is > > > > > > done by > > > > > Christoph. > > > > > > 2. Completely remove the unnecessary code related to reassign > > > > > > the used ring for card in patch 2. > > > > > > > > > > > > The original vop driver only supports dma coherent device, as > > > > > > it allocates and maps vring by _get_free_pages and > > > > > > dma_map_single, but not use dma_sync_single_for_cpu/device to > > > > > > sync the updates of device_page/vring between EP and RC, which > > > > > > will cause memory synchronization problem for device don't support > hardware dma coherent. > > > > > > > > > > > > And allocate vrings use dma_alloc_coherent is a common way in > > > > > > kernel, as the memory interacted between two systems should > > > > > > use consistent memory to avoid caching effects. So here add > > > > > > noncoherent platform > > > > > support for vop driver. > > > > > > Also add some related dma changes to make sure noncoherent > > > > > > platform works well. > > > > > > > > > > > > Sherry Sun (2): > > > > > > misc: vop: change the way of allocating vrings and device page > > > > > > misc: vop: do not allocate and reassign the used ring > > > > > > > > > > > > drivers/misc/mic/bus/vop_bus.h | 2 + > > > > > > drivers/misc/mic/host/mic_boot.c | 9 ++ > > > > > > drivers/misc/mic/host/mic_main.c | 43 ++-- > > > > > > drivers/misc/mic/vop/vop_debugfs.c | 4 - > > > > > > drivers/misc/mic/vop/vop_main.c| 70 +--- > > > > > > drivers/misc/mic/vop/vop_vringh.c | 166 > > > > > > ++--- > > > > > > include/uapi/linux/mic_common.h| 9 +- > > > > > > 7 files changed, 85 insertions(+), 218 deletions(-) > > > > > > > > > > Have you all seen: > > > > > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F% > > > > > 25 > > > > > > > > > 2Flore.kernel.org%2Fr%2F8c1443136563de34699d2c084df478181c205db4.16 > > > > > > > > > 03854416.git.sudeep.dutt%40intel.com&data=04%7C01%7Csherry.sun% > > > > > > > > > 40nxp.com%7Cc19c987667434969847e08d87b0685e8%7C686ea1d3bc2b4c6f > > > > > > > > > a92cd99c5c301635%7C0%7C0%7C637394615238940323%7CUnknown%7CTW > > > > > > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > > > > > > VCI6Mn0%3D%7C1000&sdata=Zq%2FtHWTq%2BuIVBYXFGoeBmq0JJzYd > > > > > 9zDyv4NVN4TpC%2FU%3D&reserved=0 > > > > > > > > > > Looks like this code is asking to just be deleted, is that ok with > > > > > you? > > > > > > > > Yes, I saw that patch. I'm ok with it. > > > > > > Great, can you please provide a "Reviewed-by:" or "Acked-by:" for it? > > > > > > thanks, > > > > > > greg k-h > > > > Sherry took much effort on the features support on i.MX series like > i.MX8QM/i.MX8QXP/i.MX8MM. > > > > Now it is a pity to delete the vop code. > > > > One question, > > can we resubmit vop code by clean up, now only for i.MX series as Dutt's > suggestion ? > > Or we have to drop the design and switch to select other solutions ? > Okay, we plan to switch to NTB solution. > If this whole subsystem is being deleted because it is not used and never > shipped, > yes, please use a different solution. > > I don't understand why you were trying to piggy-back on this codebase if the > hardware was totally different, for some reason I thought this was the same > hardware. What exactly is this? Not the whole codebase, just the vop framework. > > thanks, > > greg k-h
RE: [PATCH v2 tty] tty: serial: fsl_lpuart: LS1021A has a FIFO size of 16 words, like LS1028A
From: Vladimir Oltean Sent: Friday, October 23, 2020
9:34 AM
> Prior to the commit that this one fixes, the FIFO size was derived from the
> read-only register LPUARTx_FIFO[TXFIFOSIZE] using the following
> formula:
>
> TX FIFO size = 2 ^ (LPUARTx_FIFO[TXFIFOSIZE] - 1)
>
> The documentation for LS1021A is a mess. Under chapter 26.1.3 LS1021A
> LPUART module special consideration, it mentions TXFIFO_SZ and RXFIFO_SZ
> being equal to 4, and in the register description for LPUARTx_FIFO, it shows
> the
> out-of-reset value of TXFIFOSIZE and RXFIFOSIZE fields as "011", even though
> these registers read as "101" in reality.
>
> And when LPUART on LS1021A was working, the "101" value did correspond to
> "16 datawords", by applying the formula above, even though the
> documentation is wrong again () and says that "101" means 64 datawords
> (hint: it doesn't).
>
> So the "new" formula created by commit f77ebb241ce0 has all the premises of
> being wrong for LS1021A, because it relied only on false data and no actual
> experimentation.
>
> Interestingly, in commit c2f448cff22a ("tty: serial: fsl_lpuart: add LS1028A
> support"), Michael Walle applied a workaround to this by manually setting the
> FIFO widths for LS1028A. It looks like the same values are used by LS1021A as
> well, in fact.
>
> When the driver thinks that it has a deeper FIFO than it really has, getty
> (user
> space) output gets truncated.
>
> Many thanks to Michael for pointing out where to look.
>
> Fixes: f77ebb241ce0 ("tty: serial: fsl_lpuart: correct the FIFO depth size")
> Suggested-by: Michael Walle
> Signed-off-by: Vladimir Oltean
> ---
> Changes in v2:
> Reworded commit message.
For the v2 with commit message change:
Reviewed-by：Fugang Duan
>
> drivers/tty/serial/fsl_lpuart.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c
> b/drivers/tty/serial/fsl_lpuart.c index
> ff4b88c637d0..bd047e1f9bea 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -314,9 +314,10 @@ MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
> /* Forward declare this for the dma callbacks*/ static void
> lpuart_dma_tx_complete(void *arg);
>
> -static inline bool is_ls1028a_lpuart(struct lpuart_port *sport)
> +static inline bool is_layerscape_lpuart(struct lpuart_port *sport)
> {
> - return sport->devtype == LS1028A_LPUART;
> + return (sport->devtype == LS1021A_LPUART ||
> + sport->devtype == LS1028A_LPUART);
> }
>
> static inline bool is_imx8qxp_lpuart(struct lpuart_port *sport) @@ -1701,11
> +1702,11 @@ static int lpuart32_startup(struct uart_port *port)
> UARTFIFO_FIFOSIZE_MASK);
>
> /*
> - * The LS1028A has a fixed length of 16 words. Although it supports the
> - * RX/TXSIZE fields their encoding is different. Eg the reference manual
> - * states 0b101 is 16 words.
> + * The LS1021A and LS1028A have a fixed FIFO depth of 16 words.
> + * Although they support the RX/TXSIZE fields, their encoding is
> + * different. Eg the reference manual states 0b101 is 16 words.
>*/
> - if (is_ls1028a_lpuart(sport)) {
> + if (is_layerscape_lpuart(sport)) {
> sport->rxfifo_size = 16;
> sport->txfifo_size = 16;
> sport->port.fifosize = sport->txfifo_size;
> --
> 2.25.1
RE: [EXT] [PATCH] tty: serial: fsl_lpuart: LS1021A has a FIFO size of 32 datawords
From: Vladimir Oltean Sent: Thursday, October 22, 2020
11:13 PM
> From: Vladimir Oltean
>
> Similar to the workaround applied by Michael Walle in commit c2f448cff22a
> ("tty: serial: fsl_lpuart: add LS1028A support"), it turns out that the
> LPUARTx_FIFO encoding for fields TXFIFOSIZE and RXFIFOSIZE is the same for
> LS1028A as for LS1021A.
>
> The RXFIFOSIZE in the Layerscape SoCs is fixed at this value:
> 101 Receive FIFO/Buffer depth = 32 datawords.
>
> When Andy Duan wrote the commit in Fixes: below, he assumed that the 101
> encoding means 64 datawords. But this is not true for Layerscape. So that
> commit broke LS1021A, and this patch is extending the workaround for LS1028A
> which appeared in the meantime, to fix that breakage.
>
> When the driver thinks that it has a deeper FIFO than it really has, getty
> (user
> space) output gets truncated.
>
> Many thanks to Michael for suggesting this!
>
> Fixes: f77ebb241ce0 ("tty: serial: fsl_lpuart: correct the FIFO depth size")
> Suggested-by: Michael Walle
> Signed-off-by: Vladimir Oltean
Layerscape has different define for the FIFO size.
Reviewed-by: Fugang Duan
> ---
> drivers/tty/serial/fsl_lpuart.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c
> b/drivers/tty/serial/fsl_lpuart.c index
> ff4b88c637d0..bd047e1f9bea 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -314,9 +314,10 @@ MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
> /* Forward declare this for the dma callbacks*/ static void
> lpuart_dma_tx_complete(void *arg);
>
> -static inline bool is_ls1028a_lpuart(struct lpuart_port *sport)
> +static inline bool is_layerscape_lpuart(struct lpuart_port *sport)
> {
> - return sport->devtype == LS1028A_LPUART;
> + return (sport->devtype == LS1021A_LPUART ||
> + sport->devtype == LS1028A_LPUART);
> }
>
> static inline bool is_imx8qxp_lpuart(struct lpuart_port *sport) @@ -1701,11
> +1702,11 @@ static int lpuart32_startup(struct uart_port *port)
>
> UARTFIFO_FIFOSIZE_MASK);
>
> /*
> -* The LS1028A has a fixed length of 16 words. Although it supports
> the
> -* RX/TXSIZE fields their encoding is different. Eg the reference
> manual
> -* states 0b101 is 16 words.
> +* The LS1021A and LS1028A have a fixed FIFO depth of 16 words.
> +* Although they support the RX/TXSIZE fields, their encoding is
> +* different. Eg the reference manual states 0b101 is 16 words.
> */
> - if (is_ls1028a_lpuart(sport)) {
> + if (is_layerscape_lpuart(sport)) {
> sport->rxfifo_size = 16;
> sport->txfifo_size = 16;
> sport->port.fifosize = sport->txfifo_size;
> --
> 2.25.1
RE: [PATCH] tty: serial: fsl_lpuart: fix lpuart32_poll_get_char
From: Peng Fan Sent: Tuesday, September 29, 2020 5:55 PM
> The watermark is set to 1, so we need to input two chars to trigger RDRF using
> the original logic. With the new logic, we could always get the char when
> there
> is data in FIFO.
>
> Suggested-by: Fugang Duan
> Signed-off-by: Peng Fan
> ---
> drivers/tty/serial/fsl_lpuart.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c
> b/drivers/tty/serial/fsl_lpuart.c index
> 645bbb24b433..1c37280b6c0c 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -680,7 +680,7 @@ static void lpuart32_poll_put_char(struct uart_port
> *port, unsigned char c)
>
> static int lpuart32_poll_get_char(struct uart_port *port) {
> - if (!(lpuart32_read(port, UARTSTAT) & UARTSTAT_RDRF))
> + if (!(lpuart32_read(port, UARTWATER) >> UARTWATER_RXCNT_OFF))
Please remove redundant blank space.
> return NO_POLL_CHAR;
>
> return lpuart32_read(port, UARTDATA);
> --
> 2.28.0
RE: [EXT] Re: [PATCH] net: fec: Keep device numbering consistent with datasheet
From: David Miller Sent: Thursday, September 24, 2020 4:32 AM > From: Stefan Riedmueller > Date: Wed, 23 Sep 2020 16:25:28 +0200 > > > From: Christian Hemp > > > > Make use of device tree alias for device enumeration to keep the > > device order consistent with the naming in the datasheet. > > > > Otherwise for the i.MX 6UL/ULL the ENET1 interface is enumerated as > > eth1 and ENET2 as eth0. > > > > Signed-off-by: Christian Hemp > > Signed-off-by: Stefan Riedmueller > > Device naming and ordering for networking devices was never, ever, > guaranteed. > > Use udev or similar. > > > @@ -3691,6 +3692,10 @@ fec_probe(struct platform_device *pdev) > > > > ndev->max_mtu = PKT_MAXBUF_SIZE - ETH_HLEN - ETH_FCS_LEN; > > > > + eth_id = of_alias_get_id(pdev->dev.of_node, "ethernet"); > > + if (eth_id >= 0) > > + sprintf(ndev->name, "eth%d", eth_id); > > You can't ever just write into ndev->name, what if another networking device > is > already using that name? > > This change is incorrect on many levels. David is correct. For example, imx8DXL has ethernet0 is EQOS TSN, ethernet1 is FEC. EQOS TSN is andother driver and is registered early, the dev->name is eth0. So the patch will bring conflict in such case. Andy
RE: [PATCH net-next] net: fec: ptp: remove unused variable 'ns' in fec_time_keep()
From: Zhang Changzhong Sent: Monday, September 14,
2020 9:14 PM
> Fixes the following W=1 kernel build warning(s):
>
> drivers/net/ethernet/freescale/fec_ptp.c:523:6: warning:
> variable 'ns' set but not used [-Wunused-but-set-variable]
> 523 | u64 ns;
> | ^~
>
> After commit 6605b730c061 ("FEC: Add time stamping code and a PTP
> hardware clock"), variable 'ns' is never used in fec_time_keep(), so removing
> it
> to avoid build warning.
>
> Reported-by: Hulk Robot
> Signed-off-by: Zhang Changzhong
Acked-by: Fugang Duan
> ---
> drivers/net/ethernet/freescale/fec_ptp.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index a0c1f44..0405a39 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -520,13 +520,12 @@ static void fec_time_keep(struct work_struct *work)
> {
> struct delayed_work *dwork = to_delayed_work(work);
> struct fec_enet_private *fep = container_of(dwork, struct
> fec_enet_private, time_keep);
> - u64 ns;
> unsigned long flags;
>
> mutex_lock(&fep->ptp_clk_mutex);
> if (fep->ptp_clk_on) {
> spin_lock_irqsave(&fep->tmreg_lock, flags);
> - ns = timecounter_read(&fep->tc);
> + timecounter_read(&fep->tc);
> spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> }
> mutex_unlock(&fep->ptp_clk_mutex);
> --
> 2.9.5
RE: [PATCH net-next] net: ethernet: fec: remove redundant null check before clk_disable_unprepare()
From: Zhang Changzhong Sent: Monday, September 7, 2020 8:50 PM > Because clk_prepare_enable() and clk_disable_unprepare() already checked > NULL clock parameter, so the additional checks are unnecessary, just remove > them. > > Reported-by: Hulk Robot > Signed-off-by: Zhang Changzhong Acked-by: Fugang Duan > --- > drivers/net/ethernet/freescale/fec_main.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index fb37816..c043afb 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1960,8 +1960,7 @@ static int fec_enet_clk_enable(struct net_device > *ndev, bool enable) > mutex_unlock(&fep->ptp_clk_mutex); > } > failed_clk_ptp: > - if (fep->clk_enet_out) > - clk_disable_unprepare(fep->clk_enet_out); > + clk_disable_unprepare(fep->clk_enet_out); > > return ret; > } > -- > 2.9.5
RE: [EXT] Re: [PATCH v1 0/5] imx6qp QuadPlus: support improved enet clocking
From: Shawn Guo Sent: Monday, August 17, 2020 3:17 PM
> On Sun, Jul 12, 2020 at 08:25:07PM -0400, Sven Van Asbroeck wrote:
> > On the imx6qp QuadPlus, the h/w designers have improved enet clocking.
> >
> > This patchset extends the clock tree to reflect the situation on QuadPlus.
> >
> > This allows board designers to choose the enet clocking method by
> > making simple clocktree changes in the devicetree.
> >
> > Default setting: external routing of enet_ref from pad to pad.
> >
> > Example, change the default to enet_ref @ 125MHz clock routed internally:
> >
> > &fec {
> > assigned-clocks = <&clks IMX6QDL_CLK_ENET_PTP>,
> > <&clks IMX6QDL_CLK_ENET_REF>;
> > assigned-clock-parents = <&clks IMX6QDL_CLK_ENET_REF>;
> > assigned-clock-rates = <0>, <12500>; };
> >
> > To: Shawn Guo
> > To: Sascha Hauer
> > Cc: Pengutronix Kernel Team
> > Cc: Fabio Estevam
> > Cc: NXP Linux Team
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-...@vger.kernel.org
> >
> > Sven Van Asbroeck (5):
> > ARM: mach-imx6q: do not select enet PTP clock source on QuadPlus
> > clk: imx: add simple regmap-backed clock mux
> > dt-bindings: imx6qdl-clock: add QuadPlus enet clocks
> > clk: imx6q: support improved enet clocking on QuadPlus
> > ARM: dts: imx6qp: support improved enet clocking on QuadPlus
>
> Hi Fugang,
>
> Can you take a look at the series?
>
> Shawn
In fact, Sven already sent the patch list to me in private for review
due to miss cc to me. We discussed the series for long time and suppose
Sven will send v2 patch set.
Regards,
Fugang
RE: [EXT] [PATCH net-next v2 0/4] net: fec: a few improvements
From: Sergey Organov Sent: Wednesday, July 15, 2020 11:43 PM > This is a collection of simple improvements that reduce and/or simplify code. > They got developed out of attempt to use DP83640 PTP PHY connected to > built-in FEC (that has its own PTP support) of the iMX 6SX micro-controller. > The primary bug-fix was now submitted separately, and this is the rest of the > changes. > > NOTE: the patches are developed and tested on 4.9.146, and rebased on top > of recent 'net-next/master', where, besides visual inspection, I only tested > that they do compile. > > Sergey Organov (4): > net: fec: enable to use PPS feature without time stamping > net: fec: initialize clock with 0 rather than current kernel time > net: fec: get rid of redundant code in fec_ptp_set() > net: fec: replace snprintf() with strlcpy() in fec_ptp_init() > > v2: > - bug-fix patch from original series submitted separately > - added Acked-by: where applicable > > drivers/net/ethernet/freescale/fec_ptp.c | 13 +++-- > 1 file changed, 3 insertions(+), 10 deletions(-) > For the version: Acked-by: Fugang Duan Thanks!
RE: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()
From: Sergey Organov Sent: Wednesday, July 8, 2020 4:49 PM
> Andy Duan writes:
>
> > From: Sergey Organov Sent: Tuesday, July 7, 2020
> > 10:43 PM
> >> Andy Duan writes:
> >>
> >> > From: Sergey Organov Sent: Monday, July 6,
> >> > 2020
> >> 10:26 PM
> >> >> Code of the form "if(x) x = 0" replaced with "x = 0".
> >> >>
> >> >> Code of the form "if(x == a) x = a" removed.
> >> >>
> >> >> Signed-off-by: Sergey Organov
> >> >> ---
> >> >> drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
> >> >> 1 file changed, 1 insertion(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> >> >> b/drivers/net/ethernet/freescale/fec_ptp.c
> >> >> index e455343..4152cae 100644
> >> >> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> >> >> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> >> >> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev,
> >> >> struct
> >> ifreq
> >> >> *ifr)
> >> >>
> >> >> switch (config.rx_filter) {
> >> >> case HWTSTAMP_FILTER_NONE:
> >> >> - if (fep->hwts_rx_en)
> >> >> - fep->hwts_rx_en = 0;
> >> >> - config.rx_filter = HWTSTAMP_FILTER_NONE;
The original patch seems fine. Thanks!
For the patch: Acked-by: Fugang Duan
RE: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()
From: Sergey Organov Sent: Tuesday, July 7, 2020 10:43 PM
> Andy Duan writes:
>
> > From: Sergey Organov Sent: Monday, July 6, 2020
> 10:26 PM
> >> Code of the form "if(x) x = 0" replaced with "x = 0".
> >>
> >> Code of the form "if(x == a) x = a" removed.
> >>
> >> Signed-off-by: Sergey Organov
> >> ---
> >> drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
> >> 1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> >> b/drivers/net/ethernet/freescale/fec_ptp.c
> >> index e455343..4152cae 100644
> >> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> >> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> >> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev, struct
> ifreq
> >> *ifr)
> >>
> >> switch (config.rx_filter) {
> >> case HWTSTAMP_FILTER_NONE:
> >> - if (fep->hwts_rx_en)
> >> - fep->hwts_rx_en = 0;
> >> - config.rx_filter = HWTSTAMP_FILTER_NONE;
> > The line should keep according your commit log.
>
> You mean I should fix commit log like this:
>
> Code of the form "switch(x) case a: x = a; break" removed.
>
> ?
Like this:
case HWTSTAMP_FILTER_NONE:
fep->hwts_rx_en = 0;
config.rx_filter = HWTSTAMP_FILTER_NONE;
break;
>
> I'll do if it's cleaner that way.
>
> Thanks,
> -- Sergey
>
>
> >
> >> + fep->hwts_rx_en = 0;
> >> break;
> >>
> >> default:
> >> --
> >> 2.10.0.1.g57b01a3
RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
From: Sven Van Asbroeck Sent: Tuesday, July 7, 2020 11:21
PM
> Andy, Fabio,
>
> Sounds like we now have a solution which makes logical sense, although it
> requires changes and additions to drivers/clk/imx/. Before I create a patch,
> can you read the plan below and check that it makes sense, please?
>
> Problem
> ===
> On the imx6q plus, the NXP hw designers introduced an alternative way to
> clock the built-in ethernet (FEC). One single customer (archronix) seems to be
> currently using this functionality, and would like to add mainline support.
>
> Changing the ethernet (FEC) driver appears illogical, as this change is
> unrelated to the FEC itself: fundamentally, it's a clock tree change.
>
> We also need to prevent breaking existing boards with mis-configured FEC
> clocking:
>
> Some board designers indicate in the devicetree that ENET_REF_CLK is
> connected to the FEC ptp clock, but in reality, they are providing an
> unrelated
> external clock through the pad. This will work in many cases, because the FEC
> driver does not really care where its PTP clock comes from, and the enet ref
> clock is set up to mirror the external clk frequency by the NXP U-Boot fork.
>
> Proposed changes must not break these cases.
>
> Proposed Solution
> =
> On non-plus imx6q there are no changes. On imx6q plus however:
>
> Create two new clocks (mac_gtx and pad) and a new clock mux:
>
> enet_ref-o->pad->| \
>| | |
>| |M1|mac_gtx --- to FEC
>| | |
>o---|_/
>
> The defaults (indicated with ">") are carefully chosen, so these changes will
> not break any existing plus designs.
>
> We then tie the gtx clock to the FEC driver ptp clock, like so:
>
> in imx6qp.dtsi:
> &fec {
> clocks = <&clks IMX6QDL_CLK_ENET>,
> <&clks IMX6QDL_CLK_ENET>,
> <&clks IMX6QDL_CLK_MAC_GTX>;
> };
>
> Mux M1 is controlled by GPR5[9]. This mux can just write to the GPR5
> memory address. However, the GPR registers are already exposed as a mux
> controller compatible = "mmio-mux". This mux does currently not use GPR5.
>
> So we have to make a choice here: write straight to the memory address
> (easy), or create a new clock mux type, which uses an underlying mux
> controller.
>
> When that is done, board designers can choose between:
>
> 1. internal clocking (GTX clock routed internally)
>
> &fec {
> assigned-clocks = <&clks IMX6QDL_CLK_MAC_GTX>;
> assigned-clock-parents = <&clks IMX6QDL_CLK_ENET_REF>; };
The one is our requirement that gtx is from internal pll and only for
6qp boards. It is required to set in board dts file due to depends on board
design.
Here also need to assign enet_ref clk rate to 125Mhz.
>
> 2. external clocking (GTX clock from pad, pad connected to enet_ref,
>typically via GPIO_16). This is the default and does not need to be
> present.
>
> &fec {
> assigned-clocks = <&clks IMX6QDL_CLK_MAC_GTX>;
> assigned-clock-parents = <&clks IMX6QDL_CLK_PAD>; };
>
As above, here also need to assign the enet_ref clk rate to 125Mhz.
The clk tree:
Pll6 -> enet_ref -> clk_pad -> mac_gtx
Pll6 -> enet_ref -> clk_pad -> route back to supply clk for ptp
>
> 3. external clocking (GTX clock from pad, pad connected to external PHY
>clock or external oscillator).
>
> phy_osc: oscillator {
> #clock-cells = <0>;
> compatible = "fixed-clock";
> clock-frequency = <5000>;
> };
>
> &fec {
> clocks = <&clks IMX6QDL_CLK_ENET>,
> <&clks IMX6QDL_CLK_ENET>,
> <&phy_osc>;
> };
RE: [EXT] [PATCH 4/5] net: fec: get rid of redundant code in fec_ptp_set()
From: Sergey Organov Sent: Monday, July 6, 2020 10:26 PM
> Code of the form "if(x) x = 0" replaced with "x = 0".
>
> Code of the form "if(x == a) x = a" removed.
>
> Signed-off-by: Sergey Organov
> ---
> drivers/net/ethernet/freescale/fec_ptp.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index e455343..4152cae 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -485,9 +485,7 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq
> *ifr)
>
> switch (config.rx_filter) {
> case HWTSTAMP_FILTER_NONE:
> - if (fep->hwts_rx_en)
> - fep->hwts_rx_en = 0;
> - config.rx_filter = HWTSTAMP_FILTER_NONE;
The line should keep according your commit log.
> + fep->hwts_rx_en = 0;
> break;
>
> default:
> --
> 2.10.0.1.g57b01a3
RE: [EXT] [PATCH 2/5] net: fec: enable to use PPS feature without time stamping
From: Sergey Organov Sent: Monday, July 6, 2020 10:26 PM
> PPS feature could be useful even when hardware time stamping of network
> packets is not in use, so remove offending check for this condition from
> fec_ptp_enable_pps().
If hardware time stamping of network packets is not in use, PPS is based on
local
clock, what is the use case ?
>
> Signed-off-by: Sergey Organov
> ---
> drivers/net/ethernet/freescale/fec_ptp.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index f8a592c..4a12086 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -103,11 +103,6 @@ static int fec_ptp_enable_pps(struct
> fec_enet_private *fep, uint enable)
> u64 ns;
> val = 0;
>
> - if (!(fep->hwts_tx_en || fep->hwts_rx_en)) {
> - dev_err(&fep->pdev->dev, "No ptp stack is running\n");
> - return -EINVAL;
> - }
> -
> if (fep->pps_enable == enable)
> return 0;
>
> --
> 2.10.0.1.g57b01a3
RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
From: Sven Van Asbroeck Sent: Monday, July 6, 2020 11:00 PM > On Mon, Jul 6, 2020 at 10:58 AM Sven Van Asbroeck > wrote: > > > > Hi Fabio, > > > > On Mon, Jul 6, 2020 at 9:46 AM Fabio Estevam > wrote: > > > > > > Would it make sense to use compatible = "mmio-mux"; like we do on > > > imx7s, imx6qdl.dtsi and imx8mq.dtsi? > > > > Maybe "fixed-mmio-clock" is a better candidate ? > > Actually no, the mmio memory there only controls the frequency... > > I don't think the clock framework has a ready-made abstraction suitable for a > GPR clock mux yet? That needs to add GPR clock API support.
RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
From: Sven Van Asbroeck Sent: Sunday, July 5, 2020 11:34 PM > > ext phy-| \ > | | > enet_ref-o--|M |pad--| \ >| |_/ | | >| |M |mac_gtx >| | | >o---|_/ > > > How do we tell the clock framework that clk_pad has a mux that can be > connected to _any_ external clock? and also enet_ref? The clock depends on board design, HW refer guide can describe the clk usage in detail and customer select one clk path as their board design. To make thing simple, we can just control the second "M" that is controlled by gpr bit.
RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
From: Sven Van Asbroeck > Hi Fabio, Andy, > > On Thu, Jul 2, 2020 at 6:29 PM Fabio Estevam wrote: > > > > With the device tree approach, I think that a better place to touch > > GPR5 would be inside the fec driver. > > > > Are we 100% sure this is the best way forward, though? > > All the FEC driver should care about is the FEC logic block inside the SoC. It > should not concern itself with the way a SoC happens to bring a clock (PTP > clock) to the input of the FEC logic block - that is purely a SoC > implementation > detail. I also agree with that relates to SOC integration. > > It makes sense to add fsl,stop-mode (= a GPR bit) to the FEC driver, as this > bit > is a logic input to the FEC block, which the driver needs to dynamically flip. > > But the PTP clock is different, because it's statically routed by the SoC. > > Maybe this problem needs a clock routing solution? > Isn't there an imx6q plus clock mux we're not modelling? > > enet_ref-o-->ext>---pad_clk--| \ >| |M |fec_ptp_clk >o---|_/ > > Where M = mux controlled by GPR5[9] > > The issue here is that pad_clk is routed externally, so its parent could be > any > internal or external clock. I have no idea how to model this in the clock > framework. Don't consider it complex, GPR5[9] just select the rgmii gtx source from PAD or internal Like： GPR5[9] is cleared: PAD -> MAC gtx GPR5[9] is set: Pll_enet -> MAC gtx As you said, register one clock mux for the selection, assign the clock parent by board dts file, but now current clock driver doesn't support GPR clock.
RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
From: Sven Van Asbroeck Sent: Friday, July 3, 2020 8:51 AM > Hi Fabio, > > On Thu, Jul 2, 2020 at 6:29 PM Fabio Estevam wrote: > > > > With the device tree approach, I think that a better place to touch > > GPR5 would be inside the fec driver. > > > > Cool idea. I notice that the latest FEC driver (v5.8-rc3) accesses individual > bits > inside the gpr (via fsl,stop-mode). So perhaps I can do the same here, and > populate that gpr node in imx6qp.dtsi - because it doesn't exist on other > SoCs. > > > For the property name, what about fsl,txclk-from-pll? > > Sounds good. Does anyone have more suggestions? The property seems good, don't let the property confuse somebody for other platforms, binding doc describe the property is limited to use for imx6qp only.
RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Sven Van Asbroeck Sent: Wednesday, July 1, 2020
9:52 PM
> Andy, Fabio,
>
> Does the following describe the mainline situation?
> Please correct if not.
>
> 1. imx6 ethernet ref_clk can be generated internally (by imx6) or
>externally (by PHY or oscillator on PCB) 2. if internal, fec's "ptp" clock
> in
> devicetree should be
><&clks IMX6QDL_CLK_ENET_REF>
> 3. if external, fec's "ptp" clock should be that external clock,
>see 810c0ca879098 ("ARM: imx6q: support ptp and rmii clock from pad")
> 4. sabresd devicetree describes "ptp" clock as IMX6QDL_CLK_ENET_REF,
>although it's externally supplied (by PHY). This is incorrect.
No, ptp_clk is the same as enet_ref, it means ptp clock source from internal
pll.
So, for current upstream status for imx6q/6dl/6qp, ptp clock is from internal
pll,
rgmii gtx clock is from phy.
For 6qp, soc already support to route internal pll to rgmii gtx by setting
gpr5[9],
the patch force to use internal pll instead of external clk from phy. It doesn't
break imx6q/6dl. But as Fabio's said, it break old 6qp sabresd dtb.
Discuss with Fabio, an existing(old) dtb in mainline has to work in future
kernels,
without the need of being updated, so to add internal pll support for 6qp rgmii
gtx,
and not to break 6qp old dtb, add new property is one solution.
RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Fabio Estevam Sent: Wednesday, July 1, 2020 11:39 AM
> Hi Andy,
>
> On Wed, Jul 1, 2020 at 12:18 AM Andy Duan wrote:
>
> > --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > @@ -202,6 +202,8 @@
> > &fec {
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_enet>;
> > + assigned-clocks = <&clks IMX6QDL_CLK_ENET_REF>;
> > + assigned-clock-rates = <12500>;
>
> I don't think this is an acceptable solution as it breaks old dtb's.
It doesn't break old dtbs, and doesn't break imx6q/dl/solo.
RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Sven Van Asbroeck Sent: Tuesday, June 30, 2020
11:24 PM
> Andy, Fabio,
>
> On Tue, Jun 30, 2020 at 2:36 AM Andy Duan wrote:
> >
> > Sven, no matter PHY supply 125Mhz clock to pad or not, GPR5[9] is to
> > select RGMII gtx clock source from:
> > - 0 Clock from pad
> > - 1 Clock from PLL
> >
> > Since i.MX6QP can internally supply clock to MAC, we can set GPR5[9] bit by
> default.
>
> That's true. But on the sabresd I notice that the PHY's ref_clk output is from
> CLK_25M.
> The default ref_clk freq for that PHY is 25 MHz, and I don't see anyone change
> the default in the devicetree. I also see that a 25 MHz crystal is fitted,
> which
> also suggests 25 Mhz output.
>
> On the imx6, the default ref_clk frequency from ANATOP is 50Mhz. I don't see
> anyone change that default in the devicetree either.
>
> So is it possible that, when we switch GPR5[9] on, the external 25MHz clock is
> replaced by the internal 50MHz clock? If so, I'm not sure it'll work...?
Fabio, the reason is that you don't update uboot that why we cannot reproduce
the issue on imx6qp sabresd.
Sven, uboot board file set the clock rate.
board/freescale/mx6sabresd/mx6sabresd.c:
if (is_mx6dqp()) {
int ret;
/* select ENET MAC0 TX clock from PLL */
imx_iomux_set_gpr_register(5, 9, 1, 1);
ret = enable_fec_anatop_clock(0, ENET_125MHZ);
if (ret)
printf("Error fec anatop clock settings!\n");
}
Sven, to avoid to depend on uboot setting, for the patch, it is better to bind
below change for dts (even if non imx6qp, ptp clock can be set to 125Mhz):
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -202,6 +202,8 @@
&fec {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_enet>;
+ assigned-clocks = <&clks IMX6QDL_CLK_ENET_REF>;
+ assigned-clock-rates = <12500>;
phy-mode = "rgmii-id";
RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Fabio Estevam Sent: Tuesday, June 30, 2020 7:49 PM > Hi Andy, > > On Tue, Jun 30, 2020 at 3:36 AM Andy Duan wrote: > > > Fabio, our QA double verify 5.4.24_2.1.0, no matter SD boot or > > tftp/nfs boot, both work fine on i.MX6QP sabresd board, please double > check your environment. > > Is static IP or DHCP used on these tests? > SD boot into yocto system, then run dhcp. Or, nfs boot. Both works fine.
RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Sven Van Asbroeck Sent: Monday, June 29, 2020 10:37 PM > On Mon, Jun 29, 2020 at 10:26 AM Fabio Estevam > wrote: > > > > Just tested 5.4.24_2.1.0 on an imx6qp sabresd and DHCP also fails there. > > I think I discovered the problem ! > > When I compare the sabresd devicetree on mainline with the actual sabresd > schematics, the devicetree is incorrect ! Things still work, but only by > accident. > > The sabresd has an AR8131 PHY, which generates the enet ref clock, not the > imx6. So on the schematic we see that the clock output of the PHY is wired to > imx6 ENET_REF_CLK, so it can be used as a clock source. And GPIO_16 is > disconnected, as it should, because the imx6 is not generating the ref clk. > > But the devicetree is written as if the imx6 is providing the clock ! See > here: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker > nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.git%2Ftr > ee%2Farch%2Farm%2Fboot%2Fdts%2Fimx6qdl-sabresd.dtsi%3Fh%3Dv5.7.6 > %23n513&data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d > 8e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7 > C0%7C637290382588751887&sdata=hCRNGa9WrQzQ0XYwR%2FQTQ8h > jY7CDHjhIbXr0L33fr%2Bc%3D&reserved=0 > > Also there is no override of the fec PTP clock: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker > nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.git%2Ftr > ee%2Farch%2Farm%2Fboot%2Fdts%2Fimx6qdl-sabresd.dtsi%3Fh%3Dv5.7.6 > %23n202&data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d > 8e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7 > C0%7C637290382588751887&sdata=qOfiLs%2FGi01h9hSA787PHfGxTN > bfYWFXVA3IhUB553Q%3D&reserved=0 > > Although Shawn's mainline patch mandates this? > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker > nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.git%2Fco > mmit%2F%3Fh%3Dv5.7.6%26id%3D810c0ca879098a993e2ce0a190d24d11c > 17df748&data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d8 > e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C > 0%7C637290382588751887&sdata=3PiAnDlxO8iqsVR6YQWjCk1xsg8iW > RK8TSGee4LhkjI%3D&reserved=0 > > This will work, but only by accident. So on a plus, when we > (incorrectly) switch the > bypass bit on, things stop working. Fabio, our QA double verify 5.4.24_2.1.0, no matter SD boot or tftp/nfs boot, both work fine on i.MX6QP sabresd board, please double check your environment. Sven, no matter PHY supply 125Mhz clock to pad or not, GPR5[9] is to select RGMII gtx clock source from: - 0 Clock from pad - 1 Clock from PLL Since i.MX6QP can internally supply clock to MAC, we can set GPR5[9] bit by default.
RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Fabio Estevam Sent: Monday, June 29, 2020 10:26 PM > Hi Sven, > > On Mon, Jun 29, 2020 at 10:40 AM Sven Van Asbroeck > wrote: > > > Thank you for testing this out on a different platform ! > > > > I had a look at how things are done in the Freescale fork of the > > kernel > > (5.4.24_2.1.0) and I noticed that this kernel has almost the same > > behaviour as this proposed patch: the GPR5 bit is _always_ set on a > > plus. The code does not check how the enet clock is generated. > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsour > > > ce.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Ftree%2Farch%2Farm > %2Fm > > > ach-imx%2Fmach-imx6q.c%3Fh%3Drel_imx_5.4.24_2.1.0%26id%3Dbabac00 > 8e5cf1 > > > 68abca1a85bda2e8071ca27a5c0%23n269&data=02%7C01%7Cfugang.d > uan%40nx > > > p.com%7C8570de0304514796ea0208d81c385a11%7C686ea1d3bc2b4c6fa92 > cd99c5c3 > > > 01635%7C0%7C1%7C637290375659016888&sdata=I9werBT%2FDkcWu > LEKlFVzRi2 > > KD%2FLwPz2QCqw%2BHn0HY8U%3D&reserved=0 > > > > Now, I'm assuming that the sabresd-plus can run on the Freescale > > kernel fork. The GPR5 bit will always be set there. > > Just tested 5.4.24_2.1.0 on an imx6qp sabresd and DHCP also fails there. Fabio, we have LAVA daily test by networking for imx6qp sabresd board, no any issue found. Please double check the issue on your local.
RE: [EXT] [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Sven Van Asbroeck Sent: Thursday, June 25, 2020
10:01 PM
> On imx6, the ethernet reference clock (clk_enet_ref) can be generated by
> either the imx6, or an external source (e.g. an oscillator or the PHY). When
> generated by the imx6, the clock source (from ANATOP) must be routed to the
> input of clk_enet_ref via two pads on the SoC, typically via a dedicated track
> on the PCB.
>
> On an imx6 plus however, there is a new setting which enables this clock to
> be routed internally on the SoC, from its ANATOP clock source, straight to
> clk_enet_ref, without having to go through the SoC pads.
>
> Board designs where the clock is generated by the imx6 should not be
> affected by routing the clock internally. Therefore on a plus, we can enable
> internal routing by default.
>
> Signed-off-by: Sven Van Asbroeck
For the version:
Reviewed-by: Fugang Duan
> ---
> v3 -> v4:
> - avoid double-check for IS_ERR(gpr) by including Fabio Estevam's
> patch.
> v2 -> v3:
> - remove check for imx6q, which is already implied when
> of_machine_is_compatible("fsl,imx6qp")
> v1 -> v2:
> - Fabio Estevam: use of_machine_is_compatible() to determine if we
> are running on an imx6 plus.
>
> To: Shawn Guo
> To: Andy Duan
> Cc: Sascha Hauer
> Cc: Pengutronix Kernel Team
> Cc: Fabio Estevam
> Cc: NXP Linux Team
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
>
> arch/arm/mach-imx/mach-imx6q.c | 14 ++
> include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 1 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/arch/arm/mach-imx/mach-imx6q.c
> b/arch/arm/mach-imx/mach-imx6q.c index ae89ad93ca83..07cfe0d349c3
> 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -204,6 +204,20 @@ static void __init imx6q_1588_init(void)
> regmap_update_bits(gpr, IOMUXC_GPR1,
> IMX6Q_GPR1_ENET_CLK_SEL_MASK,
>clksel);
>
> + /*
> +* On imx6 plus, enet_ref from ANATOP/CCM can be internally
> routed to
> +* be the PTP clock source, instead of having to be routed through
> +* pads.
> +* Board designs which route the ANATOP/CCM clock through pads
> are
> +* unaffected when routing happens internally. So on these designs,
> +* route internally by default.
> +*/
> + if (clksel == IMX6Q_GPR1_ENET_CLK_SEL_ANATOP &&
> + of_machine_is_compatible("fsl,imx6qp"))
> + regmap_update_bits(gpr, IOMUXC_GPR5,
> + IMX6Q_GPR5_ENET_TXCLK_SEL,
> + IMX6Q_GPR5_ENET_TXCLK_SEL);
> +
> clk_put(enet_ref);
> put_ptp_clk:
> clk_put(ptp_clk);
> diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> index d4b5e527a7a3..eb65d48da0df 100644
> --- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> +++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> @@ -240,6 +240,7 @@
> #define IMX6Q_GPR4_IPU_RD_CACHE_CTLBIT(0)
>
> #define IMX6Q_GPR5_L2_CLK_STOP BIT(8)
> +#define IMX6Q_GPR5_ENET_TXCLK_SEL BIT(9)
> #define IMX6Q_GPR5_SATA_SW_PD BIT(10)
> #define IMX6Q_GPR5_SATA_SW_RST BIT(11)
>
> --
> 2.17.1
RE: [EXT] Re: [PATCH v1] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Sven Van Asbroeck Sent: Wednesday, June 24, 2020
10:56 AM
> Hi Andy,
>
> On Tue, Jun 23, 2020 at 9:40 PM Andy Duan wrote:
> >
> > The patch looks good.
> > Reviewed-by: Fugang Duan
>
> Thank you !
>
> To check we're on a plus, the patch uses:
> cpu_is_imx6q() && imx_get_soc_revision() >= IMX_CHIP_REVISION_2_0
>
> Fabio Estevam suggested that perhaps checking
> of_machine_is_compatible("fsl,imx6qp")
> might be preferable. What is your opinion on this?
Yes, I also think of_machine_is_compatible() is much better.
Thanks.
RE: [EXT] Re: [PATCH v1] ARM: imx6plus: enable internal routing of clk_enet_ref where possible
From: Shawn Guo Sent: Tuesday, June 23, 2020 7:54 PM
> Hi Fugang,
>
> Can you take a look at this patch? Thanks!
>
The patch looks good.
Reviewed-by: Fugang Duan
> Shawn
>
> On Sat, Jun 13, 2020 at 04:17:03PM -0400, Sven Van Asbroeck wrote:
> > On imx6, the ethernet reference clock (clk_enet_ref) can be generated
> > by either the imx6, or an external source (e.g. an oscillator or the
> > PHY). When generated by the imx6, the clock source (from ANATOP) must
> > be routed to the input of clk_enet_ref via two pads on the SoC,
> > typically via a dedicated track on the PCB.
> >
> > On an imx6 plus however, there is a new setting which enables this
> > clock to be routed internally on the SoC, from its ANATOP clock
> > source, straight to clk_enet_ref, without having to go through the SoC
> > pads.
> >
> > Board designs where the clock is generated by the imx6 should not be
> > affected by routing the clock internally. Therefore on a plus, we can
> > enable internal routing by default.
> >
> > To: Shawn Guo
> > Cc: Sascha Hauer
> > Cc: Pengutronix Kernel Team
> > Cc: Fabio Estevam
> > Cc: NXP Linux Team
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Sven Van Asbroeck
> > ---
> > arch/arm/mach-imx/mach-imx6q.c | 18
> ++
> > include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 1 +
> > 2 files changed, 19 insertions(+)
> >
> > Tree: next-20200613
> >
> > diff --git a/arch/arm/mach-imx/mach-imx6q.c
> > b/arch/arm/mach-imx/mach-imx6q.c index 85c084a716ab..4d22567bb650
> > 100644
> > --- a/arch/arm/mach-imx/mach-imx6q.c
> > +++ b/arch/arm/mach-imx/mach-imx6q.c
> > @@ -203,6 +203,24 @@ static void __init imx6q_1588_init(void)
> > else
> > pr_err("failed to find fsl,imx6q-iomuxc-gpr regmap\n");
> >
> > + /*
> > + * On imx6 plus, enet_ref from ANATOP/CCM can be internally
> routed to
> > + * be the PTP clock source, instead of having to be routed through
> > + * pads.
> > + * Board designs which route the ANATOP/CCM clock through pads
> are
> > + * unaffected when routing happens internally. So on these designs,
> > + * route internally by default.
> > + */
> > + if (clksel == IMX6Q_GPR1_ENET_CLK_SEL_ANATOP &&
> cpu_is_imx6q() &&
> > + imx_get_soc_revision() >=
> IMX_CHIP_REVISION_2_0) {
> > + if (!IS_ERR(gpr))
> > + regmap_update_bits(gpr, IOMUXC_GPR5,
> > +
> IMX6Q_GPR5_ENET_TXCLK_SEL,
> > +
> IMX6Q_GPR5_ENET_TXCLK_SEL);
> > + else
> > + pr_err("failed to find fsl,imx6q-iomuxc-gpr
> regmap\n");
> > + }
> > +
> > clk_put(enet_ref);
> > put_ptp_clk:
> > clk_put(ptp_clk);
> > diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> > b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> > index d4b5e527a7a3..eb65d48da0df 100644
> > --- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> > +++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> > @@ -240,6 +240,7 @@
> > #define IMX6Q_GPR4_IPU_RD_CACHE_CTL BIT(0)
> >
> > #define IMX6Q_GPR5_L2_CLK_STOP BIT(8)
> > +#define IMX6Q_GPR5_ENET_TXCLK_SELBIT(9)
> > #define IMX6Q_GPR5_SATA_SW_PDBIT(10)
> > #define IMX6Q_GPR5_SATA_SW_RST BIT(11)
> >
> > --
> > 2.17.1
> >
RE: [EXT] Re: [PATCH] net: fec: fix ref count leaking when pm_runtime_get_sync fails
From: David Miller Sent: Tuesday, June 16, 2020 4:42 AM > From: Navid Emamdoost > Date: Sun, 14 Jun 2020 00:38:01 -0500 > > > in fec_enet_mdio_read, fec_enet_mdio_write, fec_enet_get_regs, > > fec_enet_open and fec_drv_remove, pm_runtime_get_sync is called which > > increments the counter even in case of failure, leading to incorrect > > ref count. In case of failure, decrement the ref count before returning. > > > > Signed-off-by: Navid Emamdoost > > This does not apply to the net tree. Wolfram Sang wonder if such de-reference can be better handled by pm runtime core code. https://lkml.org/lkml/2020/6/14/76
RE: [EXT] [PATCH] i2c: busses: Fix a reference count leak.
From: wu000...@umn.edu Sent: Sunday, June 14, 2020 6:12 AM
> From: Qiushi Wu
>
> pm_runtime_get_sync() increments the runtime PM usage counter even
> when it returns an error code. Thus call pm_runtime_put_noidle() if
> pm_runtime_get_sync() fails.
>
> Fixes: 13d6eb20fc79 ("i2c: imx-lpi2c: add runtime pm support")
> Signed-off-by: Qiushi Wu
Again, which case can trigger the issue ?
> ---
> drivers/i2c/busses/i2c-imx-lpi2c.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 9db6ccded5e9..85b9c1fc7681 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -260,8 +260,10 @@ static int lpi2c_imx_master_enable(struct
> lpi2c_imx_struct *lpi2c_imx)
> int ret;
>
> ret = pm_runtime_get_sync(lpi2c_imx->adapter.dev.parent);
> - if (ret < 0)
> + if (ret < 0) {
> + pm_runtime_put_noidle(lpi2c_imx->adapter.dev.parent);
> return ret;
> + }
>
> temp = MCR_RST;
> writel(temp, lpi2c_imx->base + LPI2C_MCR);
> --
> 2.17.1
RE: [EXT] [PATCH] [v3] i2c: imx-lpi2c: Fix runtime PM imbalance on error
From: Dinghao Liu Sent: Monday, June 1, 2020 2:17 PM
> pm_runtime_get_sync() increments the runtime PM usage counter even the
> call returns an error code. Thus a corresponding decrement is needed on the
> error handling path to keep the counter balanced.
>
> Fix this by adding the missed function call.
>
> Fixes: 13d6eb20fc79a ("i2c: imx-lpi2c: add runtime pm support")
> Co-developed-by: Markus Elfring
> Signed-off-by: Markus Elfring
> Signed-off-by: Dinghao Liu
Reviewed-by: Fugang Duan
> ---
>
> Changelog:
>
> v2: - Use pm_runtime_put_noidle() instead of
> pm_runtime_put_autosuspend().
>
> v3: - Refine commit message.
> ---
> drivers/i2c/busses/i2c-imx-lpi2c.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 94743ba581fe..bdee02dff284 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -260,8 +260,10 @@ static int lpi2c_imx_master_enable(struct
> lpi2c_imx_struct *lpi2c_imx)
> int ret;
>
> ret = pm_runtime_get_sync(lpi2c_imx->adapter.dev.parent);
> - if (ret < 0)
> + if (ret < 0) {
> + pm_runtime_put_noidle(lpi2c_imx->adapter.dev.parent);
> return ret;
> + }
>
> temp = MCR_RST;
> writel(temp, lpi2c_imx->base + LPI2C_MCR);
> --
> 2.17.1
RE: [PATCH 0/4] Change i.MX/MXS SoCs ocotp/iim node name to efuse
From: Anson Huang Sent: Thursday, May 28, 2020 11:13 AM > In the nvmem yaml schema, it requires the nodename to be one of > "eeprom|efuse|nvram", so need to change all i.MX/MXS SoCs ocotp/iim node > name to efuse: > > MXS platforms: i.MX23/28; > i.MX platforms with IIM: i.MX25/27/31/35/51/53. > i.MX ARMv7 platforms with OCOTP: i.MX6QDL/6SL/6SX/6SLL/6UL/7S/7ULP. > i.MX ARMv8 platforms with OCOTP: i.MX8MQ/8MM/8MN/8MP. Reviewed-by: Fugang Duan > > Anson Huang (4): > ARM: dts: imx: change ocotp node name on i.MX6/7 SoCs > arm64: dts: imx8m: change ocotp node name on i.MX8M SoCs > ARM: dts: imx: change ocotp node name on MXS SoCs > ARM: dts: imx: change iim node name on i.MX SoCs > > arch/arm/boot/dts/imx23.dtsi | 2 +- > arch/arm/boot/dts/imx25.dtsi | 2 +- > arch/arm/boot/dts/imx27.dtsi | 2 +- > arch/arm/boot/dts/imx28.dtsi | 2 +- > arch/arm/boot/dts/imx31.dtsi | 2 +- > arch/arm/boot/dts/imx35.dtsi | 2 +- > arch/arm/boot/dts/imx51.dtsi | 2 +- > arch/arm/boot/dts/imx53.dtsi | 2 +- > arch/arm/boot/dts/imx6qdl.dtsi| 2 +- > arch/arm/boot/dts/imx6sl.dtsi | 2 +- > arch/arm/boot/dts/imx6sll.dtsi| 2 +- > arch/arm/boot/dts/imx6sx.dtsi | 2 +- > arch/arm/boot/dts/imx6ul.dtsi | 2 +- > arch/arm/boot/dts/imx7s.dtsi | 2 +- > arch/arm/boot/dts/imx7ulp.dtsi| 2 +- > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 +- > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 2 +- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 +- > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 2 +- > 19 files changed, 19 insertions(+), 19 deletions(-) > > -- > 2.7.4
RE: [EXT] Re: [PATCH 1/1] arm64: dts: imx8mp: add "fsl,imx6sx-fec" compatible string
From: Shawn Guo Sent: Wednesday, May 13, 2020 4:50 PM
> On Wed, Apr 29, 2020 at 06:04:14PM +0800, fugang.d...@nxp.com wrote:
> > From: Fugang Duan
> >
> > Add "fsl,imx6sx-fec" compatible string for fec node, then i.MX8MP EVK
> > ethernet function can work now.
> >
> > Signed-off-by: Fugang Duan
> > ---
> > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > index 9b1616e59d58..b5df957c5063 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -615,7 +615,7 @@
> > };
> >
> > fec: ethernet@30be {
> > - compatible = "fsl,imx8mp-fec",
> "fsl,imx8mq-fec";
> > + compatible = "fsl,imx8mp-fec",
> > + "fsl,imx8mq-fec", "fsl,imx6sx-fec";
>
> In this case, "fsl,imx8mq-fec" can be dropped?
>
> Shawn
Please don't drop the compatible string, there have little difference for 8mq
that support eee feature, the feature will be upstreamed.
Thanks.
>
> > reg = <0x30be 0x1>;
> > interrupts = IRQ_TYPE_LEVEL_HIGH>,
> > > IRQ_TYPE_LEVEL_HIGH>,
> > --
> > 2.17.1
> >
RE: [EXT] [PATCH v1 3/3] tty: serial: lpuart: Add RS485 support for 32-bit uart flavour
From: Philippe Schenker Sent: Wednesday,
October 16, 2019 11:19 PM
> This commits adds RS485 support for LPUART hardware that uses 32-bit
> registers. These are typically found in i.MX8 processors.
>
> Signed-off-by: Philippe Schenker
Reviewed-by: Fugang Duan
>
> ---
>
> drivers/tty/serial/fsl_lpuart.c | 65 -
> 1 file changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c
> b/drivers/tty/serial/fsl_lpuart.c index
> 346b4a070ce9..22df5f8f48b6 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1280,6 +1280,57 @@ static int lpuart_config_rs485(struct uart_port
> *port,
> return 0;
> }
>
> +static int lpuart32_config_rs485(struct uart_port *port,
> + struct serial_rs485 *rs485) {
> + struct lpuart_port *sport = container_of(port,
> + struct lpuart_port, port);
> +
> + unsigned long modem = lpuart32_read(&sport->port, UARTMODIR)
> + & ~(UARTMODEM_TXRTSPOL |
> UARTMODEM_TXRTSE);
> + lpuart32_write(&sport->port, modem, UARTMODIR);
> +
> + /* clear unsupported configurations */
> + rs485->delay_rts_before_send = 0;
> + rs485->delay_rts_after_send = 0;
> + rs485->flags &= ~SER_RS485_RX_DURING_TX;
> +
> + if (rs485->flags & SER_RS485_ENABLED) {
> + /* Enable auto RS-485 RTS mode */
> + modem |= UARTMODEM_TXRTSE;
> +
> + /*
> +* RTS needs to be logic HIGH either during transer _or_
> after
> +* transfer, other variants are not supported by the
> hardware.
> +*/
> +
> + if (!(rs485->flags & (SER_RS485_RTS_ON_SEND |
> + SER_RS485_RTS_AFTER_SEND)))
> + rs485->flags |= SER_RS485_RTS_ON_SEND;
> +
> + if (rs485->flags & SER_RS485_RTS_ON_SEND &&
> + rs485->flags &
> SER_RS485_RTS_AFTER_SEND)
> + rs485->flags &=
> ~SER_RS485_RTS_AFTER_SEND;
> +
> + /*
> +* The hardware defaults to RTS logic HIGH while
> transfer.
> +* Switch polarity in case RTS shall be logic HIGH
> +* after transfer.
> +* Note: UART is assumed to be active high.
> +*/
> + if (rs485->flags & SER_RS485_RTS_ON_SEND)
> + modem &= ~UARTMODEM_TXRTSPOL;
> + else if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
> + modem |= UARTMODEM_TXRTSPOL;
> + }
> +
> + /* Store the new configuration */
> + sport->port.rs485 = *rs485;
> +
> + lpuart32_write(&sport->port, modem, UARTMODIR);
> + return 0;
> +}
> +
> static unsigned int lpuart_get_mctrl(struct uart_port *port) {
> unsigned int temp = 0;
> @@ -1878,6 +1929,13 @@ lpuart32_set_termios(struct uart_port *port,
> struct ktermios *termios,
> ctrl |= UARTCTRL_M;
> }
>
> + /*
> +* When auto RS-485 RTS mode is enabled,
> +* hardware flow control need to be disabled.
> +*/
> + if (sport->port.rs485.flags & SER_RS485_ENABLED)
> + termios->c_cflag &= ~CRTSCTS;
> +
> if (termios->c_cflag & CRTSCTS) {
> modem |= (UARTMODIR_RXRTSE |
> UARTMODIR_TXCTSE);
> } else {
> @@ -2405,7 +2463,10 @@ static int lpuart_probe(struct platform_device
> *pdev)
> sport->port.ops = &lpuart_pops;
> sport->port.flags = UPF_BOOT_AUTOCONF;
>
> - sport->port.rs485_config = lpuart_config_rs485;
> + if (lpuart_is_32(sport))
> + sport->port.rs485_config = lpuart32_config_rs485;
> + else
> + sport->port.rs485_config = lpuart_config_rs485;
>
> sport->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> if (IS_ERR(sport->ipg_clk)) {
> @@ -2459,7 +2520,7 @@ static int lpuart_probe(struct platform_device
> *pdev)
> sport->port.rs485.delay_rts_after_send)
> dev_err(&pdev->dev, "driver doesn't support RTS
> delays\n");
>
> - lpuart_config_rs485(&sport->port, &sport->port.rs485);
> + sport->port.rs485_config(&sport->port, &sport->port.rs485);
>
> sport->dma_tx_chan =
> dma_request_slave_channel(sport->port.dev, "tx");
> if (!sport->dma_tx_chan)
> --
> 2.23.0
RE: [EXT] [PATCH v1 2/3] tty: serial: lpuart: Use defines that correspond to correct register
From: Philippe Schenker Sent: Wednesday,
October 16, 2019 11:19 PM
> Use UARTMODIR defines instead of UARTMODEM as it is a 32-bit function
>
> Signed-off-by: Philippe Schenker
Reviewed-by: Fugang Duan
> ---
>
> drivers/tty/serial/fsl_lpuart.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c
> b/drivers/tty/serial/fsl_lpuart.c index
> f3271857621c..346b4a070ce9 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1879,10 +1879,10 @@ lpuart32_set_termios(struct uart_port *port,
> struct ktermios *termios,
> }
>
> if (termios->c_cflag & CRTSCTS) {
> - modem |= UARTMODEM_RXRTSE |
> UARTMODEM_TXCTSE;
> + modem |= (UARTMODIR_RXRTSE |
> UARTMODIR_TXCTSE);
> } else {
> termios->c_cflag &= ~CRTSCTS;
> - modem &= ~(UARTMODEM_RXRTSE |
> UARTMODEM_TXCTSE);
> + modem &= ~(UARTMODIR_RXRTSE |
> UARTMODIR_TXCTSE);
> }
>
> if (termios->c_cflag & CSTOPB)
> --
> 2.23.0
RE: [EXT] [PATCH v1 1/3] tty: serial: lpuart: Remove unnecessary code from set_mctrl
From: Philippe Schenker Sent: Wednesday,
October 16, 2019 11:19 PM
> Currently flow control is not working due to lpuart32_set_mctrl that is
> clearing TXCTSE bit in all cases. This bit gets earlier setup by
> lpuart32_set_termios.
>
> As I read in Documentation set_mctrl is also not meant for hardware flow
> control rather than gpio setting and clearing a RTS signal.
> Therefore I guess it is safe to remove the whole code in lpuart32_set_mctrl.
>
> This was tested with console on a i.MX8QXP SoC.
>
> Signed-off-by: Philippe Schenker
Reviewed-by: Fugang Duan
> ---
>
> drivers/tty/serial/fsl_lpuart.c | 11 ---
> 1 file changed, 11 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c
> b/drivers/tty/serial/fsl_lpuart.c index
> 537896c4d887..f3271857621c 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1333,18 +1333,7 @@ static void lpuart_set_mctrl(struct uart_port
> *port, unsigned int mctrl)
>
> static void lpuart32_set_mctrl(struct uart_port *port, unsigned int mctrl) {
> - unsigned long temp;
> -
> - temp = lpuart32_read(port, UARTMODIR) &
> - ~(UARTMODIR_RXRTSE |
> UARTMODIR_TXCTSE);
> -
> - if (mctrl & TIOCM_RTS)
> - temp |= UARTMODIR_RXRTSE;
> -
> - if (mctrl & TIOCM_CTS)
> - temp |= UARTMODIR_TXCTSE;
>
> - lpuart32_write(port, temp, UARTMODIR);
> }
>
> static void lpuart_break_ctl(struct uart_port *port, int break_state)
> --
> 2.23.0
RE: [PATCH 1/2] net: fec_main: Use platform_get_irq_byname_optional() to avoid error message
From: Anson Huang Sent: Wednesday, October 9, 2019 6:16 PM
> Failed to get irq using name is NOT fatal as driver will use index to get irq
> instead, use platform_get_irq_byname_optional() instead of
> platform_get_irq_byname() to avoid below error message during
> probe:
>
> [0.819312] fec 30be.ethernet: IRQ int0 not found
> [0.824433] fec 30be.ethernet: IRQ int1 not found
> [0.829539] fec 30be.ethernet: IRQ int2 not found
>
> Fixes: 7723f4c5ecdb ("driver core: platform: Add an error message to
> platform_get_irq*()")
> Signed-off-by: Anson Huang
Acked-by: Fugang Duan
> ---
> drivers/net/ethernet/freescale/fec_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index d4d4c72..22c01b2 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3558,7 +3558,7 @@ fec_probe(struct platform_device *pdev)
>
> for (i = 0; i < irq_cnt; i++) {
> snprintf(irq_name, sizeof(irq_name), "int%d", i);
> - irq = platform_get_irq_byname(pdev, irq_name);
> + irq = platform_get_irq_byname_optional(pdev, irq_name);
> if (irq < 0)
> irq = platform_get_irq(pdev, i);
> if (irq < 0) {
> --
> 2.7.4
RE: [PATCH 2/2] net: fec_ptp: Use platform_get_irq_xxx_optional() to avoid error message
From: Anson Huang Sent: Wednesday, October 9, 2019 6:16 PM
> Use platform_get_irq_byname_optional() and platform_get_irq_optional()
> instead of platform_get_irq_byname() and platform_get_irq() for optional
> IRQs to avoid below error message during probe:
>
> [0.795803] fec 30be.ethernet: IRQ pps not found
> [0.800787] fec 30be.ethernet: IRQ index 3 not found
>
> Fixes: 7723f4c5ecdb ("driver core: platform: Add an error message to
> platform_get_irq*()")
> Signed-off-by: Anson Huang
Acked-by: Fugang Duan
> ---
> drivers/net/ethernet/freescale/fec_ptp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index 19e2365..945643c 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -600,9 +600,9 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
>
> INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
>
> - irq = platform_get_irq_byname(pdev, "pps");
> + irq = platform_get_irq_byname_optional(pdev, "pps");
> if (irq < 0)
> - irq = platform_get_irq(pdev, irq_idx);
> + irq = platform_get_irq_optional(pdev, irq_idx);
> /* Failure to get an irq is not fatal,
>* only the PTP_CLOCK_PPS clock events should stop
>*/
> --
> 2.7.4
RE: [EXT] [PATCH v3] serial: imx: adapt rx buffer and dma periods
From: Philipp Puschmann Sent: Friday, September
20, 2019 3:06 PM
> Am 20.09.19 um 05:42 schrieb Andy Duan:
> > From: Philipp Puschmann Sent: Thursday,
> > September 19, 2019 10:51 PM
> >> Using only 4 DMA periods for UART RX is very few if we have a high
> >> frequency of small transfers - like in our case using Bluetooth with
> >> many small packets via UART - causing many dma transfers but in each
> >> only filling a fraction of a single buffer. Such a case may lead to
> >> the situation that DMA RX transfer is triggered but no free buffer is
> >> available. When this happens dma channel ist stopped - with the patch
> >> "dmaengine: imx-sdma: fix dma freezes" temporarily only - with the
> >> possible consequences that:
> >> with disabled hw flow control:
> >> If enough data is incoming on UART port the RX FIFO runs over and
> >> characters will be lost. What then happens depends on upper layer.
> >>
> >> with enabled hw flow control:
> >> If enough data is incoming on UART port the RX FIFO reaches a level
> >> where CTS is deasserted and remote device sending the data stops.
> >> If it fails to stop timely the i.MX' RX FIFO may run over and data
> >> get lost. Otherwise it's internal TX buffer may getting filled to
> >> a point where it runs over and data is again lost. It depends on
> >> the remote device how this case is handled and if it is recoverable.
> >>
> >> Obviously we want to avoid having no free buffers available. So we
> >> decrease the size of the buffers and increase their number and the total
> buffer size.
> >>
> >> Signed-off-by: Philipp Puschmann
> >> Reviewed-by: Lucas Stach
> >> ---
> >>
> >> Changelog v3:
> >> - enhance description
> >>
> >> Changelog v2:
> >> - split this patch from series "Fix UART DMA freezes for iMX6"
> >> - add Reviewed-by tag
> >>
> >> drivers/tty/serial/imx.c | 5 ++---
> >> 1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> index 87c58f9f6390..51dc19833eab 100644
> >> --- a/drivers/tty/serial/imx.c
> >> +++ b/drivers/tty/serial/imx.c
> >> @@ -1034,8 +1034,6 @@ static void imx_uart_timeout(struct timer_list
> *t)
> >> }
> >> }
> >>
> >> -#define RX_BUF_SIZE(PAGE_SIZE)
> >> -
> >> /*
> >> * There are two kinds of RX DMA interrupts(such as in the MX6Q):
> >> * [1] the RX DMA buffer is full.
> >> @@ -1118,7 +1116,8 @@ static void imx_uart_dma_rx_callback(void
> >> *data) }
> >>
> >> /* RX DMA buffer periods */
> >> -#define RX_DMA_PERIODS 4
> >> +#define RX_DMA_PERIODS 16
> >> +#define RX_BUF_SIZE(PAGE_SIZE / 4)
> >>
> > Why to decrease the DMA RX buffer size here ?
> >
> > The current DMA implementation support DMA cyclic mode, one SDMA BD
> > receive one Bluetooth frame can bring better performance.
> > As you know, for L2CAP, a maximum transmission unit (MTU) associated
> > with the largest Baseband payload is 341 bytes for DH5 packets.
> >
> > So I suggest to increase RX_BUF_SIZE along with RX_DMA_PERIODS to
> feasible value.
>
> I debugged and developed this patches on a system with a 4.15 kernel. When
> prepared for upstream i have adapted some details and missed a important
> thing here. It should say:
>
> +#define RX_BUF_SIZE(RX_DMA_PERIODS * PAGE_SIZE / 4)
>
> Yes, i wanted to increase the total buffer size too, even wrote it in the
> description.
> I will prepare a version 4, thanks for the hint.
Okay, thank you for submit the SDMA/uart patch set.
>
> Just for info: A single RX DMA period aka buffer can be filled with mutliple
> packets in regard of the upper layer, here BT.
Yes, that depends on system loading.
>
>
> Regards,
> Philipp
> >
> > Andy
> >
> >> static int imx_uart_start_rx_dma(struct imx_port *sport) {
> >> --
> >> 2.23.0
> >
RE: [EXT] [PATCH v3] serial: imx: adapt rx buffer and dma periods
From: Philipp Puschmann Sent: Thursday, September
19, 2019 10:51 PM
> Using only 4 DMA periods for UART RX is very few if we have a high frequency
> of small transfers - like in our case using Bluetooth with many small packets
> via UART - causing many dma transfers but in each only filling a fraction of a
> single buffer. Such a case may lead to the situation that DMA RX transfer is
> triggered but no free buffer is available. When this happens dma channel ist
> stopped - with the patch
> "dmaengine: imx-sdma: fix dma freezes" temporarily only - with the possible
> consequences that:
> with disabled hw flow control:
> If enough data is incoming on UART port the RX FIFO runs over and
> characters will be lost. What then happens depends on upper layer.
>
> with enabled hw flow control:
> If enough data is incoming on UART port the RX FIFO reaches a level
> where CTS is deasserted and remote device sending the data stops.
> If it fails to stop timely the i.MX' RX FIFO may run over and data
> get lost. Otherwise it's internal TX buffer may getting filled to
> a point where it runs over and data is again lost. It depends on
> the remote device how this case is handled and if it is recoverable.
>
> Obviously we want to avoid having no free buffers available. So we decrease
> the size of the buffers and increase their number and the total buffer size.
>
> Signed-off-by: Philipp Puschmann
> Reviewed-by: Lucas Stach
> ---
>
> Changelog v3:
> - enhance description
>
> Changelog v2:
> - split this patch from series "Fix UART DMA freezes for iMX6"
> - add Reviewed-by tag
>
> drivers/tty/serial/imx.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> 87c58f9f6390..51dc19833eab 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1034,8 +1034,6 @@ static void imx_uart_timeout(struct timer_list *t)
> }
> }
>
> -#define RX_BUF_SIZE(PAGE_SIZE)
> -
> /*
> * There are two kinds of RX DMA interrupts(such as in the MX6Q):
> * [1] the RX DMA buffer is full.
> @@ -1118,7 +1116,8 @@ static void imx_uart_dma_rx_callback(void
> *data) }
>
> /* RX DMA buffer periods */
> -#define RX_DMA_PERIODS 4
> +#define RX_DMA_PERIODS 16
> +#define RX_BUF_SIZE(PAGE_SIZE / 4)
>
Why to decrease the DMA RX buffer size here ?
The current DMA implementation support DMA cyclic mode, one SDMA BD receive one
Bluetooth frame can
bring better performance.
As you know, for L2CAP, a maximum transmission unit (MTU) associated with the
largest Baseband payload
is 341 bytes for DH5 packets.
So I suggest to increase RX_BUF_SIZE along with RX_DMA_PERIODS to feasible
value.
Andy
> static int imx_uart_start_rx_dma(struct imx_port *sport) {
> --
> 2.23.0
RE: [EXT] [PATCH v4 0/3] Fix UART DMA freezes for i.MX SOCs
From: Philipp Puschmann Sent: Thursday, September 19, 2019 10:30 PM > For some years and since many kernel versions there are reports that RX > UART DMA channel stops working at one point. So far the usual workaround > was to disable RX DMA. This patches fix the underlying problem. > > When a running sdma script does not find any usable destination buffer to put > its data into it just leads to stopping the channel being scheduled again. As > solution we manually retrigger the sdma script for this channel and by this > dissolve the freeze. > > While this seems to work fine so far, it may come to buffer overruns when the > channel - even temporary - is stopped. This case has to be addressed by > device drivers by increasing the number of DMA periods. > > This patch series was tested with the current kernel and backported to kernel > 4.15 with a special use case using a WL1837MOD via UART and provoking the > hanging of UART RX DMA within seconds after starting a test application. It > resulted in well known > "Bluetooth: hci0: command 0x0408 tx timeout" > errors and complete stop of UART data reception. Our Bluetooth traffic > consists of many independent small packets, mostly only a few bytes, causing > high usage of periods. > > Changelog v4: > - fixed the fixes tags > > Changelog v3: > - fixes typo in dma_wmb > - add fixes tags > > Changelog v2: > - adapt title (this patches are not only for i.MX6) > - improve some comments and patch descriptions > - add a dma_wb() around BD_DONE flag > - add Reviewed-by tags > - split off "serial: imx: adapt rx buffer and dma periods" > > Philipp Puschmann (3): > dmaengine: imx-sdma: fix buffer ownership > dmaengine: imx-sdma: fix dma freezes > dmaengine: imx-sdma: drop redundant variable > > drivers/dma/imx-sdma.c | 32 ++-- > 1 file changed, 22 insertions(+), 10 deletions(-) > > -- > 2.23.0 The patch set look fine that is really to fix some corner issue from the logical view. Reviewed-by: Fugang Duan
RE: [EXT] Re: [PATCH 0/4] Fix UART DMA freezes for iMX6
From: Philipp Puschmann Sent: Monday, September 16, 2019 9:55 PM > Hi Fabio, > > Am 12.09.19 um 20:23 schrieb Fabio Estevam: > > Hi Philipp, > > > > Thanks for submitting these fixes. > > > > On Wed, Sep 11, 2019 at 11:50 AM Philipp Puschmann > > wrote: > >> > >> For some years and since many kernel versions there are reports that > >> RX UART DMA channel stops working at one point. So far the usual > >> workaround was to disable RX DMA. This patches try to fix the underlying > problem. > >> > >> When a running sdma script does not find any usable destination > >> buffer to put its data into it just leads to stopping the channel > >> being scheduled again. As solution we we manually retrigger the sdma > >> script for this channel and by this dissolve the freeze. > >> > >> While this seems to work fine so far a further patch in this series > >> increases the number of RX DMA periods for UART to reduce use cases > >> running into such a situation. > >> > >> This patch series was tested with the current kernel and backported > >> to kernel 4.15 with a special use case using a WL1837MOD via UART and > >> provoking the hanging of UART RX DMA within seconds after starting a test > application. > >> It resulted in well known > >> "Bluetooth: hci0: command 0x0408 tx timeout" > >> errors and complete stop of UART data reception. Our Bluetooth > >> traffic consists of many independent small packets, mostly only a few > >> bytes, causing high usage of periods. > >> > >> > >> Philipp Puschmann (4): > >> dmaengine: imx-sdma: fix buffer ownership > >> dmaengine: imx-sdma: fix dma freezes > >> serial: imx: adapt rx buffer and dma periods > >> dmaengine: imx-sdma: drop redundant variable > > > > I have some suggestions: > > > > 1. Please split this in two series: one for dmaengine and other one > > for serial > > > > 2. Please add Fixes tag when appropriate, so that the fixes can be > > backported to stable kernels. > > > > 3. Please Cc Robin and Andy > > > > Thanks > > > > Thanks for the hints. I will apply them if the contentual feedback is > positive. > > p.s. Did you forget to add Andy? I don't see a Andy in the to- and cc-list. For dma and uart, please to- me and yibin.g...@nxp.com, thanks. Andy
RE: [PATCH v2 net-next] net: fec: add C45 MDIO read/write support
From: Marco Hartman Sent: Wednesday, August 21, 2019 7:44 PM
> IEEE 802.3ae clause 45 defines a modified MDIO protocol that uses a two
> staged access model in order to increase the address space.
>
> This patch adds support for C45 MDIO read and write accesses, which are
> used whenever the MII_ADDR_C45 flag in the regnum argument is set.
> In case it is not set, C22 accesses are used as before.
>
> Signed-off-by: Marco Hartmann
Acked-by: Fugang Duan
> ---
> Changes in v2:
> - use bool variable is_c45
> - add missing goto statements
> ---
> ---
> drivers/net/ethernet/freescale/fec_main.c | 70
> ---
> 1 file changed, 64 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index c01d3ec3e9af..cb3ce27fb27a 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -208,8 +208,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet
> MAC address");
>
> /* FEC MII MMFR bits definition */
> #define FEC_MMFR_ST (1 << 30)
> +#define FEC_MMFR_ST_C45 (0)
> #define FEC_MMFR_OP_READ (2 << 28)
> +#define FEC_MMFR_OP_READ_C45 (3 << 28)
> #define FEC_MMFR_OP_WRITE(1 << 28)
> +#define FEC_MMFR_OP_ADDR_WRITE (0)
> #define FEC_MMFR_PA(v) ((v & 0x1f) << 23)
> #define FEC_MMFR_RA(v) ((v & 0x1f) << 18)
> #define FEC_MMFR_TA (2 << 16)
> @@ -1767,7 +1770,8 @@ static int fec_enet_mdio_read(struct mii_bus *bus,
> int mii_id, int regnum)
> struct fec_enet_private *fep = bus->priv;
> struct device *dev = &fep->pdev->dev;
> unsigned long time_left;
> - int ret = 0;
> + int ret = 0, frame_start, frame_addr, frame_op;
> + bool is_c45 = !!(regnum & MII_ADDR_C45);
>
> ret = pm_runtime_get_sync(dev);
> if (ret < 0)
> @@ -1775,9 +1779,37 @@ static int fec_enet_mdio_read(struct mii_bus
> *bus, int mii_id, int regnum)
>
> reinit_completion(&fep->mdio_done);
>
> + if (is_c45) {
> + frame_start = FEC_MMFR_ST_C45;
> +
> + /* write address */
> + frame_addr = (regnum >> 16);
> + writel(frame_start | FEC_MMFR_OP_ADDR_WRITE |
> +FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
> +FEC_MMFR_TA | (regnum & 0x),
> +fep->hwp + FEC_MII_DATA);
> +
> + /* wait for end of transfer */
> + time_left = wait_for_completion_timeout(&fep->mdio_done,
> + usecs_to_jiffies(FEC_MII_TIMEOUT));
> + if (time_left == 0) {
> + netdev_err(fep->netdev, "MDIO address write timeout\n");
> + ret = -ETIMEDOUT;
> + goto out;
> + }
> +
> + frame_op = FEC_MMFR_OP_READ_C45;
> +
> + } else {
> + /* C22 read */
> + frame_op = FEC_MMFR_OP_READ;
> + frame_start = FEC_MMFR_ST;
> + frame_addr = regnum;
> + }
> +
> /* start a read op */
> - writel(FEC_MMFR_ST | FEC_MMFR_OP_READ |
> - FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
> + writel(frame_start | frame_op |
> + FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
> FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>
> /* wait for end of transfer */
> @@ -1804,7 +1836,8 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
> int mii_id, int regnum,
> struct fec_enet_private *fep = bus->priv;
> struct device *dev = &fep->pdev->dev;
> unsigned long time_left;
> - int ret;
> + int ret, frame_start, frame_addr;
> + bool is_c45 = !!(regnum & MII_ADDR_C45);
>
> ret = pm_runtime_get_sync(dev);
> if (ret < 0)
> @@ -1814,9 +1847,33 @@ static int fec_enet_mdio_write(struct mii_bus
> *bus, int mii_id, int regnum,
>
> reinit_completion(&fep->mdio_done);
>
> + if (is_c45) {
> + frame_start = FEC_MMFR_ST_C45;
> +
> + /* write address */
> + frame_addr = (regnum >> 16);
> + writel(frame_start | FEC_MMFR_OP_ADDR_WRITE |
> +FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
> +FEC_MMFR_TA | (regnum & 0x),
> +fep->hwp + FEC_MII_DATA);
> +
> + /* wait for end of transfer */
> + time_left = wait_for_completion_timeout(&fep->mdio_done,
> + usecs_to_jiffies(FEC_MII_TIMEOUT));
> + if (time_left == 0) {
> + netdev_err(fep->netdev, "MDIO address write timeout\n");
> + ret = -ETIMEDOUT;
> + goto out;
> + }
> + } else {
> + /* C22 write */
> + frame_start = FEC_MMFR_ST;
> + frame_addr = regnum;
> + }
> +
> /* start a write op */
> - writel(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
>
RE: [EXT] Re: [PATCH net-next 0/1] net: fec: add C45 MDIO read/write support
From: Andrew Lunn Sent: Tuesday, August 20, 2019 9:04 PM > On Tue, Aug 20, 2019 at 02:32:26AM +0000, Andy Duan wrote: > > From: Andrew Lunn > > > On Mon, Aug 19, 2019 at 05:11:14PM +, Marco Hartmann wrote: > > > > As of yet, the Fast Ethernet Controller (FEC) driver only supports > > > > Clause 22 conform MDIO transactions. IEEE 802.3ae Clause 45 > > > > defines a modified MDIO protocol that uses a two staged access > > > > model in order to increase the address space. > > > > > > > > This patch adds support for Clause 45 conform MDIO read and write > > > > operations to the FEC driver. > > > > > > Hi Marco > > > > > > Do all versions of the FEC hardware support C45? Or do we need to > > > make use of the quirk support in this driver to just enable it for some > revisions of FEC? > > > > > > Thanks > > > Andrew > > > > i.MX legacy platforms like i.MX6/7 series, they doesn't support Write & Read > Increment. > > But for i.MX8MQ/MM series, it support C45 full features like Write & Read > Increment. > > > > For the patch itself, it doesn't support Write & Read Increment, so I > > think the patch doesn't need to add quirk support. > > Hi Andy > > So what happens with something older than a i.MX8MQ/MM when a C45 > transfer is attempted? This patch adds a new write. Does that write > immediately trigger a completion interrupt? Does it never trigger an > interrupt, > and we have to wait FEC_MII_TIMEOUT? > > Ideally, if the hardware does not support C45, we want it to return > EOPNOTSUPP. > > Thanks > Andrew It still trigger an interrupt to wakeup the completion, we have to wait FEC_MII_TIMEOUT. Older chips just support part of C45 feature just like the patch implementation.
RE: [EXT] Re: [PATCH net-next 0/1] net: fec: add C45 MDIO read/write support
From: Andrew Lunn > On Mon, Aug 19, 2019 at 05:11:14PM +, Marco Hartmann wrote: > > As of yet, the Fast Ethernet Controller (FEC) driver only supports > > Clause 22 conform MDIO transactions. IEEE 802.3ae Clause 45 defines a > > modified MDIO protocol that uses a two staged access model in order to > > increase the address space. > > > > This patch adds support for Clause 45 conform MDIO read and write > > operations to the FEC driver. > > Hi Marco > > Do all versions of the FEC hardware support C45? Or do we need to make use > of the quirk support in this driver to just enable it for some revisions of > FEC? > > Thanks > Andrew i.MX legacy platforms like i.MX6/7 series, they doesn't support Write & Read Increment. But for i.MX8MQ/MM series, it support C45 full features like Write & Read Increment. For the patch itself, it doesn't support Write & Read Increment, so I think the patch doesn't need to add quirk support. Andy
RE: [PATCH net-next 1/1] fec: add C45 MDIO read/write support
From: Marco Hartmann Sent: Tuesday, August 20, 2019 1:11 AM
> IEEE 802.3ae clause 45 defines a modified MDIO protocol that uses a two
> staged access model in order to increase the address space.
>
> This patch adds support for C45 MDIO read and write accesses, which are
> used whenever the MII_ADDR_C45 flag in the regnum argument is set.
> In case it is not set, C22 accesses are used as before.
>
> Co-developed-by: Christian Herber
> Signed-off-by: Christian Herber
> Signed-off-by: Marco Hartmann
> ---
> drivers/net/ethernet/freescale/fec_main.c | 65
> ---
> 1 file changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index c01d3ec3e9af..73f8f9a149a1 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -208,8 +208,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet
> MAC address");
>
> /* FEC MII MMFR bits definition */
> #define FEC_MMFR_ST (1 << 30)
> +#define FEC_MMFR_ST_C45 (0)
> #define FEC_MMFR_OP_READ (2 << 28)
> +#define FEC_MMFR_OP_READ_C45 (3 << 28)
> #define FEC_MMFR_OP_WRITE(1 << 28)
> +#define FEC_MMFR_OP_ADDR_WRITE (0)
> #define FEC_MMFR_PA(v) ((v & 0x1f) << 23)
> #define FEC_MMFR_RA(v) ((v & 0x1f) << 18)
> #define FEC_MMFR_TA (2 << 16)
> @@ -1767,7 +1770,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus,
> int mii_id, int regnum)
> struct fec_enet_private *fep = bus->priv;
> struct device *dev = &fep->pdev->dev;
> unsigned long time_left;
> - int ret = 0;
> + int ret = 0, frame_start, frame_addr, frame_op;
Add bool variable:
bool is_c45 = !!(regnum & MII_ADDR_C45);
>
> ret = pm_runtime_get_sync(dev);
> if (ret < 0)
> @@ -1775,9 +1778,36 @@ static int fec_enet_mdio_read(struct mii_bus
> *bus, int mii_id, int regnum)
>
> reinit_completion(&fep->mdio_done);
>
> + if (MII_ADDR_C45 & regnum) {
if (is_c45)
> + frame_start = FEC_MMFR_ST_C45;
> +
> + /* write address */
> + frame_addr = (regnum >> 16);
> + writel(frame_start | FEC_MMFR_OP_ADDR_WRITE |
> +FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
> +FEC_MMFR_TA | (regnum & 0x),
> +fep->hwp + FEC_MII_DATA);
> +
> + /* wait for end of transfer */
> + time_left = wait_for_completion_timeout(&fep->mdio_done,
> + usecs_to_jiffies(FEC_MII_TIMEOUT));
> + if (time_left == 0) {
> + netdev_err(fep->netdev, "MDIO address write timeout\n");
> + ret = -ETIMEDOUT;
Should be:
goto out;
> + }
> +
> + frame_op = FEC_MMFR_OP_READ_C45;
> +
> + } else {
> + /* C22 read */
> + frame_op = FEC_MMFR_OP_READ;
> + frame_start = FEC_MMFR_ST;
> + frame_addr = regnum;
> + }
> +
> /* start a read op */
> - writel(FEC_MMFR_ST | FEC_MMFR_OP_READ |
> - FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
> + writel(frame_start | frame_op |
> + FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
> FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>
> /* wait for end of transfer */
> @@ -1804,7 +1834,7 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
> int mii_id, int regnum,
> struct fec_enet_private *fep = bus->priv;
> struct device *dev = &fep->pdev->dev;
> unsigned long time_left;
> - int ret;
> + int ret, frame_start, frame_addr;
>
> ret = pm_runtime_get_sync(dev);
> if (ret < 0)
> @@ -1814,9 +1844,32 @@ static int fec_enet_mdio_write(struct mii_bus
> *bus, int mii_id, int regnum,
bool is_c45 = !!(regnum & MII_ADDR_C45);
>
> reinit_completion(&fep->mdio_done);
>
> + if (MII_ADDR_C45 & regnum) {
if (!is_c45) {
> + frame_start = FEC_MMFR_ST_C45;
> +
> + /* write address */
> + frame_addr = (regnum >> 16);
> + writel(frame_start | FEC_MMFR_OP_ADDR_WRITE |
> +FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
> +FEC_MMFR_TA | (regnum & 0x),
> +fep->hwp + FEC_MII_DATA);
> +
> + /* wait for end of transfer */
> + time_left = wait_for_completion_timeout(&fep->mdio_done,
> + usecs_to_jiffies(FEC_MII_TIMEOUT));
> + if (time_left == 0) {
> + netdev_err(fep->netdev, "MDIO address write timeout\n");
> + ret = -ETIMEDOUT;
Like mdio read, it should be:
goto out;
> + }
> + } else {
> + /* C22 write */
> + frame_start = FEC_MMFR_ST;
> + frame_addr = regnum;
> + }
> +
> /* start a write op */
> - writel(FEC_MMFR_ST | FE
RE: [EXT] Re: [PATCH nvmem 1/1] nvmem: imx: add i.MX8QM platform support
From: Srinivas Kandagatla Sent: Tuesday,
August 6, 2019 6:04 PM
> On 04/07/2019 15:20, fugang.d...@nxp.com wrote:
> > From: Fugang Duan
> >
> > i.MX8QM efuse table has some difference with i.MX8QXP platform, so add
> > i.MX8QM platform support.
> >
> > Signed-off-by: Fugang Duan
> > ---
> > drivers/nvmem/imx-ocotp-scu.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/nvmem/imx-ocotp-scu.c
> > b/drivers/nvmem/imx-ocotp-scu.c index be2f5f0..0d78ab4 100644
> > --- a/drivers/nvmem/imx-ocotp-scu.c
> > +++ b/drivers/nvmem/imx-ocotp-scu.c
> > @@ -16,6 +16,7 @@
> >
> > enum ocotp_devtype {
> > IMX8QXP,
> > + IMX8QM,
> > };
> >
> > struct ocotp_devtype_data {
> > @@ -39,6 +40,11 @@ static struct ocotp_devtype_data imx8qxp_data = {
> > .nregs = 800,
> > };
> >
> > +static struct ocotp_devtype_data imx8qm_data = {
> > + .devtype = IMX8QM,
> > + .nregs = 800,
> > +};
> > +
> > static int imx_sc_misc_otp_fuse_read(struct imx_sc_ipc *ipc, u32 word,
> >u32 *val)
> > {
> > @@ -118,6 +124,7 @@ static struct nvmem_config
> > imx_scu_ocotp_nvmem_config = {
> >
> > static const struct of_device_id imx_scu_ocotp_dt_ids[] = {
> > { .compatible = "fsl,imx8qxp-scu-ocotp", (void *)&imx8qxp_data
> > },
> > + { .compatible = "fsl,imx8qm-scu-ocotp", (void *)&imx8qm_data },
> > { },
>
> Looks like you forgot to add this new compatible to device tree bindings
> at ./Documentation/devicetree/bindings/nvmem/imx-ocotp.txt or forgot to
> add me to CC.
>
> Please resend the patch with it, I can not apply this as it is.
>
> Thanks,
> srini
There have no separated binding documentation for imx-ocotp-scu.c driver.
It is reasonable to add the new compatible string on below binding file
"fsl,scu.txt":
Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>
> > };
> > MODULE_DEVICE_TABLE(of, imx_scu_ocotp_dt_ids);
> >
RE: [EXT] Re: [PATCH nvmem 1/1] nvmem: imx: add i.MX8QM platform support
From: Srinivas Kandagatla Sent: Tuesday,
August 6, 2019 6:04 PM
> On 04/07/2019 15:20, fugang.d...@nxp.com wrote:
> > From: Fugang Duan
> >
> > i.MX8QM efuse table has some difference with i.MX8QXP platform, so add
> > i.MX8QM platform support.
> >
> > Signed-off-by: Fugang Duan
> > ---
> > drivers/nvmem/imx-ocotp-scu.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/nvmem/imx-ocotp-scu.c
> > b/drivers/nvmem/imx-ocotp-scu.c index be2f5f0..0d78ab4 100644
> > --- a/drivers/nvmem/imx-ocotp-scu.c
> > +++ b/drivers/nvmem/imx-ocotp-scu.c
> > @@ -16,6 +16,7 @@
> >
> > enum ocotp_devtype {
> > IMX8QXP,
> > + IMX8QM,
> > };
> >
> > struct ocotp_devtype_data {
> > @@ -39,6 +40,11 @@ static struct ocotp_devtype_data imx8qxp_data = {
> > .nregs = 800,
> > };
> >
> > +static struct ocotp_devtype_data imx8qm_data = {
> > + .devtype = IMX8QM,
> > + .nregs = 800,
> > +};
> > +
> > static int imx_sc_misc_otp_fuse_read(struct imx_sc_ipc *ipc, u32 word,
> >u32 *val)
> > {
> > @@ -118,6 +124,7 @@ static struct nvmem_config
> > imx_scu_ocotp_nvmem_config = {
> >
> > static const struct of_device_id imx_scu_ocotp_dt_ids[] = {
> > { .compatible = "fsl,imx8qxp-scu-ocotp", (void *)&imx8qxp_data
> > },
> > + { .compatible = "fsl,imx8qm-scu-ocotp", (void *)&imx8qm_data },
> > { },
>
> Looks like you forgot to add this new compatible to device tree bindings
> at ./Documentation/devicetree/bindings/nvmem/imx-ocotp.txt or forgot to
> add me to CC.
>
> Please resend the patch with it, I can not apply this as it is.
>
> Thanks,
> srini
Thanks for your review.
I will send the V2 version including the separated device tree bindings patch.
>
> > };
> > MODULE_DEVICE_TABLE(of, imx_scu_ocotp_dt_ids);
> >
RE: [EXT] [PATCH v1] net: fec: optionally reset PHY via a reset-controller
From: Sven Van Asbroeck Sent: Wednesday, July 17, 2019 8:48 PM > On Tue, Jul 16, 2019 at 9:32 PM Andy Duan wrote: > > > > Yes, so the old legacy code is kept there. But it is better to clean > > up all if there have enough boards to verify them. > > Would it make sense to print a warning message to the log whenever > someone tries to use the legacy phy reset on the fec? It is feasible, just as reminder to drop such usage.
RE: [EXT] [PATCH v1] net: fec: optionally reset PHY via a reset-controller
From: Sven Van Asbroeck Sent: Tuesday, July 16, 2019 9:19 PM > Hi Andy, > > On Mon, Jul 15, 2019 at 10:02 PM Andy Duan > wrote: > > > > the phylib already can handle mii bus reset and phy device reset > > That's a great suggestion, thank you !! I completely overlooked that code. > What will happen to the legacy phy reset code in fec? Are there many users > left? Yes, so the old legacy code is kept there. But it is better to clean up all if there have enough boards to verify them.
RE: [EXT] [PATCH v1] net: fec: optionally reset PHY via a reset-controller
From: Sven Van Asbroeck Sent: Tuesday, July 16, 2019 5:05
AM
> The current fec driver allows the PHY to be reset via a gpio, specified in the
> devicetree. However, some PHYs need to be reset in a more complex way.
>
> To accommodate such PHYs, allow an optional reset controller in the fec
> devicetree. If no reset controller is found, the fec will fall back to the
> legacy
> reset behaviour.
>
> Example:
> &fec {
> phy-mode = "rgmii";
> resets = <&phy_reset>;
> reset-names = "phy";
> status = "okay";
> };
>
> Signed-off-by: Sven Van Asbroeck
> ---
>
> Will send a Documentation patch if this receives a positive review.
>
> drivers/net/ethernet/freescale/fec_main.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 38f10f7dcbc3..5a5f3ed6f16d 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -61,6 +61,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -3335,6 +3336,7 @@ static int fec_enet_get_irq_cnt(struct
> platform_device *pdev) static int fec_probe(struct platform_device *pdev)
> {
> + struct reset_control *phy_reset;
> struct fec_enet_private *fep;
> struct fec_platform_data *pdata;
> struct net_device *ndev;
> @@ -3490,7 +3492,9 @@ fec_probe(struct platform_device *pdev)
> pm_runtime_set_active(&pdev->dev);
> pm_runtime_enable(&pdev->dev);
>
> - ret = fec_reset_phy(pdev);
> + phy_reset = devm_reset_control_get_exclusive(&pdev->dev,
> "phy");
> + ret = IS_ERR(phy_reset) ? fec_reset_phy(pdev) :
> + reset_control_reset(phy_reset);
> if (ret)
> goto failed_reset;
The patch looks fine.
But the reset mechanism in the driver should be abandoned since
the phylib already can handle mii bus reset and phy device reset like
below piece code:
of_mdiobus_register()
of_mdiobus_register_phy()
phy_device_register()
mdiobus_register_device()
/* Assert the reset signal */
mdio_device_reset(mdiodev, 1);
/* Deassert the reset signal */
mdio_device_reset(&phydev->mdio, 0);
dts:
ethernet-phy@0 {
compatible = "ethernet-phy-id0141.0e90",
"ethernet-phy-ieee802.3-c45";
interrupt-parent = <&PIC>;
interrupts = <35 1>;
reg = <0>;
resets = <&rst 8>;
reset-names = "phy";
reset-gpios = <&gpio1 4 1>;
reset-assert-us = <1000>;
reset-deassert-us = <2000>;
};
};
Please refer to doc:
Documentation/devicetree/bindings/net/ethernet-phy.yaml
>
> --
> 2.17.1
RE: [EXT] Re: [PATCH nvmem 1/1] nvmem: imx: add i.MX8QM platform support
From: gre...@linuxfoundation.org Sent: Monday, July 15, 2019 3:37 PM > On Mon, Jul 15, 2019 at 05:34:47AM +0000, Andy Duan wrote: > > Ping... > > It's the middle of the merge window, we can't do anything with any patches > until after that. Please be patient. > > greg k-h Thanks for kindly reminder !
RE: [PATCH nvmem 1/1] nvmem: imx: correct the fuse word index
Ping ... > From: Fugang Duan > > iMX8 fuse word index represent as one 4-bytes word, it should not be divided > by 4. > > Exp: > - MAC0 address layout in fuse: > offset 708: MAC[3] MAC[2] MAC[1] MAC[0] > offset 709: XX xx MAC[5] MAC[4] > > Signed-off-by: Fugang Duan > --- > drivers/nvmem/imx-ocotp-scu.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/nvmem/imx-ocotp-scu.c b/drivers/nvmem/imx-ocotp-scu.c > index d9dc482..be2f5f0 100644 > --- a/drivers/nvmem/imx-ocotp-scu.c > +++ b/drivers/nvmem/imx-ocotp-scu.c > @@ -71,8 +71,8 @@ static int imx_scu_ocotp_read(void *context, unsigned > int offset, > void *p; > int i, ret; > > - index = offset >> 2; > - num_bytes = round_up((offset % 4) + bytes, 4); > + index = offset; > + num_bytes = round_up(bytes, 4); > count = num_bytes >> 2; > > if (count > (priv->data->nregs - index)) @@ -100,7 +100,7 @@ static int > imx_scu_ocotp_read(void *context, unsigned int offset, > buf++; > } > > - memcpy(val, (u8 *)p + offset % 4, bytes); > + memcpy(val, (u8 *)p, bytes); > > kfree(p); > > -- > 2.7.4
RE: [PATCH nvmem 1/1] nvmem: imx: add i.MX8QM platform support
Ping...
> From: Fugang Duan
>
> i.MX8QM efuse table has some difference with i.MX8QXP platform, so add
> i.MX8QM platform support.
>
> Signed-off-by: Fugang Duan
> ---
> drivers/nvmem/imx-ocotp-scu.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/nvmem/imx-ocotp-scu.c b/drivers/nvmem/imx-ocotp-scu.c
> index be2f5f0..0d78ab4 100644
> --- a/drivers/nvmem/imx-ocotp-scu.c
> +++ b/drivers/nvmem/imx-ocotp-scu.c
> @@ -16,6 +16,7 @@
>
> enum ocotp_devtype {
> IMX8QXP,
> + IMX8QM,
> };
>
> struct ocotp_devtype_data {
> @@ -39,6 +40,11 @@ static struct ocotp_devtype_data imx8qxp_data = {
> .nregs = 800,
> };
>
> +static struct ocotp_devtype_data imx8qm_data = {
> + .devtype = IMX8QM,
> + .nregs = 800,
> +};
> +
> static int imx_sc_misc_otp_fuse_read(struct imx_sc_ipc *ipc, u32 word,
>u32 *val)
> {
> @@ -118,6 +124,7 @@ static struct nvmem_config
> imx_scu_ocotp_nvmem_config = {
>
> static const struct of_device_id imx_scu_ocotp_dt_ids[] = {
> { .compatible = "fsl,imx8qxp-scu-ocotp", (void *)&imx8qxp_data },
> + { .compatible = "fsl,imx8qm-scu-ocotp", (void *)&imx8qm_data },
> { },
> };
> MODULE_DEVICE_TABLE(of, imx_scu_ocotp_dt_ids);
> --
> 2.7.4
RE: [EXT] Re: [PATCH nvmem 1/1] nvmem: imx: correct the fuse word index
From: Andy Duan Sent: Friday, July 5, 2019 3:33 PM
> From: Lothar Waßmann Sent: Friday, July 5, 2019
> 3:13 PM
> > Hi,
> >
> > On Fri, 5 Jul 2019 02:46:32 + Andy Duan wrote:
> > > From: Andy Duan Sent: Friday, July 5, 2019 12:08 AM
> > > > From: Lothar Waßmann Sent: Thursday,
> July
> > > > 4,
> > > > 2019 11:46 PM
> > > > > Hi,
> > > > >
> > > > > On Thu, 4 Jul 2019 22:20:15 +0800 fugang.d...@nxp.com wrote:
> > > > > > From: Fugang Duan
> > > > > >
> > > > > > iMX8 fuse word index represent as one 4-bytes word, it should
> > > > > > not be divided by 4.
> > > > > >
> > > > > > Exp:
> > > > > > - MAC0 address layout in fuse:
> > > > > > offset 708: MAC[3] MAC[2] MAC[1] MAC[0]
> > > > > > offset 709: XX xx MAC[5] MAC[4]
> > > > > >
> > > > > > Signed-off-by: Fugang Duan
> > > > > > ---
> > > > > > drivers/nvmem/imx-ocotp-scu.c | 6 +++---
> > > > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/nvmem/imx-ocotp-scu.c
> > > > > > b/drivers/nvmem/imx-ocotp-scu.c index d9dc482..be2f5f0 100644
> > > > > > --- a/drivers/nvmem/imx-ocotp-scu.c
> > > > > > +++ b/drivers/nvmem/imx-ocotp-scu.c
> > > > > > @@ -71,8 +71,8 @@ static int imx_scu_ocotp_read(void *context,
> > > > > > unsigned
> > > > > int offset,
> > > > > > void *p;
> > > > > > int i, ret;
> > > > > >
> > > > > > - index = offset >> 2;
> > > > > > - num_bytes = round_up((offset % 4) + bytes, 4);
> > > > > > + index = offset;
> > > > > > + num_bytes = round_up(bytes, 4);
> > > > > > count = num_bytes >> 2;
> > > > > >
> > > > > > if (count > (priv->data->nregs - index)) @@ -100,7
> > > > > > +100,7 @@ static int imx_scu_ocotp_read(void *context, unsigned
> int offset,
> > > > > > buf++;
> > > > > > }
> > > > > >
> > > > > > - memcpy(val, (u8 *)p + offset % 4, bytes);
> > > > > > + memcpy(val, (u8 *)p, bytes);
> > > > > >
> > > > > > kfree(p);
> > > > > >
> > > > > With these changes you could use the 'val' pointer directly as
> > > > > the destination for ocotp_read() without need for an intermediate
> buffer.
> > > > >
> > > > >
> > > > > Lothar Waßmann
> > > >
> > > > You are right, in fact, we can remove "p" and "buf" pointer.
> > > > Thanks for your review, I will send out the V2.
> > >
> > > Hi Lothar,
> > >
> > > It still need intermediate buffer to read out n words (n * 4 bytes)
> > > from
> > eFuse.
> > > Because 'val' buffer size is real count parsed from DT, which is not
> > > an integer
> > multiple of 4.
> > >
> > > For example, cell->bytes is parsed from "reg" property and it is
> > > real count
> > like 6.
> > > fec_mac0: mac@2c4 {
> > > reg = <0x2c4 6>;
> > > };
> > >
> > > So we have to use intermediate buffer here.
> > >
> > val is a u32 pointer, so legally it cannot point to any buffer whose
> > size is not divisible by four!
> >
> Yes, val is a u32 pointer, as my understand, it point to the 'buf' whose size
> is
> cell->bytes (the size is not parsed from 'reg' property)
>
> The piece of code:
> buf = kzalloc(cell->bytes, GFP_KERNEL);
> nvmem_reg_read(nvmem, cell->offset, buf, cell->bytes);
> imx_scu_ocotp_read(void *context, unsigned int offset, void *val, size_t
> bytes);
>
> If so, we still need intermediate buffer to read out eFuse words, and copy
> 'cell->bytes'
> bytes to 'val' buffer.
> May I understand wrong ? Thanks very much!
Lothar, any comment ? Thanks !
RE: [EXT] Re: [PATCH nvmem 1/1] nvmem: imx: correct the fuse word index
From: Lothar Waßmann Sent: Friday, July 5, 2019 3:13
PM
> Hi,
>
> On Fri, 5 Jul 2019 02:46:32 +0000 Andy Duan wrote:
> > From: Andy Duan Sent: Friday, July 5, 2019 12:08 AM
> > > From: Lothar Waßmann Sent: Thursday, July
> > > 4,
> > > 2019 11:46 PM
> > > > Hi,
> > > >
> > > > On Thu, 4 Jul 2019 22:20:15 +0800 fugang.d...@nxp.com wrote:
> > > > > From: Fugang Duan
> > > > >
> > > > > iMX8 fuse word index represent as one 4-bytes word, it should
> > > > > not be divided by 4.
> > > > >
> > > > > Exp:
> > > > > - MAC0 address layout in fuse:
> > > > > offset 708: MAC[3] MAC[2] MAC[1] MAC[0]
> > > > > offset 709: XX xx MAC[5] MAC[4]
> > > > >
> > > > > Signed-off-by: Fugang Duan
> > > > > ---
> > > > > drivers/nvmem/imx-ocotp-scu.c | 6 +++---
> > > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/nvmem/imx-ocotp-scu.c
> > > > > b/drivers/nvmem/imx-ocotp-scu.c index d9dc482..be2f5f0 100644
> > > > > --- a/drivers/nvmem/imx-ocotp-scu.c
> > > > > +++ b/drivers/nvmem/imx-ocotp-scu.c
> > > > > @@ -71,8 +71,8 @@ static int imx_scu_ocotp_read(void *context,
> > > > > unsigned
> > > > int offset,
> > > > > void *p;
> > > > > int i, ret;
> > > > >
> > > > > - index = offset >> 2;
> > > > > - num_bytes = round_up((offset % 4) + bytes, 4);
> > > > > + index = offset;
> > > > > + num_bytes = round_up(bytes, 4);
> > > > > count = num_bytes >> 2;
> > > > >
> > > > > if (count > (priv->data->nregs - index)) @@ -100,7 +100,7
> > > > > @@ static int imx_scu_ocotp_read(void *context, unsigned int offset,
> > > > > buf++;
> > > > > }
> > > > >
> > > > > - memcpy(val, (u8 *)p + offset % 4, bytes);
> > > > > + memcpy(val, (u8 *)p, bytes);
> > > > >
> > > > > kfree(p);
> > > > >
> > > > With these changes you could use the 'val' pointer directly as the
> > > > destination for ocotp_read() without need for an intermediate buffer.
> > > >
> > > >
> > > > Lothar Waßmann
> > >
> > > You are right, in fact, we can remove "p" and "buf" pointer.
> > > Thanks for your review, I will send out the V2.
> >
> > Hi Lothar,
> >
> > It still need intermediate buffer to read out n words (n * 4 bytes) from
> eFuse.
> > Because 'val' buffer size is real count parsed from DT, which is not an
> > integer
> multiple of 4.
> >
> > For example, cell->bytes is parsed from "reg" property and it is real count
> like 6.
> > fec_mac0: mac@2c4 {
> > reg = <0x2c4 6>;
> > };
> >
> > So we have to use intermediate buffer here.
> >
> val is a u32 pointer, so legally it cannot point to any buffer whose size is
> not
> divisible by four!
>
Yes, val is a u32 pointer, as my understand, it point to the 'buf' whose size
is cell->bytes
(the size is not parsed from 'reg' property)
The piece of code:
buf = kzalloc(cell->bytes, GFP_KERNEL);
nvmem_reg_read(nvmem, cell->offset, buf, cell->bytes);
imx_scu_ocotp_read(void *context, unsigned int offset, void *val, size_t bytes);
If so, we still need intermediate buffer to read out eFuse words, and copy
'cell->bytes'
bytes to 'val' buffer.
May I understand wrong ? Thanks very much!
RE: [EXT] Re: [PATCH nvmem 1/1] nvmem: imx: correct the fuse word index
From: Andy Duan Sent: Friday, July 5, 2019 12:08 AM
> From: Lothar Waßmann Sent: Thursday, July 4,
> 2019 11:46 PM
> > Hi,
> >
> > On Thu, 4 Jul 2019 22:20:15 +0800 fugang.d...@nxp.com wrote:
> > > From: Fugang Duan
> > >
> > > iMX8 fuse word index represent as one 4-bytes word, it should not be
> > > divided by 4.
> > >
> > > Exp:
> > > - MAC0 address layout in fuse:
> > > offset 708: MAC[3] MAC[2] MAC[1] MAC[0]
> > > offset 709: XX xx MAC[5] MAC[4]
> > >
> > > Signed-off-by: Fugang Duan
> > > ---
> > > drivers/nvmem/imx-ocotp-scu.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/nvmem/imx-ocotp-scu.c
> > > b/drivers/nvmem/imx-ocotp-scu.c index d9dc482..be2f5f0 100644
> > > --- a/drivers/nvmem/imx-ocotp-scu.c
> > > +++ b/drivers/nvmem/imx-ocotp-scu.c
> > > @@ -71,8 +71,8 @@ static int imx_scu_ocotp_read(void *context,
> > > unsigned
> > int offset,
> > > void *p;
> > > int i, ret;
> > >
> > > - index = offset >> 2;
> > > - num_bytes = round_up((offset % 4) + bytes, 4);
> > > + index = offset;
> > > + num_bytes = round_up(bytes, 4);
> > > count = num_bytes >> 2;
> > >
> > > if (count > (priv->data->nregs - index)) @@ -100,7 +100,7 @@
> > > static int imx_scu_ocotp_read(void *context, unsigned int offset,
> > > buf++;
> > > }
> > >
> > > - memcpy(val, (u8 *)p + offset % 4, bytes);
> > > + memcpy(val, (u8 *)p, bytes);
> > >
> > > kfree(p);
> > >
> > With these changes you could use the 'val' pointer directly as the
> > destination for ocotp_read() without need for an intermediate buffer.
> >
> >
> > Lothar Waßmann
>
> You are right, in fact, we can remove "p" and "buf" pointer.
> Thanks for your review, I will send out the V2.
Hi Lothar,
It still need intermediate buffer to read out n words (n * 4 bytes) from eFuse.
Because 'val' buffer size is real count parsed from DT, which is not an integer
multiple of 4.
For example, cell->bytes is parsed from "reg" property and it is real count
like 6.
fec_mac0: mac@2c4 {
reg = <0x2c4 6>;
};
So we have to use intermediate buffer here.
RE: [EXT] Re: [PATCH nvmem 1/1] nvmem: imx: correct the fuse word index
From: Lothar Waßmann Sent: Thursday, July 4, 2019 11:46 PM > Hi, > > On Thu, 4 Jul 2019 22:20:15 +0800 fugang.d...@nxp.com wrote: > > From: Fugang Duan > > > > iMX8 fuse word index represent as one 4-bytes word, it should not be > > divided by 4. > > > > Exp: > > - MAC0 address layout in fuse: > > offset 708: MAC[3] MAC[2] MAC[1] MAC[0] > > offset 709: XX xx MAC[5] MAC[4] > > > > Signed-off-by: Fugang Duan > > --- > > drivers/nvmem/imx-ocotp-scu.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/nvmem/imx-ocotp-scu.c > > b/drivers/nvmem/imx-ocotp-scu.c index d9dc482..be2f5f0 100644 > > --- a/drivers/nvmem/imx-ocotp-scu.c > > +++ b/drivers/nvmem/imx-ocotp-scu.c > > @@ -71,8 +71,8 @@ static int imx_scu_ocotp_read(void *context, unsigned > int offset, > > void *p; > > int i, ret; > > > > - index = offset >> 2; > > - num_bytes = round_up((offset % 4) + bytes, 4); > > + index = offset; > > + num_bytes = round_up(bytes, 4); > > count = num_bytes >> 2; > > > > if (count > (priv->data->nregs - index)) @@ -100,7 +100,7 @@ > > static int imx_scu_ocotp_read(void *context, unsigned int offset, > > buf++; > > } > > > > - memcpy(val, (u8 *)p + offset % 4, bytes); > > + memcpy(val, (u8 *)p, bytes); > > > > kfree(p); > > > With these changes you could use the 'val' pointer directly as the destination > for ocotp_read() without need for an intermediate buffer. > > > Lothar Waßmann You are right, in fact, we can remove "p" and "buf" pointer. Thanks for your review, I will send out the V2.
RE: [EXT] [PATCH 73/87] ethernet: freescale: Remove memset after dma_alloc_coherent
From: Fuqian Huang Sent: Friday, June 28, 2019 1:47
AM
> In commit af7ddd8a627c
> ("Merge tag 'dma-mapping-4.21' of
> git://git.infradead.org/users/hch/dma-mapping"),
> dma_alloc_coherent has already zeroed the memory.
> So memset is not needed.
>
> Signed-off-by: Fuqian Huang
Acked-by: Fugang Duan
> ---
> drivers/net/ethernet/freescale/fec_main.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 38f10f7dcbc3..ec87b8b78d21 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3143,8 +3143,6 @@ static int fec_enet_init(struct net_device *ndev)
> return -ENOMEM;
> }
>
> - memset(cbd_base, 0, bd_size);
> -
> /* Get the Ethernet address */
> fec_get_mac(ndev);
> /* make sure MAC we just acquired is programmed into the hw */
> --
> 2.11.0
RE: [EXT] Re: Issue: regmap: use debugfs even when no device
From: Mark Brown Sent: Wednesday, May 29, 2019 7:16 PM > To: Andy Duan > Cc: da...@lechnology.com; raf...@kernel.org; Robby Cai > ; gre...@linuxfoundation.org; linux-kernel > > Subject: Re: [EXT] Re: Issue: regmap: use debugfs even when no device > > On Wed, May 29, 2019 at 01:33:46AM +, Andy Duan wrote: > > > Correct, regmap without device also has issue when power if off, > > because regmap doesn't implement runtime pm for the device, but maybe > > device driver implement the runtime pm for the device. > > > So regmap how to manage the clock and power when access registers by > debugfs ? > > Like I say the basic recommendation is to use a cache. Got it, thanks.
RE: [EXT] Re: Issue: regmap: use debugfs even when no device
From: Mark Brown Sent: Tuesday, May 28, 2019 9:27 PM > On Tue, May 28, 2019 at 02:20:15AM +0000, Andy Duan wrote: > > > So on i.MX8MM/8QM/8QXP platforms, we catch the issue that user dump > > regmap registers without power cause system hang. > > Maybe revert the patch is more reasonable ? > > This is an issue with or without a device - you can have the same issue with > devices that are powered off. Typically where power is dynamic the driver > will use a register cache so the registers are always available. Correct, regmap without device also has issue when power if off, because regmap doesn't implement runtime pm for the device, but maybe device driver implement the runtime pm for the device. So regmap how to manage the clock and power when access registers by debugfs ? Andy
RE: [EXT] Re: [PATCHv2 1/2] ARM: dts: imx6sx: Use MX6SX_CLK_ENET for fec 'ahb' clock
From: Fabio Estevam > Hi Andy, > > On Sun, May 5, 2019 at 5:15 AM Andy Duan wrote: > > > Nack the patch ! > > ... > > Secondly, for your issue you caught, which was fixed by patch: > > commit d7c3a206e6338e4ccdf030719dec028e26a521d5 > > Author: Andy Duan > > Date: Tue Apr 9 03:40:56 2019 + > > > > net: fec: manage ahb clock in runtime pm > > Would this also fix the case where power management support is disabled? > > If I understand correctly the explanation from Kay-Liu he would still see a > hang in the case when PM is disabled. > > Thanks From current design, it still work even if disable PM. Please double check it. Andy
RE: [PATCHv2 1/2] ARM: dts: imx6sx: Use MX6SX_CLK_ENET for fec 'ahb' clock
From: Aisheng Dong Sent: Sunday, May 5, 2019 4:03 PM > > From: Fabio Estevam [mailto:feste...@gmail.com] > > Sent: Saturday, May 4, 2019 7:04 PM > > > > Hi Kay-Liu, > > > > On Thu, Apr 25, 2019 at 8:09 AM wrote: > > > > > > From: Kay-Liu > > > > > > The imx6sx's dts file defines five clocks for fec, the 'ahb'clock's > > > value is IMX6SX_CLK_ENET_AHB, but in the i.MX6SX Reference Manual > > > there is no such enet ahb clock, there is only one "enet clock" in > > > the > > > CCM_CCGR3 register which is controlled by bits 5-4, the enet clock > > > is defined for the 'ipg' clock, this can cause problem. > > > The original phenomenon is using imx6-solox processor and Marvel > > > 88E6390 switch with linux OS, the kernel will hang during the > > > startup of the linux OS. > > > After analyzing the phenomenon, the reason of CPU hang is read/write > > > enet module's register when the enet clock is disabled. The kernel > > > code try to avoids the problem by resume enet clock before > > > read/write enet register. > > > But the enet module's clock config will cause a special environment > > > which can bypass the clock resume mechanism. > > > The CPU has only one enet clock, after kernel parses the dts file, > > > the two clock variables 'ipg' and 'ahb' > > > finnaly point to the same enet clock register. This will cause enet > > > clock be disabled after fec probe over. > > > Because the power saving module will affect the BUG, so there are > > > two situations for this problem: > > > 1)Turn off power saving > > > Turn off power saving means that the resume mechanism is disabled, > > > so after fec probe over if any one read/write enet module's > > > register, the CPU will hang because no one could resume the enet clock. > > > 2)Turn on power saving > > > Turn on power saving could resume enet clock before read/write enet > > > register by enable 'ipg' clk, this will cause 'ahb' variable state > > > and enet clock register value don't match.If any task read/write > > > enet at a high frequently, the kernel will keep resume state and > > > never enter suspend process, this means that the kernel will only > > > modifies the register value during the first resume. > > > But the kernel init will check unused clock variable in the late > > > initcall, the 'ahb' clock will be treated as unused, at this time, > > > the enet clock will be disabled bypass the resume mechanism, then > > > the next read/write enet module's register will cause the CPU hang. > > > Proposed solution is delete the 'ahb' clock's definition in the > > > clk-imx6sx.c, and modify fec device’s clocks in the dts file, point > > > ‘ahb’ from IMX6SX_CLK_ENET_AHB to IMX6SX_CLK_ENET > > > > > > Signed-off-by: Kay-Liu > > > --- > > > Change since v1: > > > -inproved commit log description > > > -add platform related clock change instead of describe is in the > > > external URL > > > > > > arch/arm/boot/dts/imx6sx.dtsi | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/arm/boot/dts/imx6sx.dtsi > > > b/arch/arm/boot/dts/imx6sx.dtsi index 5b16e65..b8b23a6 100644 > > > --- a/arch/arm/boot/dts/imx6sx.dtsi > > > +++ b/arch/arm/boot/dts/imx6sx.dtsi > > > @@ -919,7 +919,7 @@ > > > interrupts = > IRQ_TYPE_LEVEL_HIGH>, > > > > IRQ_TYPE_LEVEL_HIGH>; > > > clocks = <&clks > IMX6SX_CLK_ENET>, > > > - <&clks > > IMX6SX_CLK_ENET_AHB>, > > > +<&clks > IMX6SX_CLK_ENET>, > > > > Yes, there is really no IMX6SX_CLK_ENET_AHB as per the Refernce Manual > > and it is the same we do on imx6qdl.dtsi: > > > > Reviewed-by: Fabio Estevam Nack the patch ! Firstly, i.MX6SX has ENET AHB bus clock for MAC, and currently it is set 200Mhz like clock tree: IMX6SX_CLK_ENET_PODF 200Mhz -> IMX6SX_CLK_ENET_SEL -> IMX6SX_CLK_ENET_AHB IMX6SX_CLK_ENET the clock is IPG clock for ENET IP ipg_clk_mac0_s/ipg_clk_s. (Please check RM Table 18-3. System Clocks, Gating, and Override) Secondly, for your issue you caught, which was fixed by patch: commit d7c3a206e6338e4ccdf030719dec028e26a521d5 Author: Andy Duan Date: Tue Apr 9 03:40:56 2019 + net: fec: manage ahb clock in runtime pm Some SOC like i.MX6SX clock have some limits: - ahb clock should be disabled before ipg. - ahb and ipg clocks are required for MAC MII bus. So, move the ahb clock to runtime management together with ipg clock. Signed-off-by: Fugang Duan Signed-off-by: David S. Miller So, please don't remove ahb clock. Andy > > Copy Andy, the ENET owner, to comment. > > BTW, it's strange that I did not receive the original patch email. > Also can't grep from the open list. > > Regards > Dong Aisheng
RE: [PATCH] net: fec: get regulator optional
From: Stefan Agner Sent: Monday, January 21, 2019 10:59 PM
> According to the device tree binding the phy-supply property is optional. Use
> the regulator_get_optional API accordingly. The code already handles NULL
> just fine.
>
> This gets rid of the following warning:
> fec 2188000.ethernet: 2188000.ethernet supply phy not found, using
> dummy regulator
>
> Signed-off-by: Stefan Agner
Thanks.
Acked-by: Fugang Duan
> ---
> drivers/net/ethernet/freescale/fec_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 6db69ba30dcd..c8315d880c8c 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3479,7 +3479,7 @@ fec_probe(struct platform_device *pdev)
> if (ret)
> goto failed_clk_ipg;
>
> - fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
> + fep->reg_phy = devm_regulator_get_optional(&pdev->dev, "phy");
> if (!IS_ERR(fep->reg_phy)) {
> ret = regulator_enable(fep->reg_phy);
> if (ret) {
> --
> 2.20.1
RE: iMX6 FEC driver Linux-fslc 4.17 - IPV6 Multicast not working when unplugging/plugging ethernet cable
From: Stefano Cappa > Hi everyone, > I already posted this in NXP forum as a comment > (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fco > mmunity.nxp.com%2Fthread%2F359397&data=02%7C01%7Cfugang.dua > n%40nxp.com%7C189d5cad534e470a162508d66c068de2%7C686ea1d3bc2b > 4c6fa92cd99c5c301635%7C0%7C0%7C636815172743636032&sdata=fb > H1wYnfrMI4437OIeAEDG1W23Ry6S3fscNqhHD8Kas%3D&reserved=0), > in yocto mailing list > (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists > .yoctoproject.org%2Fpipermail%2Fyocto%2F2018-December%2F043664.html > &data=02%7C01%7Cfugang.duan%40nxp.com%7C189d5cad534e470a1 > 62508d66c068de2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C > 636815172743636032&sdata=Tomd6ITYFIQ5M8NWzSMJG3N2llTCJqQU > Qg7oSY5zs4s%3D&reserved=0) > and in meta-freescale mailing list > (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists > .yoctoproject.org%2Fpipermail%2Fmeta-freescale%2F2018-December%2F02 > 3625.html&data=02%7C01%7Cfugang.duan%40nxp.com%7C189d5cad5 > 34e470a162508d66c068de2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > %7C0%7C636815172743636032&sdata=WPGDb3B3P3nBGdkKbrgeW3ls > Po0jVrQmxk9UlfFIkCw%3D&reserved=0) > A user in meta-freescale's mailing list suggested to resend this message to > the > emails obtained running "./scripts/get_maintainer.pl -F > drivers/net/ethernet/freescale/fec_main.c". > > > The problem is: > > If I boot my iMX6 device with ethernet cable attached and I execute "ping6 > ff02::fb" to ping the multicast address I get this response: > ~# ping6 ff02::fb > PING ff02::fb (ff02::fb): 56 data bytes > 64 bytes from fe80::c2f:eff:fe11:2d71: seq=0 ttl=64 time=2.057 ms > 64 bytes from fe80::809:1bfb:8d4c:ae54: seq=0 ttl=64 time=73.101 ms > (DUP!) > 64 bytes from fe80::3e28:6dff:feed:5b97: seq=0 ttl=64 time=150.772 ms > (DUP!) > > > Otherwise, If I unplug and plug again ethernet cable, I cannot ping the > multicast ipv6 address anymore. > The result is: > ~# ping6 ff02::fb > PING ff02::fb (ff02::fb): 56 data bytes > ping6: sendto: Network is unreachable > > > The original NXP discussion was about older version of Linux, however this > issue is happening with both Linux 4.9.88 and Linux 4.17. > Probably also with the latest version, but I didn't try. > I just test it on L4.14 kernel, it works as blow log. If you unplug and plug the ethernet cable, you should see the log print out: Unplug: IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready Plug:IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready Please check the link status in your local. Log: root@imx8mqevk:~# ping6 ff02::fb PING ff02::fb (ff02::fb): 56 data bytes 64 bytes from fe80::219:19ff:fe81:9149: seq=0 ttl=64 time=0.208 ms 64 bytes from fe80::be30:5bff:feeb:81f1: seq=0 ttl=64 time=1.586 ms (DUP!) 64 bytes from fe80::baac:6fff:fe37:e4d9: seq=0 ttl=64 time=1.611 ms (DUP!) 64 bytes from fe80::85da:f9d1:6bbc:f86d: seq=0 ttl=64 time=1.633 ms (DUP!) 64 bytes from fe80::38b2:b24:bc4a:1475: seq=0 ttl=64 time=1.653 ms (DUP!) 64 bytes from fe80::204:9fff:fe02:e33d: seq=0 ttl=64 time=1.673 ms (DUP!) 64 bytes from fe80::baac:6fff:fe37:e62f: seq=0 ttl=64 time=1.692 ms (DUP!) 64 bytes from fe80::cf53:3c0e:3d04:87c6: seq=0 ttl=64 time=1.712 ms (DUP!) 64 bytes from fe80::204:9fff:fe02:7786: seq=0 ttl=64 time=1.732 ms (DUP!) 64 bytes from fe80::f781:3862:65df:dadd: seq=0 ttl=64 time=1.752 ms (DUP!) 64 bytes from fe80::f69b:5750:754b:4583: seq=0 ttl=64 time=1.771 ms (DUP!) 64 bytes from fe80::204:9fff:fe02:c5eb: seq=0 ttl=64 time=1.791 ms (DUP!) 64 bytes from fe80::204:9fff:fe02:556a: seq=0 ttl=64 time=1.811 ms (DUP!) 64 bytes from fe80::20e:c6ff:fea6:6880: seq=0 ttl=64 time=1.830 ms (DUP!) 64 bytes from fe80::82dc:2912:4288:c88f: seq=0 ttl=64 time=1.850 ms (DUP!) 64 bytes from fe80::1479:ad62:9b34:1e3e: seq=0 ttl=64 time=1.869 ms (DUP!) 64 bytes from fe80::972f:1cc8:846f:85e2: seq=0 ttl=64 time=1.889 ms (DUP!) 64 bytes from fe80::62ae:d015:196e:5d76: seq=0 ttl=64 time=1.908 ms (DUP!) 64 bytes from fe80::204:9fff:fe03:c37e: seq=0 ttl=64 time=2.439 ms (DUP!) 64 bytes from fe80::204:9fff:fe05:cf0f: seq=0 ttl=64 time=2.462 ms (DUP!) 64 bytes from fe80::204:9fff:fe05:cf11: seq=0 ttl=64 time=3.550 ms (DUP!) ^C --- ff02::fb ping statistics --- 1 packets transmitted, 1 packets received, 20 duplicates, 0% packet loss round-trip min/avg/max = 0.208/1.829/3.550 ms root@imx8mqevk:~# [ 2338.265902] fec 30be.ethernet eth0: Link is Down [ 2338.298193] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready root@imx8mqevk:~# [ 2344.410482] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready [ 2344.418129] fec 30be.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx root@imx8mqevk:~# ping6 ff02::fb PING ff02::fb (ff02::fb): 56 data bytes 64 bytes from fe80::219:19ff:fe81:9149: seq=0 ttl=64 time=0.158 ms 64 bytes from fe80::be30:5bff:feeb:81f1: seq=0 ttl=64 time=1.853 ms (DUP!) 64 bytes from fe80::38b2:b24:bc4a:1475: seq=0 ttl=64 time=1.879 ms (DUP!) 64 bytes from fe80
RE: [PATCH] serial: imx: restore handshaking irq for imx1
From: Uwe Kleine-König Sent: 2018年9月20日 20:11
> Back in 2015 when irda was dropped from the driver imx1 was broken.
> This change reintroduces the support for the third interrupt of the UART.
>
> Fixes: afe9cbb1a6ad ("serial: imx: drop support for IRDA")
> Signed-off-by: Uwe Kleine-König
> ---
> drivers/tty/serial/imx.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> 4e853570ea80..554a69db1bca 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2350,6 +2350,14 @@ static int imx_uart_probe(struct
> platform_device *pdev)
> ret);
> return ret;
> }
> +
> + ret = devm_request_irq(&pdev->dev, rtsirq, imx_uart_rtsint, 0,
> +dev_name(&pdev->dev), sport);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request rts irq: %d\n",
> + ret);
> + return ret;
> + }
> } else {
> ret = devm_request_irq(&pdev->dev, rxirq, imx_uart_int, 0,
> dev_name(&pdev->dev), sport);
> --
> 2.18.0
Reviewed-by: Fugang Duan
RE: [PATCH v1] tty: serial: imx: enable IDDMAEN for the last tail data
From: Lucas Stach Sent: 2018年9月20日 18:42 > Am Donnerstag, den 20.09.2018, 10:33 + schrieb Andy Duan: > > From: Lucas Stach Sent: 2018年9月20日 > 17:40 > > > Am Donnerstag, den 20.09.2018, 08:39 + schrieb Robin Gong: > > > > > -Original Message- > > > > > From: Uwe Kleine-König > > > > > Sent: 2018年9月20日 15:55 > > > > > To: Robin Gong > > > > > Cc: jsl...@suse.com; Andy Duan ; > > > > > linux-ser...@vger.kernel.org; linux-kernel@vger.kernel.org; dl- > > > > > linux-imx ; ker...@pengutronix.de > > > > > Subject: Re: [PATCH v1] tty: serial: imx: enable IDDMAEN for the > > > > > last tail data > > > > > > > > > > Hello, > > > > > > > > > > On Thu, Sep 20, 2018 at 11:26:00PM +0800, Robin Gong wrote: > > > > > > enable IDDMAEN in UCR4 to let sdma script has the chance to > > > > > > detect the idle status and transfer the last tail data with > > > > > > the interrupt notifying uart driver.Otherwise, the last dma > > > > > > done interrupt of the tail data in rxfifo whose size is less > > > > > > than watermark may never be received by uart driver. > > > > > > > > > > Is this a theoretic issue? Or does it fix a real problem? > > > > > > > > > > If the former I'd say UCR1_ATDMAEN being set should prevent this > > > > > stall. > > > > > > > > It's a real fix, you will see 'timeout We read 0 byte' with the > > > > attached stress test running on i.mx7d or i.mx6 if no this patch > > > > applied. > > > > ' while [ 1 ]; do /unit_tests/UART/mxc_uart_stress_test.out > > > > /dev/ttymxc5 200 D L 100 100 L; done &' > > > > Please make sure dma enabled on ttymxcX for your test, and sdma > > > > firmware setup in your rootfs /lib/firmware/imx/sdma/sdma- > > > > imx6q.bin > > > > > > or > > > > /lib/firmware/imx/sdma/sdma- imx7d.bin. > > > > You can get sdma firmware from below: > > > > > > > > > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > gi > > > t > > > > .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ffirmware%2 > Flin > > > > > > ux-& > > > > ;data=02%7C01%7Cfugang.duan%40nxp.com%7C213076f6d111414 > 8b5 > > > > > > ce08d61edcfb > > > > > > > > > > > 88%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636730331 > > > 814804545& > > > > ;sdata=d0uZMjLc%2FrIae3Wd6PWtX2gEGlvZcUjPBVqKK9550jg%3D& > amp > > > > > > ;reserved=0 > > > > firmware.git/tree/imx/sdma > > > > > > Please see commit 905c0decad28402aa166975023fb88c8f62f93c8 > on why > > > using the idle detect together with with SDMA is wrong. We don't > > > want to re- introduce the broken behavior. > > > > > > > The patch just want to use idle timer to stop SDMA transfer using the > > new ram script, whose action is not the same as rom script. > > Sdma rom script how to stop sdma transfer if only one byte in rx FIFO > > that trigger SDMA request ? > > Right, and using the idle detect doesn't work with the ROM script as this > doesn't read the remaining bytes in the FIFO when the idle detect > condition is hit. > > Instead with the ROM script the aging timer is used to terminate the DMA > transfer. So if 1 byte is sitting in the FIFO below the watermark level, the > serial core will trigger a DMA request when the aging time is hit. The ROM > script checks the aging status and reads the remaining bytes from the FIFO, > then terminates the transfer. This is all documented in the reference > manual. > > I fail to see why this shouldn't work with the RAM script. If it doesn't work > with the RAM firmware, this script is violating the interface between the > serial core and the script in an incompatible way and needs fixing. > If rom script stop DMA transfer when SDMA is triggered by aging timer, then idler timer is not needed. And the data copy action should have no problem. I will do stress test with SDMA rom script. For ram script, there have many changes and different action with rom script. Aging timer trigger cannot stop current SDMA transfer, so use idler timer as the stop condition trigger source. We will do more test on rom script, if pass our all test cases, we can align ram script with rom script. Thanks. Andy
RE: [PATCH v1] tty: serial: imx: enable IDDMAEN for the last tail data
From: Lucas Stach Sent: 2018年9月20日 17:40 > Am Donnerstag, den 20.09.2018, 08:39 + schrieb Robin Gong: > > > -Original Message- > > > From: Uwe Kleine-König > > > Sent: 2018年9月20日 15:55 > > > To: Robin Gong > > > Cc: jsl...@suse.com; Andy Duan ; > > > linux-ser...@vger.kernel.org; linux-kernel@vger.kernel.org; dl- > > > linux-imx ; ker...@pengutronix.de > > > Subject: Re: [PATCH v1] tty: serial: imx: enable IDDMAEN for the > > > last tail data > > > > > > Hello, > > > > > > On Thu, Sep 20, 2018 at 11:26:00PM +0800, Robin Gong wrote: > > > > enable IDDMAEN in UCR4 to let sdma script has the chance to detect > > > > the idle status and transfer the last tail data with the interrupt > > > > notifying uart driver.Otherwise, the last dma done interrupt of > > > > the tail data in rxfifo whose size is less than watermark may > > > > never be received by uart driver. > > > > > > Is this a theoretic issue? Or does it fix a real problem? > > > > > > If the former I'd say UCR1_ATDMAEN being set should prevent this > > > stall. > > > > It's a real fix, you will see 'timeout We read 0 byte' with the > > attached stress test running on i.mx7d or i.mx6 if no this patch > > applied. > > ' while [ 1 ]; do /unit_tests/UART/mxc_uart_stress_test.out > > /dev/ttymxc5 200 D L 100 100 L; done &' > > Please make sure dma enabled on ttymxcX for your test, and sdma > > firmware setup in your rootfs /lib/firmware/imx/sdma/sdma-imx6q.bin > or > > /lib/firmware/imx/sdma/sdma- imx7d.bin. > > You can get sdma firmware from below: > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi > t > > .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ffirmware%2Flin > ux-& > > ;data=02%7C01%7Cfugang.duan%40nxp.com%7C213076f6d1114148b5 > ce08d61edcfb > > > 88%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636730331 > 814804545& > > ;sdata=d0uZMjLc%2FrIae3Wd6PWtX2gEGlvZcUjPBVqKK9550jg%3D& > ;reserved=0 > > firmware.git/tree/imx/sdma > > Please see commit 905c0decad28402aa166975023fb88c8f62f93c8 on > why using the idle detect together with with SDMA is wrong. We don't > want to re- introduce the broken behavior. > The patch just want to use idle timer to stop SDMA transfer using the new ram script, whose action is not the same as rom script. Sdma rom script how to stop sdma transfer if only one byte in rx FIFO that trigger SDMA request ?
RE: [PATCH -next] serial: imx: remove set but not used variable 'rtsirq'
From: Leonard Crestez Sent: 2018年9月20日 16:51
> On Thu, 2018-09-20 at 08:45 +0200, Jiri Slaby wrote:
> > On 09/20/2018, 03:58 AM, YueHaibing wrote:
> > > Fixes gcc '-Wunused-but-set-variable' warning:
> > >
> > > drivers/tty/serial/imx.c: In function 'imx_uart_probe':
> > > drivers/tty/serial/imx.c:2198:20: warning:
> > > variable 'rtsirq' set but not used [-Wunused-but-set-variable]
> > >
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c @@
> > > -2220,7 +2220,6 @@ static int imx_uart_probe(struct
> platform_device
> > > *pdev)
> > >
> > > rxirq = platform_get_irq(pdev, 0);
> > > txirq = platform_get_irq(pdev, 1);
> > > - rtsirq = platform_get_irq(pdev, 2);
> >
> > I am not sure this is correct. platform_get_irq has side effects (like
> > enabling the IRQ). Are you sure this won't change the behaviour (this
> > is question mostly to IMX fellows)?
>
> As far as I can tell there was a request_irq call for rtsirq which was
> removed by mistake in commit afe9cbb1a6ad ("serial: imx: drop support
> for IRDA"):
>
> - /* do not use RTS IRQ on IrDA */
> - if (!USE_IRDA(sport)) {
> - ret = devm_request_irq(&pdev->dev, rtsirq,
> - imx_rtsint, 0,
> -
> dev_name(&pdev->dev), sport);
> - if (ret)
> - return ret;
> - }
>
> This should have just removed the IRDA check and request rtsirq
> unconditionally. Nobody noticed this by testing RTS on imx1, this is an old
> chip and later variants have a single combined irq.
>
> The correct fix for the warning would be to restore that request_irq.
>
> --
> Regards,
> Leonard
Yes, your explain is very correct! Thanks for your comment.
We should restore rtsirq request that for i.MX1.
Regards,
Andy Duan
RE: [RFC] Configure i.MX6 RGMII pad group control registers from device tree
From: A.s. Dong Sent: 2018年6月24日 12:24 > Copy Andy & Frank, > > > -Original Message- > > From: Michal Vokáč [mailto:michal.vo...@ysoft.com] > > Sent: Tuesday, June 12, 2018 11:10 PM > > To: linux-g...@vger.kernel.org > > Cc: linux-arm-ker...@lists.infradead.org; Shawn Guo > > ; Sascha Hauer ; > Fabio > > Estevam ; Rob Herring > ; > > devicet...@vger.kernel.org; A.s. Dong ; > Fabio > > Estevam ; Shawn Guo ; > Stefan > > Agner ; Pengutronix Kernel Team > > ; Linus Walleij ; > > linux- ker...@vger.kernel.org > > Subject: Re: [RFC] Configure i.MX6 RGMII pad group control registers > > from device tree > > > > On 11.6.2018 14:36, Michal Vokáč wrote: > > > Ahoj, > > > > > > To configure individual pad's characteristics on i.MX6 SoC a > > > fsl,pins = property can be used. Is there any > > > convenient way to configure the pad group control registers? > > > > > > The issue is that some bits (DDR_SEL and ODT) in the individual > > > RGMII pad control registers are read-only. To tweak those parameters > > > (signal voltage and termination resistors) one need to write to the > > > pad group control registers for the whole RGMII pad group. Namely > > > IOMUXC_SW_PAD_CTL_GRP_DDR_TYPE_RGMII and > > > IOMUXC_SW_PAD_CTL_GRP_RGMII_TERM. The group registers in > general > > are not accessible from the list in arch/arm/boot/dts/imx6dl-pinfunc.h. > > > > > > I could not find any other way to change the group registers than > > > hacking-in some lines into the imx6q_init_machine(void) function in > > > arch/arm/mach-imx/mach-imx6q.c source. As I work towards > upstreaming > > > my board this should be done from my device tree or solved in some > > universal way. > > > > > > Any hints will be much appreciated. > > > Michal > > > > I figured out this is more "pinctrl-imx.c" than "device-tree" related > > so I am kindly adding maintainers of that file in hope somebody will > > shed some light to it. > > > > I am diving deeper into the code and it seems there really is no > > generic option to set the i.MX6 pad group control registers from device > tree. > > Or am I looking at the problem from a wrong angle? > > > > Yes, there's a few special pad group ctrl registers (e.g. DRAM and RGMII > for mx6q) which are not added In the pinctrl driver support. > > > How should we deal with boards that need to configure some pad > > characteristics available only through the pad group control registers? > > > > Andy, > How do we handle it internally? No, we don't handle the pin. I remembered IC owner said It seems only RGMII 2.5v need to handle the pin. > > > I also raised this question at the NXP community forum [1] and get > > quite unsatisfying answer so far. I would love to find/implement a > > proper and universal solution. > > > > There're probably two ways to do it: > 1) handle it in fec driver by parsing a specific property > 2) Add a new pad group into pinctrl driver support e.g. > MX6Q_PAD_CTL_GRP_RGMII_TERM > MX6Q_PAD_CTL_GRP_DDR_TYPE_RGMII > > I may prefer to 2). > > Regards > Dong Aisheng > > > Thanks in advance for your time, > > Michal > > > > [1] > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fc > o > > > mmunity.nxp.com%2Fthread%2F477464&data=02%7C01%7Caisheng.dong > %4 > > > 0nxp.com%7Ca3e5463eb06a47f1121008d5d0768a04%7C686ea1d3bc2b > 4c6fa9 > > > 2cd99c5c301635%7C0%7C1%7C636644129914162495&sdata=xqtBS8uX > %2BSzq > > 5m6tbNaLzCkB7ezgHSnyu9GQ3K13cW8%3D&reserved=0
RE: [PATCH, net-next] net: ethernet: freescale: fix false-positive string overflow warning
From: Arnd Bergmann Sent: 2018年5月28日 23:50
> While compile-testing on arm64 with gcc-8.1, I ran into a build diagnostic:
>
> drivers/net/ethernet/freescale/fec_main.c: In function 'fec_probe':
> drivers/net/ethernet/freescale/fec_main.c:3517:25: error: '%d' directive
> writing between 1 and 10 bytes into a region of size 5
> [-Werror=format-overflow=]
>sprintf(irq_name, "int%d", i);
> ^~
> drivers/net/ethernet/freescale/fec_main.c:3517:21: note: directive
> argument in the range [0, 2147483646]
>sprintf(irq_name, "int%d", i);
> ^~~
> drivers/net/ethernet/freescale/fec_main.c:3517:3: note: 'sprintf' output
> between 5 and 14 bytes into a destination of size 8
>sprintf(irq_name, "int%d", i);
>^
>
> It appears this has never shown on ppc32 or arm32 for an unknown
> reason, but now gcc fails to identify that the 'irq_cnt' loop index has an
> upper bound of 3, and instead uses a bogus range.
>
> To work around the warning, this changes the sprintf to snprintf with the
> correct buffer length.
>
> Fixes: 78cc6e7ef957 ("net: ethernet: freescale: Allow FEC with
> COMPILE_TEST")
> Signed-off-by: Arnd Bergmann
Acked-by: Fugang Duan
> ---
> drivers/net/ethernet/freescale/fec_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index ab7521c04eb2..c729665107f5 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3514,7 +3514,7 @@ fec_probe(struct platform_device *pdev)
> goto failed_init;
>
> for (i = 0; i < irq_cnt; i++) {
> - sprintf(irq_name, "int%d", i);
> + snprintf(irq_name, sizeof(irq_name), "int%d", i);
> irq = platform_get_irq_byname(pdev, irq_name);
> if (irq < 0)
> irq = platform_get_irq(pdev, i);
> --
> 2.9.0
RE: [PATCH net-next] net: fec: remove stale comment
> From: YueHaibing Sent: 2018年5月24日 19:27
> This comment is outdated as fec_ptp_ioctl has been replaced by
> fec_ptp_set/fec_ptp_get since commit 1d5244d0e43b ("fec: Implement
> the SIOCGHWTSTAMP ioctl")
>
> Signed-off-by: YueHaibing
Thanks.
Acked-by: Fugang Duan
> ---
> drivers/net/ethernet/freescale/fec_ptp.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index f814397..d438ef8 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -466,12 +466,6 @@ static int fec_ptp_enable(struct ptp_clock_info
> *ptp,
> return -EOPNOTSUPP;
> }
>
> -/**
> - * fec_ptp_hwtstamp_ioctl - control hardware time stamping
> - * @ndev: pointer to net_device
> - * @ifreq: ioctl data
> - * @cmd: particular ioctl requested
> - */
> int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr) {
> struct fec_enet_private *fep = netdev_priv(ndev);
> --
> 2.7.0
>
RE: [PATCH net-next v3 2/3] net: ethernet: freescale: Allow FEC with COMPILE_TEST
From: Florian Fainelli Sent: 2018年5月18日 4:08
> The Freescale FEC driver builds fine with COMPILE_TEST, so make that
> possible.
>
> Signed-off-by: Florian Fainelli
Acked-by: Fugang Duan
> ---
> drivers/net/ethernet/freescale/Kconfig| 2 +-
> drivers/net/ethernet/freescale/fec.h | 2 +-
> drivers/net/ethernet/freescale/fec_main.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/Kconfig
> b/drivers/net/ethernet/freescale/Kconfig
> index 6e490fd2345d..a580a3dcbe59 100644
> --- a/drivers/net/ethernet/freescale/Kconfig
> +++ b/drivers/net/ethernet/freescale/Kconfig
> @@ -22,7 +22,7 @@ if NET_VENDOR_FREESCALE config FEC
> tristate "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
> depends on (M523x || M527x || M5272 || M528x || M520x ||
> M532x || \
> -ARCH_MXC || SOC_IMX28)
> +ARCH_MXC || SOC_IMX28 || COMPILE_TEST)
> default ARCH_MXC || SOC_IMX28 if ARM
> select PHYLIB
> imply PTP_1588_CLOCK
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index e7381f8ef89d..4778b663653e 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -21,7 +21,7 @@
>
> #if defined(CONFIG_M523x) || defined(CONFIG_M527x) ||
> defined(CONFIG_M528x) || \
> defined(CONFIG_M520x) || defined(CONFIG_M532x) ||
> defined(CONFIG_ARM) || \
> -defined(CONFIG_ARM64)
> +defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
> /*
> * Just figures, Motorola would have to change the offsets for
> * registers in the same peripheral device on different models
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index f3e43db0d6cb..4358f586e28f 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2107,7 +2107,7 @@ static int fec_enet_get_regs_len(struct
> net_device *ndev)
> /* List of registers that can be safety be read to dump them with ethtool
> */ #if defined(CONFIG_M523x) || defined(CONFIG_M527x) ||
> defined(CONFIG_M528x) || \
> defined(CONFIG_M520x) || defined(CONFIG_M532x) ||
> defined(CONFIG_ARM) || \
> - defined(CONFIG_ARM64)
> + defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
> static u32 fec_enet_register_offset[] = {
> FEC_IEVENT, FEC_IMASK, FEC_R_DES_ACTIVE_0,
> FEC_X_DES_ACTIVE_0,
> FEC_ECNTRL, FEC_MII_DATA, FEC_MII_SPEED, FEC_MIB_CTRLSTAT,
> FEC_R_CNTRL,
> --
> 2.14.1
RE: [PATCH net-next v2 2/2] drivers: net: Remove device_node checks with of_mdiobus_register()
From: Florian Fainelli Sent: 2018年5月16日 7:56
> A number of drivers have the following pattern:
>
> if (np)
> of_mdiobus_register()
> else
> mdiobus_register()
>
> which the implementation of of_mdiobus_register() now takes care of.
> Remove that pattern in drivers that strictly adhere to it.
>
> Signed-off-by: Florian Fainelli
For drivers/net/ethernet/freescale/fec_main.c:
Reviewed-by: Fugang Duan
> ---
> drivers/net/dsa/bcm_sf2.c | 8 ++--
> drivers/net/dsa/mv88e6xxx/chip.c | 5 +
> drivers/net/ethernet/cadence/macb_main.c | 12 +++-
> drivers/net/ethernet/freescale/fec_main.c | 8 ++--
> drivers/net/ethernet/marvell/mvmdio.c | 5 +
> drivers/net/ethernet/renesas/sh_eth.c | 11 +++
> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 5 +
> drivers/net/ethernet/ti/davinci_mdio.c| 8 +++-
> drivers/net/phy/mdio-gpio.c | 6 +-
> drivers/net/phy/mdio-mscc-miim.c | 6 +-
> drivers/net/usb/lan78xx.c | 7 ++-
> 11 files changed, 20 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index
> ac621f44237a..02e8982519ce 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -450,12 +450,8 @@ static int bcm_sf2_mdio_register(struct dsa_switch
> *ds)
> priv->slave_mii_bus->parent = ds->dev->parent;
> priv->slave_mii_bus->phy_mask = ~priv->indir_phy_mask;
>
> - if (dn)
> - err = of_mdiobus_register(priv->slave_mii_bus, dn);
> - else
> - err = mdiobus_register(priv->slave_mii_bus);
> -
> - if (err)
> + err = of_mdiobus_register(priv->slave_mii_bus, dn);
> + if (err && dn)
> of_node_put(dn);
>
> return err;
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> b/drivers/net/dsa/mv88e6xxx/chip.c
> index b23c11d9f4b2..2bb3f03ee1cb 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2454,10 +2454,7 @@ static int mv88e6xxx_mdio_register(struct
> mv88e6xxx_chip *chip,
> return err;
> }
>
> - if (np)
> - err = of_mdiobus_register(bus, np);
> - else
> - err = mdiobus_register(bus);
> + err = of_mdiobus_register(bus, np);
> if (err) {
> dev_err(chip->dev, "Cannot register MDIO bus (%d)\n", err);
> mv88e6xxx_g2_irq_mdio_free(chip, bus); diff --git
> a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index b4c9268100bb..3e93df5d4e3b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -591,16 +591,10 @@ static int macb_mii_init(struct macb *bp)
> dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
>
> np = bp->pdev->dev.of_node;
> + if (pdata)
> + bp->mii_bus->phy_mask = pdata->phy_mask;
>
> - if (np) {
> - err = of_mdiobus_register(bp->mii_bus, np);
> - } else {
> - if (pdata)
> - bp->mii_bus->phy_mask = pdata->phy_mask;
> -
> - err = mdiobus_register(bp->mii_bus);
> - }
> -
> + err = of_mdiobus_register(bp->mii_bus, np);
> if (err)
> goto err_out_free_mdiobus;
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index d4604bc8eb5b..f3e43db0d6cb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2052,13 +2052,9 @@ static int fec_enet_mii_init(struct platform_device
> *pdev)
> fep->mii_bus->parent = &pdev->dev;
>
> node = of_get_child_by_name(pdev->dev.of_node, "mdio");
> - if (node) {
> - err = of_mdiobus_register(fep->mii_bus, node);
> + err = of_mdiobus_register(fep->mii_bus, node);
> + if (node)
> of_node_put(node);
> - } else {
> - err = mdiobus_register(fep->mii_bus);
> - }
> -
> if (err)
> goto err_out_free_mdiobus;
>
> diff --git a/drivers/net/ethernet/marvell/mvmdio.c
> b/drivers/net/ethernet/marvell/mvmdio.c
> index 0495487f7b42..c5dac6bd2be4 100644
> --- a/drivers/net/ethernet/marvell/mvmdio.c
> +++ b/drivers/net/ethernet/marvell/mvmdio.c
> @@ -348,10 +348,7 @@ static int orion_mdio_probe(struct platform_device
> *pdev)
> goto out_mdio;
> }
>
> - if (pdev->dev.of_node)
> - ret = of_mdiobus_register(bus, pdev->dev.of_node);
> - else
> - ret = mdiobus_register(bus);
> + ret = of_mdiobus_register(bus, pdev->dev.of_node);
> if (ret < 0) {
> dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
> goto out_mdio;
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> b/drivers/net/ethernet/renes
RE: [PATCH net-next 2/3] net: ethernet: freescale: Allow FEC with COMPILE_TEST
From: Florian Fainelli Sent: 2018年5月16日 7:48 > The Freescale FEC driver builds fine with COMPILE_TEST, so make that > possible. > > Signed-off-by: Florian Fainelli Acked-by: Fugang Duan > --- > drivers/net/ethernet/freescale/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/Kconfig > b/drivers/net/ethernet/freescale/Kconfig > index 6e490fd2345d..a580a3dcbe59 100644 > --- a/drivers/net/ethernet/freescale/Kconfig > +++ b/drivers/net/ethernet/freescale/Kconfig > @@ -22,7 +22,7 @@ if NET_VENDOR_FREESCALE config FEC > tristate "FEC ethernet controller (of ColdFire and some i.MX CPUs)" > depends on (M523x || M527x || M5272 || M528x || M520x || M532x || \ > -ARCH_MXC || SOC_IMX28) > +ARCH_MXC || SOC_IMX28 || COMPILE_TEST) > default ARCH_MXC || SOC_IMX28 if ARM > select PHYLIB > imply PTP_1588_CLOCK > -- > 2.14.1
RE: WARNING: CPU: 0 PID: 0 at ./include/linux/netfilter.h:233 arp_rcv
From: Marco Franchi Sent: Friday, January 05, 2018 11:03 PM
>Hi,
>
>I am getting the following warning on a imx6ul-evk board running linux-next
>20180105:
>
>[9.233290] [ cut here ]
>[9.242068] WARNING: CPU: 0 PID: 0 at
>./include/linux/netfilter.h:233 arp_rcv+0x1f8/0x228
>[9.250381] Modules linked in:
>[9.253633] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>4.15.0-rc6-next-20180104-dirty #2
>[9.261764] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
>[9.268065] Backtrace:
>[9.270719] [] (dump_backtrace) from []
>(show_stack+0x18/0x1c)
>[9.278438] r7: r6:6153 r5: r4:c1079878
>[9.284258] [] (show_stack) from []
>(dump_stack+0xb4/0xe8)
>[9.291641] [] (dump_stack) from []
>(__warn+0xf0/0x11c)
>[9.298763] r9:d8053000 r8:00e9 r7:0009 r6:c0da3bdc
>r5: r4:
>[9.306662] [] (__warn) from []
>(warn_slowpath_null+0x44/0x50)
>[9.314383] r8:d8053000 r7:0608 r6:c08873f8 r5:00e9 r4:c0da3bdc
>[9.321243] [] (warn_slowpath_null) from []
>(arp_rcv+0x1f8/0x228)
>[9.329215] r6:c0887200 r5:c107ac58 r4:d899b240
>[9.333999] [] (arp_rcv) from []
>(__netif_receive_skb_core+0x878/0xbd4)
>[9.342491] r6:c0887200 r5:c100ac8c r4:d899b240
>[9.347265] [] (__netif_receive_skb_core) from
>[] (__netif_receive_skb+0x2c/0x8c)
>[9.356643] r10:0080 r9:d8053000 r8:e0a26000 r7:c107ab0d
>r6:c1008908 r5:d899b240
>[9.364598] r4:c10099c4
>[9.367288] [] (__netif_receive_skb) from []
>(netif_receive_skb_internal+0x7c/0x354)
>[9.376904] r5:d899b240 r4:c10099c4
>[9.380635] [] (netif_receive_skb_internal) from
>[] (napi_gro_receive+0x88/0xa4)
>[9.389918] r8:e0a26000 r7:0001 r6:d899b240 r5:d899b240 r4:0003
>[9.396784] [] (napi_gro_receive) from []
>(fec_enet_rx_napi+0x3a8/0x9b8)
>[9.405357] r5:d8054000 r4:
>[9.409093] [] (fec_enet_rx_napi) from []
>(net_rx_action+0x220/0x334)
>[9.417431] r10:dbbdfa00 r9:c1001d94 r8:0040 r7:012c
>r6:8e6b r5:0001
>[9.425384] r4:d8053710
>[9.428075] [] (net_rx_action) from []
>(__do_softirq+0x128/0x2a0)
>[9.436063] r10:4003 r9:c1003080 r8:0100 r7:c100308c
>r6:c100 r5:0003
>[9.444018] r4:
>[9.446708] [] (__do_softirq) from []
>(irq_exit+0x14c/0x1a8)
>[9.454262] r10:e080a000 r9:d8004400 r8:0001 r7:
>r6:c1008a98 r5:
>[9.462218] r4:e000
>[9.464911] [] (irq_exit) from []
>(__handle_domain_irq+0x74/0xe8)
>[9.472879] r5: r4:c0f7cc24
>[9.476614] [] (__handle_domain_irq) from []
>(gic_handle_irq+0x64/0xc4)
>[9.485121] r9:c1028344 r8:c1001e98 r7: r6:03ff
>r5:03eb r4:e080a00c
>[9.493016] [] (gic_handle_irq) from []
>(__irq_svc+0x70/0x98)
>[9.500632] Exception stack(0xc1001e98 to 0xc1001ee0)
>[9.505822] 1e80:
>0001 0001
>[9.514157] 1ea0: c100bf80 25963796 0002 0002
>dbbde5c8 26545294 0002
>[9.522491] 1ec0: c1001f14 c1001eb8 c1001ee8 c01724fc
>c0724464 2153
>[9.530824] r10: r9:c100 r8:26545294 r7:c1001ecc
>r6: r5:2153
>[9.538778] r4:c0724464
>[9.541470] [] (cpuidle_enter_state) from []
>(cpuidle_enter+0x1c/0x20)
>[9.549893] r10:c100f6a8 r9:dbbde5c8 r8:c0f7c5c0 r7:c1008978
>r6:0001 r5:c100892c
>[9.557857] r4:c100 r3:dbbde5c8
>[9.561589] [] (cpuidle_enter) from []
>(call_cpuidle+0x28/0x44)
>[9.569399] [] (call_cpuidle) from []
>(do_idle+0x1bc/0x230)
>[9.576861] [] (do_idle) from []
>(cpu_startup_entry+0x20/0x24)
>[9.584587] r10:c0f63a50 r9:c1008908 r8:c107b480 r7:c1008900
>r6:c107b480 r5:0002
>[9.592546] r4:00c4 r3:c0f75354
>[9.596283] [] (cpu_startup_entry) from []
>(rest_init+0x210/0x25c)
>[9.604372] [] (rest_init) from []
>(start_kernel+0x390/0x418)
>[9.611998] r5: r4:c107b4cc
>[9.615723] [] (start_kernel) from [<>] ( (null))
>[9.622235] r10:10c5387d r9:410fc075 r8:8300 r7:
>r6:10c0387d r5:0051
>[9.630191] r4:c0f0032c
>[9.632851] ---[ end trace 2d5d5f79c0c8da59 ]---
>
>Does anyone know how to fix it?
>
>Thanks
If you enable kernel config "CONFIG_NETFILTER_FAMILY_ARP" can fix the warning.
It is introduced by below commit.
commit 8de98f058360722a1a9febe3970de6dcd4d91513
Author: Florian Westphal
Date: Thu Dec 7 16:28:26 2017 +0100
netfilter: don't allocate space for arp/bridge hooks unless needed
no need to define hook points if the family isn't supported.
Because we need these hooks for either nftables, arp/ebtables
or the 'call-iptables' hack we have in the bridge layer add two
new dependencies, NETFILTER_FAMILY_{ARP,BRIDGE}, and have the
users select them.
Signed-off-by: Florian Westphal
Signed-off-by: Pablo Neira Ayuso
diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index ce4e91df..0e46cb4 100644
---
RE: [PATCH 2/7] ARM: dts: imx6ul: update i.MX 6UltraLite iomux headers
From: Rob Herring Sent: Saturday, January 06, 2018 12:45 AM >To: Stefan Agner >Cc: shawn...@kernel.org; ker...@pengutronix.de; Fabio Estevam >; mark.rutl...@arm.com; linux-arm- >ker...@lists.infradead.org; devicet...@vger.kernel.org; linux- >ker...@vger.kernel.org; Andy Duan >Subject: Re: [PATCH 2/7] ARM: dts: imx6ul: update i.MX 6UltraLite iomux >headers > >On Tue, Jan 02, 2018 at 05:42:18PM +0100, Stefan Agner wrote: >> From: Fugang Duan >> >> Update i.MX 6UltraLite IOMUXC pin defines. > >That's obvious reading the diff. The commit message should tell me why. >They were wrong? > Yes, the previous iomux header parts of pin setting were wrong, which was generated by IOMUX tool during SOC bringup. The updated header correct the wrong pin setting. >> >> Signed-off-by: Fugang Duan >> Signed-off-by: Stefan Agner
RE: [PATCH net-next v5 0/4] net: fec: fix refclk enable for SMSC LAN8710/20
From: Richard Leitner Sent: Monday, December 11, 2017 8:17 PM
>This patch series fixes the use of the SMSC LAN8710/20 with a Freescale ETH
>when the refclk is generated by the FSL.
>
>This patchset depends on the "phylib: Add device reset GPIO support" patch
>submitted by Geert Uytterhoeven/Sergei Shtylyov, which was merged to net-
>next as commit bafbdd527d56 ("phylib: Add device reset GPIO support").
>
>Changes v5:
> - fix reset delay calculation (max_t instead of min_t)
>
>Changes v4:
> - simplify dts parsing
> - simplify reset delay evaluation and execution
> - fec: ensure to only reset once during fec_enet_open()
> - remove dependency notes from commit message
> - add reviews and acks
>
>Changes v3:
> - use phylib to hard-reset the PHY
> - implement reset delays in phylib
> - add new phylib API & flag (PHY_RST_AFTER_CLK_EN) to determine if
> a PHY is affected
>
>Changes v2:
> - simplify and fix fec_reset_phy function to support multiple calls
> - include: linux: phy: harmonize phy_id{,_mask} type
> - reset the phy instead of not turning the clock on and off
> (which would have caused a power consumption regression)
>
>Richard Leitner (4):
> phylib: Add device reset delay support
> phylib: add reset after clk enable support
> net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag
> net: fec: add phy_reset_after_clk_enable() support
>
> Documentation/devicetree/bindings/net/phy.txt | 10 ++
> drivers/net/ethernet/freescale/fec_main.c | 20
> drivers/net/phy/mdio_device.c | 13 +++--
> drivers/net/phy/phy_device.c | 24
> drivers/net/phy/smsc.c| 2 +-
> drivers/of/of_mdio.c | 4
> include/linux/mdio.h | 2 ++
> include/linux/phy.h | 2 ++
> 8 files changed, 74 insertions(+), 3 deletions(-)
>
>--
>2.11.0
The series look fine better. Thanks.
Acked-by: Fugang Duan
RE: [PATCH net-next v3 4/4] net: fec: add phy_reset_after_clk_enable() support
From: Richard Leitner Sent: Wednesday, December
06, 2017 4:12 PM
>To: Andy Duan ; Richard Leitner ;
>robh...@kernel.org; mark.rutl...@arm.com; and...@lunn.ch;
>f.faine...@gmail.com; frowand.l...@gmail.com
>Cc: da...@davemloft.net; geert+rene...@glider.be;
>sergei.shtyl...@cogentembedded.com; bar...@tkos.co.il; david.wu@rock-
>chips.com; lu...@denx.de; net...@vger.kernel.org;
>devicet...@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH net-next v3 4/4] net: fec: add
>phy_reset_after_clk_enable() support
>
>Hi Andy,
>
>On 12/06/2017 02:50 AM, Andy Duan wrote:
>> From: Richard Leitner Sent: Tuesday, December 05,
>> 2017 9:26 PM
>>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow
>>> turning the refclk on and off again during operation (according to their
>datasheet).
>>> Nonetheless exactly this behaviour was introduced for power saving
>>> reasons by commit e8fcfcd5684a ("net: fec: optimize the clock
>>> management to save power").
>>> Therefore add support for the phy_reset_after_clk_enable function
>>> from phylib to mitigate this issue.
>
>...
>
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>> b/drivers/net/ethernet/freescale/fec_main.c
>>> index 610573855213..8c3d0fb7db20 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct
>>> net_device *ndev, bool enable)
>>> ret = clk_prepare_enable(fep->clk_ref);
>>> if (ret)
>>> goto failed_clk_ref;
>>> +
>>> + phy_reset_after_clk_enable(ndev->phydev);
>>> } else {
>>> clk_disable_unprepare(fep->clk_ahb);
>>> clk_disable_unprepare(fep->clk_enet_out);
>>> @@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
>>> if (ret)
>>> goto err_enet_mii_probe;
>>>
>>> + /* reset phy if needed here, due to the fact this is the first time we
>>> +* have the net_device to phy_driver link
>>> +*/
>>> + phy_reset_after_clk_enable(ndev->phydev);
>>> +
>>
>> The patch series look better.
>> But why does it need to reset phy here since phy already is hard reset after
>clock enable.
>
>The problem here is that in fec_enet_open() the fec_enet_clk_enable() call is
>done before the phy is probed. Therefore (as mentioned in the
>comment) there's no link from the net_device (ndev) to the phy_driver
>(which holds the flags).
>
>Any suggestions for a better solution are highly appreciated here! Thanks!
>
>regards;Richard.L
Okay, I see.
For the patch: Acked-by: Fugang Duan
RE: [PATCH net-next v3 4/4] net: fec: add phy_reset_after_clk_enable() support
From: Richard Leitner Sent: Tuesday, December 05, 2017 9:26 PM
>Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
>the refclk on and off again during operation (according to their datasheet).
>Nonetheless exactly this behaviour was introduced for power saving reasons
>by commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>power").
>Therefore add support for the phy_reset_after_clk_enable function from
>phylib to mitigate this issue.
>
>Generally speaking this issue is only relevant if the ref clk for the PHY is
>generated by the SoC and therefore the PHY is configured to "REF_CLK In
>Mode". In our specific case (PCB) this problem does occur at about every 10th
>to 50th POR of an LAN8710 connected to an i.MX6SOLO SoC. The typical
>symptom of this problem is a "swinging" ethernet link.
>Similar issues were reported by users of the NXP forum:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fcommunity.nxp.com%2Fthread%2F389902&data=02%7C01%7Cfugang.du
>an%40nxp.com%7C7f9fee272fc44662c2a108d53be3d1ee%7C686ea1d3bc2b4c6
>fa92cd99c5c301635%7C0%7C0%7C636480772022331090&sdata=7RdUsoWVWu
>o1nM5zKwLt7%2F6U3dxgDJtBDGlQCUWC6IM%3D&reserved=0
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fcommunity.nxp.com%2Fmessage%2F309354&data=02%7C01%7Cfugang.d
>uan%40nxp.com%7C7f9fee272fc44662c2a108d53be3d1ee%7C686ea1d3bc2b4
>c6fa92cd99c5c301635%7C0%7C0%7C636480772022331090&sdata=D56KilGWD3
>kLABxc0yOI%2B44Y%2FhLfrGtdAvupCEyvI%2BI%3D&reserved=0
>With this patch applied the issue didn't occur for at least a few hundret PORs
>of our board.
>
>Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save
>power")
>Signed-off-by: Richard Leitner
>---
> drivers/net/ethernet/freescale/fec_main.c | 7 +++
> 1 file changed, 7 insertions(+)
>
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 610573855213..8c3d0fb7db20 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device
>*ndev, bool enable)
> ret = clk_prepare_enable(fep->clk_ref);
> if (ret)
> goto failed_clk_ref;
>+
>+ phy_reset_after_clk_enable(ndev->phydev);
> } else {
> clk_disable_unprepare(fep->clk_ahb);
> clk_disable_unprepare(fep->clk_enet_out);
>@@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
> if (ret)
> goto err_enet_mii_probe;
>
>+ /* reset phy if needed here, due to the fact this is the first time we
>+ * have the net_device to phy_driver link
>+ */
>+ phy_reset_after_clk_enable(ndev->phydev);
>+
The patch series look better.
But why does it need to reset phy here since phy already is hard reset after
clock enable.
> if (fep->quirks & FEC_QUIRK_ERR006687)
> imx6q_cpuidle_fec_irqs_used();
>
>--
>2.11.0
RE: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20
From: Richard Leitner Sent: Monday, November 20,
2017 8:55 PM
>On 11/20/2017 11:35 AM, Andy Duan wrote:
>> From: Richard Leitner Sent: Monday,
>> November 20, 2017 5:57 PM
>>> To: Andy Duan ; f.faine...@gmail.com;
>>> and...@lunn.ch
>>> Cc: Richard Leitner ; net...@vger.kernel.org; linux-
>>> ker...@vger.kernel.org
>>> Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>> SMSC
>>> LAN8710/20
>>>
>>>
>>> On 11/20/2017 10:47 AM, Andy Duan wrote:
>>>> From: Richard Leitner Sent: Monday, November 20,
>>>> 2017
>>>> 4:34 PM
>>>>> To: f.faine...@gmail.com; Andy Duan ;
>>>>> and...@lunn.ch
>>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>>> richard.leit...@skidata.com
>>>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>>>> SMSC
>>>>> LAN8710/20
>>>>>
>>>>> From: Richard Leitner
>>>>>
>>>>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow
>>>>> turning the refclk on and off again during operation (according to
>>>>> their
>>> datasheet).
>>>>> Nonetheless exactly this behaviour was introduced for power saving
>>>>> reasons by commit e8fcfcd5684a ("net: fec: optimize the clock
>>>>> management to save power").
>>>>> Therefore after enabling the refclk we detect if an affected PHY is
>>>>> attached. If so reset and initialize it again.
>>> ...
>>>
>>>>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct
>>>>> +net_device
>>>>> +*ndev) {
>>>>> + struct phy_device *phy_dev = ndev->phydev;
>>>>> + u32 real_phy_id;
>>>>> + int ret;
>>>>> +
>>>>> + /* some PHYs need a reset after the refclk was enabled, so we
>>>>> + * reset them here
>>>>> + */
>>>>> + if (!phy_dev)
>>>>> + return 0;
>>>>> + if (!phy_dev->drv)
>>>>> + return 0;
>>>>> + real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>>>>> + switch (real_phy_id) {
>>>>> + case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
>>>> Don't hard code here...
>>>> I believe there have many other phys also do such operation,
>>>> hardcode is
>>> unacceptable...
>>>> And these code can be put into phy_device.c as common interface.
>>> Ok. Thank you for the feedback.
>>> So it would be fine to hardcode the affected phy_id's in a common
>>> function in phy_device.c?
>>>
>>>
>>> Another possible solution that came to my mind is to add a flag
>>> called something like "PHY_RST_AFTER_CLK_EN" to the flags variable in
>>> struct phy_driver. This flag could then be set in the smsc PHY driver
>>> for affected PHYs.
>>>
>>> Then instead of comparing the phy_id in the MAC driver this flag
>>> could be
>>> checked:
>>>
>>> if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
>>>ret = fec_reset_phy(ndev);
>>>...
>>> }
>>>
>>> Would checking the flag be OK in fec_main.c?
>> Yes, it is better than previous solution.
>> But add new common API in phy_device.c is much better like:
>> 1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
>phy_driver, all phy driver that need reset can set the flag.
>
>OK.
>
>> 2. add new common api interface phy_reset_after_clk_enable() in
>> phy_device.c driver
>
>OK. But see below...
>
>> 3. add reset gpio descriptor for common phy device driver.
>
>... if I understood it correctly the patch called "Teach phylib hard-resetting
>devices" by Geert and Sergei is exactly doing this:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fpatchwork.ozlabs.org%2Fcover%2F828503%2F&data=02%7C01%7Cfugang
>.duan%40nxp.com%7C087010d8b4784e3c88c608d53015f2ad%7C686ea1d3bc2
>b4c6fa92cd99c5c301635%7C0%7C0%7C636467793180108636&sdata=yY9thX8Q
>CCVteoF5vvUoAYYxGH0gg4wOUq7TQKtkiok%3D&reserved=0
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Flkml.org%2Flkml%2F2017%2F10%2F20%2F166&data=02%7C01%7Cfugang.
>duan%40nxp.com%7C087010d8b4784e3c88c608d53015f2ad%7C686ea1d3bc2b
>4c6fa92cd99c5c301635%7C0%7C0%7C636467793180108636&sdata=rxV12dum1
>VmbWLWvSACDuZevFSFbUoWr9AiUtVSsV6w%3D&reserved=0
>
>So I'll implement the phy_reset_after_clk_enable function atop of this patch-
>set and add a note that my patch-series depends on it. Would that be OK?
>
Yes, it is the best solution.
>> 4. then any mac driver can directly call the common
>interface .phy_reset_after_clk_enable().
>
>Sounds reasonable :-)
>
>>
>> That is only my suggestion, maybe there have better idea.
>> Thanks.
>>
>
>Thanks for your quick feedback.
>
>regards
>Richard.L
RE: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20
From: Richard Leitner Sent: Monday, November 20,
2017 5:57 PM
>To: Andy Duan ; f.faine...@gmail.com;
>and...@lunn.ch
>Cc: Richard Leitner ; net...@vger.kernel.org; linux-
>ker...@vger.kernel.org
>Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>LAN8710/20
>
>
>On 11/20/2017 10:47 AM, Andy Duan wrote:
>> From: Richard Leitner Sent: Monday, November 20, 2017
>> 4:34 PM
>>> To: f.faine...@gmail.com; Andy Duan ;
>>> and...@lunn.ch
>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>> richard.leit...@skidata.com
>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>> SMSC
>>> LAN8710/20
>>>
>>> From: Richard Leitner
>>>
>>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow
>>> turning the refclk on and off again during operation (according to their
>datasheet).
>>> Nonetheless exactly this behaviour was introduced for power saving
>>> reasons by commit e8fcfcd5684a ("net: fec: optimize the clock
>>> management to save power").
>>> Therefore after enabling the refclk we detect if an affected PHY is
>>> attached. If so reset and initialize it again.
>
>...
>
>>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>>> +*ndev) {
>>> + struct phy_device *phy_dev = ndev->phydev;
>>> + u32 real_phy_id;
>>> + int ret;
>>> +
>>> + /* some PHYs need a reset after the refclk was enabled, so we
>>> +* reset them here
>>> +*/
>>> + if (!phy_dev)
>>> + return 0;
>>> + if (!phy_dev->drv)
>>> + return 0;
>>> + real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>>> + switch (real_phy_id) {
>>> + case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
>>
>> Don't hard code here...
>> I believe there have many other phys also do such operation, hardcode is
>unacceptable...
>>
>> And these code can be put into phy_device.c as common interface.
>
>Ok. Thank you for the feedback.
>So it would be fine to hardcode the affected phy_id's in a common function in
>phy_device.c?
>
>
>Another possible solution that came to my mind is to add a flag called
>something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
>phy_driver. This flag could then be set in the smsc PHY driver for affected
>PHYs.
>
>Then instead of comparing the phy_id in the MAC driver this flag could be
>checked:
>
>if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
>ret = fec_reset_phy(ndev);
>...
>}
>
>Would checking the flag be OK in fec_main.c?
Yes, it is better than previous solution.
But add new common API in phy_device.c is much better like:
1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
phy_driver, all phy driver that need reset can set the flag.
2. add new common api interface phy_reset_after_clk_enable() in phy_device.c
driver
3. add reset gpio descriptor for common phy device driver.
4. then any mac driver can directly call the common interface
.phy_reset_after_clk_enable().
That is only my suggestion, maybe there have better idea.
Thanks.
>
>What would be the "better" approach?
>
>>
>>> + ret = fec_reset_phy(ndev);
>>> + if (ret)
>>> + return ret;
>>> + ret = phy_init_hw(phy_dev);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> static int fec_enet_clk_enable(struct net_device *ndev, bool enable) {
>>> struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6
>>> +1889,10 @@ static int fec_enet_clk_enable(struct net_device *ndev,
>>> +bool
>>> enable)
>>> ret = clk_prepare_enable(fep->clk_ref);
>>> if (ret)
>>> goto failed_clk_ref;
>>> +
>>> + ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>>> + if (ret)
>>> + netdev_warn(ndev, "Resetting PHY failed, connection
>>> may be
>>> +unstable\n");
>>> } else {
>>> clk_disable_unprepare(fep->clk_ahb);
>>> clk_disable_unprepare(fep->clk_enet_out);
>>> @@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
>>> if (ret)
>>> goto err_enet_mii_probe;
>>>
>>> + /* as the PHY is connected now, trigger the reset quirk again */
>>> + ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>>> + if (ret)
>>> + netdev_warn(ndev, "Resetting PHY failed, connection may be
>>> +unstable\n");
>>> +
>>> if (fep->quirks & FEC_QUIRK_ERR006687)
>>> imx6q_cpuidle_fec_irqs_used();
>>>
>>> napi_enable(&fep->napi);
>>> phy_start(ndev->phydev);
>>> +
>>
>> No need blank line here...
>>> netif_tx_start_all_queues(ndev);
>>>
>>> device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
>>> --
>>> 2.11.0
RE: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20
From: Richard Leitner Sent: Monday, November 20, 2017 4:34 PM
>To: f.faine...@gmail.com; Andy Duan ;
>and...@lunn.ch
>Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>richard.leit...@skidata.com
>Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>LAN8710/20
>
>From: Richard Leitner
>
>Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
>the refclk on and off again during operation (according to their datasheet).
>Nonetheless exactly this behaviour was introduced for power saving reasons
>by commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>power").
>Therefore after enabling the refclk we detect if an affected PHY is attached.
>If
>so reset and initialize it again.
>
>For a better understanding here's a outline of the time response of the clock
>and reset lines before and after this patch:
>
> v--fec_probe() v--fec_enet_open()
> v v
> w/o patch eCLK:
>___|
> w/o patch nRST: __--
> w/o patch CONF:
>___XX___
>
> w/ patch eCLK:
>___|
> w/ patch nRST: __-__---
> w/ patch CONF:
>___XX_XX
> ^ ^
> ^--fec_probe() ^--fec_enet_open()
>
>Generally speaking this issue is only relevant if the ref clk for the PHY is
>generated by the SoC. In our specific case (PCB) this problem does occur at
>about every 10th to 50th POR of an LAN8710 connected to an i.MX6DL SoC.
>The typical symptom of this problem is a "swinging"
>ethernet link. Similar issues were reported by users of the NXP forum:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fcommunity.nxp.com%2Fthread%2F389902&data=02%7C01%7Cfugang.du
>an%40nxp.com%7C507c7aafbd4744f81ebf08d52ff1acff%7C686ea1d3bc2b4c6f
>a92cd99c5c301635%7C0%7C0%7C636467637399145483&sdata=RNXVGpPrlrcyL
>0SoQl8%2BI0k8Oc8BM0Iwykd1O%2Bjmvcc%3D&reserved=0
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fcommunity.nxp.com%2Fmessage%2F309354&data=02%7C01%7Cfugang.d
>uan%40nxp.com%7C507c7aafbd4744f81ebf08d52ff1acff%7C686ea1d3bc2b4c6
>fa92cd99c5c301635%7C0%7C0%7C636467637399145483&sdata=pjeJEZGuBpb9
>uCMKGr70qa%2FmsNoak6v3nCID2vbNAeg%3D&reserved=0
>With this patch applied the issue didn't occur for at least a few hundret PORs
>of our board.
>
>Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save
>power")
>Signed-off-by: Richard Leitner
>---
> drivers/net/ethernet/freescale/fec_main.c | 37
>+++
> 1 file changed, 37 insertions(+)
>
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 06a7caca0cee..52ec9b29a70e 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -68,6 +68,7 @@
>
> static void set_multicast_list(struct net_device *ndev); static void
>fec_enet_itr_coal_init(struct net_device *ndev);
>+static int fec_reset_phy(struct net_device *ndev);
>
> #define DRIVER_NAME "fec"
>
>@@ -1833,6 +1834,32 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
>int mii_id, int regnum,
> return ret;
> }
>
>+static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>+*ndev) {
>+ struct phy_device *phy_dev = ndev->phydev;
>+ u32 real_phy_id;
>+ int ret;
>+
>+ /* some PHYs need a reset after the refclk was enabled, so we
>+ * reset them here
>+ */
>+ if (!phy_dev)
>+ return 0;
>+ if (!phy_dev->drv)
>+ return 0;
>+ real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>+ switch (real_phy_id) {
>+ case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
Don't hard code here...
I believe there have many other phys also do such operation, hardcode is
unacceptable...
And these code can be put into phy_device.c as common interface.
>+ ret = fec_reset_phy(ndev);
>+ if (ret)
>+ return ret;
>+ ret = phy_init_hw(phy_dev);
>+ if (ret)
>+ return ret;
>+ }
>+ return 0;
>+}
>+
> static int fec_enet_clk_enable(struct net_device *ndev, bool enable) {
>
RE: [PATCH v2 1/3] net: ethernet: freescale: simplify fec_reset_phy
From: Richard Leitner Sent: Monday, November 20, 2017 4:34 PM
>The fec_reset_phy function allowed only one execution during probeing.
>To make it more usable move the dt parsing and gpio allocation to the probe
>function. The parameters of the phy reset are added to the fec_enet_private
>struct. As a result the fec_reset_phy function may be called anytime after
>probe.
>
>One checkpatch.pl warning (too long line) is ignored. This is due to the fact a
>string (dt property name) otherwise needs to be split over multiple lines,
>which is counterproductive for the readability.
>
>Signed-off-by: Richard Leitner
>---
It is better to convert to gpio descriptor, and use dts gpio flag as the gpio
polarity instead of extra phy_reset_active_high variable.
Regards,
Andy
> drivers/net/ethernet/freescale/fec.h | 4 ++
> drivers/net/ethernet/freescale/fec_main.c | 88 --
>-
> 2 files changed, 50 insertions(+), 42 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/fec.h
>b/drivers/net/ethernet/freescale/fec.h
>index 5385074b3b7d..401c4eabf08a 100644
>--- a/drivers/net/ethernet/freescale/fec.h
>+++ b/drivers/net/ethernet/freescale/fec.h
>@@ -539,6 +539,10 @@ struct fec_enet_private {
> int pause_flag;
> int wol_flag;
> u32 quirks;
>+ int phy_reset;
>+ int phy_reset_duration;
>+ int phy_reset_post_delay;
>+ boolphy_reset_active_high;
>
> struct napi_struct napi;
> int csum_flags;
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 610573855213..06a7caca0cee 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -3212,62 +3212,36 @@ static int fec_enet_init(struct net_device *ndev) }
>
> #ifdef CONFIG_OF
>-static int fec_reset_phy(struct platform_device *pdev)
>+static int fec_reset_phy(struct net_device *ndev)
> {
>- int err, phy_reset;
>- bool active_high = false;
>- int msec = 1, phy_post_delay = 0;
>- struct device_node *np = pdev->dev.of_node;
>-
>- if (!np)
>- return 0;
>-
>- err = of_property_read_u32(np, "phy-reset-duration", &msec);
>- /* A sane reset duration should not be longer than 1s */
>- if (!err && msec > 1000)
>- msec = 1;
>+ struct fec_enet_private *fep = netdev_priv(ndev);
>
>- phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>- if (phy_reset == -EPROBE_DEFER)
>- return phy_reset;
>- else if (!gpio_is_valid(phy_reset))
>+ if (!fep->phy_reset)
> return 0;
>
>- err = of_property_read_u32(np, "phy-reset-post-delay",
>&phy_post_delay);
>- /* valid reset duration should be less than 1s */
>- if (!err && phy_post_delay > 1000)
>- return -EINVAL;
>-
>- active_high = of_property_read_bool(np, "phy-reset-active-high");
>+ gpio_set_value_cansleep(fep->phy_reset, fep-
>>phy_reset_active_high);
>
>- err = devm_gpio_request_one(&pdev->dev, phy_reset,
>- active_high ? GPIOF_OUT_INIT_HIGH :
>GPIOF_OUT_INIT_LOW,
>- "phy-reset");
>- if (err) {
>- dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n",
>err);
>- return err;
>- }
>-
>- if (msec > 20)
>- msleep(msec);
>+ if (fep->phy_reset_duration > 20)
>+ msleep(fep->phy_reset_duration);
> else
>- usleep_range(msec * 1000, msec * 1000 + 1000);
>+ usleep_range(fep->phy_reset_duration * 1000,
>+ fep->phy_reset_duration * 1000 + 1000);
>
>- gpio_set_value_cansleep(phy_reset, !active_high);
>+ gpio_set_value_cansleep(fep->phy_reset, !fep-
>>phy_reset_active_high);
>
>- if (!phy_post_delay)
>+ if (!fep->phy_reset_post_delay)
> return 0;
>
>- if (phy_post_delay > 20)
>- msleep(phy_post_delay);
>+ if (fep->phy_reset_post_delay > 20)
>+ msleep(fep->phy_reset_post_delay);
> else
>- usleep_range(phy_post_delay * 1000,
>- phy_post_delay * 1000 + 1000);
>+ usleep_range(fep->phy_reset_post_delay * 1000,
>+ fep->phy_reset_post_delay * 1000 + 1000);
>
> return 0;
> }
> #else /* CONFIG_OF */
>-static int fec_reset_phy(struct platform_device *pdev)
>+static int fec_reset_phy(struct net_device *ndev)
> {
> /*
>* In case of platform probe, the reset has been done @@ -3400,6
>+3374,36 @@ fec_probe(struct platform_device *pdev)
> }
> fep->phy_node = phy_node;
>
>+ fep->phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>+ if (gpio_is_valid(fep->phy_reset)) {
>+ ret = of_property_read_u32(np, "phy-reset-duration",
>+ &fep->phy_reset_duration);
>+ /* A sane r
RE: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option
From: Richard Leitner Sent: Friday, July 07, 2017 5:53 PM >To: Andy Duan ; robh...@kernel.org; >mark.rutl...@arm.com >Cc: net...@vger.kernel.org; devicet...@vger.kernel.org; linux- >ker...@vger.kernel.org; d...@g0hl1n.net; Andrew Lunn >Subject: Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable >option > > >On 07/07/2017 09:03 AM, Andy Duan wrote: >> From: Richard Leitner Sent: Friday, July >> 07, 2017 1:51 PM >>>> Since it is common issue so long as using the PHY, can you move it >>>> into smsc >>> phy driver like in .smsc_phy_reset() function ? >>>> And get the reset pin from phy dts node. >>> >>> Some more points that come into my mind: >>> - The smsc_phy_reset function is registered as "soft_reset". Would >>> it be OK to use nRST in it? >> >> It is not reasonable. >> >>> - Would it be OK to call the phy_init_hw function from within the >>> smsc_phy_reset? >> >> No, phy_init_hw() already call .drv->soft_reset(). >> >>> - IMHO I'd have to move the reset gpio binding inside the phy node >>> then. Isn't that a pretty big change doing that for all PHYs/FECs? Would it >>> be >"worth" it? >>> >> To make the change to be common, there have big change for phy driver. >> Maybe somebody can give one good suggestion/solution for it. > >Sorry, I don't think I understood everything correctly: > >1. The "phy-reset-gpios" binding should go inside the phy node. This will cause >to *change ALL FEC and PHY drivers*. Correct? > The "phy-reset-gpios" binding should go inside the phy node that is more reasonable. It is better PHY core driver handle phy hw reset. >2. Add an additonal "hard reset" function to the PHY driver which handles the >"phy-reset-gpios". Correct? > Correct. >3. Who should then trigger the "hard reset" of the PHY? phy_init_hw? The FEC? > >The point is that the LAN8710 is currently not always working correctly, >therefore this small change was proposed. Should we really change all >PHY/FECs only because of this? >Furthermore one problem still remains: The enet_refclk is controlled by the >FEC. How does the PHY recognize when it was disabled/enabled? > Your patch is workaround for the issue. As you pointed out these is a common issue. So we hope to get a better solution to handle these in common code. Andy
RE: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option
From: Richard Leitner Sent: Friday, July 07, 2017 1:51 PM >> Since it is common issue so long as using the PHY, can you move it into smsc >phy driver like in .smsc_phy_reset() function ? >> And get the reset pin from phy dts node. > >Some more points that come into my mind: > - The smsc_phy_reset function is registered as "soft_reset". Would it be OK > to >use nRST in it? It is not reasonable. > - Would it be OK to call the phy_init_hw function from within the >smsc_phy_reset? No, phy_init_hw() already call .drv->soft_reset(). > - IMHO I'd have to move the reset gpio binding inside the phy node then. > Isn't >that a pretty big change doing that for all PHYs/FECs? Would it be "worth" it? > To make the change to be common, there have big change for phy driver. Maybe somebody can give one good suggestion/solution for it. Andy
RE: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option
From: Richard Leitner Sent: Thursday, July 06,
2017 9:06 PM
>To: Andy Duan ; robh...@kernel.org;
>mark.rutl...@arm.com
>Cc: net...@vger.kernel.org; devicet...@vger.kernel.org; linux-
>ker...@vger.kernel.org; d...@g0hl1n.net; Richard Leitner
>
>Subject: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option
>
>Some PHYs (for example the LAN8710) doesn't allow turning the clocks off and
>on again without reset (according to their datasheet). Exactly this behaviour
>was introduced for power saving reasons by commit e8fcfcd5684a
>("net: fec: optimize the clock management to save power") Therefore add a
>devictree option to perform a PHY reset and configuration after every clock
>enable.
>
>For a better understanding here's a outline of the time response of the clock
>and reset lines before and after this patch:
>
> v--fec_probe() v--fec_enet_open()
> v v
> w/o patch eCLK:
>___|
> w/o patch nRST: __--
> w/o patch CONF:
>___XX___
>
> w/ patch eCLK:
>___|
> w/ patch nRST: __--__--
> w/ patch CONF:
>___XX__XX___
> ^ ^
> ^--fec_probe() ^--fec_enet_open()
>
>In our case this problem does occur at about every 10th to 50th POR of an
>LAN8710 connected to an i.MX6 SoC. The typical symptom of this problem is a
>"swinging" ethernet link. Similar issues were experienced by users of the NXP
>forum:
> https://community.nxp.com/thread/389902
> https://community.nxp.com/message/309354
>With this patch applied the issue didn't occur for at least a few hundred PORs
>of our board.
>
>Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
>Signed-off-by: Richard Leitner
>---
> Documentation/devicetree/bindings/net/fsl-fec.txt | 3 +++
> drivers/net/ethernet/freescale/fec.h | 1 +
> drivers/net/ethernet/freescale/fec_main.c | 16
> 3 files changed, 20 insertions(+)
>
>diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt
>b/Documentation/devicetree/bindings/net/fsl-fec.txt
>index 6f55bdd..1766579 100644
>--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
>+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>@@ -23,6 +23,9 @@ Optional properties:
> - phy-handle : phandle to the PHY device connected to this device.
> - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
> Use instead of phy-handle.
>+- phy-reset-after-clk-enable : If present then a phy reset and
>+configuration
>+ will be performed everytime after the clocks are enabled. This is
>+required
>+ for some PHYs to work properly.
> - fsl,num-tx-queues : The property is valid for enet-avb IP, which supports
> hw multi queues. Should specify the tx queue number, otherwise set tx
>queue
> number to 1.
>diff --git a/drivers/net/ethernet/freescale/fec.h
>b/drivers/net/ethernet/freescale/fec.h
>index 3da8084..d4f658a 100644
>--- a/drivers/net/ethernet/freescale/fec.h
>+++ b/drivers/net/ethernet/freescale/fec.h
>@@ -538,6 +538,7 @@ struct fec_enet_private {
> int phy_reset_duration;
> int phy_reset_post_delay;
> boolphy_reset_active_high;
>+ boolphy_reset_after_clk_enable;
>
> struct napi_struct napi;
> int csum_flags;
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index cbbf7b8..4e744f3 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -68,6 +68,7 @@
>
> static void set_multicast_list(struct net_device *ndev); static void
>fec_enet_itr_coal_init(struct net_device *ndev);
>+static int fec_reset_phy(struct net_device *ndev);
>
> #define DRIVER_NAME "fec"
>
>@@ -1862,6 +1863,18 @@ static int fec_enet_clk_enable(struct net_device
>*ndev, bool enable)
> ret = clk_prepare_enable(fep->clk_ref);
> if (ret)
> goto failed_clk_ref;
>+
>+ /* reset the phy after clk is enabled if it's configured */
>+ if (fep->phy_reset_after_clk_enable) {
>+ ret = fec_reset_phy(ndev);
>+ if (ret)
>+ goto failed_reset;
>+
RE: [PATCH V3 3/7] tty: serial: lpuart: add little endian 32 bit register support
From: Dong Aisheng Sent: Monday, June 12, 2017 11:37 PM
>To: linux-ser...@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
>gre...@linuxfoundation.org; jsl...@suse.com; Andy Duan
>; ste...@agner.ch; Mingkai Hu
>; Y.b. Lu ;
>nikita.yo...@cogentembedded.com; andy.shevche...@gmail.com;
>donga...@gmail.com; A.S. Dong
>Subject: [PATCH V3 3/7] tty: serial: lpuart: add little endian 32 bit register
>support
>
>Use standard port->iotype to distinguish endian difference. Note as we
>read/write register by checking iotype dynamically, we need to initialize the
>iotype correctly for earlycon as well to avoid a break.
>
>Cc: Greg Kroah-Hartman
>Cc: Jiri Slaby (supporter:TTY LAYER)
>Cc: Stefan Agner
>Cc: Mingkai Hu
>Cc: Yangbo Lu
>Cc: Fugang Duan
>Signed-off-by: Dong Aisheng
>
>ChangeLog:
>v2->v3:
> * Instead of using global var, use standard port->iotype to distinguish
> endian difference.
>v1->v2:
> * No changes
>---
> drivers/tty/serial/fsl_lpuart.c | 43 +++--
>
> 1 file changed, 29 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
>index
>ed9db2f..bbf47a0 100644
>--- a/drivers/tty/serial/fsl_lpuart.c
>+++ b/drivers/tty/serial/fsl_lpuart.c
>@@ -280,15 +280,29 @@ MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
> /* Forward declare this for the dma callbacks*/ static void
>lpuart_dma_tx_complete(void *arg);
>
>-static inline u32 lpuart32_read(struct uart_port *port, u32 reg_off) -{
>- return ioread32be(port->membase + reg_off);
>+static inline u32 lpuart32_read(struct uart_port *port, u32 off) {
>+ switch (port->iotype) {
>+ case UPIO_MEM32:
>+ return readl(port->membase + off);
>+ case UPIO_MEM32BE:
>+ return ioread32be(port->membase + off);
>+ default:
>+ return 0;
>+ };
> }
>
> static inline void lpuart32_write(struct uart_port *port, u32 val,
>-u32 reg_off)
>+u32 off)
> {
>- iowrite32be(val, port->membase + reg_off);
>+ switch (port->iotype) {
>+ case UPIO_MEM32:
>+ writel(val, port->membase + off);
>+ break;
>+ case UPIO_MEM32BE:
>+ iowrite32be(val, port->membase + off);
>+ break;
>+ };
> }
>
> static void lpuart_stop_tx(struct uart_port *port) @@ -602,7 +616,7 @@
>static irqreturn_t lpuart_txint(int irq, void *dev_id)
>
> spin_lock_irqsave(&sport->port.lock, flags);
> if (sport->port.x_char) {
>- if (sport->port.iotype & UPIO_MEM32BE)
>+ if (sport->port.iotype & (UPIO_MEM32 | UPIO_MEM32BE))
> lpuart32_write(&sport->port, sport->port.x_char,
>UARTDATA);
> else
> writeb(sport->port.x_char, sport->port.membase +
>UARTDR); @@ -610,14 +624,14 @@ static irqreturn_t lpuart_txint(int irq, void
>*dev_id)
> }
>
> if (uart_circ_empty(xmit) || uart_tx_stopped(&sport->port)) {
>- if (sport->port.iotype & UPIO_MEM32BE)
>+ if (sport->port.iotype & (UPIO_MEM32 | UPIO_MEM32BE))
Can use one macro instead of sport->port.iotype & (UPIO_MEM32 | UPIO_MEM32BE ?
> lpuart32_stop_tx(&sport->port);
> else
> lpuart_stop_tx(&sport->port);
> goto out;
> }
>
>- if (sport->port.iotype & UPIO_MEM32BE)
>+ if (sport->port.iotype & (UPIO_MEM32 | UPIO_MEM32BE))
> lpuart32_transmit_buffer(sport);
> else
> lpuart_transmit_buffer(sport);
>@@ -1890,12 +1904,12 @@ static int __init lpuart_console_setup(struct
>console *co, char *options)
> if (options)
> uart_parse_options(options, &baud, &parity, &bits, &flow);
> else
>- if (sport->port.iotype & UPIO_MEM32BE)
>+ if (sport->port.iotype & (UPIO_MEM32 | UPIO_MEM32BE))
> lpuart32_console_get_options(sport, &baud, &parity,
>&bits);
> else
> lpuart_console_get_options(sport, &baud, &parity,
>&bits);
>
>- if (sport->port.iotype & UPIO_MEM32BE)
>+ if (sport->port.iotype & (UPIO_MEM32 | UPIO_MEM32BE))
> lpuart32_setup_watermark(sport);
> else
> lpuart_setup_watermark(sport);
>@@ -1954,6 +1968,7 @@ static int __i
RE: [PATCH V3 5/7] tty: serial: lpuart: add imx7ulp support
From: Dong Aisheng Sent: Monday, June 12, 2017 11:37 PM
>The lpuart of imx7ulp is basically the same as ls1021a. It's also
>32 bit width register, but unlike ls1021a, it's little endian.
>Besides that, imx7ulp lpuart has a minor different register layout from ls1021a
>that it has four extra registers (verid, param, global,
>pincfg) located at the beginning of register map, which are currently not used
>by the driver and less to be used later.
>
>To ease the register difference handling, we add a reg_off member in
>lpuart_soc_data structure to represent if the normal lpuart32_{read|write}
>requires plus a offset to hide the issue.
>
>Cc: Greg Kroah-Hartman
>Cc: Jiri Slaby
>Cc: Stefan Agner
>Cc: Mingkai Hu
>Cc: Yangbo Lu
>Cc: Fugang Duan
>Signed-off-by: Dong Aisheng
>
>---
>ChangeLog:
>v2->v3:
> * use standard port->iotype to represent the endians.
>v1->v2:
> * remove lpuart_reg_off according to Stefan's suggestion
>---
> drivers/tty/serial/fsl_lpuart.c | 10 ++
> 1 file changed, 10 insertions(+)
>
>diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
>index
>bbf47a0..9d05e53 100644
>--- a/drivers/tty/serial/fsl_lpuart.c
>+++ b/drivers/tty/serial/fsl_lpuart.c
>@@ -231,6 +231,9 @@
> #define DEV_NAME "ttyLP"
> #define UART_NR 6
>
>+/* IMX lpuart has four extra unused regs located at the beginning */
>+#define IMX_REG_OFF 0x10
>+
> struct lpuart_port {
> struct uart_portport;
> struct clk *clk;
>@@ -259,6 +262,7 @@ struct lpuart_port {
>
> struct lpuart_soc_data {
> chariotype;
>+ u8 reg_off;
> };
>
> static const struct lpuart_soc_data vf_data = { @@ -267,12 +271,17 @@ static
>const struct lpuart_soc_data vf_data = {
>
> static const struct lpuart_soc_data ls_data = {
> .iotype = UPIO_MEM32BE,
>+};
>
>+static struct lpuart_soc_data imx_data = {
>+ .iotype = UPIO_MEM32,
>+ .reg_off = IMX_REG_OFF,
> };
>
> static const struct of_device_id lpuart_dt_ids[] = {
> { .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
> { .compatible = "fsl,ls1021a-lpuart", .data = &ls_data, },
>+ { .compatible = "fsl,imx7ulp-lpuart", .data = &imx_data, },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, lpuart_dt_ids); @@ -2020,6 +2029,7 @@ static
>int lpuart_probe(struct platform_device *pdev)
> if (IS_ERR(sport->port.membase))
> return PTR_ERR(sport->port.membase);
>
>+ sport->port.membase += sdata->reg_off;
> sport->port.mapbase = res->start;
Also update the mapbase.
> sport->port.dev = &pdev->dev;
> sport->port.type = PORT_LPUART;
>--
>2.7.4
RE: [PATCH v2] net: fec: add post PHY reset delay DT property
From: Florian Fainelli Sent: Thursday, June 01, 2017
9:53 AM
>To: Andy Duan ; Rob Herring ;
>Quentin Schulz
>Cc: mark.rutl...@arm.com; net...@vger.kernel.org;
>devicet...@vger.kernel.org; linux-kernel@vger.kernel.org;
>thomas.petazz...@free-electrons.com
>Subject: Re: [PATCH v2] net: fec: add post PHY reset delay DT property
>
>Le 05/31/17 à 18:39, Andy Duan a écrit :
>> From: Rob Herring Sent: Thursday, June 01, 2017
>> 12:44 AM
>>> On Tue, May 23, 2017 at 11:48:08AM +0200, Quentin Schulz wrote:
>>>> Some PHY require to wait for a bit after the reset GPIO has been
>>>> toggled. This adds support for the DT property
>>>> `phy-reset-post-delay` which gives the delay in milliseconds to wait after
>reset.
>>>>
>>>> If the DT property is not given, no delay is observed. Post reset
>>>> delay greater than 1000ms are invalid.
>>>>
>>>> Signed-off-by: Quentin Schulz
>>>> ---
>>>>
>>>> v2:
>>>> - return -EINVAL when phy-reset-post-delay is greater than 1000ms
>>>> instead of defaulting to 1ms,
>>>> - remove `default to 1ms` when phy-reset-post-delay > 1000Ms from DT
>>>> binding doc and commit log,
>>>> - move phy-reset-post-delay property reading before
>>>> devm_gpio_request_one(),
>>>>
>>>> Documentation/devicetree/bindings/net/fsl-fec.txt | 4
>>>> drivers/net/ethernet/freescale/fec_main.c | 16 +++-
>>>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt
>>>> b/Documentation/devicetree/bindings/net/fsl-fec.txt
>>>> index a1e3693cca16..6f55bdd52f8a 100644
>>>> --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
>>>> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>>>> @@ -15,6 +15,10 @@ Optional properties:
>>>> - phy-reset-active-high : If present then the reset sequence using the
>GPIO
>>>>specified in the "phy-reset-gpios" property is reversed (H=reset state,
>>>>L=operation state).
>>>> +- phy-reset-post-delay : Post reset delay in milliseconds. If
>>>> +present then
>>>
>>> This needs unit suffix minimally. It should also have a vendor prefix
>>> or be made generic.
>>>
>>> But really, this is a property of the phy and should be in the phy
>>> node as should phy-reset-gpios, phy-reset-active-high, phy-supply, etc.
>>>
>> Yes, it is better to make it general.
>> Last year, Uwe Kleine-König's patch "Commit da47b4572056 ("phy: add
>support for a reset-gpio specification")" did this, but it was reverted by
>commit 948350140ef0 (Revert "phy: add support for a reset-gpio
>specification"). And in all phy device driver, only at803x.c add the gpio
>reset in
>currently.
>
>Getting the binding correct does not prevent us from later moving this reset
>code into PHYLIB where it's appropriate. In fact; a correct and generic binding
>proposed for FEC here could be used as a basis for all other MAC and PHY
>drivers.
>--
>Florian
I agree with your opinion. Just hope to add the general phy reset interface in
phylib and special device driver.
Andy
RE: [PATCH v2] net: fec: add post PHY reset delay DT property
From: Rob Herring Sent: Thursday, June 01, 2017 12:44 AM
>On Tue, May 23, 2017 at 11:48:08AM +0200, Quentin Schulz wrote:
>> Some PHY require to wait for a bit after the reset GPIO has been
>> toggled. This adds support for the DT property `phy-reset-post-delay`
>> which gives the delay in milliseconds to wait after reset.
>>
>> If the DT property is not given, no delay is observed. Post reset
>> delay greater than 1000ms are invalid.
>>
>> Signed-off-by: Quentin Schulz
>> ---
>>
>> v2:
>> - return -EINVAL when phy-reset-post-delay is greater than 1000ms
>> instead of defaulting to 1ms,
>> - remove `default to 1ms` when phy-reset-post-delay > 1000Ms from DT
>> binding doc and commit log,
>> - move phy-reset-post-delay property reading before
>> devm_gpio_request_one(),
>>
>> Documentation/devicetree/bindings/net/fsl-fec.txt | 4
>> drivers/net/ethernet/freescale/fec_main.c | 16 +++-
>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt
>> b/Documentation/devicetree/bindings/net/fsl-fec.txt
>> index a1e3693cca16..6f55bdd52f8a 100644
>> --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
>> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>> @@ -15,6 +15,10 @@ Optional properties:
>> - phy-reset-active-high : If present then the reset sequence using the GPIO
>>specified in the "phy-reset-gpios" property is reversed (H=reset state,
>>L=operation state).
>> +- phy-reset-post-delay : Post reset delay in milliseconds. If present
>> +then
>
>This needs unit suffix minimally. It should also have a vendor prefix or be
>made generic.
>
>But really, this is a property of the phy and should be in the phy node as
>should phy-reset-gpios, phy-reset-active-high, phy-supply, etc.
>
Yes, it is better to make it general.
Last year, Uwe Kleine-König's patch "Commit da47b4572056 ("phy: add support for
a reset-gpio specification")" did this, but it was reverted by commit
948350140ef0 (Revert "phy: add support for a reset-gpio specification"). And
in all phy device driver, only at803x.c add the gpio reset in currently.
RE: [PATCH v2] net: fec: add post PHY reset delay DT property
From: Quentin Schulz Sent: Tuesday, May 23,
2017 5:48 PM
>Some PHY require to wait for a bit after the reset GPIO has been toggled. This
>adds support for the DT property `phy-reset-post-delay` which gives the delay
>in milliseconds to wait after reset.
>
>If the DT property is not given, no delay is observed. Post reset delay greater
>than 1000ms are invalid.
>
>Signed-off-by: Quentin Schulz
>---
>
>v2:
> - return -EINVAL when phy-reset-post-delay is greater than 1000ms
> instead of defaulting to 1ms,
> - remove `default to 1ms` when phy-reset-post-delay > 1000Ms from DT
> binding doc and commit log,
> - move phy-reset-post-delay property reading before
> devm_gpio_request_one(),
>
> Documentation/devicetree/bindings/net/fsl-fec.txt | 4
> drivers/net/ethernet/freescale/fec_main.c | 16 +++-
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
>diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt
>b/Documentation/devicetree/bindings/net/fsl-fec.txt
>index a1e3693cca16..6f55bdd52f8a 100644
>--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
>+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>@@ -15,6 +15,10 @@ Optional properties:
> - phy-reset-active-high : If present then the reset sequence using the GPIO
> specified in the "phy-reset-gpios" property is reversed (H=reset state,
> L=operation state).
>+- phy-reset-post-delay : Post reset delay in milliseconds. If present
>+then
>+ a delay of phy-reset-post-delay milliseconds will be observed after
>+the
>+ phy-reset-gpios has been toggled. Can be omitted thus no delay is
>+ observed. Delay is in range of 1ms to 1000ms. Other delays are invalid.
> - phy-supply : regulator that powers the Ethernet PHY.
> - phy-handle : phandle to the PHY device connected to this device.
> - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 56a563f90b0b..f7c8649fd28f 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -3192,7 +3192,7 @@ static int fec_reset_phy(struct platform_device
>*pdev) {
> int err, phy_reset;
> bool active_high = false;
>- int msec = 1;
>+ int msec = 1, phy_post_delay = 0;
> struct device_node *np = pdev->dev.of_node;
>
> if (!np)
>@@ -3209,6 +3209,11 @@ static int fec_reset_phy(struct platform_device
>*pdev)
> else if (!gpio_is_valid(phy_reset))
> return 0;
>
>+ err = of_property_read_u32(np, "phy-reset-post-delay",
>&phy_post_delay);
>+ /* valid reset duration should be less than 1s */
>+ if (!err && phy_post_delay > 1000)
>+ return -EINVAL;
>+
> active_high = of_property_read_bool(np, "phy-reset-active-high");
>
> err = devm_gpio_request_one(&pdev->dev, phy_reset, @@ -3226,6
>+3231,15 @@ static int fec_reset_phy(struct platform_device *pdev)
>
> gpio_set_value_cansleep(phy_reset, !active_high);
>
>+ if (!phy_post_delay)
>+ return 0;
>+
>+ if (phy_post_delay > 20)
>+ msleep(phy_post_delay);
>+ else
>+ usleep_range(phy_post_delay * 1000,
>+ phy_post_delay * 1000 + 1000);
>+
> return 0;
> }
> #else /* CONFIG_OF */
>--
>2.11.0
It looks fine.
Acked-by: Fugang Duan
RE: [PATCH] net: fec: add post PHY reset delay DT property
From: Quentin Schulz Sent: Monday, May 22,
2017 5:15 PM
>Some PHY require to wait for a bit after the reset GPIO has been toggled. This
>adds support for the DT property `phy-reset-post-delay` which gives the delay
>in milliseconds to wait after reset.
>
>If the DT property is not given, no delay is observed. Post reset delay greater
>than 1000ms are invalid and are default to 1ms.
>
>Signed-off-by: Quentin Schulz
>---
> Documentation/devicetree/bindings/net/fsl-fec.txt | 5 +
> drivers/net/ethernet/freescale/fec_main.c | 17 +++--
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt
>b/Documentation/devicetree/bindings/net/fsl-fec.txt
>index a1e3693cca16..8795e8ca5793 100644
>--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
>+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>@@ -15,6 +15,11 @@ Optional properties:
> - phy-reset-active-high : If present then the reset sequence using the GPIO
> specified in the "phy-reset-gpios" property is reversed (H=reset state,
> L=operation state).
>+- phy-reset-post-delay : Post reset delay in milliseconds. If present
>+then
>+ a delay of phy-reset-post-delay milliseconds will be observed after
>+the
>+ phy-reset-gpios has been toggled. Can be omitted thus no delay is
>+ observed. Delay is in range of 1ms to 1000ms. If given delay is
>+higher
>+ than 1000ms, 1ms delay is done instead.
> - phy-supply : regulator that powers the Ethernet PHY.
> - phy-handle : phandle to the PHY device connected to this device.
> - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 56a563f90b0b..00a7fd0bcd59 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -3192,7 +3192,7 @@ static int fec_reset_phy(struct platform_device
>*pdev) {
> int err, phy_reset;
> bool active_high = false;
>- int msec = 1;
>+ int msec = 1, phy_post_delay = 0;
> struct device_node *np = pdev->dev.of_node;
>
> if (!np)
>@@ -3210,7 +3210,6 @@ static int fec_reset_phy(struct platform_device
>*pdev)
> return 0;
>
> active_high = of_property_read_bool(np, "phy-reset-active-high");
>-
No necessary change here.
> err = devm_gpio_request_one(&pdev->dev, phy_reset,
> active_high ? GPIOF_OUT_INIT_HIGH :
>GPIOF_OUT_INIT_LOW,
> "phy-reset");
>@@ -3219,6 +3218,11 @@ static int fec_reset_phy(struct platform_device
>*pdev)
> return err;
> }
>
>+ err = of_property_read_u32(np, "phy-reset-post-delay",
>&phy_post_delay);
>+ /* valid reset duration should be less than 1s */
>+ if (!err && phy_post_delay > 1000)
>+ phy_post_delay = 1;
>+
Put the dt parse before . devm_gpio_request_one() that seems better.
Others are fine.
> if (msec > 20)
> msleep(msec);
> else
>@@ -3226,6 +3230,15 @@ static int fec_reset_phy(struct platform_device
>*pdev)
>
> gpio_set_value_cansleep(phy_reset, !active_high);
>
>+ if (!phy_post_delay)
>+ return 0;
>+
>+ if (phy_post_delay > 20)
>+ msleep(phy_post_delay);
>+ else
>+ usleep_range(phy_post_delay * 1000,
>+ phy_post_delay * 1000 + 1000);
>+
> return 0;
> }
> #else /* CONFIG_OF */
>--
>2.11.0
RE: [PATCH] net: fec: select queue depending on VLAN priority
From: Stefan Agner Sent: Monday, May 15, 2017 1:39 PM
>To: Andy Duan
>Cc: David Miller ; and...@lunn.ch;
>feste...@gmail.com; net...@vger.kernel.org; linux-
>ker...@vger.kernel.org
>Subject: Re: [PATCH] net: fec: select queue depending on VLAN priority
>
>On 2017-05-10 21:49, Andy Duan wrote:
>> From: Stefan Agner Sent: Thursday, May 11, 2017
>> 12:08 PM
>>>To: Andy Duan
>>>Cc: David Miller ; and...@lunn.ch;
>>>feste...@gmail.com; net...@vger.kernel.org; linux-
>>>ker...@vger.kernel.org
>>>Subject: Re: [PATCH] net: fec: select queue depending on VLAN priority
>>>
>>>On 2017-05-09 19:42, Andy Duan wrote:
>>>> From: David Miller Sent: Tuesday, May 09, 2017
>>>> 9:39 PM
>>>>>To: ste...@agner.ch
>>>>>Cc: Andy Duan ; and...@lunn.ch;
>>>>>feste...@gmail.com; net...@vger.kernel.org; linux-
>>>>>ker...@vger.kernel.org
>>>>>Subject: Re: [PATCH] net: fec: select queue depending on VLAN
>>>>>priority
>>>>>
>>>>>From: Stefan Agner
>>>>>Date: Mon, 8 May 2017 22:37:08 -0700
>>>>>
>>>>>> Since the addition of the multi queue code with commit
>>>>>> 59d0f7465644
>>>>>> ("net: fec: init multi queue date structure") the queue selection
>>>>>> has been handelt by the default transmit queue selection
>>>>>> implementation which tries to evenly distribute the traffic across
>>>>>> all available queues. This selection presumes that the queues are
>>>>>> using an equal priority, however, the queues 1 and 2 are actually
>>>>>> of higher priority (the classification of the queues is enabled in
>>>fec_enet_enable_ring).
>>>>>>
>>>>>> This can lead to net scheduler warnings and continuous TX ring
>>>>>> dumps when exercising the system with iperf.
>>>>>>
>>>>>> Use only queue 0 for all common traffic (no VLAN and P802.1p
>>>>>> priority
>>>>>> 0 and 1) and route level 2-7 through queue 1 and 2.
>>>>>>
>>>>>> Signed-off-by: Fugang Duan
>>>>>> Fixes: 59d0f7465644 ("net: fec: init multi queue date structure")
>>>>>
>>>>>If the queues are used for prioritization, and it does not have
>>>>>multiple normal priority level queues, multiqueue is not what the
>>>>>driver should have implemented.
>>>> Firstly, HW multiple queues support:
>>>>- Traffic-shaping bandwidth distribution supports credit-based and
>>>> round-robin-based policies. Either policy can be combined with
>>>> time-based shaping.
>>>>- AVB (Audio Video Bridging, IEEE 802.1Qav) features:
>>>>* Credit-based bandwidth distribution policy can be combined
>>>with
>>>> time-based shaping
>>>>* AVB endpoint talker and listener support
>>>>* Support for arbitration between different priority traffic
>>>> (for
>>>> example, AVB class A, AVB class B, and non-AVB) Round-robin-based
>>>> policies:
>>>>It has the same priority for three queues: In the round-robin QoS
>>>> scheme, each queue is given an equal opportunity to transmit one
>>>> frame. For example, if queue n has a frame to transmit, the queue
>>>> transmits its frame. After queue n has transmitted its frame, or if
>>>> queue n does not have a frame to transmit, queue n+1 is then allowed
>>>> to transmit its frame, and so on.
>>>>
>>>> Credit-based policies:
>>>>The AVB credit based shaper acts independently, per class, to
>>>> control the bandwidth distribution between normal traffic and
>>>> time-sensitive traffic with respect to the total link bandwidth available.
>>>>Credit based shaper conbined with time-based shaping:
>>>>- priority: ClassA queue > ClassB queue > best effort
>>>>- ensure the queue bandwidth as user set based on time-
>>>based shaping
>>>> algorithms (transmitter transmit frame from three queue in turn
>>>> based on time-based shaping algorithms)
>>>>And in real AVB case, each streaming can be independent, and are
>>>> fixed on related queue. Then driver level should implement
>>>> .ndo_select_queue() to put the streaming into related queue. That
RE: [PATCH] net: fec: select queue depending on VLAN priority
From: Stefan Agner Sent: Thursday, May 11, 2017 12:08 PM
>To: Andy Duan
>Cc: David Miller ; and...@lunn.ch;
>feste...@gmail.com; net...@vger.kernel.org; linux-
>ker...@vger.kernel.org
>Subject: Re: [PATCH] net: fec: select queue depending on VLAN priority
>
>On 2017-05-09 19:42, Andy Duan wrote:
>> From: David Miller Sent: Tuesday, May 09, 2017
>> 9:39 PM
>>>To: ste...@agner.ch
>>>Cc: Andy Duan ; and...@lunn.ch;
>>>feste...@gmail.com; net...@vger.kernel.org; linux-
>>>ker...@vger.kernel.org
>>>Subject: Re: [PATCH] net: fec: select queue depending on VLAN priority
>>>
>>>From: Stefan Agner
>>>Date: Mon, 8 May 2017 22:37:08 -0700
>>>
>>>> Since the addition of the multi queue code with commit 59d0f7465644
>>>> ("net: fec: init multi queue date structure") the queue selection
>>>> has been handelt by the default transmit queue selection
>>>> implementation which tries to evenly distribute the traffic across
>>>> all available queues. This selection presumes that the queues are
>>>> using an equal priority, however, the queues 1 and 2 are actually of
>>>> higher priority (the classification of the queues is enabled in
>fec_enet_enable_ring).
>>>>
>>>> This can lead to net scheduler warnings and continuous TX ring dumps
>>>> when exercising the system with iperf.
>>>>
>>>> Use only queue 0 for all common traffic (no VLAN and P802.1p
>>>> priority
>>>> 0 and 1) and route level 2-7 through queue 1 and 2.
>>>>
>>>> Signed-off-by: Fugang Duan
>>>> Fixes: 59d0f7465644 ("net: fec: init multi queue date structure")
>>>
>>>If the queues are used for prioritization, and it does not have
>>>multiple normal priority level queues, multiqueue is not what the
>>>driver should have implemented.
>> Firstly, HW multiple queues support:
>> - Traffic-shaping bandwidth distribution supports credit-based and
>> round-robin-based policies. Either policy can be combined with
>> time-based shaping.
>> - AVB (Audio Video Bridging, IEEE 802.1Qav) features:
>> * Credit-based bandwidth distribution policy can be combined
>with
>> time-based shaping
>> * AVB endpoint talker and listener support
>> * Support for arbitration between different priority traffic
>> (for
>> example, AVB class A, AVB class B, and non-AVB) Round-robin-based
>> policies:
>> It has the same priority for three queues: In the round-robin QoS
>> scheme, each queue is given an equal opportunity to transmit one
>> frame. For example, if queue n has a frame to transmit, the queue
>> transmits its frame. After queue n has transmitted its frame, or if
>> queue n does not have a frame to transmit, queue n+1 is then allowed
>> to transmit its frame, and so on.
>>
>> Credit-based policies:
>> The AVB credit based shaper acts independently, per class, to control
>> the bandwidth distribution between normal traffic and time-sensitive
>> traffic with respect to the total link bandwidth available.
>> Credit based shaper conbined with time-based shaping:
>> - priority: ClassA queue > ClassB queue > best effort
>> - ensure the queue bandwidth as user set based on time-
>based shaping
>> algorithms (transmitter transmit frame from three queue in turn based
>> on time-based shaping algorithms)
>> And in real AVB case, each streaming can be independent, and are
>> fixed on related queue. Then driver level should implement
>> .ndo_select_queue() to put the streaming into related queue. That is
>> what the patch did.
>>
>> The current driver config the three queue to credit-based policies
>> (AVB), the patch seems no problem for the implementation. Do you have
>> any suggestion ?
>>
>
>I tried using the round robin mode by adding this:
>
>+ /* Set Round-Robin policy */
>+ writel(1, fep->hwp + FEC_QOS_SCHEME);
>
>After a while I got the warning non the less:
>
>WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:316
>dev_watchdog+0x248/0x24c NETDEV WATCHDOG: eth0 (fec): transmit queue
>2 timed out Modules linked in:
>CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.11.0-rc1-00058-g56d22eced8bc-
>dirty #377 Hardware name: Freescale i.MX7 Dual (Device Tree) []
>(unwind_backtrace) from [] (show_stack+0x10/0x14) []
>(show_stack) from [] (dump_stack+0x78/0x8c) []
>(dump_stack) from [] (__warn+0xe8/0x100) []
>(__warn) from [] (warn_sl
RE: [PATCH] net: fec: select queue depending on VLAN priority
From: David Miller Sent: Tuesday, May 09, 2017 9:39 PM
>To: ste...@agner.ch
>Cc: Andy Duan ; and...@lunn.ch;
>feste...@gmail.com; net...@vger.kernel.org; linux-
>ker...@vger.kernel.org
>Subject: Re: [PATCH] net: fec: select queue depending on VLAN priority
>
>From: Stefan Agner
>Date: Mon, 8 May 2017 22:37:08 -0700
>
>> Since the addition of the multi queue code with commit 59d0f7465644
>> ("net: fec: init multi queue date structure") the queue selection has
>> been handelt by the default transmit queue selection implementation
>> which tries to evenly distribute the traffic across all available
>> queues. This selection presumes that the queues are using an equal
>> priority, however, the queues 1 and 2 are actually of higher priority
>> (the classification of the queues is enabled in fec_enet_enable_ring).
>>
>> This can lead to net scheduler warnings and continuous TX ring dumps
>> when exercising the system with iperf.
>>
>> Use only queue 0 for all common traffic (no VLAN and P802.1p priority
>> 0 and 1) and route level 2-7 through queue 1 and 2.
>>
>> Signed-off-by: Fugang Duan
>> Fixes: 59d0f7465644 ("net: fec: init multi queue date structure")
>
>If the queues are used for prioritization, and it does not have multiple normal
>priority level queues, multiqueue is not what the driver should have
>implemented.
Firstly, HW multiple queues support:
- Traffic-shaping bandwidth distribution supports credit-based and
round-robin-based policies. Either policy can be combined with time-based
shaping.
- AVB (Audio Video Bridging, IEEE 802.1Qav) features:
* Credit-based bandwidth distribution policy can be combined
with time-based shaping
* AVB endpoint talker and listener support
* Support for arbitration between different priority traffic
(for example, AVB class A, AVB class B, and non-AVB)
Round-robin-based policies:
It has the same priority for three queues: In the round-robin QoS
scheme, each queue is given an equal opportunity to transmit one frame. For
example, if queue n has a frame to transmit, the queue transmits its frame.
After queue n has transmitted its frame, or if queue n does not have a frame to
transmit, queue n+1 is then allowed to transmit its frame, and so on.
Credit-based policies:
The AVB credit based shaper acts independently, per class, to control
the bandwidth distribution between normal traffic and time-sensitive traffic
with respect to the total link bandwidth available.
Credit based shaper conbined with time-based shaping:
- priority: ClassA queue > ClassB queue > best effort
- ensure the queue bandwidth as user set based on time-based
shaping algorithms (transmitter transmit frame from three queue in turn based
on time-based shaping algorithms)
And in real AVB case, each streaming can be independent, and are fixed
on related queue. Then driver level should implement .ndo_select_queue() to put
the streaming into related queue. That is what the patch did.
The current driver config the three queue to credit-based policies (AVB), the
patch seems no problem for the implementation. Do you have any suggestion ?
Andy
RE: [PATCH 0/6] tty: serial: lpuart: add imx7ulp support
From: Dong Aisheng Sent: Tuesday, May 09, 2017 3:51 PM >To: linux-ser...@vger.kernel.org >Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; >gre...@linuxfoundation.org; jsl...@suse.com; Andy Duan >; ste...@agner.ch; Mingkai Hu >; Y.B. Lu ; A.S. Dong > >Subject: [PATCH 0/6] tty: serial: lpuart: add imx7ulp support > >This patch series mainly intends to add imx7ulp support which is also using FSL >lpuart. > >The lpuart in imx7ulp is basically the same as ls1021a. It's also >32 bit width register, but unlike ls1021a, it's little endian. >Besides that, imx7ulp lpuart has a minor different register layout from ls1021a >that it has four extra registers (verid, param, global, >pincfg) located at the beginning of register map, which are currently not used >by the driver and less to be used later. > >Furthermore, this patch serial also add a new more accurate baud rate >calculation method as MX7ULP can't divide a suitable baud rate with the >default setting. > >Currently the new baud rate calculation is only enabled on MX7ULP. >However, i guess the Layerscape may also be able to use it as there seems to >be no difference in baud rate setting register after checking the Layerscape >Reference Manual. > >As i don't have Layerscape boards, i can't test it, so i only enable it for >MX7ULP >by default to avoid a potential break. > >I copied LayerScape guys in this series and hope they can help test later. >If it works on Layerscape as well, then they can switch to the new setting too >and totally remove the old stuff. > >Dong Aisheng (6): > tty: serial: lpuart: introduce lpuart_soc_data to represent SoC >property > tty: serial: lpuart: add little endian 32 bit register support > dt-bindings: serial: fsl-lpuart: add i.MX7ULP support > tty: serial: lpuart: add imx7ulp support > tty: serial: lpuart: add earlycon support for imx7ulp > tty: serial: lpuart: add a more accurate baud rate calculation method > > .../devicetree/bindings/serial/fsl-lpuart.txt | 2 + > drivers/tty/serial/fsl_lpuart.c| 149 ++--- > 2 files changed, 136 insertions(+), 15 deletions(-) > >-- >2.7.4 The series looks fine. Acked-by: Fugang Duan
RE: [PATCH] ARM: imx_v6_v7_defconfig: Select SMSC_PHY
From: Leonard Crestez Sent: Wednesday, March 22, 2017 10:28 PM >To: Shawn Guo ; Sascha Hauer > >Cc: Leonard Crestez ; linux-arm- >ker...@lists.infradead.org; Fabio Estevam ; Andy >Duan ; Octavian Purdila >; Florian Fainelli ; linux- >ker...@vger.kernel.org >Subject: [PATCH] ARM: imx_v6_v7_defconfig: Select SMSC_PHY > >The imx6sl-evk board has a LAN8720A ethernet phy supported by SMSC_PHY. >Add this driver to the default imx config since the device is present on one of >the evaluation boards. > >This used to work mostly fine with the generic phy driver but since commit >0878fff1f42c18e448ab5b8b4f6a3eb32365b5b6 that driver no longer performs a >soft reset on startup. This causes netboot to sometimes timeout on DHCP >because RX doesn't work right. DHCP is eventually retried and it works the >second time but it takes 90+ seconds to get a login prompt. > >This was generated with "make savedefconfig" and it includes a few additional >minor cleanups. > >Signed-off-by: Leonard Crestez >--- > >I also tried to do some debugging in the fec driver and it apparently receives >corrupted packets when this happens. If I hack it to go into promiscuous mode >unconditionally it gets a whole bunch of rx errors (crc errors, length errors >and >so on). So the phy config is probably wrong and is confusing the mac? > >In theory it might be possible to make that driver "just work" with phy >settings >from uboot but it's not clear it's worthwhile. > > arch/arm/configs/imx_v6_v7_defconfig | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > >diff --git a/arch/arm/configs/imx_v6_v7_defconfig >b/arch/arm/configs/imx_v6_v7_defconfig >index eaba3b1..e605389 100644 >--- a/arch/arm/configs/imx_v6_v7_defconfig >+++ b/arch/arm/configs/imx_v6_v7_defconfig >@@ -143,6 +143,7 @@ CONFIG_SMSC911X=y > # CONFIG_NET_VENDOR_STMICRO is not set > CONFIG_AT803X_PHY=y > CONFIG_MICREL_PHY=y >+CONFIG_SMSC_PHY=y > CONFIG_USB_PEGASUS=m > CONFIG_USB_RTL8150=m > CONFIG_USB_RTL8152=m >@@ -152,7 +153,6 @@ CONFIG_BRCMFMAC=m > CONFIG_WL12XX=m > CONFIG_WLCORE_SDIO=m > # CONFIG_WILINK_PLATFORM_DATA is not set -# >CONFIG_INPUT_MOUSEDEV_PSAUX is not set CONFIG_INPUT_EVDEV=y >CONFIG_INPUT_EVBUG=m CONFIG_KEYBOARD_GPIO=y @@ -376,7 +376,6 >@@ CONFIG_NLS_ISO8859_1=y CONFIG_NLS_ISO8859_15=m >CONFIG_NLS_UTF8=y CONFIG_PRINTK_TIME=y -CONFIG_DEBUG_FS=y >CONFIG_MAGIC_SYSRQ=y # CONFIG_SCHED_DEBUG is not set >CONFIG_PROVE_LOCKING=y >-- >2.7.4 Acked-by: Fugang Duan
RE: [patch net] net: fec: fix compile with CONFIG_M5272
From: Nikita Yushchenko Sent: Sunday,
December 04, 2016 11:18 PM
>To: David S. Miller ; Andy Duan
>; Troy Kisky ;
>Andrew Lunn ; Eric Nelson ; Philippe
>Reynes ; Johannes Berg ;
>net...@vger.kernel.org
>Cc: Chris Healy ; Fabio Estevam
>; linux-kernel@vger.kernel.org; Nikita
>Yushchenko
>Subject: [patch net] net: fec: fix compile with CONFIG_M5272
>
>Commit 4dfb80d18d05 ("net: fec: cache statistics while device is down")
>introduced unconditional statistics-related actions.
>
>However, when driver is compiled with CONFIG_M5272, staticsics-related
>definitions do not exist, which results into build errors.
>
>Fix that by adding needed #if !defined(CONFIG_M5272).
>
>Fixes: 4dfb80d18d05 ("net: fec: cache statistics while device is down")
>Signed-off-by: Nikita Yushchenko
>---
> drivers/net/ethernet/freescale/fec_main.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 6a20c24a2003..89e902767abb 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -2884,7 +2884,9 @@ fec_enet_close(struct net_device *ndev)
> if (fep->quirks & FEC_QUIRK_ERR006687)
> imx6q_cpuidle_fec_irqs_unused();
>
>+#if !defined(CONFIG_M5272)
> fec_enet_update_ethtool_stats(ndev);
>+#endif
It is better to define fec_enet_update_ethtool_stats() for CONFIG_M5272 like
static void fec_enet_update_ethtool_stats(struct net_device *dev){}, or
directly return in .fec_enet_update_ethtool_stats():
@@ -2315,6 +2315,10 @@ static void fec_enet_update_ethtool_stats(struct
net_device *dev)
struct fec_enet_private *fep = netdev_priv(dev);
int i;
+#if defined(CONFIG_M5272)
+ return;
+#endif
+
>
> fec_enet_clk_enable(ndev, false);
> pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>@@ -3192,7 +3194,9 @@ static int fec_enet_init(struct net_device *ndev)
>
> fec_restart(ndev);
>
>+#if !defined(CONFIG_M5272)
> fec_enet_update_ethtool_stats(ndev);
>+#endif
>
ditto
> return 0;
> }
>@@ -3292,9 +3296,11 @@ fec_probe(struct platform_device *pdev)
> fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>
> /* Init network device */
>- ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private) +
>- ARRAY_SIZE(fec_stats) * sizeof(u64),
>- num_tx_qs, num_rx_qs);
>+ ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private) #if
>+!defined(CONFIG_M5272)
>+ + ARRAY_SIZE(fec_stats) * sizeof(u64)
>#endif
>+ , num_tx_qs, num_rx_qs);
> if (!ndev)
> return -ENOMEM;
>
>--
>2.1.4
RE: [patch net v2] net: fec: fix compile with CONFIG_M5272
From: Nikita Yushchenko Sent: Monday,
December 05, 2016 4:16 PM
>To: David S. Miller ; Andy Duan
>; Troy Kisky ;
>Andrew Lunn ; Eric Nelson ; Philippe
>Reynes ; Johannes Berg ;
>net...@vger.kernel.org
>Cc: Chris Healy ; Fabio Estevam
>; linux-kernel@vger.kernel.org; Nikita
>Yushchenko
>Subject: [patch net v2] net: fec: fix compile with CONFIG_M5272
>
>Commit 4dfb80d18d05 ("net: fec: cache statistics while device is down")
>introduced unconditional statistics-related actions.
>
>However, when driver is compiled with CONFIG_M5272, staticsics-related
>definitions do not exist, which results into build errors.
>
>Fix that by adding explicit handling of !defined(CONFIG_M5272) case.
>
>Fixes: 4dfb80d18d05 ("net: fec: cache statistics while device is down")
>Signed-off-by: Nikita Yushchenko
>---
>Changes since v1:
>- instead of #ifdef'ing calls to fec_enet_update_ethtool_stats(), add
> definition of empty fec_enet_update_ethtool_stats() for CONFIG_M5272
> case,
>- add FEC_STATS_SIZE macro to avoid #ifdef in the middle of
> alloc_etherdev_mqs() args.
>
> drivers/net/ethernet/freescale/fec_main.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
Acked-by: Fugang Duan
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 5f77caa59534..741cf4a57bfc 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -2313,6 +2313,8 @@ static const struct fec_stat {
> { "IEEE_rx_octets_ok", IEEE_R_OCTETS_OK }, };
>
>+#define FEC_STATS_SIZE (ARRAY_SIZE(fec_stats) * sizeof(u64))
>+
> static void fec_enet_update_ethtool_stats(struct net_device *dev) {
> struct fec_enet_private *fep = netdev_priv(dev); @@ -2330,7
>+2332,7 @@ static void fec_enet_get_ethtool_stats(struct net_device *dev,
> if (netif_running(dev))
> fec_enet_update_ethtool_stats(dev);
>
>- memcpy(data, fep->ethtool_stats, ARRAY_SIZE(fec_stats) *
>sizeof(u64));
>+ memcpy(data, fep->ethtool_stats, FEC_STATS_SIZE);
> }
>
> static void fec_enet_get_strings(struct net_device *netdev, @@ -2355,6
>+2357,12 @@ static int fec_enet_get_sset_count(struct net_device *dev, int
>sset)
> return -EOPNOTSUPP;
> }
> }
>+
>+#else/* !defined(CONFIG_M5272) */
>+#define FEC_STATS_SIZE 0
>+static inline void fec_enet_update_ethtool_stats(struct net_device
>+*dev) { }
> #endif /* !defined(CONFIG_M5272) */
>
> static int fec_enet_nway_reset(struct net_device *dev) @@ -3293,8
>+3301,7 @@ fec_probe(struct platform_device *pdev)
>
> /* Init network device */
> ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private) +
>- ARRAY_SIZE(fec_stats) * sizeof(u64),
>- num_tx_qs, num_rx_qs);
>+ FEC_STAT_SIZE, num_tx_qs, num_rx_qs);
> if (!ndev)
> return -ENOMEM;
>
>--
>2.1.4
