Re: [PATCH v3 0/2] quota: Add mountpath based quota support
On Tue, Mar 16, 2021 at 12:29:16PM +0100, Jan Kara wrote: > On Thu 04-03-21 13:35:38, Sascha Hauer wrote: > > Current quotactl syscall uses a path to a block device to specify the > > filesystem to work on which makes it unsuitable for filesystems that > > do not have a block device. This series adds a new syscall quotactl_path() > > which replaces the path to the block device with a mountpath, but otherwise > > behaves like original quotactl. > > > > This is done to add quota support to UBIFS. UBIFS quota support has been > > posted several times with different approaches to put the mountpath into > > the existing quotactl() syscall until it has been suggested to make it a > > new syscall instead, so here it is. > > > > I'm not posting the full UBIFS quota series here as it remains unchanged > > and I'd like to get feedback to the new syscall first. For those interested > > the most recent series can be found here: https://lwn.net/Articles/810463/ > > Thanks. I've merged the two patches into my tree and will push them to > Linus for the next merge window. Thanks by the way. Now that these patches are merged I'll respin my UBIFS quota series soon. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] ARM: dts: imx6ull: fix ubi mount failed on MYS-6ULX-IOT board
On Wed, Mar 10, 2021 at 10:54:05AM +0800, dillon min wrote: > Hi Sascha, > > Thanks for reviewing. > > On Tue, Mar 9, 2021 at 8:18 PM Sascha Hauer wrote: > > > > On Tue, Mar 09, 2021 at 02:15:19PM +0800, dillon.min...@gmail.com wrote: > > > From: dillon min > > > > > > This patch intend to fix ubi filesystem mount failed on MYS-6ULX-IOT > > > board, > > > from Micron MT29F2G08ABAEAWP's datasheets, we need to choose 4-bit ECC. > > > > > > Table 18: Error Management Details > > > > > > Description Requirement > > > > > > Minimum number of valid blocks (NVB) per LUN 2008 > > > Total available blocks per LUN2048 > > > First spare area location x8: byte 2048 x16: word 1024 > > > Bad-block markx8: 00h x16: h > > > Minimum required ECC 4-bit ECC per 528 bytes > > > Minimum ECC with internal ECC enabled 4-bit ECC per 516 bytes > > > (user data) + 8 > > > bytes (parity data) > > > Minimum required ECC for block 0 if PROGRAM/ > > > ERASE cycles are less than 1000 1-bit ECC per 528 > > > bytes > > > > 4-bit ECC is the minimum this chip requires. There's nothing wrong with > > choosing a better ECC like the GPMI driver does by default. > > > Yes, indeed, the mt29f2g08's minimum ecc is 4-bit, you can use 8-bits ecc. > but there is a dependency between new kernel gpmi-nand with the old > mfg-kernel's , which means > if the old nand ecc layout is 4-bits, you should use ecc 4-bit in the > new kernel (by fsl,use-minimum-ecc), > else use 8-bits. > > For my case, the ubifs filesystem was created by ecc 4-bits, without > reflash filesystem or change Then this is the justification for this patch, not anything from the datasheet like you've written in your commit message. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] ARM: dts: imx6ull: fix ubi mount failed on MYS-6ULX-IOT board
On Tue, Mar 09, 2021 at 02:15:19PM +0800, dillon.min...@gmail.com wrote: > From: dillon min > > This patch intend to fix ubi filesystem mount failed on MYS-6ULX-IOT board, > from Micron MT29F2G08ABAEAWP's datasheets, we need to choose 4-bit ECC. > > Table 18: Error Management Details > > Description Requirement > > Minimum number of valid blocks (NVB) per LUN 2008 > Total available blocks per LUN2048 > First spare area location x8: byte 2048 x16: word 1024 > Bad-block markx8: 00h x16: h > Minimum required ECC 4-bit ECC per 528 bytes > Minimum ECC with internal ECC enabled 4-bit ECC per 516 bytes (user > data) + 8 > bytes (parity data) > Minimum required ECC for block 0 if PROGRAM/ > ERASE cycles are less than 1000 1-bit ECC per 528 bytes 4-bit ECC is the minimum this chip requires. There's nothing wrong with choosing a better ECC like the GPMI driver does by default. It looks like you are papering over some other problem. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 1/2] quota: Add mountpath based quota support
Add syscall quotactl_path, a variant of quotactl which allows to specify
the mountpath instead of a path of to a block device.
The quotactl syscall expects a path to the mounted block device to
specify the filesystem to work on. This limits usage to filesystems
which actually have a block device. quotactl_path replaces the path
to the block device with a path where the filesystem is mounted at.
The global Q_SYNC command to sync all filesystems is not supported for
this new syscall, otherwise quotactl_path behaves like quotactl.
Signed-off-by: Sascha Hauer
Reviewed-by: Christoph Hellwig
---
fs/quota/quota.c | 49 +---
1 file changed, 46 insertions(+), 3 deletions(-)
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 6d16b2be5ac4..f7b4b66491fc 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include "compat.h"
@@ -827,8 +828,6 @@ static int do_quotactl(struct super_block *sb, int type,
int cmd, qid_t id,
}
}
-#ifdef CONFIG_BLOCK
-
/* Return 1 if 'cmd' will block on frozen filesystem */
static int quotactl_cmd_write(int cmd)
{
@@ -850,7 +849,6 @@ static int quotactl_cmd_write(int cmd)
}
return 1;
}
-#endif /* CONFIG_BLOCK */
/* Return true if quotactl command is manipulating quota on/off state */
static bool quotactl_cmd_onoff(int cmd)
@@ -968,3 +966,48 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char
__user *, special,
path_put(pathp);
return ret;
}
+
+SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *,
+ mountpoint, qid_t, id, void __user *, addr)
+{
+ struct super_block *sb;
+ struct path mountpath;
+ unsigned int cmds = cmd >> SUBCMDSHIFT;
+ unsigned int type = cmd & SUBCMDMASK;
+ int ret;
+
+ if (type >= MAXQUOTAS)
+ return -EINVAL;
+
+ ret = user_path_at(AT_FDCWD, mountpoint,
+LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath);
+ if (ret)
+ return ret;
+
+ sb = mountpath.mnt->mnt_sb;
+
+ if (quotactl_cmd_write(cmds)) {
+ ret = mnt_want_write(mountpath.mnt);
+ if (ret)
+ goto out;
+ }
+
+ if (quotactl_cmd_onoff(cmds))
+ down_write(&sb->s_umount);
+ else
+ down_read(&sb->s_umount);
+
+ ret = do_quotactl(sb, type, cmds, id, addr, ERR_PTR(-EINVAL));
+
+ if (quotactl_cmd_onoff(cmds))
+ up_write(&sb->s_umount);
+ else
+ up_read(&sb->s_umount);
+
+ if (quotactl_cmd_write(cmds))
+ mnt_drop_write(mountpath.mnt);
+out:
+ path_put(&mountpath);
+
+ return ret;
+}
--
2.29.2
[PATCH v3 0/2] quota: Add mountpath based quota support
Current quotactl syscall uses a path to a block device to specify the filesystem to work on which makes it unsuitable for filesystems that do not have a block device. This series adds a new syscall quotactl_path() which replaces the path to the block device with a mountpath, but otherwise behaves like original quotactl. This is done to add quota support to UBIFS. UBIFS quota support has been posted several times with different approaches to put the mountpath into the existing quotactl() syscall until it has been suggested to make it a new syscall instead, so here it is. I'm not posting the full UBIFS quota series here as it remains unchanged and I'd like to get feedback to the new syscall first. For those interested the most recent series can be found here: https://lwn.net/Articles/810463/ Changes since v2: - Rebase on v5.12-rc1 - replace mountpath.dentry->d_inode->i_sb with mountpath.mnt->mnt_sb - fix wrong macro usage in arch/x86/entry/syscalls/syscall_32.tbl - +Cc linux-...@vger.kernel.org Changes since (implicit) v1: - Ignore second path argument to Q_QUOTAON. With this quotactl_path() can only do the Q_QUOTAON operation on filesystems which use hidden inodes for quota metadata storage - Drop unnecessary quotactl_cmd_onoff() check Sascha Hauer (2): quota: Add mountpath based quota support quota: wire up quotactl_path arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 + arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl| 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + fs/quota/quota.c| 49 +++-- include/linux/syscalls.h| 2 + include/uapi/asm-generic/unistd.h | 4 +- kernel/sys_ni.c | 1 + 22 files changed, 71 insertions(+), 5 deletions(-) -- 2.29.2
[PATCH] quotactl.2: Add documentation for quotactl_path()
Expand the quotactl.2 manpage with a description for quotactl_path() that takes a mountpoint path instead of a path to a block device. Signed-off-by: Sascha Hauer --- man2/quotactl.2 | 31 --- man2/quotactl_path.2 | 1 + 2 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 man2/quotactl_path.2 diff --git a/man2/quotactl.2 b/man2/quotactl.2 index 7869c64ea..76505c668 100644 --- a/man2/quotactl.2 +++ b/man2/quotactl.2 @@ -34,6 +34,8 @@ quotactl \- manipulate disk quotas .PP .BI "int quotactl(int " cmd ", const char *" special ", int " id \ ", caddr_t " addr ); +.BI "int quotactl_path(int " cmd ", const char *" mountpoint ", int " id \ +", caddr_t " addr ); .fi .SH DESCRIPTION The quota system can be used to set per-user, per-group, and per-project limits @@ -48,7 +50,11 @@ after this, the soft limit counts as a hard limit. .PP The .BR quotactl () -call manipulates disk quotas. +and +.BR quotactl_path () +calls manipulate disk quotas. The difference between both functions is the way +how the filesystem being manipulated is specified, see description of the arguments +below. The .I cmd argument indicates a command to be applied to the user or @@ -75,10 +81,19 @@ value is described below. .PP The .I special -argument is a pointer to a null-terminated string containing the pathname +argument to +.BR quotactl () +is a pointer to a null-terminated string containing the pathname of the (mounted) block special device for the filesystem being manipulated. .PP The +.I mountpoint +argument to +.BR quotactl_path () +is a pointer to a null-terminated string containing the pathname +of the mountpoint for the filesystem being manipulated. +.PP +The .I addr argument is the address of an optional, command-specific, data structure that is copied in or out of the system. @@ -133,7 +148,17 @@ flag in the .I dqi_flags field returned by the .B Q_GETINFO -operation. +operation. The +.BR quotactl_path () +variant of this syscall generally ignores the +.IR addr +and +.IR id +arguments, so the +.B Q_QUOTAON +operation of +.BR quotactl_path () +is only suitable for work with hidden system inodes. .IP This operation requires privilege .RB ( CAP_SYS_ADMIN ). diff --git a/man2/quotactl_path.2 b/man2/quotactl_path.2 new file mode 100644 index 0..5f63187c6 --- /dev/null +++ b/man2/quotactl_path.2 @@ -0,0 +1 @@ +.so man2/quotactl.2 -- 2.20.1
[PATCH 2/2] quota: wire up quotactl_path
Wire up the quotactl_path syscall added in the previous patch. Signed-off-by: Sascha Hauer Reviewed-by: Christoph Hellwig --- arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 ++ arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl| 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + include/linux/syscalls.h| 2 ++ include/uapi/asm-generic/unistd.h | 4 +++- kernel/sys_ni.c | 1 + 21 files changed, 25 insertions(+), 2 deletions(-) diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl index 02f0244e005c..c5f7e595adab 100644 --- a/arch/alpha/kernel/syscalls/syscall.tbl +++ b/arch/alpha/kernel/syscalls/syscall.tbl @@ -482,3 +482,4 @@ 550common process_madvise sys_process_madvise 551common epoll_pwait2sys_epoll_pwait2 552common mount_setattr sys_mount_setattr +553common quotactl_path sys_quotactl_path diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl index dcc1191291a2..90cbe207cf3e 100644 --- a/arch/arm/tools/syscall.tbl +++ b/arch/arm/tools/syscall.tbl @@ -456,3 +456,4 @@ 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 442common mount_setattr sys_mount_setattr +443common quotactl_path sys_quotactl_path diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h index 949788f5ba40..d1f7d35f986e 100644 --- a/arch/arm64/include/asm/unistd.h +++ b/arch/arm64/include/asm/unistd.h @@ -38,7 +38,7 @@ #define __ARM_NR_compat_set_tls(__ARM_NR_COMPAT_BASE + 5) #define __ARM_NR_COMPAT_END(__ARM_NR_COMPAT_BASE + 0x800) -#define __NR_compat_syscalls 443 +#define __NR_compat_syscalls 444 #endif #define __ARCH_WANT_SYS_CLONE diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h index 3d874f624056..8361c5138e5f 100644 --- a/arch/arm64/include/asm/unistd32.h +++ b/arch/arm64/include/asm/unistd32.h @@ -893,6 +893,8 @@ __SYSCALL(__NR_process_madvise, sys_process_madvise) __SYSCALL(__NR_epoll_pwait2, compat_sys_epoll_pwait2) #define __NR_mount_setattr 442 __SYSCALL(__NR_mount_setattr, sys_mount_setattr) +#define __NR_quotactl_path 443 +__SYSCALL(__NR_quotactl_path, sys_quotactl_path) /* * Please add new compat syscalls above this comment and update diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl index d89231166e19..c072cd459bb5 100644 --- a/arch/ia64/kernel/syscalls/syscall.tbl +++ b/arch/ia64/kernel/syscalls/syscall.tbl @@ -363,3 +363,4 @@ 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 442common mount_setattr sys_mount_setattr +443common quotactl_path sys_quotactl_path diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl index 72bde6707dd3..5e9f81073ff4 100644 --- a/arch/m68k/kernel/syscalls/syscall.tbl +++ b/arch/m68k/kernel/syscalls/syscall.tbl @@ -442,3 +442,4 @@ 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 442common mount_setattr sys_mount_setattr +443common quotactl_path sys_quotactl_path diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl index d603a5ec9338..8e74d690c64d 100644 --- a/arch/microblaze/kernel/syscalls/syscall.tbl +++ b/arch/microblaze/kernel/syscalls/syscall.tbl @@ -448,3 +448,4 @@ 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 442common mount_setattr sys_mount_setattr +443common quotactl_path sys_quotactl_path diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl index 8fd8c1790941..6f397e56926f 100644 --- a/arch/mips/kernel/syscalls
[PATCH] ecryptfs: Fix typo in message
ecryptfs_decrypt_page() issues a warning "Error encrypting extent". This
should be "Error decrypting extent" instead.
Signed-off-by: Sascha Hauer
---
fs/ecryptfs/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index a48116aae02c..0fed4ff02f69 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -535,7 +535,7 @@ int ecryptfs_decrypt_page(struct page *page)
rc = crypt_extent(crypt_stat, page, page,
extent_offset, DECRYPT);
if (rc) {
- printk(KERN_ERR "%s: Error encrypting extent; "
+ printk(KERN_ERR "%s: Error decrypting extent; "
"rc = [%d]\n", __func__, rc);
goto out;
}
--
2.29.2
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Fri, Feb 12, 2021 at 11:05:05AM +0100, Jan Kara wrote:
> On Fri 12-02-21 09:38:35, Sascha Hauer wrote:
> > On Thu, Feb 11, 2021 at 03:38:13PM +, Christoph Hellwig wrote:
> > > > + if (!mountpoint)
> > > > + return -ENODEV;
> > > > +
> > > > + ret = user_path_at(AT_FDCWD, mountpoint,
> > > > +LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > > > &mountpath);
> > >
> > > user_path_at handles an empty path, although you'll get EFAULT instead.
> > > Do we care about the -ENODEV here?
> >
> > The quotactl manpage documents EFAULT as error code for invalid addr or
> > special argument, so we really should return -EFAULT here.
> >
> > Existing quotactl gets this wrong as well:
> >
> > if (!special) {
> > if (cmds == Q_SYNC)
> > return quota_sync_all(type);
> > return -ENODEV;
> > }
> >
> > Should we fix this or is there userspace code that is confused by a changed
> > return value?
>
> I'd leave the original quotactl(2) as is. There's no strong reason to risk
> breaking some userspace. For the new syscall, I agree we can just
> standardize the return value, there ENODEV makes even less sense since
> there's no device in that call.
Ok, will do. Who can pick this series up? Anyone else I need to Cc next
round?
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/2] quota: Add mountpath based quota support
On Thu, Feb 11, 2021 at 03:38:13PM +, Christoph Hellwig wrote:
> > + if (!mountpoint)
> > + return -ENODEV;
> > +
> > + ret = user_path_at(AT_FDCWD, mountpoint,
> > +LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath);
>
> user_path_at handles an empty path, although you'll get EFAULT instead.
> Do we care about the -ENODEV here?
The quotactl manpage documents EFAULT as error code for invalid addr or
special argument, so we really should return -EFAULT here.
Existing quotactl gets this wrong as well:
if (!special) {
if (cmds == Q_SYNC)
return quota_sync_all(type);
return -ENODEV;
}
Should we fix this or is there userspace code that is confused by a changed
return value?
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 1/2] quota: Add mountpath based quota support
Add syscall quotactl_path, a variant of quotactl which allows to specify
the mountpath instead of a path of to a block device.
The quotactl syscall expects a path to the mounted block device to
specify the filesystem to work on. This limits usage to filesystems
which actually have a block device. quotactl_path replaces the path
to the block device with a path where the filesystem is mounted at.
The global Q_SYNC command to sync all filesystems is not supported for
this new syscall, otherwise quotactl_path behaves like quotactl.
Signed-off-by: Sascha Hauer
---
fs/quota/quota.c | 49
1 file changed, 49 insertions(+)
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 6d16b2be5ac4..6f1df32abeea 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include "compat.h"
@@ -968,3 +969,51 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char
__user *, special,
path_put(pathp);
return ret;
}
+
+SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *,
+ mountpoint, qid_t, id, void __user *, addr)
+{
+ struct super_block *sb;
+ struct path mountpath;
+ unsigned int cmds = cmd >> SUBCMDSHIFT;
+ unsigned int type = cmd & SUBCMDMASK;
+ int ret;
+
+ if (type >= MAXQUOTAS)
+ return -EINVAL;
+
+ if (!mountpoint)
+ return -ENODEV;
+
+ ret = user_path_at(AT_FDCWD, mountpoint,
+LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath);
+ if (ret)
+ return ret;
+
+ sb = mountpath.dentry->d_inode->i_sb;
+
+ if (quotactl_cmd_write(cmds)) {
+ ret = mnt_want_write(mountpath.mnt);
+ if (ret)
+ goto out;
+ }
+
+ if (quotactl_cmd_onoff(cmds))
+ down_write(&sb->s_umount);
+ else
+ down_read(&sb->s_umount);
+
+ ret = do_quotactl(sb, type, cmds, id, addr, ERR_PTR(-EINVAL));
+
+ if (quotactl_cmd_onoff(cmds))
+ up_write(&sb->s_umount);
+ else
+ up_read(&sb->s_umount);
+
+ if (quotactl_cmd_write(cmds))
+ mnt_drop_write(mountpath.mnt);
+out:
+ path_put(&mountpath);
+
+ return ret;
+}
--
2.20.1
[PATCH 2/2] quota: wire up quotactl_path
Wire up the quotactl_path syscall added in the previous patch. Signed-off-by: Sascha Hauer --- arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 ++ arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl| 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + include/linux/syscalls.h| 2 ++ include/uapi/asm-generic/unistd.h | 4 +++- kernel/sys_ni.c | 1 + 21 files changed, 25 insertions(+), 2 deletions(-) diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl index a6617067dbe6..3fe90880c821 100644 --- a/arch/alpha/kernel/syscalls/syscall.tbl +++ b/arch/alpha/kernel/syscalls/syscall.tbl @@ -481,3 +481,4 @@ 549common faccessat2 sys_faccessat2 550common process_madvise sys_process_madvise 551common epoll_pwait2sys_epoll_pwait2 +552common quotactl_path sys_quotactl_path diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl index 20e1170e2e0a..a62509df217f 100644 --- a/arch/arm/tools/syscall.tbl +++ b/arch/arm/tools/syscall.tbl @@ -455,3 +455,4 @@ 439common faccessat2 sys_faccessat2 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 +442common quotactl_path sys_quotactl_path diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h index 86a9d7b3eabe..949788f5ba40 100644 --- a/arch/arm64/include/asm/unistd.h +++ b/arch/arm64/include/asm/unistd.h @@ -38,7 +38,7 @@ #define __ARM_NR_compat_set_tls(__ARM_NR_COMPAT_BASE + 5) #define __ARM_NR_COMPAT_END(__ARM_NR_COMPAT_BASE + 0x800) -#define __NR_compat_syscalls 442 +#define __NR_compat_syscalls 443 #endif #define __ARCH_WANT_SYS_CLONE diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h index cccfbbefbf95..734c254ca1b6 100644 --- a/arch/arm64/include/asm/unistd32.h +++ b/arch/arm64/include/asm/unistd32.h @@ -891,6 +891,8 @@ __SYSCALL(__NR_faccessat2, sys_faccessat2) __SYSCALL(__NR_process_madvise, sys_process_madvise) #define __NR_epoll_pwait2 441 __SYSCALL(__NR_epoll_pwait2, compat_sys_epoll_pwait2) +#define __NR_quotactl_path 442 +__SYSCALL(__NR_quotactl_path, sys_quotactl_path) /* * Please add new compat syscalls above this comment and update diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl index bfc00f2bd437..4758a22a4d80 100644 --- a/arch/ia64/kernel/syscalls/syscall.tbl +++ b/arch/ia64/kernel/syscalls/syscall.tbl @@ -362,3 +362,4 @@ 439common faccessat2 sys_faccessat2 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 +442common quotactl_path sys_quotactl_path diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl index 7fe4e45c864c..b9072d2f1fdc 100644 --- a/arch/m68k/kernel/syscalls/syscall.tbl +++ b/arch/m68k/kernel/syscalls/syscall.tbl @@ -441,3 +441,4 @@ 439common faccessat2 sys_faccessat2 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 +442common quotactl_path sys_quotactl_path diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl index a522adf194ab..95e0cb59e8c1 100644 --- a/arch/microblaze/kernel/syscalls/syscall.tbl +++ b/arch/microblaze/kernel/syscalls/syscall.tbl @@ -447,3 +447,4 @@ 439common faccessat2 sys_faccessat2 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 +442common quotactl_path sys_quotactl_path diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl index 0f03ad223f33..027fe0351e66 100644 --- a/arch/mips/kernel/syscalls/syscall_n32.tbl +++ b/arch/mips/kernel/syscalls
[PATCH v2 0/2] quota: Add mountpath based quota support
Current quotactl syscall uses a path to a block device to specify the filesystem to work on which makes it unsuitable for filesystems that do not have a block device. This series adds a new syscall quotactl_path() which replaces the path to the block device with a mountpath, but otherwise behaves like original quotactl. This is done to add quota support to UBIFS. UBIFS quota support has been posted several times with different approaches to put the mountpath into the existing quotactl() syscall until it has been suggested to make it a new syscall instead, so here it is. I'm not posting the full UBIFS quota series here as it remains unchanged and I'd like to get feedback to the new syscall first. For those interested the most recent series can be found here: https://lwn.net/Articles/810463/ Changes since (implicit) v1: - Ignore second path argument to Q_QUOTAON. With this quotactl_path() can only do the Q_QUOTAON operation on filesystems which use hidden inodes for quota metadata storage - Drop unnecessary quotactl_cmd_onoff() check Sascha Hauer (2): quota: Add mountpath based quota support quota: wire up quotactl_path arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 + arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl| 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + fs/quota/quota.c| 49 + include/linux/syscalls.h| 2 + include/uapi/asm-generic/unistd.h | 4 +- kernel/sys_ni.c | 1 + 22 files changed, 74 insertions(+), 2 deletions(-) -- 2.20.1
[PATCH] quotactl.2: Add documentation for quotactl_path()
Expand the quotactl.2 manpage with a description for quotactl_path() that takes a mountpoint path instead of a path to a block device. Signed-off-by: Sascha Hauer --- man2/quotactl.2 | 31 --- man2/quotactl_path.2 | 1 + 2 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 man2/quotactl_path.2 diff --git a/man2/quotactl.2 b/man2/quotactl.2 index 7869c64ea..76505c668 100644 --- a/man2/quotactl.2 +++ b/man2/quotactl.2 @@ -34,6 +34,8 @@ quotactl \- manipulate disk quotas .PP .BI "int quotactl(int " cmd ", const char *" special ", int " id \ ", caddr_t " addr ); +.BI "int quotactl_path(int " cmd ", const char *" mountpoint ", int " id \ +", caddr_t " addr ); .fi .SH DESCRIPTION The quota system can be used to set per-user, per-group, and per-project limits @@ -48,7 +50,11 @@ after this, the soft limit counts as a hard limit. .PP The .BR quotactl () -call manipulates disk quotas. +and +.BR quotactl_path () +calls manipulate disk quotas. The difference between both functions is the way +how the filesystem being manipulated is specified, see description of the arguments +below. The .I cmd argument indicates a command to be applied to the user or @@ -75,10 +81,19 @@ value is described below. .PP The .I special -argument is a pointer to a null-terminated string containing the pathname +argument to +.BR quotactl () +is a pointer to a null-terminated string containing the pathname of the (mounted) block special device for the filesystem being manipulated. .PP The +.I mountpoint +argument to +.BR quotactl_path () +is a pointer to a null-terminated string containing the pathname +of the mountpoint for the filesystem being manipulated. +.PP +The .I addr argument is the address of an optional, command-specific, data structure that is copied in or out of the system. @@ -133,7 +148,17 @@ flag in the .I dqi_flags field returned by the .B Q_GETINFO -operation. +operation. The +.BR quotactl_path () +variant of this syscall generally ignores the +.IR addr +and +.IR id +arguments, so the +.B Q_QUOTAON +operation of +.BR quotactl_path () +is only suitable for work with hidden system inodes. .IP This operation requires privilege .RB ( CAP_SYS_ADMIN ). diff --git a/man2/quotactl_path.2 b/man2/quotactl_path.2 new file mode 100644 index 0..5f63187c6 --- /dev/null +++ b/man2/quotactl_path.2 @@ -0,0 +1 @@ +.so man2/quotactl.2 -- 2.20.1
Re: [PATCH] staging: fix ignoring return value warning
Hi Dan, On Mon, Feb 08, 2021 at 04:45:17PM +0300, Dan Carpenter wrote: > On Sun, Feb 07, 2021 at 05:23:28PM +0800, Youling Tang wrote: > > Fix the below ignoring return value warning for device_reset. > > > > drivers/staging/mt7621-dma/mtk-hsdma.c:685:2: warning: ignoring return value > > of function declared with 'warn_unused_result' attribute [-Wunused-result] > > device_reset(&pdev->dev); > > ^~~~ ~~ > > drivers/staging/ralink-gdma/ralink-gdma.c:836:2: warning: ignoring return > > value > > of function declared with 'warn_unused_result' attribute [-Wunused-result] > > device_reset(&pdev->dev); > > ^~~~ ~~ > > > > We can't really do this sort of fix without the hardware to test it. > This could be the correct fix or perhaps switching to device_reset_optional() > is the correct fix. We can't know unless we have the hardware to test. When device_reset() is the wrong function then adding a return value check will turn this into a runtime error for those who have the hardware which will hopefully trigger them to tell us why reset_device is wrong for them. At least for a staging driver I find this procedure opportune. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] staging: fix ignoring return value warning
On Sun, Feb 07, 2021 at 05:23:28PM +0800, Youling Tang wrote:
> Fix the below ignoring return value warning for device_reset.
>
> drivers/staging/mt7621-dma/mtk-hsdma.c:685:2: warning: ignoring return value
> of function declared with 'warn_unused_result' attribute [-Wunused-result]
> device_reset(&pdev->dev);
> ^~~~ ~~
> drivers/staging/ralink-gdma/ralink-gdma.c:836:2: warning: ignoring return
> value
> of function declared with 'warn_unused_result' attribute [-Wunused-result]
> device_reset(&pdev->dev);
> ^~~~ ~~
>
> Signed-off-by: Youling Tang
> ---
> drivers/staging/mt7621-dma/mtk-hsdma.c| 6 +-
> drivers/staging/ralink-gdma/ralink-gdma.c | 6 +-
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/mt7621-dma/mtk-hsdma.c
> b/drivers/staging/mt7621-dma/mtk-hsdma.c
> index bc4bb43..d4ffa52 100644
> --- a/drivers/staging/mt7621-dma/mtk-hsdma.c
> +++ b/drivers/staging/mt7621-dma/mtk-hsdma.c
> @@ -682,7 +682,11 @@ static int mtk_hsdma_probe(struct platform_device *pdev)
> return ret;
> }
>
> - device_reset(&pdev->dev);
> + ret = device_reset(&pdev->dev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to reset device\n");
> + return ret;
> + }
Normally you don't want to see this error message when -EPROBE_DEFER is
returned because that would mean the reset controller is not yet
available and the driver should probe later again. There is
dev_err_probe() now for exactly this usecase.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 0/2] quota: Add mountpath based quota support
Current quotactl syscall uses a path to a block device to specify the filesystem to work on which makes it unsuitable for filesystems that do not have a block device. This series adds a new syscall quotactl_path() which replaces the path to the block device with a mountpath, but otherwise behaves like original quotactl. This is done to add quota support to UBIFS. UBIFS quota support has been posted several times with different approaches to put the mountpath into the existing quotactl() syscall until it has been suggested to make it a new syscall instead, so here it is. I'm not posting the full UBIFS quota series here as it remains unchanged and I'd like to get feedback to the new syscall first. For those interested the most recent series can be found here: https://lwn.net/Articles/810463/ Sascha Sascha Hauer (2): quota: Add mountpath based quota support quota: wire up quotactl_path arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 + arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl| 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + fs/quota/quota.c| 77 + include/linux/syscalls.h| 2 + include/uapi/asm-generic/unistd.h | 4 +- kernel/sys_ni.c | 1 + 22 files changed, 102 insertions(+), 2 deletions(-) -- 2.20.1
[PATCH 1/2] quota: Add mountpath based quota support
Add syscall quotactl_path, a variant of quotactl which allows to specify
the mountpath instead of a path of to a block device.
The quotactl syscall expects a path to the mounted block device to
specify the filesystem to work on. This limits usage to filesystems
which actually have a block device. quotactl_path replaces the path
to the block device with a path where the filesystem is mounted at.
The global Q_SYNC command to sync all filesystems is not supported for
this new syscall, otherwise quotactl_path behaves like quotactl.
Signed-off-by: Sascha Hauer
---
fs/quota/quota.c | 77
1 file changed, 77 insertions(+)
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 6d16b2be5ac4..9ac09e128686 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include "compat.h"
@@ -968,3 +969,79 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char
__user *, special,
path_put(pathp);
return ret;
}
+
+SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *,
+ mountpoint, qid_t, id, void __user *, addr)
+{
+ uint cmds, type;
+ struct super_block *sb = NULL;
+ struct path path, *pathp = NULL;
+ struct path mountpath;
+ bool excl = false, thawed = false;
+ int ret;
+
+ cmds = cmd >> SUBCMDSHIFT;
+ type = cmd & SUBCMDMASK;
+
+ if (type >= MAXQUOTAS)
+ return -EINVAL;
+
+ if (!mountpoint)
+ return -ENODEV;
+
+ /*
+* Path for quotaon has to be resolved before grabbing superblock
+* because that gets s_umount sem which is also possibly needed by path
+* resolution (think about autofs) and thus deadlocks could arise.
+*/
+ if (cmds == Q_QUOTAON) {
+ ret = user_path_at(AT_FDCWD, addr,
+ LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &path);
+ if (ret)
+ pathp = ERR_PTR(ret);
+ else
+ pathp = &path;
+ }
+
+ ret = user_path_at(AT_FDCWD, mountpoint,
+LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath);
+ if (ret)
+ goto out;
+
+ if (quotactl_cmd_onoff(cmds)) {
+ excl = true;
+ thawed = true;
+ } else if (quotactl_cmd_write(cmds)) {
+ thawed = true;
+ }
+
+ if (thawed) {
+ ret = mnt_want_write(mountpath.mnt);
+ if (ret)
+ goto out1;
+ }
+
+ sb = mountpath.dentry->d_inode->i_sb;
+
+ if (excl)
+ down_write(&sb->s_umount);
+ else
+ down_read(&sb->s_umount);
+
+ ret = do_quotactl(sb, type, cmds, id, addr, pathp);
+
+ if (excl)
+ up_write(&sb->s_umount);
+ else
+ up_read(&sb->s_umount);
+
+ if (thawed)
+ mnt_drop_write(mountpath.mnt);
+out1:
+ path_put(&mountpath);
+
+out:
+ if (pathp && !IS_ERR(pathp))
+ path_put(pathp);
+ return ret;
+}
--
2.20.1
[PATCH] quotactl.2: Add documentation for quotactl_path()
Expand the quotactl.2 manpage with a description for quotactl_path() that takes a mountpoint path instead of a path to a block device. Signed-off-by: Sascha Hauer --- man2/quotactl.2 | 19 +-- man2/quotactl_path.2 | 1 + 2 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 man2/quotactl_path.2 diff --git a/man2/quotactl.2 b/man2/quotactl.2 index 7869c64ea..08f2ba02f 100644 --- a/man2/quotactl.2 +++ b/man2/quotactl.2 @@ -34,6 +34,8 @@ quotactl \- manipulate disk quotas .PP .BI "int quotactl(int " cmd ", const char *" special ", int " id \ ", caddr_t " addr ); +.BI "int quotactl_path(int " cmd ", const char *" mountpoint ", int " id \ +", caddr_t " addr ); .fi .SH DESCRIPTION The quota system can be used to set per-user, per-group, and per-project limits @@ -48,7 +50,11 @@ after this, the soft limit counts as a hard limit. .PP The .BR quotactl () -call manipulates disk quotas. +and +.BR quotactl_path () +calls manipulate disk quotas. The difference between both functions is the way +how the filesystem being manipulated is specified, see description of the arguments +below. The .I cmd argument indicates a command to be applied to the user or @@ -75,10 +81,19 @@ value is described below. .PP The .I special -argument is a pointer to a null-terminated string containing the pathname +argument to +.BR quotactl () +is a pointer to a null-terminated string containing the pathname of the (mounted) block special device for the filesystem being manipulated. .PP The +.I mountpoint +argument to +.BR quotactl_path () +is a pointer to a null-terminated string containing the pathname +of the mountpoint for the filesystem being manipulated. +.PP +The .I addr argument is the address of an optional, command-specific, data structure that is copied in or out of the system. diff --git a/man2/quotactl_path.2 b/man2/quotactl_path.2 new file mode 100644 index 0..5f63187c6 --- /dev/null +++ b/man2/quotactl_path.2 @@ -0,0 +1 @@ +.so man2/quotactl.2 -- 2.20.1
[PATCH 2/2] quota: wire up quotactl_path
Wire up the quotactl_path syscall added in the previous patch. Signed-off-by: Sascha Hauer --- arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 ++ arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl| 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + include/linux/syscalls.h| 2 ++ include/uapi/asm-generic/unistd.h | 4 +++- kernel/sys_ni.c | 1 + 21 files changed, 25 insertions(+), 2 deletions(-) diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl index a6617067dbe6..3fe90880c821 100644 --- a/arch/alpha/kernel/syscalls/syscall.tbl +++ b/arch/alpha/kernel/syscalls/syscall.tbl @@ -481,3 +481,4 @@ 549common faccessat2 sys_faccessat2 550common process_madvise sys_process_madvise 551common epoll_pwait2sys_epoll_pwait2 +552common quotactl_path sys_quotactl_path diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl index 20e1170e2e0a..a62509df217f 100644 --- a/arch/arm/tools/syscall.tbl +++ b/arch/arm/tools/syscall.tbl @@ -455,3 +455,4 @@ 439common faccessat2 sys_faccessat2 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 +442common quotactl_path sys_quotactl_path diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h index 86a9d7b3eabe..949788f5ba40 100644 --- a/arch/arm64/include/asm/unistd.h +++ b/arch/arm64/include/asm/unistd.h @@ -38,7 +38,7 @@ #define __ARM_NR_compat_set_tls(__ARM_NR_COMPAT_BASE + 5) #define __ARM_NR_COMPAT_END(__ARM_NR_COMPAT_BASE + 0x800) -#define __NR_compat_syscalls 442 +#define __NR_compat_syscalls 443 #endif #define __ARCH_WANT_SYS_CLONE diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h index cccfbbefbf95..734c254ca1b6 100644 --- a/arch/arm64/include/asm/unistd32.h +++ b/arch/arm64/include/asm/unistd32.h @@ -891,6 +891,8 @@ __SYSCALL(__NR_faccessat2, sys_faccessat2) __SYSCALL(__NR_process_madvise, sys_process_madvise) #define __NR_epoll_pwait2 441 __SYSCALL(__NR_epoll_pwait2, compat_sys_epoll_pwait2) +#define __NR_quotactl_path 442 +__SYSCALL(__NR_quotactl_path, sys_quotactl_path) /* * Please add new compat syscalls above this comment and update diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl index bfc00f2bd437..4758a22a4d80 100644 --- a/arch/ia64/kernel/syscalls/syscall.tbl +++ b/arch/ia64/kernel/syscalls/syscall.tbl @@ -362,3 +362,4 @@ 439common faccessat2 sys_faccessat2 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 +442common quotactl_path sys_quotactl_path diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl index 7fe4e45c864c..b9072d2f1fdc 100644 --- a/arch/m68k/kernel/syscalls/syscall.tbl +++ b/arch/m68k/kernel/syscalls/syscall.tbl @@ -441,3 +441,4 @@ 439common faccessat2 sys_faccessat2 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 +442common quotactl_path sys_quotactl_path diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl index a522adf194ab..95e0cb59e8c1 100644 --- a/arch/microblaze/kernel/syscalls/syscall.tbl +++ b/arch/microblaze/kernel/syscalls/syscall.tbl @@ -447,3 +447,4 @@ 439common faccessat2 sys_faccessat2 440common process_madvise sys_process_madvise 441common epoll_pwait2sys_epoll_pwait2 +442common quotactl_path sys_quotactl_path diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl index 0f03ad223f33..027fe0351e66 100644 --- a/arch/mips/kernel/syscalls/syscall_n32.tbl +++ b/arch/mips/kernel/syscalls
Re: [RFC] clk: Mark HW enabled clocks as enabled in core
On Wed, Jan 27, 2021 at 12:12:20PM +0200, Abel Vesa wrote:
> On 21-01-26 15:30:17, Sascha Hauer wrote:
> > On Tue, Jan 26, 2021 at 03:12:39PM +0200, Abel Vesa wrote:
> > > On 21-01-26 12:51:05, Sascha Hauer wrote:
> > > > On Tue, Jan 26, 2021 at 01:21:36PM +0200, Abel Vesa wrote:
> > > > > Some clocks are already enabled in HW even before the kernel
> > > > > starts to boot. So, in order to make sure that these clocks do not
> > > > > get disabled when clk_disable_unused call is done or when
> > > > > reparenting clocks, we enable them in core on clock registration.
> > > > > Such a clock will have to be registered with CLK_IGNORE_UNUSED flag
> > > > > and also needs to have the is_enabled ops implemented.
> > > > >
> > > > > Signed-off-by: Abel Vesa
> > > > > ---
> > > > > drivers/clk/clk.c | 11 ++-
> > > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > index 3d751ae5bc70..26d55851cfa5 100644
> > > > > --- a/drivers/clk/clk.c
> > > > > +++ b/drivers/clk/clk.c
> > > > > @@ -3416,6 +3416,7 @@ static int __clk_core_init(struct clk_core
> > > > > *core)
> > > > > int ret;
> > > > > struct clk_core *parent;
> > > > > unsigned long rate;
> > > > > + bool is_hw_enabled = false;
> > > > > int phase;
> > > > >
> > > > > if (!core)
> > > > > @@ -3558,12 +3559,20 @@ static int __clk_core_init(struct clk_core
> > > > > *core)
> > > > > rate = 0;
> > > > > core->rate = core->req_rate = rate;
> > > > >
> > > > > + /*
> > > > > + * If the clock has the CLK_IGNORE_UNUSED flag set and it is
> > > > > already
> > > > > + * enabled in HW, enable it in core too so it won't get
> > > > > accidentally
> > > > > + * disabled when walking the orphan tree and reparenting clocks
> > > > > + */
> > > > > + if (core->flags & CLK_IGNORE_UNUSED && core->ops->is_enabled)
> > > > > + is_hw_enabled = clk_core_is_enabled(core);
> > > > > +
> > > > > /*
> > > > >* Enable CLK_IS_CRITICAL clocks so newly added critical clocks
> > > > >* don't get accidentally disabled when walking the orphan tree
> > > > > and
> > > > >* reparenting clocks
> > > > >*/
> > > > > - if (core->flags & CLK_IS_CRITICAL) {
> > > > > + if (core->flags & CLK_IS_CRITICAL || is_hw_enabled) {
> > > > > unsigned long flags;
> > > > >
> > > > > ret = clk_core_prepare(core);
> > > >
> > > > This means that a bootloader enabled clock with CLK_IGNORE_UNUSED flag
> > > > can effectively never be disabled because the prepare/enable count is 1
> > > > without any user. This is the behaviour we want to have with critical
> > > > clocks, but I don't think this is desired for clocks with the
> > > > CLK_IGNORE_UNUSED flag.
> > > >
> > >
> > > Here is the way I see it. Critical clocks means the system can't work
> > > without, so do not ever disable/unprepare. The "ignore unused" flag
> > > tells the core to not do anything to this clock, even if it is unused.
> > > For now, it just leaves the clock alone, but the flag could be used for
> > > some other stuff in the future.
> > >
> > > Now, the behavior is entirely different.
> > >
> > > For the "critical" clock disable/unprepare, the core does nothing
> > > (returns without calling the disable/unprepare ops).
> > >
> > > As for the "ignore unused", the clock can be disabled later on,
> > > which would decrement the prepare/enable counter.
> > > The imx earlycon serial driver could implement a late initcall,
> > > that takes the clocks from the devicetree uart node and disables
> > > them. The user doesn't even count in this situation.
> > >
> > > Plus, there is no other reason someone would use the CLK_IGNORE_UNUSED,
>
Re: [RFC] clk: Mark HW enabled clocks as enabled in core
On Tue, Jan 26, 2021 at 03:12:39PM +0200, Abel Vesa wrote:
> On 21-01-26 12:51:05, Sascha Hauer wrote:
> > On Tue, Jan 26, 2021 at 01:21:36PM +0200, Abel Vesa wrote:
> > > Some clocks are already enabled in HW even before the kernel
> > > starts to boot. So, in order to make sure that these clocks do not
> > > get disabled when clk_disable_unused call is done or when
> > > reparenting clocks, we enable them in core on clock registration.
> > > Such a clock will have to be registered with CLK_IGNORE_UNUSED flag
> > > and also needs to have the is_enabled ops implemented.
> > >
> > > Signed-off-by: Abel Vesa
> > > ---
> > > drivers/clk/clk.c | 11 ++-
> > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 3d751ae5bc70..26d55851cfa5 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -3416,6 +3416,7 @@ static int __clk_core_init(struct clk_core *core)
> > > int ret;
> > > struct clk_core *parent;
> > > unsigned long rate;
> > > + bool is_hw_enabled = false;
> > > int phase;
> > >
> > > if (!core)
> > > @@ -3558,12 +3559,20 @@ static int __clk_core_init(struct clk_core *core)
> > > rate = 0;
> > > core->rate = core->req_rate = rate;
> > >
> > > + /*
> > > + * If the clock has the CLK_IGNORE_UNUSED flag set and it is already
> > > + * enabled in HW, enable it in core too so it won't get accidentally
> > > + * disabled when walking the orphan tree and reparenting clocks
> > > + */
> > > + if (core->flags & CLK_IGNORE_UNUSED && core->ops->is_enabled)
> > > + is_hw_enabled = clk_core_is_enabled(core);
> > > +
> > > /*
> > >* Enable CLK_IS_CRITICAL clocks so newly added critical clocks
> > >* don't get accidentally disabled when walking the orphan tree and
> > >* reparenting clocks
> > >*/
> > > - if (core->flags & CLK_IS_CRITICAL) {
> > > + if (core->flags & CLK_IS_CRITICAL || is_hw_enabled) {
> > > unsigned long flags;
> > >
> > > ret = clk_core_prepare(core);
> >
> > This means that a bootloader enabled clock with CLK_IGNORE_UNUSED flag
> > can effectively never be disabled because the prepare/enable count is 1
> > without any user. This is the behaviour we want to have with critical
> > clocks, but I don't think this is desired for clocks with the
> > CLK_IGNORE_UNUSED flag.
> >
>
> Here is the way I see it. Critical clocks means the system can't work
> without, so do not ever disable/unprepare. The "ignore unused" flag
> tells the core to not do anything to this clock, even if it is unused.
> For now, it just leaves the clock alone, but the flag could be used for
> some other stuff in the future.
>
> Now, the behavior is entirely different.
>
> For the "critical" clock disable/unprepare, the core does nothing
> (returns without calling the disable/unprepare ops).
>
> As for the "ignore unused", the clock can be disabled later on,
> which would decrement the prepare/enable counter.
> The imx earlycon serial driver could implement a late initcall,
> that takes the clocks from the devicetree uart node and disables
> them. The user doesn't even count in this situation.
>
> Plus, there is no other reason someone would use the CLK_IGNORE_UNUSED,
> other than leaving a clock that is already enabled stay as is (at least,
> not with the current implementation). So why not mark it as enabled in
> the core, if the HW says it is enabled ?
The CLK_IGNORE_UNUSED is there from the start of the clock framework, so
there is no commit message that tells what it shall be used for. AFAIR
the flag was thought for being used with clocks which should not be
disabled, but had no driver initially that used them.
Implementation of this flag was likely broken from the start as well,
because in this situation:
a
/ \
b c (CLK_IGNORE_UNUSED)
When clk b is enabled/disabled then the parent of clock c is disabled as
well, so CLK_IGNORE_UNUSED doesn't help at all. In that sense your patch
really improves things, because the above example would be fixed.
Anyway, CLK_IGNORE_UNUSED is excessively used in the kernel, we have
over 1000 clocks that have this flag set. With your patch all of a
sudden all these clocks won't be disabled anymore and all these clocks
will require some fixup to finally disable them when desired. I don't
think this is a good idea.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [RFC] clk: Mark HW enabled clocks as enabled in core
On Tue, Jan 26, 2021 at 01:21:36PM +0200, Abel Vesa wrote:
> Some clocks are already enabled in HW even before the kernel
> starts to boot. So, in order to make sure that these clocks do not
> get disabled when clk_disable_unused call is done or when
> reparenting clocks, we enable them in core on clock registration.
> Such a clock will have to be registered with CLK_IGNORE_UNUSED flag
> and also needs to have the is_enabled ops implemented.
>
> Signed-off-by: Abel Vesa
> ---
> drivers/clk/clk.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 3d751ae5bc70..26d55851cfa5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3416,6 +3416,7 @@ static int __clk_core_init(struct clk_core *core)
> int ret;
> struct clk_core *parent;
> unsigned long rate;
> + bool is_hw_enabled = false;
> int phase;
>
> if (!core)
> @@ -3558,12 +3559,20 @@ static int __clk_core_init(struct clk_core *core)
> rate = 0;
> core->rate = core->req_rate = rate;
>
> + /*
> + * If the clock has the CLK_IGNORE_UNUSED flag set and it is already
> + * enabled in HW, enable it in core too so it won't get accidentally
> + * disabled when walking the orphan tree and reparenting clocks
> + */
> + if (core->flags & CLK_IGNORE_UNUSED && core->ops->is_enabled)
> + is_hw_enabled = clk_core_is_enabled(core);
> +
> /*
>* Enable CLK_IS_CRITICAL clocks so newly added critical clocks
>* don't get accidentally disabled when walking the orphan tree and
>* reparenting clocks
>*/
> - if (core->flags & CLK_IS_CRITICAL) {
> + if (core->flags & CLK_IS_CRITICAL || is_hw_enabled) {
> unsigned long flags;
>
> ret = clk_core_prepare(core);
This means that a bootloader enabled clock with CLK_IGNORE_UNUSED flag
can effectively never be disabled because the prepare/enable count is 1
without any user. This is the behaviour we want to have with critical
clocks, but I don't think this is desired for clocks with the
CLK_IGNORE_UNUSED flag.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH V3] clk: imx: Fix reparenting of UARTs not associated with sdout
On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote: > On 21-01-20 16:50:01, Sascha Hauer wrote: > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote: > > > On 21-01-20 16:13:05, Sascha Hauer wrote: > > > > Hi Abel, > > > > > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote: > > > > > On 21-01-18 08:00:43, Adam Ford wrote: > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa wrote: > > > > > > ... > > > > > > > > > > > > > > > > > > > > > > > TBH, I'm against the idea of having to call consumer API from a > > > > > > > clock provider driver. > > > > > > > I'm still investigating a way of moving the uart clock control > > > > > > > calls in drivers/serial/imx, > > > > > > > where they belong. > > > > > > > > > > > > That makes sense. > > > > > > > > > > > > > > > > Just a thought. The uart clock used for console remains on from > > > > > u-boot, > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the > > > > > uart root clocks and remove the prepare/enable calls for uart clocks > > > > > for good. I don't really have a way to test it right now, but maybe > > > > > you could give it a try. > > > > > > > > That would mean that UART clocks will never be disabled, regardless of > > > > whether they are used for console or not. That doesn't sound very > > > > appealing. > > > > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for > > > the console. Later on, when the serial driver probes, it will enable it > > > itself. > > > > > > Unless I'm missing something, this is exactly what we need. > > > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be > > disabled again when a port is closed after usage > > OK, tell me what I'm getting wrong in the following scenario: > > U-boot leaves the console uart clock enabled. All the other ones are disabled. > > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED. I was wrong at that point. I originally thought the kernel will never disable these clocks, but in fact it only leaves them enabled during the clk_disable_unused call. However, when CLK_IGNORE_UNUSED becomes relevant it's too late already. I just chatted with Lucas and he told me what the original problem was that his patch solved. The problem comes when an unrelated device and the earlycon UART have the same parent clocks. The parent clock is enabled, but it's reference count is zero. Now when the unrelated device probes and toggles its clocks then the shared parent clock will be disabled due to the reference count being zero. Next time earlycon prints a character the system hangs because the UART gates are still enabled, but their parent clocks no longer are. Overall I think Lucas' patches are still valid and relevant and with Adams patches we even no longer have to enable all UART clocks, but only the ones which are actually needed. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH V3] clk: imx: Fix reparenting of UARTs not associated with sdout
On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote: > On 21-01-20 16:13:05, Sascha Hauer wrote: > > Hi Abel, > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote: > > > On 21-01-18 08:00:43, Adam Ford wrote: > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa wrote: > > ... > > > > > > > > > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock > > > > > provider driver. > > > > > I'm still investigating a way of moving the uart clock control calls > > > > > in drivers/serial/imx, > > > > > where they belong. > > > > > > > > That makes sense. > > > > > > > > > > Just a thought. The uart clock used for console remains on from u-boot, > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the > > > uart root clocks and remove the prepare/enable calls for uart clocks > > > for good. I don't really have a way to test it right now, but maybe > > > you could give it a try. > > > > That would mean that UART clocks will never be disabled, regardless of > > whether they are used for console or not. That doesn't sound very > > appealing. > > AFAIK, the only uart clock that is enabled by u-boot is the one used for > the console. Later on, when the serial driver probes, it will enable it > itself. > > Unless I'm missing something, this is exactly what we need. It might enable it, but with CLK_IGNORE_UNUSED the clock won't be disabled again when a port is closed after usage Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH V3] clk: imx: Fix reparenting of UARTs not associated with sdout
Hi Abel,
On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> On 21-01-18 08:00:43, Adam Ford wrote:
> > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa wrote:
> > >
> > > On 21-01-15 12:29:08, Adam Ford wrote:
> > >
> > > ...
> > >
> > > > diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> > > > index a66cabfbf94f..66192fe0a898 100644
> > > > --- a/drivers/clk/imx/clk-imx25.c
> > > > +++ b/drivers/clk/imx/clk-imx25.c
> > > > @@ -73,16 +73,6 @@ enum mx25_clks {
> > > >
> > > > static struct clk *clk[clk_max];
> > > >
> > > > -static struct clk ** const uart_clks[] __initconst = {
> > > > - &clk[uart_ipg_per],
> > > > - &clk[uart1_ipg],
> > > > - &clk[uart2_ipg],
> > > > - &clk[uart3_ipg],
> > > > - &clk[uart4_ipg],
> > > > - &clk[uart5_ipg],
> > > > - NULL
> > > > -};
> > > > -
> > >
> > > I'm assuming there is another patch that updates the dts files. Right ?
> >
> > I have only been able to test this on an i.MX8M Mini. I need to set
> > the parent clock of the i.MX8M Mini to an 80 MHz clock in order to run
> > the UART at 4Mbps. With this patch, I can stop enabling the all the
> > UART clocks early and allow the clock parent configuration to occur.
> > From what I can tell, the remaining clocks should get activated as
> > they are needed, because I was able to use Bluetooth connected to
> > UART1 running at 4MBps using a 80MHz clock source with this patch, and
> > the clk_summary shows the clock is running at the proper speed.
> > Without this patch, the UART fails to re-parent, so I'm stuck at lower
> > speeds and that means choppy Bluetooth audio.
> >
> > The Kernel that NXP hosts on Code Aurora that they use for Yocto
> > attempts scan through stdout to only enable those clocks [1]. I
> > attempted to push it upstream, but it was rejected [2]. Sascha
> > suggested creating an array which could be filled when the clocks are
> > enabled and that array would be used to deactivate the clocks at
> > shutdown. That's what I attempted to do here.
> >
> > I don't have older imx boards to know if their device trees are
> > configured in such a way without modifications to the device tree or
> > not, but since it appears to work for NXP in [2], I assumed it would
> > work here.
> >
> > [1] -
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2Fdrivers%2Fclk%2Fimx%2Fclk.c%3Fh%3Dimx_5.4.47_2.2.0%26id%3D754ae82cc55b7445545fc2f092a70e0f490e9c1b&data=04%7C01%7Cabel.vesa%40nxp.com%7Cf8922e76fa85485b86c508d8bbb97c47%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637465752633257569%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bVmPaM1nN7Sm%2BISVvlIBoWYozfJE96fHpw431IEuggk%3D&reserved=0
> > [2] -
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux-arm-kernel%2Fpatch%2F20201229145130.2680442-1-aford173%40gmail.com%2F&data=04%7C01%7Cabel.vesa%40nxp.com%7Cf8922e76fa85485b86c508d8bbb97c47%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637465752633257569%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=226HwbeVhZUW%2FJ3hsCSaVIxghOsPBH9EWeF8vFxaTWE%3D&reserved=0
> >
> > >
> > > TBH, I'm against the idea of having to call consumer API from a clock
> > > provider driver.
> > > I'm still investigating a way of moving the uart clock control calls in
> > > drivers/serial/imx,
> > > where they belong.
> >
> > That makes sense.
> >
>
> Just a thought. The uart clock used for console remains on from u-boot,
> so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> uart root clocks and remove the prepare/enable calls for uart clocks
> for good. I don't really have a way to test it right now, but maybe
> you could give it a try.
That would mean that UART clocks will never be disabled, regardless of
whether they are used for console or not. That doesn't sound very
appealing.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/2] clk: imx: enable the earlycon uart clocks by parsing from dt
Hi Adam,
On Tue, Dec 29, 2020 at 08:51:28AM -0600, Adam Ford wrote:
> Remove the earlycon uart clocks that are hard cord in platforms
> clock driver, instead of parsing the earlycon uart port from dt
"instead parse the earlycon uart..."
Otherwise it's confusing what you mean here.
> and enable these clocks from clock property in dt node.
>
> Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection")
> Signed-off-by: Fugang Duan
> Signed-off-by: Adam Ford
> ---
> Based on NXP's code base and adapted for 5.11-rc1.
> https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/clk/imx/clk.c?h=imx_5.4.47_2.2.0&id=754ae82cc55b7445545fc2f092a70e0f490e9c1b
>
> The original signed-off was retained.
> Added the fixes tag.
> ---
> drivers/clk/imx/clk.c | 43 +--
> 1 file changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index 47882c51cb85..c32b46890945 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -148,7 +148,7 @@ void imx_cscmr1_fixup(u32 *val)
>
> #ifndef MODULE
> static int imx_keep_uart_clocks;
> -static struct clk ** const *imx_uart_clocks;
> +static bool imx_uart_clks_on;
>
> static int __init imx_keep_uart_clocks_param(char *str)
> {
> @@ -161,25 +161,40 @@ __setup_param("earlycon", imx_keep_uart_earlycon,
> __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> imx_keep_uart_clocks_param, 0);
>
> -void imx_register_uart_clocks(struct clk ** const clks[])
> +static void imx_earlycon_uart_clks_onoff(bool is_on)
"is_on" sounds like it's the current state of the clock, but actually
the variable is used for the desired state, so I suggest using plain
"on" as name.
> {
> - if (imx_keep_uart_clocks) {
> - int i;
> + struct clk *uart_clk;
> + int i = 0;
>
> - imx_uart_clocks = clks;
> - for (i = 0; imx_uart_clocks[i]; i++)
> - clk_prepare_enable(*imx_uart_clocks[i]);
> - }
> + if (!imx_keep_uart_clocks || (!is_on && !imx_uart_clks_on))
> + return;
> +
> + /* only support dt */
> + if (!of_stdout)
> + return;
> +
> + do {
> + uart_clk = of_clk_get(of_stdout, i++);
of_clk_get() allocates memory and gets you a reference to the clock. You
have to release the clock with clk_put(). I think what you have to do
here is to fill an array with clks when called from
imx_register_uart_clocks() and when called from imx_clk_disable_uart()
use that array to clk_disable_unprepare()/clk_put() the clocks.
Sascha
> + if (IS_ERR(uart_clk))
> + break;
> +
> + if (is_on)
> + clk_prepare_enable(uart_clk);
> + else
> + clk_disable_unprepare(uart_clk);
> + } while (true);
> +
> + if (is_on)
> + imx_uart_clks_on = true;
> +}
> +void imx_register_uart_clocks(struct clk ** const clks[])
> +{
> + imx_earlycon_uart_clks_onoff(true);
> }
>
> static int __init imx_clk_disable_uart(void)
> {
> - if (imx_keep_uart_clocks && imx_uart_clocks) {
> - int i;
> -
> - for (i = 0; imx_uart_clocks[i]; i++)
> - clk_disable_unprepare(*imx_uart_clocks[i]);
> - }
> + imx_earlycon_uart_clks_onoff(false);
>
> return 0;
> }
> --
> 2.25.1
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] clk: imx: Fix reparenting of UARTs not associated with sdout
Hi Adam,
On Mon, Dec 07, 2020 at 09:51:33AM -0600, Adam Ford wrote:
> On Sun, Dec 6, 2020 at 11:24 PM Sascha Hauer wrote:
> >
> > Hi Adam,
> >
> > On Fri, Dec 04, 2020 at 12:31:54PM -0600, Adam Ford wrote:
> > > The default clock source on i.MX8M Mini and Nano boards use a 24MHz clock,
> > > but users who need to re-parent the clock source run into issues because
> > > all the UART clocks are enabled whether or not they're needed by sdout.
> > >
> > > Any attempt to change the parent results in an busy error because the
> > > clocks have been enabled already.
> > >
> > > clk: failed to reparent uart1 to sys_pll1_80m: -16
> > >
> > > Instead of pre-initializing all UARTS, scan the device tree to see if UART
> > > clock is used as stdout before initializing it. Only enable the UART
> > > clock
> > > if it's needed in order to delay the clock initialization until after the
> > > re-parenting of the clocks.
> > >
> > > Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate
> > > protection")
> > > Suggested-by: Aisheng Dong
> > > Signed-off-by: Adam Ford
> > >
> > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > > index 47882c51cb85..6dcc5fbd8f3f 100644
> > > --- a/drivers/clk/imx/clk.c
> > > +++ b/drivers/clk/imx/clk.c
> > > @@ -163,12 +163,18 @@ __setup_param("earlyprintk",
> > > imx_keep_uart_earlyprintk,
> > >
> > > void imx_register_uart_clocks(struct clk ** const clks[])
> > > {
> > > + struct clk *uart_clk;
> > > if (imx_keep_uart_clocks) {
> > > int i;
> > >
> > > imx_uart_clocks = clks;
> > > - for (i = 0; imx_uart_clocks[i]; i++)
> > > - clk_prepare_enable(*imx_uart_clocks[i]);
> > > + for (i = 0; imx_uart_clocks[i]; i++) {
> > > + uart_clk = of_clk_get(of_stdout, i);
> >
> > This looks wrong. imx_uart_clocks is an array containing all clocks that
> > could possibly be used for an UART. With of_clk_get(of_stdout, i) you
> > get the nth clock for one specific UART.
> > What you have to do here is: For each of imx_uart_clocks[] you have to
> > iterate over all clocks of the of_stdout node.
>
> Sascha,
>
> Thanks for the review.
>
> I agree that I'm using the wrong index when checking of_stdout, but I
> am not sure we need to loop through imx_uart_clocks at all.
> I looked at the NXP repo, and they just focus on looping through
> of_stdout and don't reference the imx_uart_clocks. Can't we just loop
> through the of_stdout and enable any clocks found in that?
This sounds good. That way we could get rid of the imx_uart_clocks array
entirely.
Note that right below imx_register_uart_clocks() there is imx_clk_disable_uart()
which disables the previously enabled clocks again later when the real
driver has taken over. You'll have to change that as well accordingly.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] clk: imx: Fix reparenting of UARTs not associated with sdout
Hi Adam,
On Fri, Dec 04, 2020 at 12:31:54PM -0600, Adam Ford wrote:
> The default clock source on i.MX8M Mini and Nano boards use a 24MHz clock,
> but users who need to re-parent the clock source run into issues because
> all the UART clocks are enabled whether or not they're needed by sdout.
>
> Any attempt to change the parent results in an busy error because the
> clocks have been enabled already.
>
> clk: failed to reparent uart1 to sys_pll1_80m: -16
>
> Instead of pre-initializing all UARTS, scan the device tree to see if UART
> clock is used as stdout before initializing it. Only enable the UART clock
> if it's needed in order to delay the clock initialization until after the
> re-parenting of the clocks.
>
> Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection")
> Suggested-by: Aisheng Dong
> Signed-off-by: Adam Ford
>
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index 47882c51cb85..6dcc5fbd8f3f 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -163,12 +163,18 @@ __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
>
> void imx_register_uart_clocks(struct clk ** const clks[])
> {
> + struct clk *uart_clk;
> if (imx_keep_uart_clocks) {
> int i;
>
> imx_uart_clocks = clks;
> - for (i = 0; imx_uart_clocks[i]; i++)
> - clk_prepare_enable(*imx_uart_clocks[i]);
> + for (i = 0; imx_uart_clocks[i]; i++) {
> + uart_clk = of_clk_get(of_stdout, i);
This looks wrong. imx_uart_clocks is an array containing all clocks that
could possibly be used for an UART. With of_clk_get(of_stdout, i) you
get the nth clock for one specific UART.
What you have to do here is: For each of imx_uart_clocks[] you have to
iterate over all clocks of the of_stdout node.
Sascha
> + if (IS_ERR(uart_clk))
> + continue;
> + clk_prepare_enable(uart_clk);
> + clk_put(uart_clk);
> + }
> }
> }
>
> --
> 2.25.1
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH resend] clk: si5351: Wait for bit clear after PLL reset
Documentation states that SI5351_PLL_RESET_B and SI5351_PLL_RESET_A bits
are self clearing bits, so wait until they are cleared before
continuing.
This fixes a case when the clock doesn't come up properly after a PLL
reset. It worked properly when the frequency was below 900MHz, but with
900MHz it only works when we are waiting for the bit to clear.
Signed-off-by: Sascha Hauer
---
I've already sent this in October without any reaction, this is just a
resend with +Cc Stephen Boyd and +Cc linux-kernel@vger.kernel.org
drivers/clk/clk-si5351.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 1e1702e609cb..57e4597cdf4c 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -902,6 +902,10 @@ static int _si5351_clkout_set_disable_state(
static void _si5351_clkout_reset_pll(struct si5351_driver_data *drvdata, int
num)
{
u8 val = si5351_reg_read(drvdata, SI5351_CLK0_CTRL + num);
+ u8 mask = val & SI5351_CLK_PLL_SELECT ? SI5351_PLL_RESET_B :
+ SI5351_PLL_RESET_A;
+ unsigned int v;
+ int err;
switch (val & SI5351_CLK_INPUT_MASK) {
case SI5351_CLK_INPUT_XTAL:
@@ -909,9 +913,12 @@ static void _si5351_clkout_reset_pll(struct
si5351_driver_data *drvdata, int num
return; /* pll not used, no need to reset */
}
- si5351_reg_write(drvdata, SI5351_PLL_RESET,
-val & SI5351_CLK_PLL_SELECT ? SI5351_PLL_RESET_B :
- SI5351_PLL_RESET_A);
+ si5351_reg_write(drvdata, SI5351_PLL_RESET, mask);
+
+ err = regmap_read_poll_timeout(drvdata->regmap, SI5351_PLL_RESET, v,
+!(v & mask), 0, 2);
+ if (err < 0)
+ dev_err(&drvdata->client->dev, "Reset bit didn't clear\n");
dev_dbg(&drvdata->client->dev, "%s - %s: pll = %d\n",
__func__, clk_hw_get_name(&drvdata->clkout[num].hw),
--
2.20.1
Re: [PATCH] ubifs: Fix error return code in ubifs_init_authentication()
On Tue, Nov 24, 2020 at 02:33:20PM +0800, Wang ShaoBo wrote:
> Fix to return PTR_ERR() error code from the error handling case where
> ubifs_hash_get_desc() failed instead of 0 in ubifs_init_authentication(),
> as done elsewhere in this function.
>
> Fixes: 49525e5eecca5 ("ubifs: Add helper functions for authentication
> support")
> Signed-off-by: Wang ShaoBo
> ---
> fs/ubifs/auth.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Sascha Hauer
Sascha
>
> diff --git a/fs/ubifs/auth.c b/fs/ubifs/auth.c
> index b93b3cd10bfd..8c50de693e1d 100644
> --- a/fs/ubifs/auth.c
> +++ b/fs/ubifs/auth.c
> @@ -338,8 +338,10 @@ int ubifs_init_authentication(struct ubifs_info *c)
> c->authenticated = true;
>
> c->log_hash = ubifs_hash_get_desc(c);
> - if (IS_ERR(c->log_hash))
> + if (IS_ERR(c->log_hash)) {
> + err = PTR_ERR(c->log_hash);
> goto out_free_hmac;
> + }
>
> err = 0;
>
> --
> 2.17.1
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] bdi: Fix error return code in alloc_wbufs()
On Sun, Nov 15, 2020 at 04:23:43PM +0800, Wang ShaoBo wrote:
> Fix to return PTR_ERR() error code from the error handling case instead
> fo 0 in function alloc_wbufs(), as done elsewhere in this function.
>
> Fixes: 6a98bc4614de ("ubifs: Add authentication nodes to journal")
> Signed-off-by: Wang ShaoBo
> ---
> fs/ubifs/super.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Prefix for this patch should be "ubifs:" rather than "bdi:". With this:
Reviewed-by: Sascha Hauer
Sascha
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index cb3acfb7dd1f..dacbb999ae34 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -838,8 +838,10 @@ static int alloc_wbufs(struct ubifs_info *c)
> c->jheads[i].wbuf.jhead = i;
> c->jheads[i].grouped = 1;
> c->jheads[i].log_hash = ubifs_hash_get_desc(c);
> - if (IS_ERR(c->jheads[i].log_hash))
> + if (IS_ERR(c->jheads[i].log_hash)) {
> + err = PTR_ERR(c->jheads[i].log_hash);
> goto out;
> + }
> }
>
> /*
> --
> 2.25.1
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: mtd: rawnand: gpmi: regression since e5e5631cc88987a6f3cd8304660bd9190da95916
On Tue, Nov 17, 2020 at 11:16:26AM +0100, Juergen Borleis wrote:
> Hi,
>
> reading a NAND page in raw mode is required to check the consistence of the
> so-
> called FCBs (used to boot the SoC from NAND content).
>
> Before e5e5631cc88987a6f3cd8304660bd9190da95916 ("mtd: rawnand: gpmi: Use
> nand_extract_bits()") it reads the first page of the NAND correctly as:
>
> 00 00 88 fb ff ff 46 43 42 20 00 00 00 01 50 3c |..FCB P<|
> 0010 19 06 00 00 00 00 00 08 00 00 80 08 00 00 40 00 |..@.|
> 0020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 09 00 ||
> 0030 00 00 00 02 00 00 00 02 00 00 09 00 00 00 0a 00 ||
> 0040 00 00 03 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> 0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> 0060 00 00 00 00 00 00 00 00 00 00 00 01 00 00 80 10 ||
> 0070 00 00 55 01 00 00 55 01 00 00 01 00 00 00 9e 07 |..U...U.|
> 0080 00 00 02 00 00 00 00 08 00 00 00 00 00 00 00 00 ||
> 0090 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> *
> 0200 40 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |@...|
> 0210 40 01 00 00 00 80 05 00 00 80 05 00 40 01 00 00 |@...@...|
> 0220 c0 03 00 00 80 02 00 00 00 00 00 00 00 00 00 00 ||
> 0230 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> 0240 00 00 00 00 00 00 00 00 00 00 00 00 00 00 07 00 ||
> 0250 80 83 06 00 00 00 07 00 00 00 07 00 00 07 00 00 ||
> 0260 00 42 06 00 80 05 00 00 00 40 06 00 00 00 00 00 |.B...@..|
> 0270 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> *
> 0790 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fc ||
> 07a0 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> 07b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> *
> 0800 ff 00 00 00 00 00 00 00 00 00 00 00 17 15 06 06 ||
> 0810 10 1f 03 07 00 00 00 1c 0f 17 1f 05 00 00 00 00 ||
> 0820 00 19 00 00 0e 19 00 00 00 00 00 00 00 00 00 00 ||
> 0830 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> *
> 0880
>
> After applying e5e5631cc88987a6f3cd8304660bd9190da95916 reading the same page
> the reported content is broken (the NAND page still contains correct data):
>
> 00 00 88 fb ff ff 46 43 42 20 00 00 00 01 50 3c |..FCB P<|
> 0010 19 06 00 00 00 00 00 08 00 00 80 08 00 00 40 00 |..@.|
> 0020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 09 00 ||
> 0030 00 00 00 02 00 00 00 02 00 00 09 00 00 00 0a 00 ||
> 0040 40 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |@...|
> 0050 40 01 00 00 00 80 05 00 00 80 05 00 40 01 00 00 |@...@...|
> 0060 c0 03 00 00 80 02 00 00 00 00 00 00 00 00 00 00 ||
> 0070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> *
> 0250 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fc ||
> 0260 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> 0270 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ||
> *
> 02c0 06 70 c0 a8 00 00 00 00 00 00 00 00 40 00 00 00 |.p..@...|
Note beginning from offset 0x2c0 we get some uninitialized data. Among
other things we saw systemd unit files there.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2 0/5] Fix the gate2 and make it more flexible
On Wed, Oct 28, 2020 at 02:58:57PM +0200, Abel Vesa wrote: > First version here: https://lkml.org/lkml/2020/10/26/988 > > Changes since v1: > * split the work in multiple iterative patches > > Abel Vesa (5): > clk: imx: gate2: Remove the IMX_CLK_GATE2_SINGLE_BIT special case > clk: imx: gate2: Keep the register writing in on place > clk: imx: gate2: Check if clock is enabled against cgr_val > clk: imx: gate2: Add cgr_mask for more flexible number of control bits > clk: imx: gate2: Add locking in is_enabled op For the series: Reviewed-by: Sascha Hauer Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] clk: imx: gate2: Fix the is_enabled op
On Wed, Oct 28, 2020 at 11:50:57AM +0200, Abel Vesa wrote:
> On 20-10-28 09:24:12, Sascha Hauer wrote:
> > Hi Abel,
> >
> > On Mon, Oct 26, 2020 at 08:50:48PM +0200, Abel Vesa wrote:
> > > The clock is considered to be enabled only if the controlling bits
> > > match the cgr_val mask. Also make sure the is_enabled returns the
> > > correct vaule by locking the access to the register.
> > >
> > > Signed-off-by: Abel Vesa
> > > Fixes: 1e54afe9fcfe ("clk: imx: gate2: Allow single bit gating clock")
> > > ---
> > > drivers/clk/imx/clk-gate2.c | 60
> > > -
> > > drivers/clk/imx/clk.h | 8 ++
> > > 2 files changed, 29 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c
> > > index 7eed708..f320bd2b 100644
> > > --- a/drivers/clk/imx/clk-gate2.c
> > > +++ b/drivers/clk/imx/clk-gate2.c
> > > @@ -37,10 +37,22 @@ struct clk_gate2 {
> > >
> > > #define to_clk_gate2(_hw) container_of(_hw, struct clk_gate2, hw)
> > >
> > > +static void clk_gate2_do_shared_clks(struct clk_hw *hw, bool enable)
> > > +{
> > > + struct clk_gate2 *gate = to_clk_gate2(hw);
> > > + u32 reg;
> > > +
> > > + reg = readl(gate->reg);
> > > + if (enable)
> > > + reg |= gate->cgr_val << gate->bit_idx;
> > > + else
> > > + reg &= ~(gate->cgr_val << gate->bit_idx);
> >
> > Shouldn't this be:
> >
> > reg &= ~(3 << gate->bit_idx);
> > if (enable)
> > reg |= gate->cgr_val << gate->bit_idx;
> >
> > At least that's how it was without this patch and that's how it makes
> > sense to me with cgr_val != 3.
> >
>
> Well, that's the actual problem. The value 3 forces all the clocks
> that register with this clock type to have 2 bits for controlling the gate.
>
> My patch (though now I think I should split it into 2 separate patches) allows
> two HW gates to be controlled by as many bits necessary. For example, there
> could be multiple HW gates that are controled by the same bit. By passing
> the cgr_val when registering the clocks you can specify how many bits (as a
> mask)
> control all those HW gates that share their control bits.
cgr_val is not a mask, it's a value that shall be written to the two
bits to enable the clock. cgr_val could also be 0b10, see imx_clk_gate2_cgr().
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] clk: imx: gate2: Fix the is_enabled op
Hi Abel,
On Mon, Oct 26, 2020 at 08:50:48PM +0200, Abel Vesa wrote:
> The clock is considered to be enabled only if the controlling bits
> match the cgr_val mask. Also make sure the is_enabled returns the
> correct vaule by locking the access to the register.
>
> Signed-off-by: Abel Vesa
> Fixes: 1e54afe9fcfe ("clk: imx: gate2: Allow single bit gating clock")
> ---
> drivers/clk/imx/clk-gate2.c | 60
> -
> drivers/clk/imx/clk.h | 8 ++
> 2 files changed, 29 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c
> index 7eed708..f320bd2b 100644
> --- a/drivers/clk/imx/clk-gate2.c
> +++ b/drivers/clk/imx/clk-gate2.c
> @@ -37,10 +37,22 @@ struct clk_gate2 {
>
> #define to_clk_gate2(_hw) container_of(_hw, struct clk_gate2, hw)
>
> +static void clk_gate2_do_shared_clks(struct clk_hw *hw, bool enable)
> +{
> + struct clk_gate2 *gate = to_clk_gate2(hw);
> + u32 reg;
> +
> + reg = readl(gate->reg);
> + if (enable)
> + reg |= gate->cgr_val << gate->bit_idx;
> + else
> + reg &= ~(gate->cgr_val << gate->bit_idx);
Shouldn't this be:
reg &= ~(3 << gate->bit_idx);
if (enable)
reg |= gate->cgr_val << gate->bit_idx;
At least that's how it was without this patch and that's how it makes
sense to me with cgr_val != 3.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
On Fri, Oct 09, 2020 at 09:48:29AM +0200, Christian Eggers wrote:
> Hi Sascha,
>
> On Friday, 9 October 2020, 09:39:44 CEST, Sascha Hauer wrote:
> > On Fri, Oct 09, 2020 at 06:27:38AM +0200, Christian Eggers wrote:
> > > This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c.
> > >
> > > If CONFIG_PM is disabled, the system completely freezes on probe as
> > > nothing enables the clock of the SPI peripheral.
> >
> > Instead of reverting it, why not just fix it?
> I expect that 5.9 will be released soon. I think that in this late stage
> reverting is more safe than fixing...
>
> > Normally the device should be brought to active state manually in probe
> > before pm_runtime takes over, then CONFIG_PM disabled doesn't hurt.
> > Using pm_runtime to put the device to active state initially has the
> > problem you describe.
> Thanks for the hint. I've no experience in runtime power management. If you
> could provide a patch against the current version, I can make a quick test. I
> can also try to fix it myself, but this will take until next week.
Here we go. The patch basically works, but I am also not very confident
with pm_runtime, so please have a close look ;)
Sascha
---8<----
>From 6c584eb8a2fbff46bf8bbebae6c10609c169309b Mon Sep 17 00:00:00 2001
From: Sascha Hauer
Date: Mon, 12 Oct 2020 15:59:50 +0200
Subject: [PATCH] spi: imx: fix runtime pm support for !CONFIG_PM
525c9e5a32bd introduced pm_runtime support for the i.MX SPI driver. With
this pm_runtime is used to bring up the clocks initially. When CONFIG_PM
is disabled the clocks are no longer enabled and the driver doesn't work
anymore. Fix this by enabling the clocks in the probe function and
telling pm_runtime that the device is active using
pm_runtime_set_active().
Fixes: 525c9e5a32bd spi: imx: enable runtime pm support
Signed-off-by: Sascha Hauer
---
drivers/spi/spi-imx.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 38a5f1304cec..c796e937dc6a 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1674,15 +1674,18 @@ static int spi_imx_probe(struct platform_device *pdev)
goto out_master_put;
}
- pm_runtime_enable(spi_imx->dev);
+ ret = clk_prepare_enable(spi_imx->clk_per);
+ if (ret)
+ goto out_master_put;
+
+ ret = clk_prepare_enable(spi_imx->clk_ipg);
+ if (ret)
+ goto out_put_per;
+
pm_runtime_set_autosuspend_delay(spi_imx->dev, MXC_RPM_TIMEOUT);
pm_runtime_use_autosuspend(spi_imx->dev);
-
- ret = pm_runtime_get_sync(spi_imx->dev);
- if (ret < 0) {
- dev_err(spi_imx->dev, "failed to enable clock\n");
- goto out_runtime_pm_put;
- }
+ pm_runtime_set_active(spi_imx->dev);
+ pm_runtime_enable(spi_imx->dev);
spi_imx->spi_clk = clk_get_rate(spi_imx->clk_per);
/*
@@ -1719,8 +1722,12 @@ static int spi_imx_probe(struct platform_device *pdev)
out_runtime_pm_put:
pm_runtime_dont_use_autosuspend(spi_imx->dev);
- pm_runtime_put_sync(spi_imx->dev);
+ pm_runtime_set_suspended(&pdev->dev);
pm_runtime_disable(spi_imx->dev);
+
+ clk_disable_unprepare(spi_imx->clk_ipg);
+out_put_per:
+ clk_disable_unprepare(spi_imx->clk_per);
out_master_put:
spi_master_put(master);
--
2.28.0
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
On Mon, Oct 12, 2020 at 12:59:34PM +0200, Christian Eggers wrote: > Hi Sascha, > > On Friday, 9 October 2020, 09:39:44 CEST, Sascha Hauer wrote: > > On Fri, Oct 09, 2020 at 06:27:38AM +0200, Christian Eggers wrote: > > > This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c. > > > > > > If CONFIG_PM is disabled, the system completely freezes on probe as > > > nothing enables the clock of the SPI peripheral. > > > > Instead of reverting it, why not just fix it? > > > > Normally the device should be brought to active state manually in probe > > before pm_runtime takes over, then CONFIG_PM disabled doesn't hurt. > > Using pm_runtime to put the device to active state initially has the > > problem you describe. > > prior introducing runtime pm for spi-imx, the clock was "manually" enabled and > disabled around each transfer (so the power usage should already have been > optimal). If we would manually enable the clock in probe() as you suggested, > for users without CONFIG_PM there would be a drawback compared with the > previous state (as the clock will always be on now). > > What is the benefit of controlling the SPI clock with runtime PM instead of > doing it manually? The clocks are reconfigured less frequently with pm_runtime. Especially when enabling/disabling PLLs are involved pm_runtime can increase performance. Also you can put other stuff you need to handle for your device into pm_runtime, think of regulators for example. All this is then abstracted behind a common kernel API. Generally when you disable CONFIG_PM then you are not interested in power consumption and in that case you shouldn't be interested in disabling your SPI clocks. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
On Fri, Oct 09, 2020 at 06:27:38AM +0200, Christian Eggers wrote: > This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c. > > If CONFIG_PM is disabled, the system completely freezes on probe as > nothing enables the clock of the SPI peripheral. Instead of reverting it, why not just fix it? Normally the device should be brought to active state manually in probe before pm_runtime takes over, then CONFIG_PM disabled doesn't hurt. Using pm_runtime to put the device to active state initially has the problem you describe. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/3] ubifs: Fix a memleak after dumping authentication mount options
On Tue, Sep 29, 2020 at 08:45:29PM +0800, Zhihao Cheng wrote:
> Fix a memory leak after dumping authentication mount options in error
> handling branch.
>
> Signed-off-by: Zhihao Cheng
> Cc: # 4.20+
> Fixes: d8a22773a12c6d7 ("ubifs: Enable authentication support")
Reviewed-by: Sascha Hauer
I wonder if patches like in this series should really go to stable. There's
always the risk of regressions, and there's not much to win in fixing
such low probability, low frequency memory holes.
Sascha
> ---
> fs/ubifs/super.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index a2420c900275..6f85cd618766 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1141,6 +1141,18 @@ static int ubifs_parse_options(struct ubifs_info *c,
> char *options,
> return 0;
> }
>
> +/*
> + * ubifs_release_options - release mount parameters which have been dumped.
> + * @c: UBIFS file-system description object
> + */
> +static void ubifs_release_options(struct ubifs_info *c)
> +{
> + kfree(c->auth_key_name);
> + c->auth_key_name = NULL;
> + kfree(c->auth_hash_name);
> + c->auth_hash_name = NULL;
> +}
> +
> /**
> * destroy_journal - destroy journal data structures.
> * @c: UBIFS file-system description object
> @@ -1650,8 +1662,7 @@ static void ubifs_umount(struct ubifs_info *c)
> ubifs_lpt_free(c, 0);
> ubifs_exit_authentication(c);
>
> - kfree(c->auth_key_name);
> - kfree(c->auth_hash_name);
> + ubifs_release_options(c);
> kfree(c->cbuf);
> kfree(c->rcvrd_mst_node);
> kfree(c->mst_node);
> @@ -2219,6 +2230,7 @@ static int ubifs_fill_super(struct super_block *sb,
> void *data, int silent)
> out_unlock:
> mutex_unlock(&c->umount_mutex);
> out_close:
> + ubifs_release_options(c);
> ubi_close_volume(c->ubi);
> out:
> return err;
> --
> 2.25.4
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 2/3] ubifs: Don't parse authentication mount options in remount process
On Tue, Sep 29, 2020 at 08:45:30PM +0800, Zhihao Cheng wrote:
> There is no need to dump authentication options while remounting,
> because authentication initialization can only be doing once in
> the first mount process. Dumping authentication mount options in
> remount process may cause memory leak if UBIFS has already been
> mounted with old authentication mount options.
>
> Signed-off-by: Zhihao Cheng
> Cc: # 4.20+
> Fixes: d8a22773a12c6d7 ("ubifs: Enable authentication support")
Reviewed-by: Sascha Hauer
Sascha
> ---
> fs/ubifs/super.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 6f85cd618766..9796f5df2f7f 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1110,14 +1110,20 @@ static int ubifs_parse_options(struct ubifs_info *c,
> char *options,
> break;
> }
> case Opt_auth_key:
> - c->auth_key_name = kstrdup(args[0].from, GFP_KERNEL);
> - if (!c->auth_key_name)
> - return -ENOMEM;
> + if (!is_remount) {
> + c->auth_key_name = kstrdup(args[0].from,
> + GFP_KERNEL);
> + if (!c->auth_key_name)
> + return -ENOMEM;
> + }
> break;
> case Opt_auth_hash_name:
> - c->auth_hash_name = kstrdup(args[0].from, GFP_KERNEL);
> - if (!c->auth_hash_name)
> - return -ENOMEM;
> + if (!is_remount) {
> + c->auth_hash_name = kstrdup(args[0].from,
> + GFP_KERNEL);
> + if (!c->auth_hash_name)
> + return -ENOMEM;
> + }
> break;
> case Opt_ignore:
> break;
> --
> 2.25.4
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 3/3] ubifs: mount_ubifs: Release authentication resource in error handling path
On Tue, Sep 29, 2020 at 08:45:31PM +0800, Zhihao Cheng wrote:
> Release the authentication related resource in some error handling
> branches in mount_ubifs().
>
> Signed-off-by: Zhihao Cheng
> Cc: # 4.20+
> Fixes: d8a22773a12c6d7 ("ubifs: Enable authentication support")
Reviewed-by: Sascha Hauer
> ---
> fs/ubifs/super.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 9796f5df2f7f..732218ef6656 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1331,7 +1331,7 @@ static int mount_ubifs(struct ubifs_info *c)
>
> err = ubifs_read_superblock(c);
> if (err)
> - goto out_free;
> + goto out_auth;
>
> c->probing = 0;
>
> @@ -1343,18 +1343,18 @@ static int mount_ubifs(struct ubifs_info *c)
> ubifs_err(c, "'compressor \"%s\" is not compiled in",
> ubifs_compr_name(c, c->default_compr));
> err = -ENOTSUPP;
> - goto out_free;
> + goto out_auth;
> }
>
> err = init_constants_sb(c);
> if (err)
> - goto out_free;
> + goto out_auth;
>
> sz = ALIGN(c->max_idx_node_sz, c->min_io_size) * 2;
> c->cbuf = kmalloc(sz, GFP_NOFS);
> if (!c->cbuf) {
> err = -ENOMEM;
> - goto out_free;
> + goto out_auth;
> }
>
> err = alloc_wbufs(c);
> @@ -1629,6 +1629,8 @@ static int mount_ubifs(struct ubifs_info *c)
> free_wbufs(c);
> out_cbuf:
> kfree(c->cbuf);
> +out_auth:
> + ubifs_exit_authentication(c);
> out_free:
> kfree(c->write_reserve_buf);
> kfree(c->bu.buf);
> --
> 2.25.4
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] ubifs: journal: Make sure to not dirty twice for auth nodes
On Mon, Sep 28, 2020 at 09:06:12PM +0200, Richard Weinberger wrote:
> When removing the last reference of an inode the size of an auth node
> is already part of write_len. So we must not call ubifs_add_auth_dirt().
> Call it only when needed.
>
> Cc:
> Cc: Sascha Hauer
> Cc: Kristof Havasi
> Fixes: 6a98bc4614de ("ubifs: Add authentication nodes to journal")
> Reported-by: Kristof Havasi
> Signed-off-by: Richard Weinberger
Looked at the code until I understood what the problem is and how it is
fixed, so:
Reviewed-by: Sascha Hauer
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH AUTOSEL 5.8 28/29] spi: fsl-dspi: fix use-after-free in remove path
Hi Sasha,
On Mon, Sep 28, 2020 at 09:30:25PM -0400, Sasha Levin wrote:
> From: Sascha Hauer
>
> [ Upstream commit 530b5affc675ade5db4a03f04ed7cd66806c8a1a ]
>
> spi_unregister_controller() not only unregisters the controller, but
> also frees the controller. This will free the driver data with it, so
> we must not access it later dspi_remove().
>
> Solve this by allocating the driver data separately from the SPI
> controller.
>
> Signed-off-by: Sascha Hauer
> Link: https://lore.kernel.org/r/20200923131026.20707-1-s.ha...@pengutronix.de
> Signed-off-by: Mark Brown
> Signed-off-by: Sasha Levin
> ---
> drivers/spi/spi-fsl-dspi.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
This patch causes a regression and shouldn't be applied without the fix
in https://lkml.org/lkml/2020/9/28/300.
Sascha
> index 91c6affe139c9..aae9f9a7aea6c 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -1273,11 +1273,14 @@ static int dspi_probe(struct platform_device *pdev)
> void __iomem *base;
> bool big_endian;
>
> - ctlr = spi_alloc_master(&pdev->dev, sizeof(struct fsl_dspi));
> + dspi = devm_kzalloc(&pdev->dev, sizeof(*dspi), GFP_KERNEL);
> + if (!dspi)
> + return -ENOMEM;
> +
> + ctlr = spi_alloc_master(&pdev->dev, 0);
> if (!ctlr)
> return -ENOMEM;
>
> - dspi = spi_controller_get_devdata(ctlr);
> dspi->pdev = pdev;
> dspi->ctlr = ctlr;
>
> @@ -1414,7 +1417,7 @@ static int dspi_probe(struct platform_device *pdev)
> if (dspi->devtype_data->trans_mode != DSPI_DMA_MODE)
> ctlr->ptp_sts_supported = true;
>
> - platform_set_drvdata(pdev, ctlr);
> + platform_set_drvdata(pdev, dspi);
>
> ret = spi_register_controller(ctlr);
> if (ret != 0) {
> @@ -1437,8 +1440,7 @@ static int dspi_probe(struct platform_device *pdev)
>
> static int dspi_remove(struct platform_device *pdev)
> {
> - struct spi_controller *ctlr = platform_get_drvdata(pdev);
> - struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
> + struct fsl_dspi *dspi = platform_get_drvdata(pdev);
>
> /* Disconnect from the SPI framework */
> spi_unregister_controller(dspi->ctlr);
> --
> 2.25.1
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] spi: fsl-dspi: fix NULL pointer dereference
On Mon, Sep 28, 2020 at 02:27:47AM +0300, Vladimir Oltean wrote:
> On Mon, Sep 28, 2020 at 12:43:36AM +0200, Michael Walle wrote:
> > Since commit 530b5affc675 ("spi: fsl-dspi: fix use-after-free in remove
> > path") this driver causes a kernel oops:
> >
> > [1.891065] Unable to handle kernel NULL pointer dereference at virtual
> > address 0080
> > [1.899889] Mem abort info:
> > [1.902692] ESR = 0x9604
> > [1.905754] EC = 0x25: DABT (current EL), IL = 32 bits
> > [1.911089] SET = 0, FnV = 0
> > [1.914156] EA = 0, S1PTW = 0
> > [1.917303] Data abort info:
> > [1.920193] ISV = 0, ISS = 0x0004
> > [1.924044] CM = 0, WnR = 0
> > [1.927022] [0080] user address but active_mm is swapper
> > [1.933403] Internal error: Oops: 9604 [#1] PREEMPT SMP
> > [1.938995] Modules linked in:
> > [1.942060] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> > 5.9.0-rc6-next-20200925-00026-gae556cc74e28-dirty #94
> > [1.951838] Hardware name: Kontron SMARC-sAL28 (Single PHY) on SMARC
> > Eval 2.0 carrier (DT)
> > [1.960135] pstate: 4005 (nZcv daif -PAN -UAO -TCO BTYPE=--)
> > [1.966168] pc : dspi_setup+0xc8/0x2e0
> > [1.969926] lr : dspi_setup+0xbc/0x2e0
> > [1.973684] sp : 80001139b930
> > [1.977005] x29: 80001139b930 x28: 00207a5d2000
> > [1.982338] x27: 0006 x26: 00207a44d410
> > [1.987669] x25: 002079c08100 x24: 00207a5d2400
> > [1.993000] x23: 00207a5d2600 x22: 800011169948
> > [1.998332] x21: 800010cbcd20 x20: 00207a58a800
> > [2.003663] x19: 00207a76b700 x18: 0010
> > [2.008994] x17: 0001 x16: 0019
> > [2.014326] x15: x14: 0720072007200720
> > [2.019657] x13: 0720072007200720 x12: 8000111fc5e0
> > [2.024989] x11: 0003 x10: 8000111e45a0
> > [2.030320] x9 : x8 : 00207a76b780
> > [2.035651] x7 : x6 : 003f
> > [2.040982] x5 : 0040 x4 : 80001139b918
> > [2.046313] x3 : 0001 x2 : 64b62cc917af5100
> > [2.051643] x1 : x0 :
> > [2.056973] Call trace:
> > [2.059425] dspi_setup+0xc8/0x2e0
> > [2.062837] spi_setup+0xcc/0x248
> > [2.066160] spi_add_device+0xb4/0x198
> > [2.069918] of_register_spi_device+0x250/0x370
> > [2.074462] spi_register_controller+0x4f4/0x770
> > [2.079094] dspi_probe+0x5bc/0x7b0
> > [2.082594] platform_drv_probe+0x5c/0xb0
> > [2.086615] really_probe+0xec/0x3c0
> > [2.090200] driver_probe_device+0x60/0xc0
> > [2.094308] device_driver_attach+0x7c/0x88
> > [2.098503] __driver_attach+0x60/0xe8
> > [2.102263] bus_for_each_dev+0x7c/0xd0
> > [2.106109] driver_attach+0x2c/0x38
> > [2.109692] bus_add_driver+0x194/0x1f8
> > [2.113538] driver_register+0x6c/0x128
> > [2.117385] __platform_driver_register+0x50/0x60
> > [2.122105] fsl_dspi_driver_init+0x24/0x30
> > [2.126302] do_one_initcall+0x54/0x2d0
> > [2.130149] kernel_init_freeable+0x1ec/0x258
> > [2.134520] kernel_init+0x1c/0x120
> > [2.138018] ret_from_fork+0x10/0x34
> > [2.141606] Code: 97e0b11d aa0003f3 b4000680 f94006e0 (f9404000)
> > [2.147723] ---[ end trace 26cf63e6cbba33a8 ]---
> > [2.152374] Kernel panic - not syncing: Attempted to kill init!
> > exitcode=0x000b
> > [2.160061] SMP: stopping secondary CPUs
> > [2.163999] Kernel Offset: disabled
> > [2.167496] CPU features: 0x0040022,20006008
> > [2.171777] Memory Limit: none
> > [2.174840] ---[ end Kernel panic - not syncing: Attempted to kill init!
> > exitcode=0x000b ]---
> >
> > This is because since this commit, the allocation of the drivers private
> > data is done explicitly and in this case spi_alloc_master() won't set the
> > correct pointer.
> >
> > Fixes: 530b5affc675 ("spi: fsl-dspi: fix use-after-free in remove path")
> > Signed-off-by: Michael Walle
> > ---
>
> Sascha, how did you test commit 530b5affc675?
My intention was to test it, but it seems I somehow failed to copy the
new kernel onto my target without noticing it, shame on me. Anyway, I
get the same kernel panic with my patch appied now and this patch fixes
it.
Tested-by: Sascha Hauer
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 02/16] dt-bindings: mtd: gpmi-nand: Fix matching of clocks on different SoCs
On Mon, Aug 24, 2020 at 09:06:47PM +0200, Krzysztof Kozlowski wrote: > Driver requires different amount of clocks for different SoCs. Describe > these requirements properly to fix dtbs_check warnings like: > > arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: > nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected > > Signed-off-by: Krzysztof Kozlowski > --- > .../devicetree/bindings/mtd/gpmi-nand.yaml| 76 +++ > 1 file changed, 61 insertions(+), 15 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml > b/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml > index 28ff8c581837..9d764e654e1d 100644 > --- a/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml > +++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml > + - if: > + properties: > +compatible: > + contains: > +enum: > + - fsl,imx6q-gpmi-nand > + - fsl,imx6sx-gpmi-nand > +then: > + properties: > +clocks: > + items: > +- description: SoC gpmi io clock > +- description: SoC gpmi apb clock > +- description: SoC gpmi bch clock > +- description: SoC gpmi bch apb clock > +- description: SoC per1 bch clock > +clock-names: > + items: > +- const: gpmi_io > +- const: gpmi_apb > +- const: gpmi_bch > +- const: gpmi_bch_apb > +- const: per1_bch This enforces this specific order of the clocks given in the dts. The clock binding itself doesn't require any specific order, that's what we have the names array for. Is this really what we want? Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: pcm|dmaengine|imx-sdma race condition on i.MX6
On Fri, Aug 21, 2020 at 09:52:00AM +, Robin Gong wrote: > On 2020/08/20 14:52 Sascha Hauer wrote: > > On Wed, Aug 19, 2020 at 01:08:29PM +0200, Lars-Peter Clausen wrote: > > > > For the first option, which is potentially more performant, we have > > > > to leave the atomic PCM context and we are not sure if we are allowed > > > > to. > > > > For the second option, we would have to divide the dma_device > > > > terminate_all into an atomic sync and an async one, which would > > > > align with the dmaengine API, giving it the option to ensure > > > > termination in > > an atomic context. > > > > Based on my understanding, most of them are synchronous anyways, for > > > > the currently async ones we would have to implement busy waits. > > > > However, with this approach, we reach the WARN_ON [6] inside of an > > > > atomic context, indicating we might not do the right thing. > > > > > > I don't know how feasible this is to implement in the SDMA dmaengine > > driver. > > > But I think what is should do is to have some flag to indicate if a > > > terminate is in progress. If a new transfer is issued while terminate > > > is in progress the transfer should go on a list. Once terminate > > > finishes it should check the list and start the transfer if there are any > > > on the > > list. > > > > The list is already there in form of the vchan helpers the driver uses. > Seems Lars major concern is on the race condition between next descriptor > and sdma_channel_terminate_work which free the last terminated descriptor, > not the ability of vchan to support multi descriptors. But anyway, I think we > should take care vchan_get_all_descriptors to free descriptors during > terminate > phase in case it's done in worker like sdma_channel_terminate_work, since that > may free the next descriptor wrongly. That's what my patch attached in > 0001-dmaengine-imx-sdma-add-terminated-list-for-freed-des.patch > https://www.spinics.net/lists/arm-kernel/msg829972.html Indeed this should solve the problem of freeing descriptors allocated between terminate_all and a following prep_slave*. > > > > > I think the big mistake the driver makes is to configure fields in struct > > sdma_channel and also the hardware directly in sdma_prep_memcpy(), > > sdma_prep_slave_sg() and sdma_prep_dma_cyclic(). All information should be > > stored in the struct sdma_desc allocated in the prep functions and only be > > used > > when it's time to fire that specific descriptor. > Sorry Sascha, seems that's another topic and your intention is to make sure > only > software involved in sdma_prep_* and all HW moved into one function inside > sdma_start_desc. I agree that will make code more clean but my concern is > sdma_start_desc is protect by spin_lock which should be short as possible > while > some HW touch as context_load may cost some time. Anyway, that's another > topic, > maybe we can refine it in the future. Yes, you are right. This is another topic. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2 13/19] dt-bindings: nvmem: imx-ocotp: Update i.MX 8M compatibles
On Mon, Aug 24, 2020 at 06:26:46PM +0200, Krzysztof Kozlowski wrote: > +oneOf: > + - items: > + - enum: > + - fsl,imx6q-ocotp > + - fsl,imx6sl-ocotp > + - fsl,imx6sx-ocotp > + - fsl,imx6ul-ocotp > + - fsl,imx6ull-ocotp > + - fsl,imx7d-ocotp > + - fsl,imx6sll-ocotp > + - fsl,imx7ulp-ocotp > + - fsl,imx8mq-ocotp > + - fsl,imx8mm-ocotp > + - fsl,imx8mn-ocotp > + - fsl,imx8mp-ocotp > + - const: syscon > + - items: > + # The devices are not really compatible with fsl,imx8mm-ocotp, > however > + # the code for getting SoC revision depends on fsl,imx8mm-ocotp > compatible. Shouldn't this be fixed? It seems strange to justify a binding with existing code. Sascha > + - enum: > + - fsl,imx8mn-ocotp > + - fsl,imx8mp-ocotp > + - const: fsl,imx8mm-ocotp > + - const: syscon > >reg: > maxItems: 1 > -- > 2.17.1 > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 02/22] dt-bindings: gpio: fsl-imx-gpio: Add gpio-ranges property
On Mon, Aug 24, 2020 at 08:38:06AM +0200, Krzysztof Kozlowski wrote:
> On Mon, Aug 24, 2020 at 07:24:46AM +0200, Sascha Hauer wrote:
> > On Sun, Aug 23, 2020 at 06:15:30PM +0200, Krzysztof Kozlowski wrote:
> > > The GPIO controller node can have gpio-ranges property. This fixes
> > > dtbs_check warnings like:
> > >
> > > arch/arm64/boot/dts/freescale/imx8mm-evk.dt.yaml: gpio@3020:
> > > 'gpio-ranges' does not match any of the regexes: 'pinctrl-[0-9]+'
> > > From schema: Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml
> > >
> > > Signed-off-by: Krzysztof Kozlowski
> > > ---
> > > .../devicetree/bindings/gpio/fsl-imx-gpio.yaml| 15 +++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml
> > > b/Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml
> > > index 454db20c2d1a..1fac69573bb9 100644
> > > --- a/Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml
> > > +++ b/Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml
> > > @@ -51,6 +51,9 @@ properties:
> > >
> > >gpio-controller: true
> > >
> > > + gpio-ranges:
> > > +maxItems: 1
> > > +
> > > required:
> > >- compatible
> > >- reg
> > > @@ -62,6 +65,18 @@ required:
> > >
> > > additionalProperties: false
> > >
> > > +allOf:
> > > + - if:
> > > + properties:
> > > +compatible:
> > > + contains:
> > > +const: fsl,imx8mp-gpio
> > > +then:
> > > + properties:
> > > +gpio-ranges:
> > > + minItems: 1
> > > + maxItems: 2
> >
> > Why do you limit this to fsl,imx8mp-gpio? The i.MX5,6,7 dtsi files use
> > gpio-ranges as well and other i.MX dtsi files could also use it.
>
> All other cases use maximum one element in gpio-ranges, so they are
> covered so I assumed they are continuous. But if it not the case, I can
> make all them maximum 2.
I misread this, I thought you allow gpio-ranges only for imx8mp, but
it's only the maxItems you set differently for that SoC. Anyway,
arch/arm/boot/dts/imx6dl.dtsi has this:
&gpio1 {
gpio-ranges = <&iomuxc 0 131 2>, <&iomuxc 2 137 8>, <&iomuxc 10 189
2>,
<&iomuxc 12 194 1>, <&iomuxc 13 193 1>, <&iomuxc 14 192
1>,
<&iomuxc 15 191 1>, <&iomuxc 16 185 2>, <&iomuxc 18 184
1>,
<&iomuxc 19 187 1>, <&iomuxc 20 183 1>, <&iomuxc 21 188
1>,
<&iomuxc 22 123 3>, <&iomuxc 25 121 1>, <&iomuxc 26 127
1>,
<&iomuxc 27 126 1>, <&iomuxc 28 128 1>, <&iomuxc 29 130
1>,
<&iomuxc 30 129 1>, <&iomuxc 31 122 1>;
};
I don't think it makes sense to specify maxItems.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 07/22] dt-bindings: serial: fsl-imx-uart: imx-pwm: Add i.MX 8M compatibles
The subject contains a "imx-pwm", presumably from the last patch.
Sascha
On Sun, Aug 23, 2020 at 06:15:35PM +0200, Krzysztof Kozlowski wrote:
> DTSes with new i.MX 8M SoCs introduce their own compatibles so add them
> to fix dtbs_check warnings like:
>
> arch/arm64/boot/dts/freescale/imx8mm-evk.dt.yaml: pwm@3066:
> compatible:0: 'fsl,imx8mm-pwm' is not one of ['fsl,imx1-pwm',
> 'fsl,imx27-pwm']
> From schema: Documentation/devicetree/bindings/pwm/imx-pwm.yaml
>
> arch/arm64/boot/dts/freescale/imx8mm-evk.dt.yaml: pwm@3066:
> compatible: ['fsl,imx8mm-pwm', 'fsl,imx27-pwm'] is too long
>
> arch/arm64/boot/dts/freescale/imx8mm-evk.dt.yaml: pwm@3066:
> compatible: Additional items are not allowed ('fsl,imx27-pwm' was
> unexpected)
>
> Signed-off-by: Krzysztof Kozlowski
> ---
> Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
> b/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
> index cba3f83ccd5f..3d896173b3b0 100644
> --- a/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
> @@ -36,6 +36,10 @@ properties:
> - fsl,imx6sx-uart
> - fsl,imx6ul-uart
> - fsl,imx7d-uart
> +- fsl,imx8mm-uart
> +- fsl,imx8mn-uart
> +- fsl,imx8mp-uart
> +- fsl,imx8mq-uart
>- const: fsl,imx6q-uart
>
>reg:
> --
> 2.17.1
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 02/22] dt-bindings: gpio: fsl-imx-gpio: Add gpio-ranges property
On Sun, Aug 23, 2020 at 06:15:30PM +0200, Krzysztof Kozlowski wrote: > The GPIO controller node can have gpio-ranges property. This fixes > dtbs_check warnings like: > > arch/arm64/boot/dts/freescale/imx8mm-evk.dt.yaml: gpio@3020: > 'gpio-ranges' does not match any of the regexes: 'pinctrl-[0-9]+' > From schema: Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml > > Signed-off-by: Krzysztof Kozlowski > --- > .../devicetree/bindings/gpio/fsl-imx-gpio.yaml| 15 +++ > 1 file changed, 15 insertions(+) > > diff --git a/Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml > b/Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml > index 454db20c2d1a..1fac69573bb9 100644 > --- a/Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml > +++ b/Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml > @@ -51,6 +51,9 @@ properties: > >gpio-controller: true > > + gpio-ranges: > +maxItems: 1 > + > required: >- compatible >- reg > @@ -62,6 +65,18 @@ required: > > additionalProperties: false > > +allOf: > + - if: > + properties: > +compatible: > + contains: > +const: fsl,imx8mp-gpio > +then: > + properties: > +gpio-ranges: > + minItems: 1 > + maxItems: 2 Why do you limit this to fsl,imx8mp-gpio? The i.MX5,6,7 dtsi files use gpio-ranges as well and other i.MX dtsi files could also use it. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v2] lib/fonts: add font 6x8 for OLED display
From: Sven Schneider
This font is derived from lib/fonts/font_6x10.c and is useful for small
OLED displays
Signed-off-by: Sven Schneider
Signed-off-by: Sascha Hauer
---
Changes since v1:
- re-added lost Signed-off-by lines
- do not mode change include/linux/font.h to 0755
- Add SPDX tag
- Whitespace cleanup
include/linux/font.h |4 +-
lib/fonts/Kconfig|7 +
lib/fonts/Makefile |1 +
lib/fonts/font_6x8.c | 2576 ++
lib/fonts/fonts.c|3 +
5 files changed, 2590 insertions(+), 1 deletion(-)
create mode 100644 lib/fonts/font_6x8.c
diff --git a/include/linux/font.h b/include/linux/font.h
index 51b91c8b69d58..4a3f8741bb7e5 100644
--- a/include/linux/font.h
+++ b/include/linux/font.h
@@ -33,6 +33,7 @@ struct font_desc {
#defineMINI4x6_IDX 9
#define FONT6x10_IDX 10
#define TER16x32_IDX 11
+#define FONT6x8_IDX12
extern const struct font_desc font_vga_8x8,
font_vga_8x16,
@@ -45,7 +46,8 @@ extern const struct font_desc font_vga_8x8,
font_acorn_8x8,
font_mini_4x6,
font_6x10,
- font_ter_16x32;
+ font_ter_16x32,
+ font_6x8;
/* Find a font with a specific name */
diff --git a/lib/fonts/Kconfig b/lib/fonts/Kconfig
index 37baa79cdd71f..c035fde66aebe 100644
--- a/lib/fonts/Kconfig
+++ b/lib/fonts/Kconfig
@@ -119,6 +119,12 @@ config FONT_TER16x32
This is the high resolution, large version for use with HiDPI screens.
If the standard font is unreadable for you, say Y, otherwise say N.
+config FONT_6x8
+ bool "OLED 6x8 font" if FONTS
+ depends on FRAMEBUFFER_CONSOLE
+ help
+ This font is useful for small displays (OLED).
+
config FONT_AUTOSELECT
def_bool y
depends on !FONT_8x8
@@ -132,6 +138,7 @@ config FONT_AUTOSELECT
depends on !FONT_SUN12x22
depends on !FONT_10x18
depends on !FONT_TER16x32
+ depends on !FONT_6x8
select FONT_8x16
endif # FONT_SUPPORT
diff --git a/lib/fonts/Makefile b/lib/fonts/Makefile
index ed95070860deb..e16f68492174a 100644
--- a/lib/fonts/Makefile
+++ b/lib/fonts/Makefile
@@ -15,6 +15,7 @@ font-objs-$(CONFIG_FONT_ACORN_8x8) += font_acorn_8x8.o
font-objs-$(CONFIG_FONT_MINI_4x6) += font_mini_4x6.o
font-objs-$(CONFIG_FONT_6x10) += font_6x10.o
font-objs-$(CONFIG_FONT_TER16x32) += font_ter16x32.o
+font-objs-$(CONFIG_FONT_6x8) += font_6x8.o
font-objs += $(font-objs-y)
diff --git a/lib/fonts/font_6x8.c b/lib/fonts/font_6x8.c
new file mode 100644
index 0..e064477884186
--- /dev/null
+++ b/lib/fonts/font_6x8.c
@@ -0,0 +1,2576 @@
+// SPDX-License-Identifier: GPL-2.0
+#include
+
+#define FONTDATAMAX 2048
+
+static const unsigned char fontdata_6x8[FONTDATAMAX] = {
+
+ /* 0 0x00 '^@' */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+
+ /* 1 0x01 '^A' */
+ 0x78, /* 00 */
+ 0x84, /* 11 */
+ 0xCC, /* 110011 */
+ 0x84, /* 11 */
+ 0xCC, /* 110011 */
+ 0xB4, /* 101101 */
+ 0x78, /* 00 */
+ 0x00, /* 00 */
+
+ /* 2 0x02 '^B' */
+ 0x78, /* 00 */
+ 0xFC, /* 11 */
+ 0xB4, /* 101101 */
+ 0xFC, /* 11 */
+ 0xB4, /* 101101 */
+ 0xCC, /* 110011 */
+ 0x78, /* 00 */
+ 0x00, /* 00 */
+
+ /* 3 0x03 '^C' */
+ 0x00, /* 00 */
+ 0x28, /* 001010 */
+ 0x7C, /* 01 */
+ 0x7C, /* 01 */
+ 0x38, /* 001110 */
+ 0x10, /* 000100 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+
+ /* 4 0x04 '^D' */
+ 0x00, /* 00 */
+ 0x10, /* 000100 */
+ 0x38, /* 001110 */
+ 0x7C, /* 01 */
+ 0x38, /* 001110 */
+ 0x10, /* 000100 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+
+ /* 5 0x05 '^E' */
+ 0x00, /* 00 */
+ 0x38, /* 001110 */
+ 0x38, /* 001110 */
+ 0x6C, /* 011011 */
+ 0x6C, /* 011011 */
+ 0x10, /* 000100 */
+ 0x38, /* 001110 */
+ 0x00, /* 00 */
+
+ /* 6 0x06 '^F' */
+ 0x00, /* 00 */
+ 0x10, /* 000100 */
+ 0x38, /* 001110 */
+ 0x7C, /* 01 */
+ 0x7C, /* 01 */
+ 0x10, /* 000100 */
+ 0x38, /* 001110 */
+ 0x00, /* 00 */
+
+ /* 7 0x07 '^G' */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x30, /* 001100 */
+ 0x78, /* 00 */
+ 0x30, /* 001100 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+
+ /* 8 0x08 '^H' */
+
[PATCH RESEND] lib/fonts: add font 6x8 for oled display
From: Sven Schneider
This font is derived from lib/fonts/font_6x10.c and is useful for small OLED
displays.
---
Hi All,
I am not sure any new fonts are desired in the kernel. If yes, please consider
for inclusion, otherwise some "go away, there are enough fonts in the kernel
already" would be nice as well so I can stop trying getting it in :)
Sascha
include/linux/font.h |4 +-
lib/fonts/Kconfig|7 +
lib/fonts/Makefile |1 +
lib/fonts/font_6x8.c | 2575 ++
lib/fonts/fonts.c|3 +
5 files changed, 2589 insertions(+), 1 deletion(-)
mode change 100644 => 100755 include/linux/font.h
create mode 100644 lib/fonts/font_6x8.c
diff --git a/include/linux/font.h b/include/linux/font.h
old mode 100644
new mode 100755
index 51b91c8b69d58..4a3f8741bb7e5
--- a/include/linux/font.h
+++ b/include/linux/font.h
@@ -33,6 +33,7 @@ struct font_desc {
#defineMINI4x6_IDX 9
#define FONT6x10_IDX 10
#define TER16x32_IDX 11
+#define FONT6x8_IDX12
extern const struct font_desc font_vga_8x8,
font_vga_8x16,
@@ -45,7 +46,8 @@ extern const struct font_desc font_vga_8x8,
font_acorn_8x8,
font_mini_4x6,
font_6x10,
- font_ter_16x32;
+ font_ter_16x32,
+ font_6x8;
/* Find a font with a specific name */
diff --git a/lib/fonts/Kconfig b/lib/fonts/Kconfig
index 37baa79cdd71f..c035fde66aebe 100644
--- a/lib/fonts/Kconfig
+++ b/lib/fonts/Kconfig
@@ -119,6 +119,12 @@ config FONT_TER16x32
This is the high resolution, large version for use with HiDPI screens.
If the standard font is unreadable for you, say Y, otherwise say N.
+config FONT_6x8
+ bool "OLED 6x8 font" if FONTS
+ depends on FRAMEBUFFER_CONSOLE
+ help
+ This font is useful for small displays (OLED).
+
config FONT_AUTOSELECT
def_bool y
depends on !FONT_8x8
@@ -132,6 +138,7 @@ config FONT_AUTOSELECT
depends on !FONT_SUN12x22
depends on !FONT_10x18
depends on !FONT_TER16x32
+ depends on !FONT_6x8
select FONT_8x16
endif # FONT_SUPPORT
diff --git a/lib/fonts/Makefile b/lib/fonts/Makefile
index ed95070860deb..e16f68492174a 100644
--- a/lib/fonts/Makefile
+++ b/lib/fonts/Makefile
@@ -15,6 +15,7 @@ font-objs-$(CONFIG_FONT_ACORN_8x8) += font_acorn_8x8.o
font-objs-$(CONFIG_FONT_MINI_4x6) += font_mini_4x6.o
font-objs-$(CONFIG_FONT_6x10) += font_6x10.o
font-objs-$(CONFIG_FONT_TER16x32) += font_ter16x32.o
+font-objs-$(CONFIG_FONT_6x8) += font_6x8.o
font-objs += $(font-objs-y)
diff --git a/lib/fonts/font_6x8.c b/lib/fonts/font_6x8.c
new file mode 100644
index 0..d70a9e4e30be9
--- /dev/null
+++ b/lib/fonts/font_6x8.c
@@ -0,0 +1,2575 @@
+#include
+
+#define FONTDATAMAX 2048
+
+static const unsigned char fontdata_6x8[FONTDATAMAX] = {
+
+ /* 0 0x00 '^@' */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+
+ /* 1 0x01 '^A' */
+ 0x78, /* 00 */
+ 0x84, /* 11 */
+ 0xCC, /* 110011 */
+ 0x84, /* 11 */
+ 0xCC, /* 110011 */
+ 0xB4, /* 101101 */
+ 0x78, /* 00 */
+ 0x00, /* 00 */
+
+ /* 2 0x02 '^B' */
+ 0x78, /* 00 */
+ 0xFC, /* 11 */
+ 0xB4, /* 101101 */
+ 0xFC, /* 11 */
+ 0xB4, /* 101101 */
+ 0xCC, /* 110011 */
+ 0x78, /* 00 */
+ 0x00, /* 00 */
+
+ /* 3 0x03 '^C' */
+ 0x00, /* 00 */
+ 0x28, /* 001010 */
+ 0x7C, /* 01 */
+ 0x7C, /* 01 */
+ 0x38, /* 001110 */
+ 0x10, /* 000100 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+
+ /* 4 0x04 '^D' */
+ 0x00, /* 00 */
+ 0x10, /* 000100 */
+ 0x38, /* 001110 */
+ 0x7C, /* 01 */
+ 0x38, /* 001110 */
+ 0x10, /* 000100 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+
+ /* 5 0x05 '^E' */
+ 0x00, /* 00 */
+ 0x38, /* 001110 */
+ 0x38, /* 001110 */
+ 0x6C, /* 011011 */
+ 0x6C, /* 011011 */
+ 0x10, /* 000100 */
+ 0x38, /* 001110 */
+ 0x00, /* 00 */
+
+ /* 6 0x06 '^F' */
+ 0x00, /* 00 */
+ 0x10, /* 000100 */
+ 0x38, /* 001110 */
+ 0x7C, /* 01 */
+ 0x7C, /* 01 */
+ 0x10, /* 000100 */
+ 0x38, /* 001110 */
+ 0x00, /* 00 */
+
+ /* 7 0x07 '^G' */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x30, /* 001100 */
+ 0x78, /* 00 */
+ 0x30, /* 001100 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+ 0x00, /* 00 */
+
+ /* 8 0x08 '^H' */
+ 0xFC, /* 11 */
+
Re: pcm|dmaengine|imx-sdma race condition on i.MX6
On Wed, Aug 19, 2020 at 01:08:29PM +0200, Lars-Peter Clausen wrote: > > For the first option, which is potentially more performant, we have to > > leave the atomic PCM context > > and we are not sure if we are allowed to. > > For the second option, we would have to divide the dma_device terminate_all > > into an atomic sync and > > an async one, which would align with the dmaengine API, giving it the > > option to ensure termination > > in an atomic context. > > Based on my understanding, most of them are synchronous anyways, for the > > currently async ones we > > would have to implement busy waits. > > However, with this approach, we reach the WARN_ON [6] inside of an atomic > > context, > > indicating we might not do the right thing. > > I don't know how feasible this is to implement in the SDMA dmaengine driver. > But I think what is should do is to have some flag to indicate if a > terminate is in progress. If a new transfer is issued while terminate is in > progress the transfer should go on a list. Once terminate finishes it should > check the list and start the transfer if there are any on the list. The list is already there in form of the vchan helpers the driver uses. I think the big mistake the driver makes is to configure fields in struct sdma_channel and also the hardware directly in sdma_prep_memcpy(), sdma_prep_slave_sg() and sdma_prep_dma_cyclic(). All information should be stored in the struct sdma_desc allocated in the prep functions and only be used when it's time to fire that specific descriptor. More specifically sdma_config_write() may not be called from sdma_prep_slave_sg() or sdma_prep_dma_cyclic(), but instead must be called from sdma_start_desc(). sdma_config_ownership() also must be called later in sdma_start_desc(). 'direction' must be a member of struct sdma_desc, not of struct sdma_channel. Overall this sounds like a fair amount of work to do, but should be feasible and IMO is a step in the right direction. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 2/2] net: ethernet: mvneta: Add back interface mode validation
When writing the serdes configuration register was moved to
mvneta_config_interface() the whole code block was removed from
mvneta_port_power_up() in the assumption that its only purpose was to
write the serdes configuration register. As mentioned by Russell King
its purpose was also to check for valid interface modes early so that
later in the driver we do not have to care for unexpected interface
modes.
Add back the test to let the driver bail out early on unhandled
interface modes.
Signed-off-by: Sascha Hauer
---
drivers/net/ethernet/marvell/mvneta.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c
b/drivers/net/ethernet/marvell/mvneta.c
index c4552f868157c..c639e3a293024 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -5009,10 +5009,18 @@ static void mvneta_conf_mbus_windows(struct mvneta_port
*pp,
}
/* Power up the port */
-static void mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
+static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
{
/* MAC Cause register should be cleared */
mvreg_write(pp, MVNETA_UNIT_INTR_CAUSE, 0);
+
+ if (phy_mode != PHY_INTERFACE_MODE_QSGMII &&
+ phy_mode != PHY_INTERFACE_MODE_SGMII &&
+ !phy_interface_mode_is_8023z(phy_mode) &&
+ !phy_interface_mode_is_rgmii(phy_mode))
+ return -EINVAL;
+
+ return 0;
}
/* Device initialization routine */
@@ -5198,7 +5206,11 @@ static int mvneta_probe(struct platform_device *pdev)
if (err < 0)
goto err_netdev;
- mvneta_port_power_up(pp, phy_mode);
+ err = mvneta_port_power_up(pp, pp->phy_interface);
+ if (err < 0) {
+ dev_err(&pdev->dev, "can't power up port\n");
+ return err;
+ }
/* Armada3700 network controller does not support per-cpu
* operation, so only single NAPI should be initialized.
@@ -5352,7 +5364,11 @@ static int mvneta_resume(struct device *device)
}
}
mvneta_defaults_set(pp);
- mvneta_port_power_up(pp, pp->phy_interface);
+ err = mvneta_port_power_up(pp, pp->phy_interface);
+ if (err < 0) {
+ dev_err(device, "can't power up port\n");
+ return err;
+ }
netif_device_attach(dev);
--
2.27.0
[PATCH 1/2] net: ethernet: mvneta: Do not error out in non serdes modes
In mvneta_config_interface() the RGMII modes are catched by the default case which is an error return. The RGMII modes are valid modes for the driver, so instead of returning an error add a break statement to return successfully. This avoids this warning for non comphy SoCs which use RGMII, like SolidRun Clearfog: WARNING: CPU: 0 PID: 268 at drivers/net/ethernet/marvell/mvneta.c:3512 mvneta_start_dev+0x220/0x23c Signed-off-by: Sascha Hauer --- drivers/net/ethernet/marvell/mvneta.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index af60001728481..c4552f868157c 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -3571,7 +3571,7 @@ static int mvneta_config_interface(struct mvneta_port *pp, MVNETA_HSGMII_SERDES_PROTO); break; default: - return -EINVAL; + break; } } -- 2.27.0
Re: [PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy
On Tue, Jun 23, 2020 at 12:53:40AM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Jun 16, 2020 at 10:31:39AM +0200, Sascha Hauer wrote:
> > The MVNETA_SERDES_CFG register is only available on older SoCs like the
> > Armada XP. On newer SoCs like the Armada 38x the fields are moved to
> > comphy. This patch moves the writes to this register next to the comphy
> > initialization, so that depending on the SoC either comphy or
> > MVNETA_SERDES_CFG is configured.
> > With this we no longer write to the MVNETA_SERDES_CFG on SoCs where it
> > doesn't exist.
> >
> > Suggested-by: Russell King
> > Signed-off-by: Sascha Hauer
> > ---
> > drivers/net/ethernet/marvell/mvneta.c | 80 +++
> > 1 file changed, 44 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index 51889770958d8..9933eb4577d43 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -106,6 +106,7 @@
> > #define MVNETA_TX_IN_PRGRS BIT(1)
> > #define MVNETA_TX_FIFO_EMPTYBIT(8)
> > #define MVNETA_RX_MIN_FRAME_SIZE 0x247c
> > +/* Only exists on Armada XP and Armada 370 */
> > #define MVNETA_SERDES_CFG 0x24A0
> > #define MVNETA_SGMII_SERDES_PROTO 0x0cc7
> > #define MVNETA_QSGMII_SERDES_PROTO 0x0667
> > @@ -3514,26 +3515,55 @@ static int mvneta_setup_txqs(struct mvneta_port *pp)
> > return 0;
> > }
> >
> > -static int mvneta_comphy_init(struct mvneta_port *pp)
> > +static int mvneta_comphy_init(struct mvneta_port *pp, phy_interface_t
> > interface)
> > {
> > int ret;
> >
> > - if (!pp->comphy)
> > - return 0;
> > -
> > - ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
> > - pp->phy_interface);
> > + ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET, interface);
> > if (ret)
> > return ret;
> >
> > return phy_power_on(pp->comphy);
> > }
> >
> > +static int mvneta_config_interface(struct mvneta_port *pp,
> > + phy_interface_t interface)
> > +{
> > + int ret = 0;
> > +
> > + if (pp->comphy) {
> > + if (interface == PHY_INTERFACE_MODE_SGMII ||
> > + interface == PHY_INTERFACE_MODE_1000BASEX ||
> > + interface == PHY_INTERFACE_MODE_2500BASEX) {
> > + ret = mvneta_comphy_init(pp, interface);
> > + }
> > + } else {
> > + switch (interface) {
> > + case PHY_INTERFACE_MODE_QSGMII:
> > + mvreg_write(pp, MVNETA_SERDES_CFG,
> > + MVNETA_QSGMII_SERDES_PROTO);
> > + break;
> > +
> > + case PHY_INTERFACE_MODE_SGMII:
> > + case PHY_INTERFACE_MODE_1000BASEX:
> > + mvreg_write(pp, MVNETA_SERDES_CFG,
> > + MVNETA_SGMII_SERDES_PROTO);
> > + break;
> > + default:
> > + return -EINVAL;
>
> I've just noticed that you made changes to the patch I sent, such as
> adding this default case that errors out, and by doing so, you have
> caused a regression by causing a WARN_ON() splat.
>
> It was not accidental that my patch had "break;" here instead of an
> error return, and I left the interface mode checking in
> mvneta_port_power_up() that you also removed.
>
> mvneta supports RGMII, and since RGMII doesn't use the serdes, there
> is no need to write to MVNETA_SGMII_SERDES_PROTO, and so we want to
> ignore those, not return -EINVAL.
>
> Since the interface type was already validated both by phylink when
> the interface is brought up, and also by the driver at probe time
> through mvneta_port_power_up(), which performs early validation of
> the mode given in DT this was not a problem... there is no need to
> consider anything but the RGMII case in the "default" case here.
>
> So, please fix this... at minimum fixing this switch() statement not
> to error out in the RGMII cases. However, I think actually following
> what was in my patch (which was there for good reason) rather than
> randomly changing it would have been better.
>
> This will have made the kernel on the SolidRun Clearfog platform
> trigger the WARN_ON()s for the dedicated gigabit port, which uses
> RGMII, and doesn't have a comphy specified in DT... and having
> waited for the compile to finish and the resulting kernel to boot...
I'll send a fixup shortly
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 2/2] net: ethernet: mvneta: Add 2500BaseX support for SoCs without comphy
The older SoCs like Armada XP support a 2500BaseX mode in the datasheets referred to as DR-SGMII (Double rated SGMII) or HS-SGMII (High Speed SGMII). This is an upclocked 1000BaseX mode, thus PHY_INTERFACE_MODE_2500BASEX is the appropriate mode define for it. adding support for it merely means writing the correct magic value into the MVNETA_SERDES_CFG register. Signed-off-by: Sascha Hauer --- drivers/net/ethernet/marvell/mvneta.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 9933eb4577d43..23d41550edb0d 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -110,6 +110,7 @@ #define MVNETA_SERDES_CFG 0x24A0 #define MVNETA_SGMII_SERDES_PROTO 0x0cc7 #define MVNETA_QSGMII_SERDES_PROTO 0x0667 +#define MVNETA_HSGMII_SERDES_PROTO 0x1107 #define MVNETA_TYPE_PRIO 0x24bc #define MVNETA_FORCE_UNIBIT(21) #define MVNETA_TXQ_CMD_1 0x24e4 @@ -3549,6 +3550,11 @@ static int mvneta_config_interface(struct mvneta_port *pp, mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO); break; + + case PHY_INTERFACE_MODE_2500BASEX: + mvreg_write(pp, MVNETA_SERDES_CFG, + MVNETA_HSGMII_SERDES_PROTO); + break; default: return -EINVAL; } -- 2.27.0
[PATCH 1/2] net: ethernet: mvneta: Fix Serdes configuration for SoCs without comphy
The MVNETA_SERDES_CFG register is only available on older SoCs like the
Armada XP. On newer SoCs like the Armada 38x the fields are moved to
comphy. This patch moves the writes to this register next to the comphy
initialization, so that depending on the SoC either comphy or
MVNETA_SERDES_CFG is configured.
With this we no longer write to the MVNETA_SERDES_CFG on SoCs where it
doesn't exist.
Suggested-by: Russell King
Signed-off-by: Sascha Hauer
---
drivers/net/ethernet/marvell/mvneta.c | 80 +++
1 file changed, 44 insertions(+), 36 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c
b/drivers/net/ethernet/marvell/mvneta.c
index 51889770958d8..9933eb4577d43 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -106,6 +106,7 @@
#define MVNETA_TX_IN_PRGRS BIT(1)
#define MVNETA_TX_FIFO_EMPTYBIT(8)
#define MVNETA_RX_MIN_FRAME_SIZE 0x247c
+/* Only exists on Armada XP and Armada 370 */
#define MVNETA_SERDES_CFG 0x24A0
#define MVNETA_SGMII_SERDES_PROTO 0x0cc7
#define MVNETA_QSGMII_SERDES_PROTO 0x0667
@@ -3514,26 +3515,55 @@ static int mvneta_setup_txqs(struct mvneta_port *pp)
return 0;
}
-static int mvneta_comphy_init(struct mvneta_port *pp)
+static int mvneta_comphy_init(struct mvneta_port *pp, phy_interface_t
interface)
{
int ret;
- if (!pp->comphy)
- return 0;
-
- ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
- pp->phy_interface);
+ ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET, interface);
if (ret)
return ret;
return phy_power_on(pp->comphy);
}
+static int mvneta_config_interface(struct mvneta_port *pp,
+ phy_interface_t interface)
+{
+ int ret = 0;
+
+ if (pp->comphy) {
+ if (interface == PHY_INTERFACE_MODE_SGMII ||
+ interface == PHY_INTERFACE_MODE_1000BASEX ||
+ interface == PHY_INTERFACE_MODE_2500BASEX) {
+ ret = mvneta_comphy_init(pp, interface);
+ }
+ } else {
+ switch (interface) {
+ case PHY_INTERFACE_MODE_QSGMII:
+ mvreg_write(pp, MVNETA_SERDES_CFG,
+ MVNETA_QSGMII_SERDES_PROTO);
+ break;
+
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_1000BASEX:
+ mvreg_write(pp, MVNETA_SERDES_CFG,
+ MVNETA_SGMII_SERDES_PROTO);
+ break;
+ default:
+ return -EINVAL;
+ }
+ }
+
+ pp->phy_interface = interface;
+
+ return ret;
+}
+
static void mvneta_start_dev(struct mvneta_port *pp)
{
int cpu;
- WARN_ON(mvneta_comphy_init(pp));
+ WARN_ON(mvneta_config_interface(pp, pp->phy_interface));
mvneta_max_rx_size_set(pp, pp->pkt_size);
mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
@@ -3907,14 +3937,10 @@ static void mvneta_mac_config(struct phylink_config
*config, unsigned int mode,
if (state->speed == SPEED_2500)
new_ctrl4 |= MVNETA_GMAC4_SHORT_PREAMBLE_ENABLE;
- if (pp->comphy && pp->phy_interface != state->interface &&
- (state->interface == PHY_INTERFACE_MODE_SGMII ||
-state->interface == PHY_INTERFACE_MODE_1000BASEX ||
-state->interface == PHY_INTERFACE_MODE_2500BASEX)) {
- pp->phy_interface = state->interface;
-
- WARN_ON(phy_power_off(pp->comphy));
- WARN_ON(mvneta_comphy_init(pp));
+ if (pp->phy_interface != state->interface) {
+ if (pp->comphy)
+ WARN_ON(phy_power_off(pp->comphy));
+ WARN_ON(mvneta_config_interface(pp, state->interface));
}
if (new_ctrl0 != gmac_ctrl0)
@@ -4958,20 +4984,10 @@ static void mvneta_conf_mbus_windows(struct mvneta_port
*pp,
}
/* Power up the port */
-static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
+static void mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
{
/* MAC Cause register should be cleared */
mvreg_write(pp, MVNETA_UNIT_INTR_CAUSE, 0);
-
- if (phy_mode == PHY_INTERFACE_MODE_QSGMII)
- mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_QSGMII_SERDES_PROTO);
- else if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
-phy_interface_mode_is_8023z(phy_mode))
- mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO);
- else if (!phy_interface_mode_is_rgmii(phy_mode))
- return -EINVAL;
-
- re
Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes
On Fri, Jun 12, 2020 at 11:18:20AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 11:01:15AM +0100, Russell King - ARM Linux admin
> wrote:
> > On Fri, Jun 12, 2020 at 09:47:10AM +0100, Russell King - ARM Linux admin
> > wrote:
> > > On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> > > > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> > > > called DRSGMII. Depending on the Port MAC Control Register0 PortType
> > > > setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> > > >
> > > > This patch adds the necessary Serdes Configuration setting for the
> > > > 2.5Gbps modes. There is no phy interface mode define for overclocked
> > > > SGMII, so only 2500BaseX is handled for now.
> > > >
> > > > As phy_interface_mode_is_8023z() returns true for both
> > > > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> > > > explicitly test for 1000BaseX instead of using
> > > > phy_interface_mode_is_8023z() to differentiate the different
> > > > possibilities.
> > > >
> > > > Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> > > > Signed-off-by: Sascha Hauer
> > >
> > > 2500base-X is used today on Armada 388 and Armada 3720 platforms and
> > > works - it is known to interoperate with Marvell PP2.2 hardware, as
> > > well was various SFPs such as the Huawei MA5671A at 2.5Gbps. The way
> > > it is handled on these platforms is via the COMPHY, requesting that
> > > the serdes is upclocked from 1.25Gbps to 3.125Gbps.
> > >
> > > This "DRSGMII" mode is not mentioned in the functional specs for either
> > > the Armada 388 or Armada 3720, the value you poke into the register is
> > > not mentioned either. As I've already requested, some information on
> > > exactly what this "DRSGMII" is would be very useful, it can't be
> > > "double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.
> > >
> > > So, I suspect this breaks the platforms that are known to work.
> > >
> > > We need a proper description of what DRSGMII is before we can consider
> > > taking any patches for it.
> >
> > Okay, having dug through the Armada XP, 370, 388, 3720 specs, I think
> > this is fine after all - but something that will help for the future
> > would be to document that this register does not exist on the 388 and
> > 3720 devices (which brings up the question whether we should be writing
> > it there.) The field was moved into the comphy on those devices.
> >
> > So, it looks like if we have a comphy, we should not be writing this
> > register.
> >
> > What's more, the write to MVNETA_SERDES_CFG should not be in
> > mvneta_port_power_up(); it's likely that XP and 370 will not work
> > properly with phylink. It needs to be done in a similar location to
> > mvneta_comphy_init(), so that phylink can switch between 1G and 2.5G
> > speeds.
> >
> > As you have an Armada XP system, you are best placed to test moving
> > that write.
>
> Here's my suggestion - it won't apply to mainline or net* trees, but
> gives you the idea I'm proposing:
>
And here is the same patch which applies on master and the net tree.
It works as expected on my Armada XP in 2.5Gbps mode. Provided you are
happy with the patch I can send it as a formal patch on monday if by
then you haven't done that already.
Thanks for your help on that
Sascha
-8<--
diff --git a/drivers/net/ethernet/marvell/mvneta.c
b/drivers/net/ethernet/marvell/mvneta.c
index 946925bbcb2de..a1c48efd91b17 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -106,9 +106,11 @@
#define MVNETA_TX_IN_PRGRS BIT(1)
#define MVNETA_TX_FIFO_EMPTYBIT(8)
#define MVNETA_RX_MIN_FRAME_SIZE 0x247c
+/* Only exists on Armada XP and Armada 370 */
#define MVNETA_SERDES_CFG 0x24A0
#define MVNETA_SGMII_SERDES_PROTO 0x0cc7
#define MVNETA_QSGMII_SERDES_PROTO 0x0667
+#define MVNETA_HSGMII_SERDES_PROTO 0x1107
#define MVNETA_TYPE_PRIO 0x24bc
#define MVNETA_FORCE_UNIBIT(21)
#define MVNETA_TXQ_CMD_1 0x24e4
@@ -3533,9 +3535,6 @@ static int mvneta_comphy_init(struct mvneta_port *pp)
{
int ret
Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes
On Fri, Jun 12, 2020 at 12:30:31PM +0100, Russell King - ARM Linux admin wrote: > On Fri, Jun 12, 2020 at 12:22:13PM +0100, Russell King - ARM Linux admin > wrote: > > On Fri, Jun 12, 2020 at 11:42:08AM +0100, Russell King - ARM Linux admin > > wrote: > > > With the obvious mistakes fixed (extraneous 'i' and lack of default > > > case), it seems to still work on Armada 388 Clearfog Pro with 2.5G > > > modules. > > > > ... and the other bug fixed - mvneta_comphy_init() needs to be passed > > the interface mode. > > Unrelated to the patch, has anyone noticed that mvneta's performance > seems to have reduced? I've only just noticed it (which makes 2.5Gbps > rather pointless). This is iperf between two clearfogs with a 2.5G > fibre link: > > root@clearfog21:~# iperf -V -c fe80::250:43ff:fe02:303%eno2 > > Client connecting to fe80::250:43ff:fe02:303%eno2, TCP port 5001 > TCP window size: 43.8 KByte (default) > > [ 3] local fe80::250:43ff:fe21:203 port 48928 connected with > fe80::250:43ff:fe02:303 port 5001 > [ ID] Interval Transfer Bandwidth > [ 3] 0.0-10.0 sec 553 MBytes 464 Mbits/sec > > I checked with Jon Nettleton, and he confirms my recollection that > mvneta on Armada 388 used to be able to fill a 2.5Gbps link. > > If Armada 388 can't manage, then I suspect Armada XP will have worse > performance being an earlier revision SoC. I only have one board with a Armada XP here which has a loopback cable between two ports. It gives me: [ 3] local 172.16.1.4 port 47002 connected with 172.16.1.0 port 5001 [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.0 sec 1.27 GBytes 1.09 Gbits/sec Still not 2.5Gbps, but at least twice the data rate you get, plus my board has to handle both ends of the link. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes
On Fri, Jun 12, 2020 at 09:47:10AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 12, 2020 at 10:38:47AM +0200, Sascha Hauer wrote:
> > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
> > called DRSGMII. Depending on the Port MAC Control Register0 PortType
> > setting this seems to be either an overclocked SGMII mode or 2500BaseX.
> >
> > This patch adds the necessary Serdes Configuration setting for the
> > 2.5Gbps modes. There is no phy interface mode define for overclocked
> > SGMII, so only 2500BaseX is handled for now.
> >
> > As phy_interface_mode_is_8023z() returns true for both
> > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
> > explicitly test for 1000BaseX instead of using
> > phy_interface_mode_is_8023z() to differentiate the different
> > possibilities.
> >
> > Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
> > Signed-off-by: Sascha Hauer
>
> 2500base-X is used today on Armada 388 and Armada 3720 platforms and
> works - it is known to interoperate with Marvell PP2.2 hardware, as
> well was various SFPs such as the Huawei MA5671A at 2.5Gbps. The way
> it is handled on these platforms is via the COMPHY, requesting that
> the serdes is upclocked from 1.25Gbps to 3.125Gbps.
Unfortunately the functional specs I have available for the Armada 38x
completely lack the ethernet registers, So I can't tell what has to be
done there. What about the other values that are poked into
MVNETA_SERDES_CFG? Are these documented in the Armada 388 functional
spec or are they just ignored by this hardware? I'm talking about
mvneta_port_power_up():
if (phy_mode == PHY_INTERFACE_MODE_QSGMII)
mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_QSGMII_SERDES_PROTO);
else if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
phy_mode == PHY_INTERFACE_MODE_1000BASEX)
mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO);
else if (!phy_interface_mode_is_rgmii(phy_mode))
return -EINVAL;
In the Armada 38x functional specs we have this to configure the SGMII
mode:
PIN_PHY_GEN Setting:
SGMII SGMII (1.25 Gbps) 0x6
HS-SGMII (3.125 Gbps) 0x8
The Armada XP doesn't have Comphy, so I guess what is being done in
mvneta_port_power_up() is just the old way for configuring the Serdes
lanes for different bitrates. Also they seem to have renamed DRSGMII
to HS-SGMII.
>
> This "DRSGMII" mode is not mentioned in the functional specs for either
> the Armada 388 or Armada 3720, the value you poke into the register is
> not mentioned either. As I've already requested, some information on
> exactly what this "DRSGMII" is would be very useful, it can't be
> "double-rate SGMII" because that would give you 2Gbps instead of 1Gbps.
As said, despite the fact that two times 1Gbps is not 2.5Gbps DRSGMII
still stands for "Double Rated-SGMII", as found in the MV78260 Hardware
specifications. Another place in the same document talks about "DRSGMII
(SGMII at 2.5Gbps)". Otherwise documentation is sparse, to my
information it is really only a higher bitrate.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v2] net: mvneta: Fix Serdes configuration for 2.5Gbps modes
The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode
called DRSGMII. Depending on the Port MAC Control Register0 PortType
setting this seems to be either an overclocked SGMII mode or 2500BaseX.
This patch adds the necessary Serdes Configuration setting for the
2.5Gbps modes. There is no phy interface mode define for overclocked
SGMII, so only 2500BaseX is handled for now.
As phy_interface_mode_is_8023z() returns true for both
PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we
explicitly test for 1000BaseX instead of using
phy_interface_mode_is_8023z() to differentiate the different
possibilities.
Fixes: da58a931f248f ("net: mvneta: Add support for 2500Mbps SGMII")
Signed-off-by: Sascha Hauer
---
Changes since v1:
- Add Fixes: tag
drivers/net/ethernet/marvell/mvneta.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c
b/drivers/net/ethernet/marvell/mvneta.c
index 51889770958d8..3b13048931412 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -109,6 +109,7 @@
#define MVNETA_SERDES_CFG 0x24A0
#define MVNETA_SGMII_SERDES_PROTO 0x0cc7
#define MVNETA_QSGMII_SERDES_PROTO 0x0667
+#define MVNETA_DRSGMII_SERDES_PROTO0x1107
#define MVNETA_TYPE_PRIO 0x24bc
#define MVNETA_FORCE_UNIBIT(21)
#define MVNETA_TXQ_CMD_1 0x24e4
@@ -4966,8 +4967,10 @@ static int mvneta_port_power_up(struct mvneta_port *pp,
int phy_mode)
if (phy_mode == PHY_INTERFACE_MODE_QSGMII)
mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_QSGMII_SERDES_PROTO);
else if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
-phy_interface_mode_is_8023z(phy_mode))
+phy_mode == PHY_INTERFACE_MODE_1000BASEX)
mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO);
+ else if (phy_mode == PHY_INTERFACE_MODE_2500BASEX)
+ mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_DRSGMII_SERDES_PROTO);
else if (!phy_interface_mode_is_rgmii(phy_mode))
return -EINVAL;
--
2.27.0
Re: [PATCH] net: mvneta: Fix Serdes configuration for 2.5Gbps modes
Hi Andrew, +Cc Maxime Chevallier On Tue, Jun 09, 2020 at 03:28:48PM +0200, Andrew Lunn wrote: > On Tue, Jun 09, 2020 at 03:11:52PM +0200, Sascha Hauer wrote: > > The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode > > called DRSGMII. Depending on the Port MAC Control Register0 PortType > > setting this seems to be either an overclocked SGMII mode or 2500BaseX. > > > > This patch adds the necessary Serdes Configuration setting for the > > 2.5Gbps modes. There is no phy interface mode define for overclocked > > SGMII, so only 2500BaseX is handled for now. > > > > As phy_interface_mode_is_8023z() returns true for both > > PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we > > explicitly test for 1000BaseX instead of using > > phy_interface_mode_is_8023z() to differentiate the different > > possibilities. > > Hi Sascha > > This seems like it should have a Fixes: tag, and be submitted to the > net tree. Please see the Networking FAQ. This might be a candidate for a Fixes: tag: | commit da58a931f248f423f917c3a0b3c94303aa30a738 | Author: Maxime Chevallier | Date: Tue Sep 25 15:59:39 2018 +0200 | | net: mvneta: Add support for 2500Mbps SGMII What do you mean by "submitted to the net tree"? I usually send network driver related patches to net...@vger.kernel.org and from there David applies them. Is there anything more to it I haven't respected? Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] net: ethernet: mvneta: add support for 2.5G DRSGMII mode
Hi Andrew, On Tue, Jun 09, 2020 at 03:12:16PM +0200, Andrew Lunn wrote: > On Tue, Jun 09, 2020 at 02:55:35PM +0200, Sascha Hauer wrote: > > On Mon, Jun 08, 2020 at 04:57:37PM +0200, Andrew Lunn wrote: > > > On Mon, Jun 08, 2020 at 09:47:16AM +0200, Sascha Hauer wrote: > > > > The Marvell MVNETA Ethernet controller supports a 2.5 Gbps SGMII mode > > > > called DRSGMII. > > > > > > > > This patch adds a corresponding phy-mode string 'drsgmii' and parses it > > > > from DT. The MVNETA then configures the SERDES protocol value > > > > accordingly. > > > > > > > > It was successfully tested on a MV78460 connected to a FPGA. > > > > > > Hi Sascha > > > > > > Is this really overclocked SGMII, or 2500BaseX? How does it differ > > > from 2500BaseX, which mvneta already supports? > > > > I think it is overclocked SGMII or 2500BaseX depending on the Port MAC > > Control Register0 PortType setting bit. > > As said to Russell we have a fixed link so nobody really cares if it's > > SGMII or 2500BaseX. This boils down the patch to fixing the Serdes > > configuration setting for 2500BaseX. > > Hi Sascha > > Does 2500BaseX work for your use case? Since this drsmgii mode is not > well defined, i would prefer to not add it, unless it is really > needed. Yes, it does, see updated patch I just sent. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] net: mvneta: Fix Serdes configuration for 2.5Gbps modes
The Marvell MVNETA Ethernet controller supports a 2.5Gbps SGMII mode called DRSGMII. Depending on the Port MAC Control Register0 PortType setting this seems to be either an overclocked SGMII mode or 2500BaseX. This patch adds the necessary Serdes Configuration setting for the 2.5Gbps modes. There is no phy interface mode define for overclocked SGMII, so only 2500BaseX is handled for now. As phy_interface_mode_is_8023z() returns true for both PHY_INTERFACE_MODE_1000BASEX and PHY_INTERFACE_MODE_2500BASEX we explicitly test for 1000BaseX instead of using phy_interface_mode_is_8023z() to differentiate the different possibilities. Signed-off-by: Sascha Hauer --- drivers/net/ethernet/marvell/mvneta.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 51889770958d8..3b13048931412 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -109,6 +109,7 @@ #define MVNETA_SERDES_CFG 0x24A0 #define MVNETA_SGMII_SERDES_PROTO 0x0cc7 #define MVNETA_QSGMII_SERDES_PROTO 0x0667 +#define MVNETA_DRSGMII_SERDES_PROTO0x1107 #define MVNETA_TYPE_PRIO 0x24bc #define MVNETA_FORCE_UNIBIT(21) #define MVNETA_TXQ_CMD_1 0x24e4 @@ -4966,8 +4967,10 @@ static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode) if (phy_mode == PHY_INTERFACE_MODE_QSGMII) mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_QSGMII_SERDES_PROTO); else if (phy_mode == PHY_INTERFACE_MODE_SGMII || -phy_interface_mode_is_8023z(phy_mode)) +phy_mode == PHY_INTERFACE_MODE_1000BASEX) mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO); + else if (phy_mode == PHY_INTERFACE_MODE_2500BASEX) + mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_DRSGMII_SERDES_PROTO); else if (!phy_interface_mode_is_rgmii(phy_mode)) return -EINVAL; -- 2.27.0
Re: [PATCH] net: ethernet: mvneta: add support for 2.5G DRSGMII mode
On Mon, Jun 08, 2020 at 04:57:37PM +0200, Andrew Lunn wrote: > On Mon, Jun 08, 2020 at 09:47:16AM +0200, Sascha Hauer wrote: > > The Marvell MVNETA Ethernet controller supports a 2.5 Gbps SGMII mode > > called DRSGMII. > > > > This patch adds a corresponding phy-mode string 'drsgmii' and parses it > > from DT. The MVNETA then configures the SERDES protocol value > > accordingly. > > > > It was successfully tested on a MV78460 connected to a FPGA. > > Hi Sascha > > Is this really overclocked SGMII, or 2500BaseX? How does it differ > from 2500BaseX, which mvneta already supports? I think it is overclocked SGMII or 2500BaseX depending on the Port MAC Control Register0 PortType setting bit. As said to Russell we have a fixed link so nobody really cares if it's SGMII or 2500BaseX. This boils down the patch to fixing the Serdes configuration setting for 2500BaseX. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] net: ethernet: mvneta: add support for 2.5G DRSGMII mode
On Mon, Jun 08, 2020 at 05:08:01PM +0100, Russell King - ARM Linux admin wrote: > On Mon, Jun 08, 2020 at 09:47:16AM +0200, Sascha Hauer wrote: > > The Marvell MVNETA Ethernet controller supports a 2.5 Gbps SGMII mode > > called DRSGMII. > > > > This patch adds a corresponding phy-mode string 'drsgmii' and parses it > > from DT. The MVNETA then configures the SERDES protocol value > > accordingly. > > > > It was successfully tested on a MV78460 connected to a FPGA. > > Digging around, this is Armada XP? Which SoCs is this mode supported? > There's no mention of DRSGMII in the A38x nor A37xx documentation which > are later than Armada XP. It's an Armada XP MV78460 in my case. I have no idea what other SoCs this mode is supported on. > > What exactly is "drsgmii"? It can't be "double-rate" SGMII because that > would give you 2Gbps max instead of the 1Gbps, but this gives 2.5Gbps, > so I'm really not sure using "drsgmii" is a good idea. It may be what > Marvell call it, but we really need to know if there's some vendor > neutral way to refer to it. The abbreviation really is for "Double Rated SGMII". It seems it has 2.5 times the clock rate than ordinary SGMII. Another term I found is HSGMII (High serial gigabit media-independent interface) which also has 2.5Gbps. Anyway, I just learned from the paragraph you added to Documentation/networking/phy.rst that 1000BASEX differs from SGMII in the format of the control word. As we have a fixed link to a FPGA the control word seems to be unused, at least the Port MAC Control Register0 PortType setting bit doesn't change anything. So I can equally well use the existing 2500BASEX mode. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] net: ethernet: mvneta: add support for 2.5G DRSGMII mode
The Marvell MVNETA Ethernet controller supports a 2.5 Gbps SGMII mode
called DRSGMII.
This patch adds a corresponding phy-mode string 'drsgmii' and parses it
from DT. The MVNETA then configures the SERDES protocol value
accordingly.
It was successfully tested on a MV78460 connected to a FPGA.
Signed-off-by: Sascha Hauer
---
.../devicetree/bindings/net/ethernet-controller.yaml | 1 +
drivers/net/ethernet/marvell/mvneta.c | 7 ++-
include/linux/phy.h| 3 +++
3 files changed, 10 insertions(+), 1 deletion(-)
This patch has already been sent 3 years ago here:
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20170123142206.5390-1-...@pengutronix.de/
Since then the driver has evolved a lot. 2.5Gbps is properly configured in the
MAC now.
diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index ac471b60ed6ae..4eead3c89bd3e 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -66,6 +66,7 @@ properties:
- gmii
- sgmii
- qsgmii
+ - drsgmii
- tbi
- rev-mii
- rmii
diff --git a/drivers/net/ethernet/marvell/mvneta.c
b/drivers/net/ethernet/marvell/mvneta.c
index 51889770958d8..807c698576c74 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -109,6 +109,7 @@
#define MVNETA_SERDES_CFG 0x24A0
#define MVNETA_SGMII_SERDES_PROTO 0x0cc7
#define MVNETA_QSGMII_SERDES_PROTO 0x0667
+#define MVNETA_DRSGMII_SERDES_PROTO0x1107
#define MVNETA_TYPE_PRIO 0x24bc
#define MVNETA_FORCE_UNIBIT(21)
#define MVNETA_TXQ_CMD_1 0x24e4
@@ -3734,10 +3735,11 @@ static void mvneta_validate(struct phylink_config
*config,
struct mvneta_port *pp = netdev_priv(ndev);
__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
- /* We only support QSGMII, SGMII, 802.3z and RGMII modes */
+ /* We only support QSGMII, SGMII, DRSGMII, 802.3z and RGMII modes */
if (state->interface != PHY_INTERFACE_MODE_NA &&
state->interface != PHY_INTERFACE_MODE_QSGMII &&
state->interface != PHY_INTERFACE_MODE_SGMII &&
+ state->interface != PHY_INTERFACE_MODE_DRSGMII &&
!phy_interface_mode_is_8023z(state->interface) &&
!phy_interface_mode_is_rgmii(state->interface)) {
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
@@ -3851,6 +3853,7 @@ static void mvneta_mac_config(struct phylink_config
*config, unsigned int mode,
if (state->interface == PHY_INTERFACE_MODE_QSGMII ||
state->interface == PHY_INTERFACE_MODE_SGMII ||
+ state->interface == PHY_INTERFACE_MODE_DRSGMII ||
phy_interface_mode_is_8023z(state->interface))
new_ctrl2 |= MVNETA_GMAC2_PCS_ENABLE;
@@ -4968,6 +4971,8 @@ static int mvneta_port_power_up(struct mvneta_port *pp,
int phy_mode)
else if (phy_mode == PHY_INTERFACE_MODE_SGMII ||
phy_interface_mode_is_8023z(phy_mode))
mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_SGMII_SERDES_PROTO);
+ else if (phy_mode == PHY_INTERFACE_MODE_DRSGMII)
+ mvreg_write(pp, MVNETA_SERDES_CFG, MVNETA_DRSGMII_SERDES_PROTO);
else if (!phy_interface_mode_is_rgmii(phy_mode))
return -EINVAL;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2432ca463ddc0..bf3276b330f9e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -109,6 +109,7 @@ typedef enum {
PHY_INTERFACE_MODE_USXGMII,
/* 10GBASE-KR - with Clause 73 AN */
PHY_INTERFACE_MODE_10GKR,
+ PHY_INTERFACE_MODE_DRSGMII,
PHY_INTERFACE_MODE_MAX,
} phy_interface_t;
@@ -190,6 +191,8 @@ static inline const char *phy_modes(phy_interface_t
interface)
return "usxgmii";
case PHY_INTERFACE_MODE_10GKR:
return "10gbase-kr";
+ case PHY_INTERFACE_MODE_DRSGMII:
+ return "drsgmii";
default:
return "unknown";
}
--
2.27.0
Re: [PATCH] ubi: fastmap: Don't produce the initial anchor PEB when fastmap is disabled
Hi,
On Mon, Jun 01, 2020 at 05:11:34PM +0800, Zhihao Cheng wrote:
> Following process triggers a memleak caused by forgetting to release the
> initial anchor PEB (CONFIG_MTD_UBI_FASTMAP is disabled):
> 1. attach -> __erase_worker -> produce the initial anchor PEB
> 2. detach -> ubi_fastmap_close (Do nothing, it should have released the
>initial anchor PEB)
>
> Don't produce the initial anchor PEB in __erase_worker() when fastmap
> is disabled.
>
> Signed-off-by: Zhihao Cheng
> Fixes: f9c34bb529975fe ("ubi: Fix producing anchor PEBs")
> Reported-by: syzbot+d9aab50b1154e3d16...@syzkaller.appspotmail.com
> ---
> drivers/mtd/ubi/wl.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 5146cce5fe32..5ebe1084a8e7 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -1079,13 +1079,19 @@ static int __erase_worker(struct ubi_device *ubi,
> struct ubi_work *wl_wrk)
> if (!err) {
> spin_lock(&ubi->wl_lock);
>
> - if (!ubi->fm_anchor && e->pnum < UBI_FM_MAX_START) {
> +#ifdef CONFIG_MTD_UBI_FASTMAP
> + if (!ubi->fm_disabled && !ubi->fm_anchor &&
> + e->pnum < UBI_FM_MAX_START) {
Rather than introducing another #ifdef you could do a
if (IS_ENABLED(CONFIG_MTD_UBI_FASTMAP) &&
!ubi->fm_disabled && !ubi->fm_anchor &&
e->pnum < UBI_FM_MAX_START)
And I am not sure if the IS_ENABLED(CONFIG_MTD_UBI_FASTMAP) is necessary
at all because we do a ubi->fm_disabled = 1 when fastmap is disabled.
Regards,
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v8 00/13] add ecspi ERR009165 for i.mx6/7 soc family
On Thu, May 21, 2020 at 04:34:12AM +0800, Robin Gong wrote: > There is ecspi ERR009165 on i.mx6/7 soc family, which cause FIFO > transfer to be send twice in DMA mode. Please get more information from: > https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf. The workaround is adding > new sdma ram script which works in XCH mode as PIO inside sdma instead > of SMC mode, meanwhile, 'TX_THRESHOLD' should be 0. The issue should be > exist on all legacy i.mx6/7 soc family before i.mx6ul. > NXP fix this design issue from i.mx6ul, so newer chips including i.mx6ul/ > 6ull/6sll do not need this workaroud anymore. All other i.mx6/7/8 chips > still need this workaroud. This patch set add new 'fsl,imx6ul-ecspi' > for ecspi driver and 'ecspi_fixed' in sdma driver to choose if need errata > or not. > The first two reverted patches should be the same issue, though, it > seems 'fixed' by changing to other shp script. Hope Sean or Sascha could > have the chance to test this patch set if could fix their issues. > Besides, enable sdma support for i.mx8mm/8mq and fix ecspi1 not work > on i.mx8mm because the event id is zero. For the series: Acked-by: Sascha Hauer Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 RESEND 07/13] spi: imx: fix ERR009165
On Wed, May 13, 2020 at 08:52:39AM +, Robin Gong wrote: > On 2020/05/13 16:48 Sascha Hauer wrote: > > On Wed, May 13, 2020 at 08:38:26AM +, Robin Gong wrote: > > > On 2020/05/13 Sascha Hauer wrote: > > > > This patch is the one bisecting will end up with when somebody uses > > > > an older SDMA firmware or the ROM scripts. It should have a better > > > > description what happens and what should be done about it. > > > Emm..That's true. Timeout will be caught in such case, hence, maybe we can > > fall back it to pio always. > > > > With my patch applied sdma_load_context() will fail. I don't know how > > exactly > > this hits into the SPI driver, but it won't be a timeout. > Thanks for your quick test, assume you use ROM firmware, right? Yes. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 RESEND 07/13] spi: imx: fix ERR009165
On Wed, May 13, 2020 at 09:05:33AM +, Robin Gong wrote: > On 2020/05/13 Sascha Hauer wrote:d > > > drivers/spi/spi-imx.c | 16 > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index > > > f4f28a4..70df8e6 100644 > > > --- a/drivers/spi/spi-imx.c > > > +++ b/drivers/spi/spi-imx.c > > > @@ -585,8 +585,8 @@ static int mx51_ecspi_prepare_transfer(struct > > spi_imx_data *spi_imx, > > > ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk); > > > spi_imx->spi_bus_clk = clk; > > > > > > - if (spi_imx->usedma) > > > - ctrl |= MX51_ECSPI_CTRL_SMC; > > > + /* ERR009165: work in XHC mode as PIO */ > > > + ctrl &= ~MX51_ECSPI_CTRL_SMC; > > > > > > writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL); > > > > > > @@ -617,7 +617,7 @@ static void mx51_setup_wml(struct spi_imx_data > > *spi_imx) > > >* and enable DMA request. > > >*/ > > > writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) | > > > - MX51_ECSPI_DMA_TX_WML(spi_imx->wml) | > > > + MX51_ECSPI_DMA_TX_WML(0) | > > > MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) | > > > MX51_ECSPI_DMA_TEDEN | MX51_ECSPI_DMA_RXDEN | > > > MX51_ECSPI_DMA_RXTDEN, spi_imx->base + MX51_ECSPI_DMA); > > @@ -1171,7 > > > +1171,11 @@ static int spi_imx_dma_configure(struct spi_master *master) > > > tx.direction = DMA_MEM_TO_DEV; > > > tx.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA; > > > tx.dst_addr_width = buswidth; > > > - tx.dst_maxburst = spi_imx->wml; > > > + /* > > > + * For ERR009165 with tx_wml = 0 could enlarge burst size to fifo size > > > + * to speed up fifo filling as possible. > > > + */ > > > + tx.dst_maxburst = spi_imx->devtype_data->fifo_size; > > > > In the next patch this is changed again to: > > > > + if (spi_imx->devtype_data->tx_glitch_fixed) > > + tx.dst_maxburst = spi_imx->wml; > > + else > > + tx.dst_maxburst = spi_imx->devtype_data->fifo_size; > > > > So with tx_glitch_fixed we end up with tx.dst_maxburst being the same as two > > patches before which is rather confusing. Better introduce tx_glitch_fixed > > in > > this patch, or maybe even merge this patch and the next one. > Sorry confused you, I should repleace 'tx_wml=0' in the above comments > with ' TX_THRESHOLD=0', which means tx transfer dma have to wait all > the tx data in tx fifo transferred with ERR009165 rather than > generically 'tx_wml' (for example --half fifo size used as > TX_THRESHOLD). Obviously TX_THRESHOLD=0 would down performance, so > enlarge dst_maxburst to fifo size as PIO with ERR009165. After > ERR009165 fixed at HW level. TX_THRESHOLD could be used as common > 'spi_imx->wml' so change it back. Will add more detail information in > v8. I am not confused, I meant the patches are confusing. What you are doing is: No patch: tx.dst_maxburst = a; 1st patch tx.dst_maxburst = b; 2nd patch: if (foo) tx.dst_maxburst = a; else tx.dst_maxburst = b; It would be better readable and understandable if you did that in one patch, because that would directly say "Under certain conditions we have to choose a, otherwise b". That's much better than changing "a" to "b" and then to "a or b" Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 RESEND 07/13] spi: imx: fix ERR009165
On Wed, May 13, 2020 at 08:38:26AM +, Robin Gong wrote:
> On 2020/05/13 Sascha Hauer wrote:
> > This patch is the one bisecting will end up with when somebody uses an older
> > SDMA firmware or the ROM scripts. It should have a better description what
> > happens and what should be done about it.
> Emm..That's true. Timeout will be caught in such case, hence, maybe we can
> fall back it to pio always.
With my patch applied sdma_load_context() will fail. I don't know how
exactly this hits into the SPI driver, but it won't be a timeout.
Sascha
> > >
> > > Signed-off-by: Robin Gong
> > > Acked-by: Mark Brown
> > > ---
> > > drivers/spi/spi-imx.c | 16
> > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> > > f4f28a4..70df8e6 100644
> > > --- a/drivers/spi/spi-imx.c
> > > +++ b/drivers/spi/spi-imx.c
> > > @@ -585,8 +585,8 @@ static int mx51_ecspi_prepare_transfer(struct
> > spi_imx_data *spi_imx,
> > > ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk);
> > > spi_imx->spi_bus_clk = clk;
> > >
> > > - if (spi_imx->usedma)
> > > - ctrl |= MX51_ECSPI_CTRL_SMC;
> > > + /* ERR009165: work in XHC mode as PIO */
> > > + ctrl &= ~MX51_ECSPI_CTRL_SMC;
> > >
> > > writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> > >
> > > @@ -617,7 +617,7 @@ static void mx51_setup_wml(struct spi_imx_data
> > *spi_imx)
> > >* and enable DMA request.
> > >*/
> > > writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) |
> > > - MX51_ECSPI_DMA_TX_WML(spi_imx->wml) |
> > > + MX51_ECSPI_DMA_TX_WML(0) |
> > > MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) |
> > > MX51_ECSPI_DMA_TEDEN | MX51_ECSPI_DMA_RXDEN |
> > > MX51_ECSPI_DMA_RXTDEN, spi_imx->base + MX51_ECSPI_DMA);
> > @@ -1171,7
> > > +1171,11 @@ static int spi_imx_dma_configure(struct spi_master *master)
> > > tx.direction = DMA_MEM_TO_DEV;
> > > tx.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA;
> > > tx.dst_addr_width = buswidth;
> > > - tx.dst_maxburst = spi_imx->wml;
> > > + /*
> > > + * For ERR009165 with tx_wml = 0 could enlarge burst size to fifo size
> > > + * to speed up fifo filling as possible.
> > > + */
> > > + tx.dst_maxburst = spi_imx->devtype_data->fifo_size;
> > > ret = dmaengine_slave_config(master->dma_tx, &tx);
> > > if (ret) {
> > > dev_err(spi_imx->dev, "TX dma configuration failed with %d\n",
> > > ret); @@ -1265,10 +1269,6 @@ static int spi_imx_sdma_init(struct
> > > device *dev, struct spi_imx_data *spi_imx, {
> > > int ret;
> > >
> > > - /* use pio mode for i.mx6dl chip TKT238285 */
> > > - if (of_machine_is_compatible("fsl,imx6dl"))
> > > - return 0;
> > > -
> > > spi_imx->wml = spi_imx->devtype_data->fifo_size / 2;
> > >
> > > /* Prepare for TX DMA: */
> > > --
> > > 2.7.4
> > >
> > >
> >
> > --
> > Pengutronix e.K. |
> > |
> > Steuerwalder Str. 21 |
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> > ngutronix.de%2F&data=02%7C01%7Cyibin.gong%40nxp.com%7C2f49309
> > 819cc4c45418108d7f70e46fb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> > 7C1%7C637249513003506970&sdata=RoLVnDaCfG20i88OmmlpbMH6lZu
> > qqW2CJv4VSSDkPcM%3D&reserved=0 |
> > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> > |
> > Amtsgericht Hildesheim, HRA 2686 | Fax:
> > +49-5121-206917- |
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 RESEND 07/13] spi: imx: fix ERR009165
On Tue, May 12, 2020 at 01:32:30AM +0800, Robin Gong wrote: > Change to XCH mode even in dma mode, please refer to the below > errata: > https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf > > Signed-off-by: Robin Gong > Acked-by: Mark Brown > --- > drivers/spi/spi-imx.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > index f4f28a4..70df8e6 100644 > --- a/drivers/spi/spi-imx.c > +++ b/drivers/spi/spi-imx.c > @@ -585,8 +585,8 @@ static int mx51_ecspi_prepare_transfer(struct > spi_imx_data *spi_imx, > ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk); > spi_imx->spi_bus_clk = clk; > > - if (spi_imx->usedma) > - ctrl |= MX51_ECSPI_CTRL_SMC; > + /* ERR009165: work in XHC mode as PIO */ > + ctrl &= ~MX51_ECSPI_CTRL_SMC; > > writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL); > > @@ -617,7 +617,7 @@ static void mx51_setup_wml(struct spi_imx_data *spi_imx) >* and enable DMA request. >*/ > writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) | > - MX51_ECSPI_DMA_TX_WML(spi_imx->wml) | > + MX51_ECSPI_DMA_TX_WML(0) | > MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) | > MX51_ECSPI_DMA_TEDEN | MX51_ECSPI_DMA_RXDEN | > MX51_ECSPI_DMA_RXTDEN, spi_imx->base + MX51_ECSPI_DMA); > @@ -1171,7 +1171,11 @@ static int spi_imx_dma_configure(struct spi_master > *master) > tx.direction = DMA_MEM_TO_DEV; > tx.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA; > tx.dst_addr_width = buswidth; > - tx.dst_maxburst = spi_imx->wml; > + /* > + * For ERR009165 with tx_wml = 0 could enlarge burst size to fifo size > + * to speed up fifo filling as possible. > + */ > + tx.dst_maxburst = spi_imx->devtype_data->fifo_size; In the next patch this is changed again to: + if (spi_imx->devtype_data->tx_glitch_fixed) + tx.dst_maxburst = spi_imx->wml; + else + tx.dst_maxburst = spi_imx->devtype_data->fifo_size; So with tx_glitch_fixed we end up with tx.dst_maxburst being the same as two patches before which is rather confusing. Better introduce tx_glitch_fixed in this patch, or maybe even merge this patch and the next one. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 RESEND 07/13] spi: imx: fix ERR009165
On Tue, May 12, 2020 at 01:32:30AM +0800, Robin Gong wrote:
> Change to XCH mode even in dma mode, please refer to the below
> errata:
> https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf
This patch is the one bisecting will end up with when somebody uses an
older SDMA firmware or the ROM scripts. It should have a better
description what happens and what should be done about it.
Sascha
>
> Signed-off-by: Robin Gong
> Acked-by: Mark Brown
> ---
> drivers/spi/spi-imx.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index f4f28a4..70df8e6 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -585,8 +585,8 @@ static int mx51_ecspi_prepare_transfer(struct
> spi_imx_data *spi_imx,
> ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk);
> spi_imx->spi_bus_clk = clk;
>
> - if (spi_imx->usedma)
> - ctrl |= MX51_ECSPI_CTRL_SMC;
> + /* ERR009165: work in XHC mode as PIO */
> + ctrl &= ~MX51_ECSPI_CTRL_SMC;
>
> writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
>
> @@ -617,7 +617,7 @@ static void mx51_setup_wml(struct spi_imx_data *spi_imx)
>* and enable DMA request.
>*/
> writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) |
> - MX51_ECSPI_DMA_TX_WML(spi_imx->wml) |
> + MX51_ECSPI_DMA_TX_WML(0) |
> MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) |
> MX51_ECSPI_DMA_TEDEN | MX51_ECSPI_DMA_RXDEN |
> MX51_ECSPI_DMA_RXTDEN, spi_imx->base + MX51_ECSPI_DMA);
> @@ -1171,7 +1171,11 @@ static int spi_imx_dma_configure(struct spi_master
> *master)
> tx.direction = DMA_MEM_TO_DEV;
> tx.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA;
> tx.dst_addr_width = buswidth;
> - tx.dst_maxburst = spi_imx->wml;
> + /*
> + * For ERR009165 with tx_wml = 0 could enlarge burst size to fifo size
> + * to speed up fifo filling as possible.
> + */
> + tx.dst_maxburst = spi_imx->devtype_data->fifo_size;
> ret = dmaengine_slave_config(master->dma_tx, &tx);
> if (ret) {
> dev_err(spi_imx->dev, "TX dma configuration failed with %d\n",
> ret);
> @@ -1265,10 +1269,6 @@ static int spi_imx_sdma_init(struct device *dev,
> struct spi_imx_data *spi_imx,
> {
> int ret;
>
> - /* use pio mode for i.mx6dl chip TKT238285 */
> - if (of_machine_is_compatible("fsl,imx6dl"))
> - return 0;
> -
> spi_imx->wml = spi_imx->devtype_data->fifo_size / 2;
>
> /* Prepare for TX DMA: */
> --
> 2.7.4
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 RESEND 00/13] add ecspi ERR009165 for i.mx6/7 soc family
On Tue, May 12, 2020 at 01:32:23AM +0800, Robin Gong wrote: > There is ecspi ERR009165 on i.mx6/7 soc family, which cause FIFO > transfer to be send twice in DMA mode. Please get more information from: > https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf. The workaround is adding > new sdma ram script which works in XCH mode as PIO inside sdma instead > of SMC mode, meanwhile, 'TX_THRESHOLD' should be 0. The issue should be > exist on all legacy i.mx6/7 soc family before i.mx6ul. > NXP fix this design issue from i.mx6ul, so newer chips including i.mx6ul/ > 6ull/6sll do not need this workaroud anymore. All other i.mx6/7/8 chips > still need this workaroud. This patch set add new 'fsl,imx6ul-ecspi' > for ecspi driver and 'ecspi_fixed' in sdma driver to choose if need errata > or not. > The first two reverted patches should be the same issue, though, it > seems 'fixed' by changing to other shp script. Hope Sean or Sascha could > have the chance to test this patch set if could fix their issues. > Besides, enable sdma support for i.mx8mm/8mq and fix ecspi1 not work > on i.mx8mm because the event id is zero. It's not nice to break SPI support when the new firmware is not present and I think we can do better. Wouldn't it be possible to fall back to PIO in this case? Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 RESEND 03/13] Revert "dmaengine: imx-sdma: fix context cache"
On Tue, May 12, 2020 at 01:32:26AM +0800, Robin Gong wrote: > This reverts commit d288bddd8374e0a043ac9dde64a1ae6a09411d74, since > 'context_loaded' finally removed. > > Signed-off-by: Robin Gong > --- > drivers/dma/imx-sdma.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 4d4477d..3d4aac9 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -1338,7 +1338,6 @@ static void sdma_free_chan_resources(struct dma_chan > *chan) > > sdmac->event_id0 = 0; > sdmac->event_id1 = 0; > - sdmac->context_loaded = false; > > sdma_set_channel_priority(sdmac, 0); I think this can safely be folded into the next patch which makes it more clear what is happening. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 RESEND 05/13] dmaengine: imx-sdma: remove dupilicated sdma_load_context
In the subject: s/dupilicated/duplicated/
Sascha
On Tue, May 12, 2020 at 01:32:28AM +0800, Robin Gong wrote:
> Since sdma_transfer_init() will do sdma_load_context before any
> sdma transfer, no need once more in sdma_config_channel().
>
> Signed-off-by: Robin Gong
> Acked-by: Vinod Koul
> ---
> drivers/dma/imx-sdma.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 397f11d..69ea44d 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1137,7 +1137,6 @@ static void sdma_set_watermarklevel_for_p2p(struct
> sdma_channel *sdmac)
> static int sdma_config_channel(struct dma_chan *chan)
> {
> struct sdma_channel *sdmac = to_sdma_chan(chan);
> - int ret;
>
> sdma_disable_channel(chan);
>
> @@ -1177,9 +1176,7 @@ static int sdma_config_channel(struct dma_chan *chan)
> sdmac->watermark_level = 0; /* FIXME: M3_BASE_ADDRESS */
> }
>
> - ret = sdma_load_context(sdmac);
> -
> - return ret;
> + return 0;
> }
>
> static int sdma_set_channel_priority(struct sdma_channel *sdmac,
> --
> 2.7.4
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] net/davicom: Add SOC to dm9000 to initialize SROM_BANK clock.
On Sun, May 10, 2020 at 07:02:13PM +0800, zhoubo...@foxmail.com wrote:
> From: To-run-away
>
> Increase the use of dm9000 to initialize the SROM_BANK clock in the SOC,
> otherwise the chip will not work.
The dm9000 doesn't have anything called SROM in it. You have to
describe the clock input pin of the dm9000 here, not the pin of
your SoC where the clock is coming out.
> The device tree file can be increased like this:
> ethernet@8800 {
> compatible = "davicom,dm9000";
>
> clocks = <&clocks CLK_SROMC>;
> clock-names = "sromc";
This must be documented in
Documentation/devicetree/bindings/net/davicom-dm9000.txt.
> + /* Enable clock if specified */
> + if (!of_property_read_string(dev->of_node, "clock-names", &clk_name)) {
> + struct clk *clk = devm_clk_get(dev, clk_name);
> + if (IS_ERR(clk)) {
> + dev_err(dev, "cannot get clock of %s\n", clk_name);
> + ret = PTR_ERR(clk);
> + goto out;
> + }
> + clk_prepare_enable(clk);
> + dev_info(dev, "enable clock '%s'\n", clk_name);
> + }
There's devm_clk_get_optional() which you should use here.
the "name" passed to devm_clk_get_optional() should match the name of
the clock, you must specify it according to the dm9000 datasheet. It
makes no sense to read the name from the device tree, instead pick a
name which you expect to be there.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] ata: sata_nv: use enum values as array indeces
Positions of the entries in nv_port_info[] must be consistent to enum
nv_host_type. Ensure this by using the enum as array index directly.
Signed-off-by: Sascha Hauer
---
drivers/ata/sata_nv.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 20190f66ced9..25c53fa17b33 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -520,8 +520,7 @@ struct nv_pi_priv {
&(struct nv_pi_priv){ .irq_handler = _irq_handler, .sht = _sht }
static const struct ata_port_info nv_port_info[] = {
- /* generic */
- {
+ [GENERIC] = {
.flags = ATA_FLAG_SATA,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
@@ -529,8 +528,7 @@ static const struct ata_port_info nv_port_info[] = {
.port_ops = &nv_generic_ops,
.private_data = NV_PI_PRIV(nv_generic_interrupt, &nv_sht),
},
- /* nforce2/3 */
- {
+ [NFORCE2] = {
.flags = ATA_FLAG_SATA,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
@@ -538,8 +536,7 @@ static const struct ata_port_info nv_port_info[] = {
.port_ops = &nv_nf2_ops,
.private_data = NV_PI_PRIV(nv_nf2_interrupt, &nv_sht),
},
- /* ck804 */
- {
+ [CK804] = {
.flags = ATA_FLAG_SATA,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
@@ -547,8 +544,7 @@ static const struct ata_port_info nv_port_info[] = {
.port_ops = &nv_ck804_ops,
.private_data = NV_PI_PRIV(nv_ck804_interrupt, &nv_sht),
},
- /* ADMA */
- {
+ [ADMA] = {
.flags = ATA_FLAG_SATA | ATA_FLAG_NCQ,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
@@ -556,8 +552,7 @@ static const struct ata_port_info nv_port_info[] = {
.port_ops = &nv_adma_ops,
.private_data = NV_PI_PRIV(nv_adma_interrupt, &nv_adma_sht),
},
- /* MCP5x */
- {
+ [MCP5x] = {
.flags = ATA_FLAG_SATA,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
@@ -565,8 +560,7 @@ static const struct ata_port_info nv_port_info[] = {
.port_ops = &nv_generic_ops,
.private_data = NV_PI_PRIV(nv_generic_interrupt, &nv_sht),
},
- /* SWNCQ */
- {
+ [SWNCQ] = {
.flags = ATA_FLAG_SATA | ATA_FLAG_NCQ,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
--
2.26.2
Re: [PATCH v2] libata: Fix retrieving of active qcs
On Sun, May 03, 2020 at 11:46:27PM +0200, Pali Rohár wrote: > On Monday 27 January 2020 12:24:28 Sascha Hauer wrote: > > On Mon, Jan 27, 2020 at 12:16:30PM +0100, Pali Rohár wrote: > > > On Monday 06 January 2020 09:16:05 Sascha Hauer wrote: > > > > On Wed, Dec 25, 2019 at 07:18:40PM +0100, Pali Rohár wrote: > > > > > Hello Sascha! > > > > > > > > > > On Friday 13 December 2019 09:04:08 Sascha Hauer wrote: > > > > > > ata_qc_complete_multiple() is called with a mask of the still active > > > > > > tags. > > > > > > > > > > > > mv_sata doesn't have this information directly and instead > > > > > > calculates > > > > > > the still active tags from the started tags (ap->qc_active) and the > > > > > > finished tags as (ap->qc_active ^ done_mask) > > > > > > > > > > > > Since 28361c40368 the hw_tag and tag are no longer the same and the > > > > > > equation is no longer valid. In ata_exec_internal_sg() > > > > > > ap->qc_active is > > > > > > initialized as 1ULL << ATA_TAG_INTERNAL, but in hardware tag 0 is > > > > > > started and this will be in done_mask on completion. ap->qc_active ^ > > > > > > done_mask becomes 0x1 ^ 0x1 = 0x10001 and thus tag 0 > > > > > > used as > > > > > > the internal tag will never be reported as completed. > > > > > > > > > > > > This is fixed by introducing ata_qc_get_active() which returns the > > > > > > active hardware tags and calling it where appropriate. > > > > > > > > > > > > This is tested on mv_sata, but sata_fsl and sata_nv suffer from the > > > > > > same > > > > > > problem. There is another case in sata_nv that most likely needs > > > > > > fixing > > > > > > as well, but this looks a little different, so I wasn't confident > > > > > > enough > > > > > > to change that. > > > > > > > > > > I can confirm that sata_nv.ko does not work in 4.18 (and new) kernel > > > > > version correctly. More details are in email: > > > > > > > > > > https://lore.kernel.org/linux-ide/20191225180824.bql2o5whougii4ch@pali/T/ > > > > > > > > > > I tried this patch and it fixed above problems with sata_nv.ko. It > > > > > just > > > > > needs small modification (see below). > > > > > > > > > > So you can add my: > > > > > > > > > > Tested-by: Pali Rohár > > > > > > > > > > And I hope that patch would be backported to 4.18 and 4.19 stable > > > > > branches soon as distributions kernels are broken for machines with > > > > > these nvidia sata controllers. > > > > > > > > > > Anyway, what is that another case in sata_nv which needs to be fixed > > > > > too? > > > > > > > > It's in nv_swncq_sdbfis(). Here we have: > > > > > > > > sactive = readl(pp->sactive_block); > > > > done_mask = pp->qc_active ^ sactive; > > > > > > > > pp->qc_active &= ~done_mask; > > > > pp->dhfis_bits &= ~done_mask; > > > > pp->dmafis_bits &= ~done_mask; > > > > pp->sdbfis_bits |= done_mask; > > > > ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask); > > > > > > > > Sascha > > > > > > Ok. Are you going to fix also this case? > > > > As said, this one looks slightly different than the others and I would > > prefer if somebody could fix it who actually has a hardware and can test > > it. > > Well, I have hardware and could test changes. But I'm not really sure > that I understand this part of code. So it would be better if somebody > else with better knowledge prepares patches I could test them. But > currently during coronavirus I have only remote ssh access, so boot, > modify/compile/reboot process is quite slower. Ok, here we go. Compile tested only. Regards, Sascha --8<--- >From fcdcfa9e7a4ee4faf411de1df4f3c4e12c78545c Mon Sep 17 00:00:00 2001 From: Sascha Hauer Date: Fri, 8 May 2020 07:28:19 +0200 Subj
Re: [EXT] Re: i2c: imx: support slave mode for imx I2C driver
On Fri, Aug 09, 2019 at 04:04:45AM +, Biwen Li wrote:
> >
> > Hi,
> >
> > On Thu, Aug 08, 2019 at 11:53:43AM +0800, Biwen Li wrote:
> > > The patch supports slave mode for imx I2C driver
> > >
> > > Signed-off-by: Biwen Li
> > > ---
> > > drivers/i2c/busses/i2c-imx.c | 199
> > > ---
> > > 1 file changed, 185 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-imx.c
> > > b/drivers/i2c/busses/i2c-imx.c index b1b8b938d7f4..f7583a9fa56f 100644
> > > --- a/drivers/i2c/busses/i2c-imx.c
> > > +++ b/drivers/i2c/busses/i2c-imx.c
> > > @@ -202,6 +202,9 @@ struct imx_i2c_struct {
> > > struct pinctrl_state *pinctrl_pins_gpio;
> > >
> > > struct imx_i2c_dma *dma;
> > > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > > + struct i2c_client *slave;
> > > +#endif /* CONFIG_I2C_SLAVE */
> >
> > Other drivers just do a "select I2C_SLAVE" in Kconfig to get rid of these
> > #ifs. We
> > should do the same.
> Hi sascha, I don't know your meaning, could you let it clearer?
Other drivers have "select I2C_SLAVE" in Kconfig, so they do not need
any #ifdef CONFIG_I2C_SLAVE as this is always true.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: i2c: imx: support slave mode for imx I2C driver
Hi,
On Thu, Aug 08, 2019 at 11:53:43AM +0800, Biwen Li wrote:
> The patch supports slave mode for imx I2C driver
>
> Signed-off-by: Biwen Li
> ---
> drivers/i2c/busses/i2c-imx.c | 199 ---
> 1 file changed, 185 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index b1b8b938d7f4..f7583a9fa56f 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -202,6 +202,9 @@ struct imx_i2c_struct {
> struct pinctrl_state *pinctrl_pins_gpio;
>
> struct imx_i2c_dma *dma;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + struct i2c_client *slave;
> +#endif /* CONFIG_I2C_SLAVE */
Other drivers just do a "select I2C_SLAVE" in Kconfig to get rid of
these #ifs. We should do the same.
> };
>
> static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -583,23 +586,40 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> }
>
> -static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> +/* Clear interrupt flag bit */
> +static void i2c_imx_clr_if_bit(struct imx_i2c_struct *i2c_imx)
> {
> - struct imx_i2c_struct *i2c_imx = dev_id;
> - unsigned int temp;
> + unsigned int status;
>
> - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> - if (temp & I2SR_IIF) {
> - /* save status register */
> - i2c_imx->i2csr = temp;
> - temp &= ~I2SR_IIF;
> - temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> - wake_up(&i2c_imx->queue);
> - return IRQ_HANDLED;
> - }
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + status &= ~I2SR_IIF;
> + status |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> + imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
> +}
> +
> +/* Clear arbitration lost bit */
> +static void i2c_imx_clr_al_bit(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status;
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + status &= ~I2SR_IAL;
> + imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
> +}
>
> - return IRQ_NONE;
> +static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status;
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>: master interrupt\n", __func__);
Generally this driver has way too many dev_dbg spread around in hot
pathes already. IMO adding more doesn't make the output more useful.
> +
> + /* Save status register */
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + i2c_imx->i2csr = status | I2SR_IIF;
> +
> + wake_up(&i2c_imx->queue);
> +
> + return IRQ_HANDLED;
> }
>
> static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> @@ -1043,11 +1063,162 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
> | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> }
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int temp;
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> +
> + /* Set slave addr. */
> + imx_i2c_write_reg((i2c_imx->slave->addr << 1), i2c_imx, IMX_I2C_IADR);
> +
> + /* Disable i2c module */
> + temp = i2c_imx->hwdata->i2cr_ien_opcode
> + ^ I2CR_IEN;
unnecessary line break.
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + /* Reset status register */
> + imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx,
> + IMX_I2C_I2SR);
> +
> + /* Enable module and enable interrupt from i2c module */
> + temp = i2c_imx->hwdata->i2cr_ien_opcode
> + | I2CR_IIEN;
ditto.
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + /* Wait controller to be stable */
> + usleep_range(50, 150);
> +}
> +
> +static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status, ctl;
> + u8 value;
> +
> + if (!i2c_imx->slave) {
> + dev_err(&i2c_imx->adapter.dev, "cannot deal with slave
> irq,i2c_imx->slave is null");
> + return IRQ_NONE;
> + }
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + if (status & I2SR_IAL) { /* Arbitration lost */
> + i2c_imx_clr_al_bit(i2c_imx);
> + } else if (status & I2SR_IAAS) { /* Addressed as a slave */
> + if (status & I2SR_SRW) { /* Master wants to read from us*/
> + dev_dbg(&i2c_imx->adapter.dev, "read requested");
> + i2c_slave_event(i2c_imx->slave,
> I2C_SLAVE_READ_REQUESTED, &value);
> +
> + /* Slave transimt */
s/transimt/transmit/
> + ctl |= I2CR_MTX;
> + imx_i2c_write_reg(ct
Re: nvmem creates multiple devices with the same name
On Tue, Jul 02, 2019 at 05:54:54PM +0100, Srinivas Kandagatla wrote: > Hi Sascha, > > On 01/07/2019 09:06, Sascha Hauer wrote: > > Hi Srinivas, > > > > On Tue, May 21, 2019 at 11:21:07AM +0200, Sascha Hauer wrote: > > > On Tue, May 21, 2019 at 10:02:32AM +0100, Srinivas Kandagatla wrote: > > > > > > > > > > > > On 21/05/2019 09:56, Sascha Hauer wrote: > > > > > . Are there any suggestions how to register the nvmem devices > > > > > with a different name? > > > > > > > > struct nvmem_config provides id field for this purpose, this will be > > > > used by > > > > nvmem to set the device name space along with name field. > > > > > > There's no way for a caller to know a unique name/id combination. > > > The mtd layer could initialize the id field with the mtd number, but > > > that would still not guarantee that another caller, like an EEPROM > > > driver or such, doesn't use the same name/id combination. > > > > This is still an unresolved issue. Do you have any input how we could > > proceed here? > > Sorry for the delay! > I think simplest solution would be to check if there is already an nvmem > provider with the same name before assigning name to the device and then > append the id in case it exists. > > Let me know if below patch helps the situation so that I can take this in > next cycle! > > --->cut< > nvmem: core: Check nvmem device name before adding the same one > > In some usecases where nvmem names are directly derived from > partition names, its likely that different devices might have > same partition name. > This will be an issue as we will be creating two different > nvmem devices with same name and sysfs will not be very happy with that. > > Simple solution is to check the existance of the nvmem provider with > same name and append an id if it exists before creating the device name. This solution obviously works for me. I am not sure if that's really what we want as the resulting names in sysfs are not predictable in any way. In that case we might be better off using mtdx as Boris suggested. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: nvmem creates multiple devices with the same name
Hi Srinivas, On Tue, May 21, 2019 at 11:21:07AM +0200, Sascha Hauer wrote: > On Tue, May 21, 2019 at 10:02:32AM +0100, Srinivas Kandagatla wrote: > > > > > > On 21/05/2019 09:56, Sascha Hauer wrote: > > > . Are there any suggestions how to register the nvmem devices > > > with a different name? > > > > struct nvmem_config provides id field for this purpose, this will be used by > > nvmem to set the device name space along with name field. > > There's no way for a caller to know a unique name/id combination. > The mtd layer could initialize the id field with the mtd number, but > that would still not guarantee that another caller, like an EEPROM > driver or such, doesn't use the same name/id combination. This is still an unresolved issue. Do you have any input how we could proceed here? Thanks Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 0/3] Marvell HCI fixes and serdev support
Hi, On Fri, Jun 14, 2019 at 09:23:48AM +0200, Sascha Hauer wrote: > First two patches are a fix for the Marvell HCI driver which fails to > properly upload the firmware. Third patch adds simple serdev support > to the driver. > > Sascha > > Sascha Hauer (3): > Bluetooth: hci_ldisc: Add function to wait for characters to be sent > Bluetooth: hci_mrvl: Wait for final ack before switching baudrate > Bluetooth: hci_mrvl: Add serdev support Any comments to this series? There are more issues with the firmware upload in the hci_mrvl driver. The firmware upload is done in multiple threads and only works on an idle system and even then only most of the time. I'd like to address these issues soon, so be prepared for more patches ;) Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 0/3] Marvell HCI fixes and serdev support
First two patches are a fix for the Marvell HCI driver which fails to properly upload the firmware. Third patch adds simple serdev support to the driver. Sascha Sascha Hauer (3): Bluetooth: hci_ldisc: Add function to wait for characters to be sent Bluetooth: hci_mrvl: Wait for final ack before switching baudrate Bluetooth: hci_mrvl: Add serdev support .../bindings/net/marvell-bluetooth.txt| 25 +++ drivers/bluetooth/Kconfig | 1 + drivers/bluetooth/hci_ldisc.c | 8 +++ drivers/bluetooth/hci_mrvl.c | 72 ++- drivers/bluetooth/hci_uart.h | 1 + 5 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/net/marvell-bluetooth.txt -- 2.20.1
[PATCH 2/3] Bluetooth: hci_mrvl: Wait for final ack before switching baudrate
For the Marvell HCI UART we have to upload two firmware files. The first one is only for switching the baudrate of the device to a higher baudrate. After the baudrate switching firmware has been uploaded the device waits for a final ack (0x5a) before actually switching the baudrate. To send this final ack with the old baudrate give the hci ldisc workqueue a chance to run before switching the baudrate. Without this the final ack will never be received by the device and firmware upload fails. Signed-off-by: Sascha Hauer --- drivers/bluetooth/hci_mrvl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c index 50212ac629e3..a0a74362455e 100644 --- a/drivers/bluetooth/hci_mrvl.c +++ b/drivers/bluetooth/hci_mrvl.c @@ -339,6 +339,9 @@ static int mrvl_setup(struct hci_uart *hu) return -EINVAL; } + /* Let the final ack go out before switching the baudrate */ + hci_uart_wait_until_sent(hu); + hci_uart_set_baudrate(hu, 300); hci_uart_set_flow_control(hu, false); -- 2.20.1
[PATCH 1/3] Bluetooth: hci_ldisc: Add function to wait for characters to be sent
The hci UART line discipline sends its characters in a workqueue. Some
devices like the Marvell Bluetooth chips need to make sure that all
queued characters are sent before switching the baudrate. This adds
a function to synchronize with the workqueue.
Signed-off-by: Sascha Hauer
---
drivers/bluetooth/hci_ldisc.c | 8
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index c84f985f348d..8950e07889fe 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -178,6 +178,7 @@ static void hci_uart_write_work(struct work_struct *work)
goto restart;
clear_bit(HCI_UART_SENDING, &hu->tx_state);
+ wake_up_bit(&hu->tx_state, HCI_UART_SENDING);
}
void hci_uart_init_work(struct work_struct *work)
@@ -213,6 +214,13 @@ int hci_uart_init_ready(struct hci_uart *hu)
return 0;
}
+int hci_uart_wait_until_sent(struct hci_uart *hu)
+{
+ return wait_on_bit_timeout(&hu->tx_state, HCI_UART_SENDING,
+ TASK_INTERRUPTIBLE,
+ msecs_to_jiffies(2000));
+}
+
/* --- Interface to HCI layer -- */
/* Reset device */
static int hci_uart_flush(struct hci_dev *hdev)
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index d8cf005e3c5d..f11af3912ce6 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -100,6 +100,7 @@ int hci_uart_register_device(struct hci_uart *hu, const
struct hci_uart_proto *p
void hci_uart_unregister_device(struct hci_uart *hu);
int hci_uart_tx_wakeup(struct hci_uart *hu);
+int hci_uart_wait_until_sent(struct hci_uart *hu);
int hci_uart_init_ready(struct hci_uart *hu);
void hci_uart_init_work(struct work_struct *work);
void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
--
2.20.1
[PATCH 3/3] Bluetooth: hci_mrvl: Add serdev support
This adds serdev support to the Marvell hci uart driver. Only basic
serdev support, none of the fancier features like regulator or enable
GPIO support is added for now.
Signed-off-by: Sascha Hauer
---
.../bindings/net/marvell-bluetooth.txt| 25 +++
drivers/bluetooth/Kconfig | 1 +
drivers/bluetooth/hci_mrvl.c | 69 ++-
3 files changed, 94 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/net/marvell-bluetooth.txt
diff --git a/Documentation/devicetree/bindings/net/marvell-bluetooth.txt
b/Documentation/devicetree/bindings/net/marvell-bluetooth.txt
new file mode 100644
index ..0e2842296032
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell-bluetooth.txt
@@ -0,0 +1,25 @@
+Marvell Bluetooth Chips
+---
+
+This documents the binding structure and common properties for serial
+attached Marvell Bluetooth devices. The following chips are included in
+this binding:
+
+* Marvell 88W8897 Bluetooth devices
+
+Required properties:
+ - compatible: should be:
+"mrvl,88w8897"
+
+Optional properties:
+None so far
+
+Example:
+
+&serial0 {
+ compatible = "ns16550a";
+ ...
+ bluetooth {
+ compatible = "mrvl,88w8897";
+ };
+};
diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index b9c34ff9a0d3..a3fafd781aa1 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -237,6 +237,7 @@ config BT_HCIUART_AG6XX
config BT_HCIUART_MRVL
bool "Marvell protocol support"
depends on BT_HCIUART
+ depends on BT_HCIUART_SERDEV
select BT_HCIUART_H4
help
Marvell is serial protocol for communication between Bluetooth
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index a0a74362455e..f98e5cc343b2 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -13,6 +13,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -40,6 +42,10 @@ struct mrvl_data {
u8 id, rev;
};
+struct mrvl_serdev {
+ struct hci_uart hu;
+};
+
struct hci_mrvl_pkt {
__le16 lhs;
__le16 rhs;
@@ -49,6 +55,7 @@ struct hci_mrvl_pkt {
static int mrvl_open(struct hci_uart *hu)
{
struct mrvl_data *mrvl;
+ int ret;
BT_DBG("hu %p", hu);
@@ -62,7 +69,18 @@ static int mrvl_open(struct hci_uart *hu)
set_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
hu->priv = mrvl;
+
+ if (hu->serdev) {
+ ret = serdev_device_open(hu->serdev);
+ if (ret)
+ goto err;
+ }
+
return 0;
+err:
+ kfree(mrvl);
+
+ return ret;
}
static int mrvl_close(struct hci_uart *hu)
@@ -71,6 +89,9 @@ static int mrvl_close(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ if (hu->serdev)
+ serdev_device_close(hu->serdev);
+
skb_queue_purge(&mrvl->txq);
skb_queue_purge(&mrvl->rawq);
kfree_skb(mrvl->rx_skb);
@@ -342,7 +363,11 @@ static int mrvl_setup(struct hci_uart *hu)
/* Let the final ack go out before switching the baudrate */
hci_uart_wait_until_sent(hu);
- hci_uart_set_baudrate(hu, 300);
+ if (hu->serdev)
+ serdev_device_set_baudrate(hu->serdev, 300);
+ else
+ hci_uart_set_baudrate(hu, 300);
+
hci_uart_set_flow_control(hu, false);
err = mrvl_load_firmware(hu->hdev, "mrvl/uart8897_bt.bin");
@@ -365,12 +390,54 @@ static const struct hci_uart_proto mrvl_proto = {
.dequeue= mrvl_dequeue,
};
+static int mrvl_serdev_probe(struct serdev_device *serdev)
+{
+ struct mrvl_serdev *mrvldev;
+
+ mrvldev = devm_kzalloc(&serdev->dev, sizeof(*mrvldev), GFP_KERNEL);
+ if (!mrvldev)
+ return -ENOMEM;
+
+ mrvldev->hu.serdev = serdev;
+ serdev_device_set_drvdata(serdev, mrvldev);
+
+ return hci_uart_register_device(&mrvldev->hu, &mrvl_proto);
+}
+
+static void mrvl_serdev_remove(struct serdev_device *serdev)
+{
+ struct mrvl_serdev *mrvldev = serdev_device_get_drvdata(serdev);
+
+ hci_uart_unregister_device(&mrvldev->hu);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id mrvl_bluetooth_of_match[] = {
+ { .compatible = "mrvl,88w8897" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, mrvl_bluetooth_of_match);
+#endif
+
+static struct serdev_device_driver mrvl_serdev_driver = {
+ .probe = mrvl_serdev_probe,
+ .remove = mrvl_serdev_remove,
+ .driver = {
+ .name = "hci_uart_mrvl",
+ .of_match_table = of_match_ptr(mrvl_bluetooth_of_match),
+ },
+};
+
int __in
Re: [PATCH v2 4/7] dmaengine: fsl-edma-common: version check for v2 instead
On Mon, May 27, 2019 at 04:51:15PM +0800, yibin.g...@nxp.com wrote: > From: Robin Gong > > The next v3 i.mx7ulp edma is based on v1, so change version > check logic for v2 instead. > > Signed-off-by: Robin Gong > --- > drivers/dma/fsl-edma-common.c | 40 > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c > index bb24251..45d70d3 100644 > --- a/drivers/dma/fsl-edma-common.c > +++ b/drivers/dma/fsl-edma-common.c > @@ -657,26 +657,26 @@ void fsl_edma_setup_regs(struct fsl_edma_engine *edma) > edma->regs.erql = edma->membase + EDMA_ERQ; > edma->regs.eeil = edma->membase + EDMA_EEI; > > - edma->regs.serq = edma->membase + ((edma->version == v1) ? > - EDMA_SERQ : EDMA64_SERQ); > - edma->regs.cerq = edma->membase + ((edma->version == v1) ? > - EDMA_CERQ : EDMA64_CERQ); > - edma->regs.seei = edma->membase + ((edma->version == v1) ? > - EDMA_SEEI : EDMA64_SEEI); > - edma->regs.ceei = edma->membase + ((edma->version == v1) ? > - EDMA_CEEI : EDMA64_CEEI); > - edma->regs.cint = edma->membase + ((edma->version == v1) ? > - EDMA_CINT : EDMA64_CINT); > - edma->regs.cerr = edma->membase + ((edma->version == v1) ? > - EDMA_CERR : EDMA64_CERR); > - edma->regs.ssrt = edma->membase + ((edma->version == v1) ? > - EDMA_SSRT : EDMA64_SSRT); > - edma->regs.cdne = edma->membase + ((edma->version == v1) ? > - EDMA_CDNE : EDMA64_CDNE); > - edma->regs.intl = edma->membase + ((edma->version == v1) ? > - EDMA_INTR : EDMA64_INTL); > - edma->regs.errl = edma->membase + ((edma->version == v1) ? > - EDMA_ERR : EDMA64_ERRL); > + edma->regs.serq = edma->membase + ((edma->version == v2) ? > + EDMA64_SERQ : EDMA_SERQ); > + edma->regs.cerq = edma->membase + ((edma->version == v2) ? > + EDMA64_CERQ : EDMA_CERQ); > + edma->regs.seei = edma->membase + ((edma->version == v2) ? > + EDMA64_SEEI : EDMA_SEEI); > + edma->regs.ceei = edma->membase + ((edma->version == v2) ? > + EDMA64_CEEI : EDMA_CEEI); > + edma->regs.cint = edma->membase + ((edma->version == v2) ? > + EDMA64_CINT : EDMA_CINT); > + edma->regs.cerr = edma->membase + ((edma->version == v2) ? > + EDMA64_CERR : EDMA_CERR); > + edma->regs.ssrt = edma->membase + ((edma->version == v2) ? > + EDMA64_SSRT : EDMA_SSRT); > + edma->regs.cdne = edma->membase + ((edma->version == v2) ? > + EDMA64_CDNE : EDMA_CDNE); > + edma->regs.intl = edma->membase + ((edma->version == v2) ? > + EDMA64_INTL : EDMA_INTR); > + edma->regs.errl = edma->membase + ((edma->version == v2) ? > + EDMA64_ERRL : EDMA_ERR); Following to what I have said to 6/7 you can put the register offsets into that new struct aswell. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2 6/7] dmaengine: fsl-edma: add i.mx7ulp edma2 version support
On Mon, May 27, 2019 at 04:51:17PM +0800, yibin.g...@nxp.com wrote:
> From: Robin Gong
>
> +static const struct of_device_id fsl_edma_dt_ids[] = {
> + { .compatible = "fsl,vf610-edma", .data = (void *)v1 },
> + { .compatible = "fsl,imx7ulp-edma", .data = (void *)v3 },
> + { /* sentinel */ }
Please put a struct type behind the .data pointer so that you can
configure...
> +};
> +MODULE_DEVICE_TABLE(of, fsl_edma_dt_ids);
> +
> @@ -218,6 +272,22 @@ static int fsl_edma_probe(struct platform_device *pdev)
> fsl_edma_setup_regs(fsl_edma);
> regs = &fsl_edma->regs;
>
> + if (fsl_edma->version == v3) {
> + fsl_edma->dmamux_nr = 1;
...things like this...
> @@ -264,7 +334,11 @@ static int fsl_edma_probe(struct platform_device *pdev)
> }
>
> edma_writel(fsl_edma, ~0, regs->intl);
> - ret = fsl_edma_irq_init(pdev, fsl_edma);
> +
> + if (fsl_edma->version == v3)
> + ret = fsl_edma2_irq_init(pdev, fsl_edma);
> + else
> + ret = fsl_edma_irq_init(pdev, fsl_edma);
...and this one in that struct rather than littering the code more and
more with such version tests.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: nvmem creates multiple devices with the same name
On Tue, May 21, 2019 at 10:02:32AM +0100, Srinivas Kandagatla wrote: > > > On 21/05/2019 09:56, Sascha Hauer wrote: > > . Are there any suggestions how to register the nvmem devices > > with a different name? > > struct nvmem_config provides id field for this purpose, this will be used by > nvmem to set the device name space along with name field. There's no way for a caller to know a unique name/id combination. The mtd layer could initialize the id field with the mtd number, but that would still not guarantee that another caller, like an EEPROM driver or such, doesn't use the same name/id combination. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
nvmem creates multiple devices with the same name
Hi all, nvmem derives the device name directly from the partition name of the underlying device. IMO this is wrong since it's not possible to create two partitions with the same name on different devices. In my case I have a NAND device and a SPI NOR device which both happen to have a partition named 'barebox'. This ends up with: [ 11.222196] sysfs: cannot create duplicate filename '/bus/nvmem/devices/barebox' [ 11.230136] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW 5.2.0-rc1-00014-g793f23e5adb0-dirty #676 [ 11.240414] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 11.247174] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 11.255171] [] (show_stack) from [] (dump_stack+0xd8/0x110) [ 11.262722] [] (dump_stack) from [] (sysfs_warn_dup+0x50/0x64) [ 11.270527] [] (sysfs_warn_dup) from [] (sysfs_do_create_link_sd+0xcc/0xd8) [ 11.279487] [] (sysfs_do_create_link_sd) from [] (bus_add_device+0x80/0xfc) [ 11.288441] [] (bus_add_device) from [] (device_add+0x328/0x608) [ 11.296423] [] (device_add) from [] (nvmem_register.part.1+0x168/0x5e4) [ 11.305030] [] (nvmem_register.part.1) from [] (add_mtd_device+0x1e8/0x404) [ 11.313988] [] (add_mtd_device) from [] (add_mtd_partitions+0x74/0x15c) [ 11.322589] [] (add_mtd_partitions) from [] (parse_mtd_partitions+0x180/0x368) [ 11.331807] [] (parse_mtd_partitions) from [] (mtd_device_parse_register+0x40/0x164) [ 11.341560] [] (mtd_device_parse_register) from [] (m25p_probe+0x118/0x200) [ 11.350513] [] (m25p_probe) from [] (spi_drv_probe+0x80/0xa4) While it's easy to rename the partitions I see no reason why it should be illegal to have two different (mtd) devices with eqeally named partitions. Are there any suggestions how to register the nvmem devices with a different name? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] ecryptfs: use print_hex_dump_bytes for hexdump
The Kernel has nice hexdump facilities, use them rather a homebrew
hexdump function.
Signed-off-by: Sascha Hauer
---
fs/ecryptfs/debug.c | 22 +++---
1 file changed, 3 insertions(+), 19 deletions(-)
diff --git a/fs/ecryptfs/debug.c b/fs/ecryptfs/debug.c
index 3d2bdf546ec6..ee9d8ac4a809 100644
--- a/fs/ecryptfs/debug.c
+++ b/fs/ecryptfs/debug.c
@@ -97,25 +97,9 @@ void ecryptfs_dump_auth_tok(struct ecryptfs_auth_tok
*auth_tok)
*/
void ecryptfs_dump_hex(char *data, int bytes)
{
- int i = 0;
- int add_newline = 1;
-
if (ecryptfs_verbosity < 1)
return;
- if (bytes != 0) {
- printk(KERN_DEBUG "0x%.2x.", (unsigned char)data[i]);
- i++;
- }
- while (i < bytes) {
- printk("0x%.2x.", (unsigned char)data[i]);
- i++;
- if (i % 16 == 0) {
- printk("\n");
- add_newline = 0;
- } else
- add_newline = 1;
- }
- if (add_newline)
- printk("\n");
-}
+ print_hex_dump(KERN_DEBUG, "ecryptfs: ", DUMP_PREFIX_OFFSET, 16, 1,
+ data, bytes, false);
+}
--
2.20.1
Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
On Thu, May 09, 2019 at 04:35:33AM +, Sumit Batra wrote: > > > > So the clock driver reports the wrong clock. Please fix the clock > > > > driver then. > > > No, this is a problem with the i2c driver. It is not a problem with > > > the clock driver, so the i2c driver needs to be modified. > > > > So how does this RCW bit get evaluated? > According to the reference manual > > only one clock goes to the i2c module (described as 1/2 Platform > > Clock) and the i2c module only takes one clock. So it seems there must > > be a /2 divider somewhere, either in each i2c module or somewhere > > outside. Can your IC guys tell you where it is? > I need to confirm this with the IC team [Reformated a bit to make it readable]: > There are 2 places where clock division takes place - > > 1) There is a clock divider outside of I2C block, which makes the clock > reaching >I2C module as - Platform Clock/2 > 2) There is another clock divider which specifically divides the clock to the > I2C block, >based on RCW bit 424 (if 424th bit is 0 then the baud clock source is > Platform Clock/4, >if 424th bit is 1 then it remains Platform Clock/2) So there is a clock divider which based on RCW bit 424 divides the clock *to* the i2c module. This suggests the divider is outside of the i2c module itself and thus part of the clock module. We could argue that this divider sits between the clock module and the i2c module, but for sure it's not in the i2c module. I really suggest to put this SoC specific into the SoC specific clock driver rather than littering the i2c driver with it. Sascha > > Now based on the what is the desired SCL value (100KHz etc) and the clock > which is > received by I2C block, there is a calculation that goes on inside the I2C > driver > module which is used to map a value in this imx_i2c_clk_div table. This value > is used > to program the IMX_I2C_IFDR register of the I2C block. Now if we don't > consider the > RCW bit 424 in our I2C driver calculation then the IMX_I2C_IFDR value that > gets set > makes SCL half of what is desired by the user. This is because if you make > the RCW > 424th bit as 0 then anyhow I2C block (hardware) will receive Platform > Clock/4, but > the driver (since it has not considered this bit) will consider it as > Platform Clock/2 > so it'll program a bigger divider from the imx_i2c_clk_div table -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
Hi,
In case we end up with the handling of this issue in the i2c driver,
here are the things to consider for v2.
On Tue, Apr 30, 2019 at 12:47:18PM +0800, Chuanhua Han wrote:
> The current kernel driver does not consider I2C_IPGCLK_SEL (424 bit
> of RCW) in deciding i2c_clk_rate in function i2c_imx_set_clk()
> { 0 Platform clock/4, 1 Platform clock/2}.
>
> When using ls1046a SoC, this populates incorrect value in IBFD register
> if I2C_IPGCLK_SEL = 0, which generates half of the desired Clock.
>
> Therefore, if ls1046a SoC is used, we need to set the i2c clock
> according to the corresponding RCW.
>
> Signed-off-by: Sumit Batra
> Signed-off-by: Chuanhua Han
> ---
> drivers/i2c/busses/i2c-imx.c | 64
> 1 file changed, 64 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 422f1a445b55..7186cf3c7d24 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -45,6 +45,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> /* This will be the driver name the kernel reports */
> #define DRIVER_NAME "imx-i2c"
> @@ -109,6 +111,21 @@
>
> #define I2C_PM_TIMEOUT 10 /* ms */
>
> +/* 14-1 Since array index starts from 0 */
> +#define RCW_I2C_IPGCLK_WORD (14 - 1)
> +/*
> + * Set mask for RCW 424th bit, reading from DCFG_CCSR RCW Status Registers
> + * Since this register in RM depicted as big endian,
> + * so consider 31st bit as LSB for creating the mask.
> + */
> +#define RCW_I2C_IPGCLK_MASK0x80
> +int i2c_ipgclk_sel = 1;
should be static.
> +
> +static const struct soc_device_attribute ls1046a_soc[] = {
> +{.family = "QorIQ LS1046A"},
> +{ /* sentinel */ }
> +};
> +
> /*
> * sorted list of clock divider, register value pairs
> * taken from table 26-5, p.26-9, Freescale i.MX
> @@ -304,6 +321,11 @@ static const struct platform_device_id imx_i2c_devtype[]
> = {
> };
> MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>
> +static const struct of_device_id guts_device_ids[] = {
> + { .compatible = "fsl,qoriq-device-config", },
> + {}
> +};
> +
> static const struct of_device_id i2c_imx_dt_ids[] = {
> { .compatible = "fsl,imx1-i2c", .data = &imx1_i2c_hwdata, },
> { .compatible = "fsl,imx21-i2c", .data = &imx21_i2c_hwdata, },
> @@ -533,6 +555,9 @@ static void i2c_imx_set_clk(struct imx_i2c_struct
> *i2c_imx,
> unsigned int div;
> int i;
>
> + if (!i2c_ipgclk_sel)
> + i2c_clk_rate = i2c_clk_rate / 2;
It would be nice to have the variable inverted. You wouldn't have to
initialize a global variable with something else but 0 then.
> +
> /* Divider value calculation */
> if (i2c_imx->cur_clk == i2c_clk_rate)
> return;
> @@ -551,6 +576,10 @@ static void i2c_imx_set_clk(struct imx_i2c_struct
> *i2c_imx,
> /* Store divider value */
> i2c_imx->ifdr = i2c_clk_div[i].val;
>
> + pr_alert("[%s] CLK Rate=%u Bitrate =%u Div =%u Value =%d\n",
> + __func__, i2c_clk_rate, i2c_imx->bitrate,
> + div, i2c_clk_div[i].val);
Please drop your debugging aids, for sure they shouldn't be pr_alert.
> +
> /*
>* There dummy delay is calculated.
>* It should be about one I2C clock period long.
> @@ -1116,6 +1145,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
> int irq, ret;
> dma_addr_t phy_addr;
> u32 mul_value;
> + struct device_node *guts_node;
> + static struct ccsr_guts __iomem *guts_regs;
> + u32 rcw_reg;
>
> dev_dbg(&pdev->dev, "<%s>\n", __func__);
>
> @@ -1135,6 +1167,38 @@ static int i2c_imx_probe(struct platform_device *pdev)
> if (!i2c_imx)
> return -ENOMEM;
>
> + if (soc_device_match(ls1046a_soc)) {
> + /*
> + * Make device node for GUTS/DCFG (global utilities block)
> + * to read RCW.
> + */
> + guts_node = of_find_matching_node(NULL, guts_device_ids);
> + if (!guts_node) {
> + dev_err(&pdev->dev, "Could not find GUTS node\n");
> + return -ENODEV;
> + }
> + /*
> + * Memory (IO) MAP the DCFG registers(for RCW) to
> + * be used in kernel virtual address space.
> + */
> + guts_regs = of_iomap(guts_node, 0);
> + of_node_put(guts_node);
> + if (!guts_regs) {
> + dev_err(&pdev->dev, "IOREMAP of GUTS node failed\n");
> + return -ENOMEM;
> + }
> + /* Read rcw bit 424 (starting from 0) */
> + rcw_reg = ioread32be(&guts_regs->rcwsr[RCW_I2C_IPGCLK_WORD]);
> + pr_alert("RCW REG[%d]=0x%x\n", RCW_I2C_IPGCLK_WORD, rcw_reg);
> + if (rcw_reg & RCW_I2C_IPGCLK_MASK) {
> + pr_alert("Div by 2 Case Detected in
Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts
On Tue, Apr 30, 2019 at 12:47:19PM +0800, Chuanhua Han wrote:
> For NXP ls1046a SoC, the i2c clock needs to be configured with the
> appropriate bit of RCW, so we add the guts node (GUTS/DCFG global
> utilities block) for the driver to read.
>
> Signed-off-by: Sumit Batra
> Signed-off-by: Chuanhua Han
> ---
> arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> index 373310e4c0ea..f88599df18bb 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> @@ -205,6 +205,11 @@
> status = "disabled";
> };
>
> + guts: global-utilities@1ee {
> + compatible = "fsl,qoriq-device-config";
> + reg = <0x0 0x1ee 0x0 0x1000>;
> + };
According to Documentation/devicetree/bindings/soc/fsl/guts.txt we have
the following compatibles:
"fsl,qoriq-device-config-1.0"
"fsl,qoriq-device-config-2.0"
"fsl,-device-config"
"fsl,-guts"
"fsl,qoriq-device-config" is none of them and I don't think you should
give this SoC specific thing a generic compatible.
"fsl,ls1046a-device-config" would be better.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
On Sat, May 04, 2019 at 09:28:48AM +, Chuanhua Han wrote:
>
>
> > -Original Message-
> > From: Sascha Hauer
> > Sent: 2019年4月30日 20:51
> > To: Chuanhua Han
> > Cc: shawn...@kernel.org; Leo Li ; robh...@kernel.org;
> > mark.rutl...@arm.com; linux-kernel@vger.kernel.org;
> > linux-...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> > devicet...@vger.kernel.org; feste...@gmail.com; dl-linux-imx
> > ; wsa+rene...@sang-engineering.com;
> > u.kleine-koe...@pengutronix.de; e...@deif.com; li...@rempel-privat.de;
> > l.st...@pengutronix.de; p...@axentia.se; Sumit Batra
> >
> > Subject: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider
> > I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
> >
> > Caution: EXT Email
> >
> > On Tue, Apr 30, 2019 at 12:47:18PM +0800, Chuanhua Han wrote:
> > > The current kernel driver does not consider I2C_IPGCLK_SEL (424 bit of
> > > RCW) in deciding i2c_clk_rate in function i2c_imx_set_clk() { 0
> > > Platform clock/4, 1 Platform clock/2}.
> > >
> > > When using ls1046a SoC, this populates incorrect value in IBFD
> > > register if I2C_IPGCLK_SEL = 0, which generates half of the desired Clock.
> > >
> > > Therefore, if ls1046a SoC is used, we need to set the i2c clock
> > > according to the corresponding RCW.
> >
> > So the clock driver reports the wrong clock. Please fix the clock driver
> > then.
> No, this is a problem with the i2c driver. It is not a problem with
> the clock driver, so the i2c driver needs to be modified.
So how does this RCW bit get evaluated? According to the reference
manual only one clock goes to the i2c module (described as 1/2 Platform
Clock) and the i2c module only takes one clock. So it seems there must
be a /2 divider somewhere, either in each i2c module or somewhere
outside. Can your IC guys tell you where it is?
One reason I suggested the clock driver is that the clock driver
contains SoC specific code already, so it should be easier to integrate
there.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
