* Re: [RESEND][PATCH 3/6] dt-bindings: mtd: fsl-quadspi: Update qspi bindings for LS2088A
From: Rob Herring @ 2016-11-14 17:13 UTC (permalink / raw)
To: Abhimanyu Saini
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
shawnguo-DgEjT+Ai2ygdnm+yROfE0A, scott.wood-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Priyanka Jain,
Ashish Kumar
In-Reply-To: <1478597664-14799-4-git-send-email-abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>
On Tue, Nov 08, 2016 at 03:04:21PM +0530, Abhimanyu Saini wrote:
> Signed-off-by: Priyanka Jain <priyanka.jain-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Ashish Kumar <ashish.kumar-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>
> ---
> Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RESEND][PATCH 3/6] dt-bindings: mtd: fsl-quadspi: Update qspi bindings for LS2088A
From: Rob Herring @ 2016-11-14 17:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478597664-14799-4-git-send-email-abhimanyu.saini@nxp.com>
On Tue, Nov 08, 2016 at 03:04:21PM +0530, Abhimanyu Saini wrote:
> Signed-off-by: Priyanka Jain <priyanka.jain@nxp.com>
> Signed-off-by: Ashish Kumar <ashish.kumar@nxp.com>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini@nxp.com>
> ---
> Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH 09/14] xfs: cleanup xfs_reflink_find_cow_mapping
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
Use xfs_iext_lookup_extent to look up the extent, drop a useless check,
drop a unneeded return value and clean up the general style a little bit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_aops.c | 16 ++++++++--------
fs/xfs/xfs_reflink.c | 30 +++++++++---------------------
fs/xfs/xfs_reflink.h | 2 +-
3 files changed, 18 insertions(+), 30 deletions(-)
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index dd6cacf..ab266d6 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -777,7 +777,7 @@ xfs_map_cow(
{
struct xfs_inode *ip = XFS_I(inode);
struct xfs_bmbt_irec imap;
- bool is_cow = false, need_alloc = false;
+ bool is_cow = false;
int error;
/*
@@ -795,7 +795,7 @@ xfs_map_cow(
* Else we need to check if there is a COW mapping at this offset.
*/
xfs_ilock(ip, XFS_ILOCK_SHARED);
- is_cow = xfs_reflink_find_cow_mapping(ip, offset, &imap, &need_alloc);
+ is_cow = xfs_reflink_find_cow_mapping(ip, offset, &imap);
xfs_iunlock(ip, XFS_ILOCK_SHARED);
if (!is_cow)
@@ -805,7 +805,7 @@ xfs_map_cow(
* And if the COW mapping has a delayed extent here we need to
* allocate real space for it now.
*/
- if (need_alloc) {
+ if (isnullstartblock(imap.br_startblock)) {
error = xfs_iomap_write_allocate(ip, XFS_COW_FORK, offset,
&imap);
if (error)
@@ -1311,7 +1311,6 @@ __xfs_get_blocks(
ssize_t size;
int new = 0;
bool is_cow = false;
- bool need_alloc = false;
BUG_ON(create && !direct);
@@ -1337,9 +1336,11 @@ __xfs_get_blocks(
end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + size);
offset_fsb = XFS_B_TO_FSBT(mp, offset);
- if (create && direct && xfs_is_reflink_inode(ip))
- is_cow = xfs_reflink_find_cow_mapping(ip, offset, &imap,
- &need_alloc);
+ if (create && direct && xfs_is_reflink_inode(ip)) {
+ is_cow = xfs_reflink_find_cow_mapping(ip, offset, &imap);
+ ASSERT(!is_cow || !isnullstartblock(imap.br_startblock));
+ }
+
if (!is_cow) {
error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
&imap, &nimaps, XFS_BMAPI_ENTIRE);
@@ -1356,7 +1357,6 @@ __xfs_get_blocks(
xfs_reflink_trim_irec_to_next_cow(ip, offset_fsb,
&imap);
}
- ASSERT(!need_alloc);
if (error)
goto out_unlock;
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 35e02ce..6056fd1 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -427,37 +427,25 @@ bool
xfs_reflink_find_cow_mapping(
struct xfs_inode *ip,
xfs_off_t offset,
- struct xfs_bmbt_irec *imap,
- bool *need_alloc)
+ struct xfs_bmbt_irec *imap)
{
- struct xfs_bmbt_irec irec;
- struct xfs_ifork *ifp;
- struct xfs_bmbt_rec_host *gotp;
- xfs_fileoff_t bno;
+ struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+ xfs_fileoff_t offset_fsb;
+ struct xfs_bmbt_irec got;
xfs_extnum_t idx;
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
ASSERT(xfs_is_reflink_inode(ip));
- /* Find the extent in the CoW fork. */
- ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
- bno = XFS_B_TO_FSBT(ip->i_mount, offset);
- gotp = xfs_iext_bno_to_ext(ifp, bno, &idx);
- if (!gotp)
+ offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
+ if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &idx, &got))
return false;
-
- xfs_bmbt_get_all(gotp, &irec);
- if (bno >= irec.br_startoff + irec.br_blockcount ||
- bno < irec.br_startoff)
+ if (got.br_startoff > offset_fsb)
return false;
trace_xfs_reflink_find_cow_mapping(ip, offset, 1, XFS_IO_OVERWRITE,
- &irec);
-
- /* If it's still delalloc, we must allocate later. */
- *imap = irec;
- *need_alloc = !!(isnullstartblock(irec.br_startblock));
-
+ &got);
+ *imap = got;
return true;
}
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 97ea9b4..cff5fc3 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -31,7 +31,7 @@ extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
extern int xfs_reflink_allocate_cow_range(struct xfs_inode *ip,
xfs_off_t offset, xfs_off_t count);
extern bool xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
- struct xfs_bmbt_irec *imap, bool *need_alloc);
+ struct xfs_bmbt_irec *imap);
extern int xfs_reflink_trim_irec_to_next_cow(struct xfs_inode *ip,
xfs_fileoff_t offset_fsb, struct xfs_bmbt_irec *imap);
--
2.1.4
^ permalink raw reply related
* Re: [Qemu-devel] [RFC 0/3] aio: experimental virtio-blk polling mode
From: Fam Zheng @ 2016-11-14 17:13 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Karl Rister, Stefan Hajnoczi, qemu-devel,
Andrew Theurer
In-Reply-To: <20161114170611.GE1352@stefanha-x1.localdomain>
On Mon, 11/14 17:06, Stefan Hajnoczi wrote:
> On Mon, Nov 14, 2016 at 04:29:49PM +0100, Paolo Bonzini wrote:
> > On 14/11/2016 16:26, Stefan Hajnoczi wrote:
> > > On Fri, Nov 11, 2016 at 01:59:25PM -0600, Karl Rister wrote:
> > >> QEMU_AIO_POLL_MAX_NS IOPs
> > >> unset 31,383
> > >> 1 46,860
> > >> 2 46,440
> > >> 4 35,246
> > >> 8 34,973
> > >> 16 46,794
> > >> 32 46,729
> > >> 64 35,520
> > >> 128 45,902
> > >
> > > The environment variable is in nanoseconds. The range of values you
> > > tried are very small (all <1 usec). It would be interesting to try
> > > larger values in the ballpark of the latencies you have traced. For
> > > example 2000, 4000, 8000, 16000, and 32000 ns.
> > >
> > > Very interesting that QEMU_AIO_POLL_MAX_NS=1 performs so well without
> > > much CPU overhead.
> >
> > That basically means "avoid a syscall if you already know there's
> > something to do", so in retrospect it's not that surprising. Still
> > interesting though, and it means that the feature is useful even if you
> > don't have CPU to waste.
>
> Can you spell out which syscall you mean? Reading the ioeventfd?
>
> The benchmark uses virtio-blk dataplane and iodepth=1 so there shouldn't
> be much IOThread event loop activity besides the single I/O request.
>
> The reason this puzzles me is that I wouldn't expect poll to succeed
> with QEMU_AIO_POLL_MAX_NS and iodepth=1.
I see the guest shouldn't send more requests, but isn't it possible for
the linux-aio poll to succeed?
Fam
>
> Thanks,
> Stefan
^ permalink raw reply
* [PATCH 08/14] xfs: use new extent lookup helpers in __xfs_reflink_reserve_cow
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_reflink.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 52cdfba..35e02ce 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -243,10 +243,11 @@ xfs_reflink_reserve_cow(
struct xfs_bmbt_irec *imap,
bool *shared)
{
- struct xfs_bmbt_irec got, prev;
+ struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+ struct xfs_bmbt_irec got;
xfs_fileoff_t end_fsb, orig_end_fsb;
- int eof = 0, error = 0;
- bool trimmed;
+ int error = 0;
+ bool eof = false, trimmed;
xfs_extnum_t idx;
xfs_extlen_t align;
@@ -258,8 +259,9 @@ xfs_reflink_reserve_cow(
* extent list is generally faster than going out to the shared extent
* tree.
*/
- xfs_bmap_search_extents(ip, imap->br_startoff, XFS_COW_FORK, &eof, &idx,
- &got, &prev);
+
+ if (!xfs_iext_lookup_extent(ip, ifp, imap->br_startoff, &idx, &got))
+ eof = true;
if (!eof && got.br_startoff <= imap->br_startoff) {
trace_xfs_reflink_cow_found(ip, imap);
xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
--
2.1.4
^ permalink raw reply related
* [PATCH 07/14] xfs: use new extent lookup helpers xfs_file_iomap_begin_delay
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
And only lookup the previous extent inside xfs_iomap_prealloc_size
if we actually need it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_iomap.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 59ffcac..2272190 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -395,11 +395,12 @@ xfs_iomap_prealloc_size(
struct xfs_inode *ip,
loff_t offset,
loff_t count,
- xfs_extnum_t idx,
- struct xfs_bmbt_irec *prev)
+ xfs_extnum_t idx)
{
struct xfs_mount *mp = ip->i_mount;
+ struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset);
+ struct xfs_bmbt_irec prev;
int shift = 0;
int64_t freesp;
xfs_fsblock_t qblocks;
@@ -419,8 +420,8 @@ xfs_iomap_prealloc_size(
*/
if ((mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) ||
XFS_ISIZE(ip) < XFS_FSB_TO_B(mp, mp->m_dalign) ||
- idx == 0 ||
- prev->br_startoff + prev->br_blockcount < offset_fsb)
+ !xfs_iext_get_extent(ifp, idx - 1, &prev) ||
+ prev.br_startoff + prev.br_blockcount < offset_fsb)
return mp->m_writeio_blocks;
/*
@@ -439,8 +440,8 @@ xfs_iomap_prealloc_size(
* always extends to MAXEXTLEN rather than falling short due to things
* like stripe unit/width alignment of real extents.
*/
- if (prev->br_blockcount <= (MAXEXTLEN >> 1))
- alloc_blocks = prev->br_blockcount << 1;
+ if (prev.br_blockcount <= (MAXEXTLEN >> 1))
+ alloc_blocks = prev.br_blockcount << 1;
else
alloc_blocks = XFS_B_TO_FSB(mp, offset);
if (!alloc_blocks)
@@ -538,7 +539,6 @@ xfs_file_iomap_begin_delay(
xfs_fileoff_t end_fsb, orig_end_fsb;
int error = 0, eof = 0;
struct xfs_bmbt_irec got;
- struct xfs_bmbt_irec prev;
xfs_extnum_t idx;
ASSERT(!XFS_IS_REALTIME_INODE(ip));
@@ -563,8 +563,7 @@ xfs_file_iomap_begin_delay(
goto out_unlock;
}
- xfs_bmap_search_extents(ip, offset_fsb, XFS_DATA_FORK, &eof, &idx,
- &got, &prev);
+ eof = !xfs_iext_lookup_extent(ip, ifp, offset_fsb, &idx, &got);
if (!eof && got.br_startoff <= offset_fsb) {
if (xfs_is_reflink_inode(ip)) {
bool shared;
@@ -601,8 +600,7 @@ xfs_file_iomap_begin_delay(
if (eof) {
xfs_fsblock_t prealloc_blocks;
- prealloc_blocks =
- xfs_iomap_prealloc_size(ip, offset, count, idx, &prev);
+ prealloc_blocks = xfs_iomap_prealloc_size(ip, offset, count, idx);
if (prealloc_blocks) {
xfs_extlen_t align;
xfs_off_t end_offset;
--
2.1.4
^ permalink raw reply related
* [PATCH 06/14] xfs: remove prev argument to xfs_bmapi_reserve_delalloc
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
We can easily lookup the previous extent for the cases where we need it,
which saves the callers from looking it up for us later in the series.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_bmap.c | 8 ++++++--
fs/xfs/libxfs/xfs_bmap.h | 3 +--
fs/xfs/xfs_iomap.c | 3 +--
fs/xfs/xfs_reflink.c | 2 +-
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 18de89c..4aa9c07 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4235,7 +4235,6 @@ xfs_bmapi_reserve_delalloc(
xfs_fileoff_t aoff,
xfs_filblks_t len,
struct xfs_bmbt_irec *got,
- struct xfs_bmbt_irec *prev,
xfs_extnum_t *lastx,
int eof)
{
@@ -4257,7 +4256,12 @@ xfs_bmapi_reserve_delalloc(
else
extsz = xfs_get_extsz_hint(ip);
if (extsz) {
- error = xfs_bmap_extsize_align(mp, got, prev, extsz, rt, eof,
+ struct xfs_bmbt_irec prev;
+
+ if (!xfs_iext_get_extent(ifp, *lastx - 1, &prev))
+ prev.br_startoff = NULLFILEOFF;
+
+ error = xfs_bmap_extsize_align(mp, got, &prev, extsz, rt, eof,
1, 0, &aoff, &alen);
ASSERT(!error);
}
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 7cae6ec..e3c2b5a 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -243,8 +243,7 @@ struct xfs_bmbt_rec_host *
struct xfs_bmbt_irec *gotp, struct xfs_bmbt_irec *prevp);
int xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork,
xfs_fileoff_t aoff, xfs_filblks_t len,
- struct xfs_bmbt_irec *got, struct xfs_bmbt_irec *prev,
- xfs_extnum_t *lastx, int eof);
+ struct xfs_bmbt_irec *got, xfs_extnum_t *lastx, int eof);
enum xfs_bmap_intent_type {
XFS_BMAP_MAP = 1,
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 436e109..59ffcac 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -622,8 +622,7 @@ xfs_file_iomap_begin_delay(
retry:
error = xfs_bmapi_reserve_delalloc(ip, XFS_DATA_FORK, offset_fsb,
- end_fsb - offset_fsb, &got,
- &prev, &idx, eof);
+ end_fsb - offset_fsb, &got, &idx, eof);
switch (error) {
case 0:
break;
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 0edf835..52cdfba 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -293,7 +293,7 @@ xfs_reflink_reserve_cow(
retry:
error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, imap->br_startoff,
- end_fsb - imap->br_startoff, &got, &prev, &idx, eof);
+ end_fsb - imap->br_startoff, &got, &idx, eof);
switch (error) {
case 0:
break;
--
2.1.4
^ permalink raw reply related
* [PATCH 05/14] xfs: use new extent lookup helpers in __xfs_bunmapi
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_bmap.c | 39 ++++++++++++---------------------------
1 file changed, 12 insertions(+), 27 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9a8621d..18de89c 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5433,8 +5433,6 @@ __xfs_bunmapi(
{
xfs_btree_cur_t *cur; /* bmap btree cursor */
xfs_bmbt_irec_t del; /* extent being deleted */
- int eof; /* is deleting at eof */
- xfs_bmbt_rec_host_t *ep; /* extent record pointer */
int error; /* error return value */
xfs_extnum_t extno; /* extent number in list */
xfs_bmbt_irec_t got; /* current extent record */
@@ -5444,7 +5442,6 @@ __xfs_bunmapi(
int logflags; /* transaction logging flags */
xfs_extlen_t mod; /* rt extent offset */
xfs_mount_t *mp; /* mount structure */
- xfs_bmbt_irec_t prev; /* previous extent record */
xfs_fileoff_t start; /* first file offset deleted */
int tmp_logflags; /* partial logging flags */
int wasdel; /* was a delayed alloc extent */
@@ -5483,18 +5480,17 @@ __xfs_bunmapi(
isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
start = bno;
bno = start + len - 1;
- ep = xfs_bmap_search_extents(ip, bno, whichfork, &eof, &lastx, &got,
- &prev);
/*
* Check to see if the given block number is past the end of the
* file, back up to the last block if so...
*/
- if (eof) {
- ep = xfs_iext_get_ext(ifp, --lastx);
- xfs_bmbt_get_all(ep, &got);
+ if (!xfs_iext_lookup_extent(ip, ifp, bno, &lastx, &got)) {
+ ASSERT(lastx > 0);
+ xfs_iext_get_extent(ifp, --lastx, &got);
bno = got.br_startoff + got.br_blockcount - 1;
}
+
logflags = 0;
if (ifp->if_flags & XFS_IFBROOT) {
ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_BTREE);
@@ -5525,8 +5521,7 @@ __xfs_bunmapi(
if (got.br_startoff > bno) {
if (--lastx < 0)
break;
- ep = xfs_iext_get_ext(ifp, lastx);
- xfs_bmbt_get_all(ep, &got);
+ xfs_iext_get_extent(ifp, lastx, &got);
}
/*
* Is the last block of this extent before the range
@@ -5540,7 +5535,6 @@ __xfs_bunmapi(
* Then deal with the (possibly delayed) allocated space
* we found.
*/
- ASSERT(ep != NULL);
del = got;
wasdel = isnullstartblock(del.br_startblock);
if (got.br_startoff < start) {
@@ -5621,15 +5615,11 @@ __xfs_bunmapi(
*/
ASSERT(bno >= del.br_blockcount);
bno -= del.br_blockcount;
- if (got.br_startoff > bno) {
- if (--lastx >= 0) {
- ep = xfs_iext_get_ext(ifp,
- lastx);
- xfs_bmbt_get_all(ep, &got);
- }
- }
+ if (got.br_startoff > bno && --lastx >= 0)
+ xfs_iext_get_extent(ifp, lastx, &got);
continue;
} else if (del.br_state == XFS_EXT_UNWRITTEN) {
+ xfs_bmbt_irec_t prev;
/*
* This one is already unwritten.
* It must have a written left neighbor.
@@ -5637,8 +5627,7 @@ __xfs_bunmapi(
* try again.
*/
ASSERT(lastx > 0);
- xfs_bmbt_get_all(xfs_iext_get_ext(ifp,
- lastx - 1), &prev);
+ xfs_iext_get_extent(ifp, lastx - 1, &prev);
ASSERT(prev.br_state == XFS_EXT_NORM);
ASSERT(!isnullstartblock(prev.br_startblock));
ASSERT(del.br_startblock ==
@@ -5736,13 +5725,9 @@ __xfs_bunmapi(
*/
if (bno != (xfs_fileoff_t)-1 && bno >= start) {
if (lastx >= 0) {
- ep = xfs_iext_get_ext(ifp, lastx);
- if (xfs_bmbt_get_startoff(ep) > bno) {
- if (--lastx >= 0)
- ep = xfs_iext_get_ext(ifp,
- lastx);
- }
- xfs_bmbt_get_all(ep, &got);
+ xfs_iext_get_extent(ifp, lastx, &got);
+ if (got.br_startoff > bno && --lastx >= 0)
+ xfs_iext_get_extent(ifp, lastx, &got);
}
extno++;
}
--
2.1.4
^ permalink raw reply related
* [PATCH 04/14] xfs: use new extent lookup helpers in xfs_bmapi_write
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_bmap.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 1a0fee4..9a8621d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4561,7 +4561,7 @@ xfs_bmapi_write(
struct xfs_ifork *ifp;
struct xfs_bmalloca bma = { NULL }; /* args for xfs_bmap_alloc */
xfs_fileoff_t end; /* end of mapped file region */
- int eof; /* after the end of extents */
+ bool eof = false; /* after the end of extents */
int error; /* error return */
int n; /* current extent index */
xfs_fileoff_t obno; /* old block number (offset) */
@@ -4639,12 +4639,14 @@ xfs_bmapi_write(
goto error0;
}
- xfs_bmap_search_extents(ip, bno, whichfork, &eof, &bma.idx, &bma.got,
- &bma.prev);
n = 0;
end = bno + len;
obno = bno;
+ if (!xfs_iext_lookup_extent(ip, ifp, bno, &bma.idx, &bma.got))
+ eof = true;
+ if (!xfs_iext_get_extent(ifp, bma.idx - 1, &bma.prev))
+ bma.prev.br_startoff = NULLFILEOFF;
bma.tp = tp;
bma.ip = ip;
bma.total = total;
@@ -4731,11 +4733,8 @@ xfs_bmapi_write(
/* Else go on to the next record. */
bma.prev = bma.got;
- if (++bma.idx < xfs_iext_count(ifp)) {
- xfs_bmbt_get_all(xfs_iext_get_ext(ifp, bma.idx),
- &bma.got);
- } else
- eof = 1;
+ if (!xfs_iext_get_extent(ifp, ++bma.idx, &bma.got))
+ eof = true;
}
*nmap = n;
--
2.1.4
^ permalink raw reply related
* [PATCH 03/14] xfs: use new extent lookup helpers in xfs_bmapi_read
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_bmap.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 98f490b..1a0fee4 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4145,12 +4145,11 @@ xfs_bmapi_read(
struct xfs_mount *mp = ip->i_mount;
struct xfs_ifork *ifp;
struct xfs_bmbt_irec got;
- struct xfs_bmbt_irec prev;
xfs_fileoff_t obno;
xfs_fileoff_t end;
- xfs_extnum_t lastx;
+ xfs_extnum_t idx;
int error;
- int eof;
+ bool eof = false;
int n = 0;
int whichfork = xfs_bmapi_whichfork(flags);
@@ -4190,7 +4189,8 @@ xfs_bmapi_read(
return error;
}
- xfs_bmap_search_extents(ip, bno, whichfork, &eof, &lastx, &got, &prev);
+ if (!xfs_iext_lookup_extent(ip, ifp, bno, &idx, &got))
+ eof = true;
end = bno + len;
obno = bno;
@@ -4221,10 +4221,8 @@ xfs_bmapi_read(
break;
/* Else go on to the next record. */
- if (++lastx < xfs_iext_count(ifp))
- xfs_bmbt_get_all(xfs_iext_get_ext(ifp, lastx), &got);
- else
- eof = 1;
+ if (!xfs_iext_get_extent(ifp, ++idx, &got))
+ eof = true;
}
*nmap = n;
return 0;
--
2.1.4
^ permalink raw reply related
* [PATCH 02/14] xfs: cleanup xfs_bmap_last_before
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
Rewrite the function using xfs_iext_lookup_extent and xfs_iext_get_extent,
and massage the flow into something easily understandable.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_bmap.c | 64 ++++++++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 5c3c4dd..98f490b 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1523,44 +1523,44 @@ xfs_bmap_first_unused(
*/
int /* error */
xfs_bmap_last_before(
- xfs_trans_t *tp, /* transaction pointer */
- xfs_inode_t *ip, /* incore inode */
- xfs_fileoff_t *last_block, /* last block */
- int whichfork) /* data or attr fork */
+ struct xfs_trans *tp, /* transaction pointer */
+ struct xfs_inode *ip, /* incore inode */
+ xfs_fileoff_t *last_block, /* last block */
+ int whichfork) /* data or attr fork */
{
- xfs_fileoff_t bno; /* input file offset */
- int eof; /* hit end of file */
- xfs_bmbt_rec_host_t *ep; /* pointer to last extent */
- int error; /* error return value */
- xfs_bmbt_irec_t got; /* current extent value */
- xfs_ifork_t *ifp; /* inode fork pointer */
- xfs_extnum_t lastx; /* last extent used */
- xfs_bmbt_irec_t prev; /* previous extent value */
+ struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
+ struct xfs_bmbt_irec got;
+ xfs_extnum_t idx;
+ int error;
- if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
- XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
- XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
- return -EIO;
- if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
+ switch (XFS_IFORK_FORMAT(ip, whichfork)) {
+ case XFS_DINODE_FMT_LOCAL:
*last_block = 0;
return 0;
+ case XFS_DINODE_FMT_BTREE:
+ case XFS_DINODE_FMT_EXTENTS:
+ break;
+ default:
+ return -EIO;
}
- ifp = XFS_IFORK_PTR(ip, whichfork);
- if (!(ifp->if_flags & XFS_IFEXTENTS) &&
- (error = xfs_iread_extents(tp, ip, whichfork)))
- return error;
- bno = *last_block - 1;
- ep = xfs_bmap_search_extents(ip, bno, whichfork, &eof, &lastx, &got,
- &prev);
- if (eof || xfs_bmbt_get_startoff(ep) > bno) {
- if (prev.br_startoff == NULLFILEOFF)
- *last_block = 0;
- else
- *last_block = prev.br_startoff + prev.br_blockcount;
+
+ if (!(ifp->if_flags & XFS_IFEXTENTS)) {
+ error = xfs_iread_extents(tp, ip, whichfork);
+ if (error)
+ return error;
}
- /*
- * Otherwise *last_block is already the right answer.
- */
+
+ if (xfs_iext_lookup_extent(ip, ifp, *last_block - 1, &idx, &got)) {
+ if (got.br_startoff <= *last_block - 1)
+ return 0;
+ }
+
+ if (xfs_iext_get_extent(ifp, idx - 1, &got)) {
+ *last_block = got.br_startoff + got.br_blockcount;
+ return 0;
+ }
+
+ *last_block = 0;
return 0;
}
--
2.1.4
^ permalink raw reply related
* [PATCH 01/14] xfs: new inode extent list lookup helpers
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>
xfs_iext_lookup_extent looks up a single extent at the passed in offset,
and returns the extent covering the area, or the one behind it in case
of a hole, as well as the index of the returned extent in arguments,
as well as a simple bool as return value that is set to false if no
extent could be found because the offset is behind EOF. It is a simpler
replacement for xfs_bmap_search_extent that leaves looking up the rarely
needed previous extent to the caller and has a nicer calling convention.
xfs_iext_get_extent is a helper for iterating over the extent list,
it takes an extent index as input, and returns the extent at that index
in it's expanded form in an argument if it exists. The actual return
value is a bool whether the index is valid or not.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_inode_fork.c | 46 ++++++++++++++++++++++++++++++++++++++++++
fs/xfs/libxfs/xfs_inode_fork.h | 6 ++++++
2 files changed, 52 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 5fbe24c..749fbfb 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -2003,3 +2003,49 @@ xfs_ifork_init_cow(
ip->i_cformat = XFS_DINODE_FMT_EXTENTS;
ip->i_cnextents = 0;
}
+
+/*
+ * Lookup the extent covering bno.
+ *
+ * If there is an extent covering bno return the extent index, and store the
+ * expanded extent structure in *gotp, and the extent indes in *idx.
+ * If there is no extent covering bno, but there is an extent after it (e.g.
+ * it lies in a hole) return that extent in *gotp and its index in *idx
+ * instead.
+ * If bno is beyond the last extent return false, and return the index after
+ * the last valid index in *idxp.
+ */
+bool
+xfs_iext_lookup_extent(
+ struct xfs_inode *ip,
+ struct xfs_ifork *ifp,
+ xfs_fileoff_t bno,
+ xfs_extnum_t *idxp,
+ struct xfs_bmbt_irec *gotp)
+{
+ struct xfs_bmbt_rec_host *ep;
+
+ XFS_STATS_INC(ip->i_mount, xs_look_exlist);
+
+ ep = xfs_iext_bno_to_ext(ifp, bno, idxp);
+ if (!ep)
+ return false;
+ xfs_bmbt_get_all(ep, gotp);
+ return true;
+}
+
+/*
+ * Return true if there is an extent at index idx, and return the expanded
+ * extent structure at idx in that case. Else return false.
+ */
+bool
+xfs_iext_get_extent(
+ struct xfs_ifork *ifp,
+ xfs_extnum_t idx,
+ struct xfs_bmbt_irec *gotp)
+{
+ if (idx < 0 || idx >= xfs_iext_count(ifp))
+ return false;
+ xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), gotp);
+ return true;
+}
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 8bf112e..7fb8365 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -182,6 +182,12 @@ void xfs_iext_irec_compact_pages(struct xfs_ifork *);
void xfs_iext_irec_compact_full(struct xfs_ifork *);
void xfs_iext_irec_update_extoffs(struct xfs_ifork *, int, int);
+bool xfs_iext_lookup_extent(struct xfs_inode *ip,
+ struct xfs_ifork *ifp, xfs_fileoff_t bno,
+ xfs_extnum_t *idxp, struct xfs_bmbt_irec *gotp);
+bool xfs_iext_get_extent(struct xfs_ifork *ifp, xfs_extnum_t idx,
+ struct xfs_bmbt_irec *gotp);
+
extern struct kmem_zone *xfs_ifork_zone;
extern void xfs_ifork_init_cow(struct xfs_inode *ip);
--
2.1.4
^ permalink raw reply related
* new helpers to clean up extent tree lookups
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-xfs
Now that Eric send out xfs_iext_count before I could get to it I need
to get my other iext helpers out ASAP before running into another
rebase :)
This series adds two new helpers that make iterating the extent list
a lot cleaner. They are the first step towards better abstracting out
access to the extent list and thus allowing us to experiment with
better data structures for it. The next step is the manipulations
in xfs_bmap_{add,del}_extent*, which will be more work than these
helpers, but I have some of that in progress.
^ permalink raw reply
* Re: debugfs question...
From: Mike Marshall @ 2016-11-14 17:12 UTC (permalink / raw)
To: Nicolai Stange
Cc: Greg KH, Al Viro, linux-fsdevel, Linus Torvalds,
Martin Brandenburg
In-Reply-To: <877f876wo9.fsf@gmail.com>
OK,
I did this:
diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c
index d484068..38887cc 100644
--- a/fs/orangefs/orangefs-debugfs.c
+++ b/fs/orangefs/orangefs-debugfs.c
@@ -114,6 +114,7 @@ static ssize_t orangefs_debug_write(struct file *,
};
const struct file_operations debug_help_fops = {
+ .owner = THIS_MODULE,
.open = orangefs_debug_help_open,
.read = seq_read,
.release = seq_release,
@@ -121,6 +122,7 @@ static ssize_t orangefs_debug_write(struct file *,
};
static const struct file_operations kernel_debug_fops = {
+ .owner = THIS_MODULE,
.open = orangefs_debug_open,
.read = orangefs_debug_read,
.write = orangefs_debug_write,
I changed my little tester program to read from
/sys/kernel/debug/orangefs/debug-help a byte at a time and sleep for
a second between bytes. Now I get nice error messages if I try
to unload the module while someone is reading debug-help, and it
unloads as normal when the reader is done:
[root@be1 hubcap]# rmmod orangefs.ko
rmmod: ERROR: Module orangefs is in use
[root@be1 hubcap]# rmmod orangefs.ko
rmmod: ERROR: Module orangefs is in use
[root@be1 hubcap]# rmmod orangefs.ko
rmmod: ERROR: Module orangefs is in use
[root@be1 hubcap]# rmmod orangefs.ko
[root@be1 hubcap]#
If this seems right, I'll see about getting it pulled...
Thanks!
-Mike
On Sun, Nov 13, 2016 at 1:51 PM, Nicolai Stange <nicstange@gmail.com> wrote:
> Hi again,
>
> bad news: my previous analysis was completely wrong, c.f. below.
> Good news (from my point of view): debugfs is correct, no fix needed for
> it.
>
> Apologies for the confusion...
>
>
> Nicolai Stange <nicstange@gmail.com> writes:
>
>> Greg KH <greg@kroah.com> writes:
>>
>>> On Mon, Oct 31, 2016 at 02:32:56PM -0400, Mike Marshall wrote:
>>>
>>>> But... really bad things happen if someone unloads the Orangefs
>>>> module after my test program does the open and before the read
>>>> starts. So I picked another debugfs-using-filesystem (f2fs) and
>>>> pointed my tester-program at /sys/kernel/debug/f2fs/status, and
>>>> the same bad thing happens there.
>>
>> [...]
>>
>>>> [ 1240.144316] Call Trace:
>>>> [ 1240.144450] [<ffffffff8122907f>] __fput+0xdf/0x1d0
>>>> [ 1240.144704] [<ffffffff812291ae>] ____fput+0xe/0x10
>>>> [ 1240.144962] [<ffffffff810b97de>] task_work_run+0x8e/0xc0
>>>> [ 1240.145243] [<ffffffff8109b98e>] do_exit+0x2ae/0xae0
>>
>>
>> Thank you very much for this detailed report!
>>
>> At least for the .../f2fs/status file, your splat at fput() can be
>> readily explained with the full proxy's releaser not being protected
>> against file removals in any way.
>>
>> Partly this is on purpose, c.f. the comment in full_proxy_release().
>>
>> However, I should have at least tried to acquire a reference to the
>> owning module before accessing some static struct file_operations or
>> even calling some ->release() within it. Meh.
>
> This is what I got wrong: debugfs does acquire the needed references
> correctly (details below). For the case of f2fs' "status" file, the
> file_operations ->owner is simply not set as it should have been,
> i.e. to THIS_MODULE.
>
>
> Details on debugfs' reference acquisition:
> The open proxy, full_proxy_open(), gets a reference to the "real"
> file_operations, hence to its module. (Only in its error path it
> releases it again).
>
> full_proxy_release() is in charge of dropping that reference again, but
> only *after* it has attempted to call the "real" ->release().
>
> So, as long as a file has been (successfully) opened, a reference to the
> original file_operation's ->owner is owned, preventing it from getting
> unloaded.
>
>
> Can you confirm that you didn't set ->owner in your Orangefs related
> tests, too?
>
>
> Thanks,
>
> Nicolai
^ permalink raw reply related
* Re: [RESEND][PATCH 2/6] dt-bindings: pci: Update bindings for LS2088A
From: Rob Herring @ 2016-11-14 17:12 UTC (permalink / raw)
To: Abhimanyu Saini
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
shawnguo-DgEjT+Ai2ygdnm+yROfE0A, scott.wood-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Priyanka Jain,
Ashish Kumar
In-Reply-To: <1478597664-14799-3-git-send-email-abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>
On Tue, Nov 08, 2016 at 03:04:20PM +0530, Abhimanyu Saini wrote:
> Signed-off-by: Priyanka Jain <priyanka.jain-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Ashish Kumar <ashish.kumar-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>
> ---
> Documentation/devicetree/bindings/pci/layerscape-pci.txt | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RESEND][PATCH 2/6] dt-bindings: pci: Update bindings for LS2088A
From: Rob Herring @ 2016-11-14 17:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478597664-14799-3-git-send-email-abhimanyu.saini@nxp.com>
On Tue, Nov 08, 2016 at 03:04:20PM +0530, Abhimanyu Saini wrote:
> Signed-off-by: Priyanka Jain <priyanka.jain@nxp.com>
> Signed-off-by: Ashish Kumar <ashish.kumar@nxp.com>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini@nxp.com>
> ---
> Documentation/devicetree/bindings/pci/layerscape-pci.txt | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [RESEND][PATCH 1/6] dt-bindings: arm: Update bindings for LS2088A targets
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
To: Abhimanyu Saini
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
shawnguo-DgEjT+Ai2ygdnm+yROfE0A, scott.wood-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Priyanka Jain,
Ashish Kumar
In-Reply-To: <1478597664-14799-2-git-send-email-abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>
On Tue, Nov 08, 2016 at 03:04:19PM +0530, Abhimanyu Saini wrote:
> Add compatible strings for LS2088A RDB and QDS board.
>
> Signed-off-by: Priyanka Jain <priyanka.jain-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Ashish Kumar <ashish.kumar-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>
> ---
> Documentation/devicetree/bindings/arm/fsl.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RESEND][PATCH 1/6] dt-bindings: arm: Update bindings for LS2088A targets
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478597664-14799-2-git-send-email-abhimanyu.saini@nxp.com>
On Tue, Nov 08, 2016 at 03:04:19PM +0530, Abhimanyu Saini wrote:
> Add compatible strings for LS2088A RDB and QDS board.
>
> Signed-off-by: Priyanka Jain <priyanka.jain@nxp.com>
> Signed-off-by: Ashish Kumar <ashish.kumar@nxp.com>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini@nxp.com>
> ---
> Documentation/devicetree/bindings/arm/fsl.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v6 5/9] drm/hisilicon/hibmc: Add crtc for DE
From: Sean Paul @ 2016-11-14 17:10 UTC (permalink / raw)
To: Rongrong Zou
Cc: Mark Rutland, lijianhua, Will Deacon, Tomeu Vizoso,
Jonathan Corbet, catalin.marinas, Emil Velikov, linuxarm,
dri-devel, james.xiong, shenhui, Rongrong Zou, Linux ARM Kernel
In-Reply-To: <5826ECA6.6030900@huawei.com>
On Sat, Nov 12, 2016 at 5:19 AM, Rongrong Zou <zourongrong@huawei.com> wrote:
> 在 2016/11/11 6:14, Sean Paul 写道:
>>
>> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com>
>> wrote:
>>>
>>> Add crtc funcs and helper funcs for DE.
>>>
>>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>>> ---
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 318
>>> ++++++++++++++++++++++++
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 +
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 +
>>> 3 files changed, 326 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> index 9c1a68c..9b5d0d0 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> @@ -23,6 +23,7 @@
>>>
>>> #include "hibmc_drm_drv.h"
>>> #include "hibmc_drm_regs.h"
>>> +#include "hibmc_drm_de.h"
>>> #include "hibmc_drm_power.h"
>>
>>
>> nit: alphabetize
>
>
> ok, thanks.
>
>>
>>>
>>> /*
>>> ---------------------------------------------------------------------- */
>>
>>
>> Remove
>
>
> will do, thanks.
>
>>
>>> @@ -168,3 +169,320 @@ int hibmc_plane_init(struct hibmc_drm_device
>>> *hidev)
>>> drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
>>> return 0;
>>> }
>>> +
>>> +static void hibmc_crtc_enable(struct drm_crtc *crtc)
>>> +{
>>> + unsigned int reg;
>>> + /* power mode 0 is default. */
>>
>>
>> This comment seems to be in the wrong place
>
>
> will remove it, thanks.
>
>
>>
>>> + struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>>> +
>>> + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>>> +
>>> + /* Enable display power gate & LOCALMEM power gate*/
>>> + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> + reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>>> + reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>>> + hibmc_set_current_gate(hidev, reg);
>>> + drm_crtc_vblank_on(crtc);
>>> +}
>>> +
>>> +static void hibmc_crtc_disable(struct drm_crtc *crtc)
>>> +{
>>> + unsigned int reg;
>>> + struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>>> +
>>> + drm_crtc_vblank_off(crtc);
>>> +
>>> + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_SLEEP);
>>> +
>>> + /* Enable display power gate & LOCALMEM power gate*/
>>> + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> + reg |= HIBMC_CURR_GATE_LOCALMEM(OFF);
>>> + reg |= HIBMC_CURR_GATE_DISPLAY(OFF);
>>> + hibmc_set_current_gate(hidev, reg);
>>> +}
>>> +
>>> +static int hibmc_crtc_atomic_check(struct drm_crtc *crtc,
>>> + struct drm_crtc_state *state)
>>> +{
>>> + return 0;
>>> +}
>>
>>
>> Caller NULL-checks, no need for stub
>
>
> thanks for pointing it out.
>
>
>>
>>> +
>>> +static unsigned int format_pll_reg(void)
>>> +{
>>> + unsigned int pllreg = 0;
>>> + struct panel_pll pll = {0};
>>> +
>>> + /* Note that all PLL's have the same format. Here,
>>> + * we just use Panel PLL parameter to work out the bit
>>> + * fields in the register.On returning a 32 bit number, the value
>>> can
>>> + * be applied to any PLL in the calling function.
>>> + */
>>> + pllreg |= HIBMC_PLL_CTRL_BYPASS(OFF) &
>>> HIBMC_PLL_CTRL_BYPASS_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_POWER(ON) & HIBMC_PLL_CTRL_POWER_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_INPUT(OSC) & HIBMC_PLL_CTRL_INPUT_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_POD(pll.POD) & HIBMC_PLL_CTRL_POD_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_OD(pll.OD) & HIBMC_PLL_CTRL_OD_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_N(pll.N) & HIBMC_PLL_CTRL_N_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_M(pll.M) & HIBMC_PLL_CTRL_M_MASK;
>>> +
>>> + return pllreg;
>>> +}
>>> +
>>> +static void set_vclock_hisilicon(struct drm_device *dev, unsigned long
>>> pll)
>>> +{
>>> + unsigned long tmp0, tmp1;
>>> + struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> + /* 1. outer_bypass_n=0 */
>>> + tmp0 = readl(hidev->mmio + CRT_PLL1_HS);
>>> + tmp0 &= 0xBFFFFFFF;
>>> + writel(tmp0, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 2. pll_pd=1?inter_bypass=1 */
>>> + writel(0x21000000, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 3. config pll */
>>> + writel(pll, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 4. delay */
>>> + mdelay(1);
>>
>>
>> These should be usleep_range() see
>> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>
>
> This looks better to me. i think a 'usleep_range(1000, 2000)' is ok.
>
>>
>>> +
>>> + /* 5. pll_pd =0 */
>>> + tmp1 = pll & ~0x01000000;
>>> + writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 6. delay */
>>> + mdelay(1);
>>> +
>>> + /* 7. inter_bypass=0 */
>>> + tmp1 &= ~0x20000000;
>>> + writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 8. delay */
>>> + mdelay(1);
>>> +
>>> + /* 9. outer_bypass_n=1 */
>>> + tmp1 |= 0x40000000;
>>> + writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>
>>
>> This function is a whole lot of magic. Any chance you can pull the
>> values out into #defines?
>
>
> will do. thanks.
>
>>
>>> +}
>>> +
>>> +/* This function takes care the extra registers and bit fields required
>>> to
>>
>>
>> nit: multi-line comments have a leading /* line with the comment
>> starting on the following line
>
>
> thanks for pointing it out.
>
>>
>> applies below as well
>>
>>
>>> + *setup a mode in board.
>>
>>
>> nit: space between * and comment, ie: * setup a mode in board
>
>
> understood, thanks.
>
>
>>
>> applies to the rest of the comment too
>>
>>
>>> + *Explanation about Display Control register:
>>> + *FPGA only supports 7 predefined pixel clocks, and clock select is
>>> + *in bit 4:0 of new register 0x802a8.
>>> + */
>>> +static unsigned int display_ctrl_adjust(struct drm_device *dev,
>>> + struct drm_display_mode *mode,
>>> + unsigned int ctrl)
>>> +{
>>> + unsigned long x, y;
>>> + unsigned long pll1; /* bit[31:0] of PLL */
>>> + unsigned long pll2; /* bit[63:32] of PLL */
>>> + struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> + x = mode->hdisplay;
>>> + y = mode->vdisplay;
>>> +
>>> + /* Hisilicon has to set up a new register for PLL control
>>> + *(CRT_PLL1_HS & CRT_PLL2_HS).
>>> + */
>>> + if (x == 800 && y == 600) {
>>> + pll1 = CRT_PLL1_HS_40MHZ;
>>> + pll2 = CRT_PLL2_HS_40MHZ;
>>> + } else if (x == 1024 && y == 768) {
>>> + pll1 = CRT_PLL1_HS_65MHZ;
>>> + pll2 = CRT_PLL2_HS_65MHZ;
>>> + } else if (x == 1152 && y == 864) {
>>> + pll1 = CRT_PLL1_HS_80MHZ_1152;
>>> + pll2 = CRT_PLL2_HS_80MHZ;
>>> + } else if (x == 1280 && y == 768) {
>>> + pll1 = CRT_PLL1_HS_80MHZ;
>>> + pll2 = CRT_PLL2_HS_80MHZ;
>>> + } else if (x == 1280 && y == 720) {
>>> + pll1 = CRT_PLL1_HS_74MHZ;
>>> + pll2 = CRT_PLL2_HS_74MHZ;
>>> + } else if (x == 1280 && y == 960) {
>>> + pll1 = CRT_PLL1_HS_108MHZ;
>>> + pll2 = CRT_PLL2_HS_108MHZ;
>>> + } else if (x == 1280 && y == 1024) {
>>> + pll1 = CRT_PLL1_HS_108MHZ;
>>> + pll2 = CRT_PLL2_HS_108MHZ;
>>> + } else if (x == 1600 && y == 1200) {
>>> + pll1 = CRT_PLL1_HS_162MHZ;
>>> + pll2 = CRT_PLL2_HS_162MHZ;
>>> + } else if (x == 1920 && y == 1080) {
>>> + pll1 = CRT_PLL1_HS_148MHZ;
>>> + pll2 = CRT_PLL2_HS_148MHZ;
>>> + } else if (x == 1920 && y == 1200) {
>>> + pll1 = CRT_PLL1_HS_193MHZ;
>>> + pll2 = CRT_PLL2_HS_193MHZ;
>>> + } else /* default to VGA clock */ {
>>> + pll1 = CRT_PLL1_HS_25MHZ;
>>> + pll2 = CRT_PLL2_HS_25MHZ;
>>> + }
>>
>>
>> This seems like something that should be checked in atomic_check so
>> you can be sure the mode is supported.
>>
>> It would also be nice to pull this out into a separate function (and a
>> lookup table if you're feeling adventurous)
>
>
> a lookup table seems good, thanks.
>
>
>>
>>> +
>>> + writel(pll2, hidev->mmio + CRT_PLL2_HS);
>>> + set_vclock_hisilicon(dev, pll1);
>>> +
>>> + /* Hisilicon has to set up the top-left and bottom-right
>>> + * registers as well.
>>> + * Note that normal chip only use those two register for
>>> + * auto-centering mode.
>>> + */
>>> + writel((HIBMC_CRT_AUTO_CENTERING_TL_TOP(0) &
>>> + HIBMC_CRT_AUTO_CENTERING_TL_TOP_MSK) |
>>> + (HIBMC_CRT_AUTO_CENTERING_TL_LEFT(0) &
>>> + HIBMC_CRT_AUTO_CENTERING_TL_LEFT_MSK),
>>> + hidev->mmio + HIBMC_CRT_AUTO_CENTERING_TL);
>>> +
>>> + writel((HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM(y - 1) &
>>> + HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM_MASK) |
>>> + (HIBMC_CRT_AUTO_CENTERING_BR_RIGHT(x - 1) &
>>> + HIBMC_CRT_AUTO_CENTERING_BR_RIGHT_MASK),
>>> + hidev->mmio + HIBMC_CRT_AUTO_CENTERING_BR);
>>> +
>>> + /* Assume common fields in ctrl have been properly set before
>>> + * calling this function.
>>> + * This function only sets the extra fields in ctrl.
>>> + */
>>> +
>>> + /* Set bit 25 of display controller: Select CRT or VGA clock */
>>> + ctrl &= ~HIBMC_CRT_DISP_CTL_CRTSELECT_MASK;
>>> + ctrl &= ~HIBMC_CRT_DISP_CTL_CLOCK_PHASE_MASK;
>>> +
>>> + ctrl |= HIBMC_CRT_DISP_CTL_CRTSELECT(CRTSELECT_CRT);
>>> +
>>> + /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL, CRTSELECT, CRT);*/
>>
>>
>> What's the deal with this commented code?
>
>
> sorry, will clean up.
>
>>
>>> +
>>> + /* Set bit 14 of display controller */
>>> + /*ctrl &= FIELD_CLEAR(HIBMC_CRT_DISP_CTL, CLOCK_PHASE);*/
>>> +
>>> + /* clock_phase_polarity is 0 */
>>> + ctrl |= HIBMC_CRT_DISP_CTL_CLOCK_PHASE(PHASE_ACTIVE_HIGH);
>>> + /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL,*/
>>> + /*CLOCK_PHASE, ACTIVE_HIGH);*/
>>
>>
>> Here too...
>
>
> ditto.
>
>>
>>> +
>>> + writel(ctrl, hidev->mmio + HIBMC_CRT_DISP_CTL);
>>> +
>>> + return ctrl;
>>> +}
>>> +
>>> +static void hibmc_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>> +{
>>> + unsigned int val;
>>> + struct drm_display_mode *mode = &crtc->state->mode;
>>> + struct drm_device *dev = crtc->dev;
>>> + struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> + writel(format_pll_reg(), hidev->mmio + HIBMC_CRT_PLL_CTRL);
>>> + writel((HIBMC_CRT_HORZ_TOTAL_TOTAL(mode->htotal - 1) &
>>> + HIBMC_CRT_HORZ_TOTAL_TOTAL_MASK) |
>>> + (HIBMC_CRT_HORZ_TOTAL_DISPLAY_END(mode->hdisplay - 1) &
>>> + HIBMC_CRT_HORZ_TOTAL_DISPLAY_END_MASK),
>>
>>
>> You could probably macroize this code to make it more readable
>
>
>
> #define HIBMC_FIELD(field, value) (field(value) & filed##_MASK)
>
> writel(HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_TOTAL, mode->htotal - 1) |
> HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_DISPLAY_END, mode->hdisplay
> - 1),
> hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>
> Is above ok?
>
Seems like an improvement.
>
>
>
>>
>>> + hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>>> +
>>> + writel((HIBMC_CRT_HORZ_SYNC_WIDTH(mode->hsync_end -
>>> mode->hsync_start)
>>> + & HIBMC_CRT_HORZ_SYNC_WIDTH_MASK) |
>>> + (HIBMC_CRT_HORZ_SYNC_START(mode->hsync_start - 1)
>>> + & HIBMC_CRT_HORZ_SYNC_START_MASK),
>>> + hidev->mmio + HIBMC_CRT_HORZ_SYNC);
>>> +
>>> + writel((HIBMC_CRT_VERT_TOTAL_TOTAL(mode->vtotal - 1) &
>>> + HIBMC_CRT_VERT_TOTAL_TOTAL_MASK) |
>>> + (HIBMC_CRT_VERT_TOTAL_DISPLAY_END(mode->vdisplay - 1) &
>>> + HIBMC_CRT_VERT_TOTAL_DISPLAY_END_MASK),
>>> + hidev->mmio + HIBMC_CRT_VERT_TOTAL);
>>> +
>>> + writel((HIBMC_CRT_VERT_SYNC_HEIGHT(mode->vsync_end -
>>> mode->vsync_start)
>>> + & HIBMC_CRT_VERT_SYNC_HEIGHT_MASK) |
>>> + (HIBMC_CRT_VERT_SYNC_START(mode->vsync_start - 1) &
>>> + HIBMC_CRT_VERT_SYNC_START_MASK),
>>> + hidev->mmio + HIBMC_CRT_VERT_SYNC);
>>> +
>>> + val = HIBMC_CRT_DISP_CTL_VSYNC_PHASE(0) &
>>> + HIBMC_CRT_DISP_CTL_VSYNC_PHASE_MASK;
>>> + val |= HIBMC_CRT_DISP_CTL_HSYNC_PHASE(0) &
>>> + HIBMC_CRT_DISP_CTL_HSYNC_PHASE_MASK;
>>> + val |= HIBMC_CRT_DISP_CTL_TIMING(ENABLE);
>>> + val |= HIBMC_CRT_DISP_CTL_PLANE(ENABLE);
>>> +
>>> + display_ctrl_adjust(dev, mode, val);
>>> +}
>>> +
>>> +static void hibmc_crtc_atomic_begin(struct drm_crtc *crtc,
>>> + struct drm_crtc_state *old_state)
>>> +{
>>> + unsigned int reg;
>>> + struct drm_device *dev = crtc->dev;
>>> + struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>>> +
>>> + /* Enable display power gate & LOCALMEM power gate*/
>>> + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> + reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>>> + reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>>> + hibmc_set_current_gate(hidev, reg);
>>> +
>>> + /* We can add more initialization as needed. */
>>> +}
>>> +
>>> +static void hibmc_crtc_atomic_flush(struct drm_crtc *crtc,
>>> + struct drm_crtc_state *old_state)
>>> +
>>> +{
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&crtc->dev->event_lock, flags);
>>> + if (crtc->state->event)
>>> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>>> + crtc->state->event = NULL;
>>> +
>>> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>> +}
>>> +
>>> +/* These provide the minimum set of functions required to handle a CRTC
>>> */
>>
>>
>> nit: don't need this comment
>
>
> will delete, thanks.
>
>
>>
>>> +static const struct drm_crtc_funcs hibmc_crtc_funcs = {
>>> + .page_flip = drm_atomic_helper_page_flip,
>>> + .set_config = drm_atomic_helper_set_config,
>>> + .destroy = drm_crtc_cleanup,
>>> + .reset = drm_atomic_helper_crtc_reset,
>>> + .atomic_duplicate_state =
>>> drm_atomic_helper_crtc_duplicate_state,
>>> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> +};
>>> +
>>> +static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
>>> + .enable = hibmc_crtc_enable,
>>> + .disable = hibmc_crtc_disable,
>>> + .mode_set_nofb = hibmc_crtc_mode_set_nofb,
>>> + .atomic_check = hibmc_crtc_atomic_check,
>>> + .atomic_begin = hibmc_crtc_atomic_begin,
>>> + .atomic_flush = hibmc_crtc_atomic_flush,
>>> +};
>>> +
>>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev)
>>> +{
>>> + struct drm_device *dev = hidev->dev;
>>> + struct drm_crtc *crtc = &hidev->crtc;
>>> + struct drm_plane *plane = &hidev->plane;
>>> + int ret;
>>> +
>>> + ret = drm_crtc_init_with_planes(dev, crtc, plane,
>>> + NULL, &hibmc_crtc_funcs, NULL);
>>> + if (ret) {
>>> + DRM_ERROR("failed to init crtc.\n");
>>
>>
>> print return code
>
>
> agreed, thanks.
>
>>
>>> + return ret;
>>> + }
>>> +
>>> + drm_mode_crtc_set_gamma_size(crtc, 256);
>>
>>
>> check return code
>
>
> agreed though none of other drivers has done this,
> thanks.
>
>>
>>> + drm_crtc_helper_add(crtc, &hibmc_crtc_helper_funcs);
>>> + return 0;
>>> +}
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> index 7d96583..303cd36 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> @@ -119,6 +119,12 @@ static int hibmc_kms_init(struct hibmc_drm_device
>>> *hidev)
>>> return ret;
>>> }
>>>
>>> + ret = hibmc_crtc_init(hidev);
>>> + if (ret) {
>>> + DRM_ERROR("failed to init crtc.\n");
>>> + return ret;
>>> + }
>>
>>
>> Typically the plane is initialized internally in the crtc driver. I
>> think this is a good design pattern, and you should probably use it.
>>
>> So how about squashing this down with the plane patch and keeping the
>> plane inside hibmc_drm_de.c?
>
>
> understood after i looked at intel_display.c, this file will be merged
> with patch 4/9, and the tile will be: 'drm/hisilicon/hibmc: Add display
> engine'.
>
Great, thanks.
>>
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> index 49e39d2..5731ec2 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> @@ -46,6 +46,7 @@ struct hibmc_drm_device {
>>> /* drm */
>>> struct drm_device *dev;
>>> struct drm_plane plane;
>>
>>
>> I don't think you should be keeping track of plane here. plane is only
>> used in the crtc init, which should be addressed by the previous
>> comment.
>
>
> so allocate with devm_kzalloc(sizeof(*plane)) and remove it from
> hibmc_drm_device?
>
>>
>>
>>> + struct drm_crtc crtc;
>>
>>
>> crtc is only used in the irq handler, so you could remove this here
>> and just call drm_handle_vblank(dev, 0) in the handler.
>
>
> so allocate with devm_kzalloc(sizeof(*crtc)) and remove it from
> hibmc_drm_device, when driver unload drm_crtc_cleanup() will be
> called and finally memory will be freed before quit.
>
Yep, precisely.
Sean
>>
>>
>>> bool mode_config_initialized;
>>>
>>> /* ttm */
>>> @@ -85,6 +86,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct
>>> drm_gem_object *gem)
>>> #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>>>
>>> int hibmc_plane_init(struct hibmc_drm_device *hidev);
>>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev);
>>> int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
>>> void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
>>>
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> _______________________________________________
>> linuxarm mailing list
>> linuxarm@huawei.com
>> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>>
>> .
>>
>
>
> --
> Regards, Rongrong
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v5 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
To: Rick Chang
Cc: Hans Verkuil, Laurent Pinchart, Mauro Carvalho Chehab,
Matthias Brugger, linux-kernel, linux-media, srv_heupstream,
linux-mediatek, linux-arm-kernel, devicetree, Minghsiu Tsai
In-Reply-To: <1478586880-3923-2-git-send-email-rick.chang@mediatek.com>
On Tue, Nov 08, 2016 at 02:34:38PM +0800, Rick Chang wrote:
> Add a DT binding documentation for Mediatek JPEG Decoder of
> MT2701 SoC.
>
> Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> ---
> .../bindings/media/mediatek-jpeg-decoder.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH v5 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478586880-3923-2-git-send-email-rick.chang@mediatek.com>
On Tue, Nov 08, 2016 at 02:34:38PM +0800, Rick Chang wrote:
> Add a DT binding documentation for Mediatek JPEG Decoder of
> MT2701 SoC.
>
> Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> ---
> .../bindings/media/mediatek-jpeg-decoder.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
To: Rick Chang
Cc: Hans Verkuil, Laurent Pinchart, Mauro Carvalho Chehab,
Matthias Brugger, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Minghsiu Tsai
In-Reply-To: <1478586880-3923-2-git-send-email-rick.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
On Tue, Nov 08, 2016 at 02:34:38PM +0800, Rick Chang wrote:
> Add a DT binding documentation for Mediatek JPEG Decoder of
> MT2701 SoC.
>
> Signed-off-by: Rick Chang <rick.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
> .../bindings/media/mediatek-jpeg-decoder.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: vf610-zii-dev: Add .dts file for rev. C
From: Andrew Lunn @ 2016-11-14 17:10 UTC (permalink / raw)
To: Andrey Smirnov
Cc: linux-arm-kernel, Shawn Guo, Rob Herring, Mark Rutland,
Russell King, Sascha Hauer, Stefan Agner, devicetree,
linux-kernel, cphealy
In-Reply-To: <1479141306-15141-2-git-send-email-andrew.smirnov@gmail.com>
> + mdio_mux_1: mdio@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + switch0: switch0@0 {
> + compatible = "marvell,mv88e6390";
Hi Andrey
The driver for this is not in net-next yet. And when it is, it will
probably be called "marvell,mv88e6190", keeping to the pattern of the
lowest product ID which supports these features.
> + port@4 {
> + reg = <4>;
> + label = "lan4";
> + };
> +
> + port@9 {
> + reg = <9>;
> + label = "lan4";
> + phy-handle = <&switch0phy0>;
> + };
> +
You have two "lan4". I would also suggest leaving port 9 out for the
moment. It needs clause 45 MDIO to talk to the PHY, which we don't
have yet. Hence it cannot find it, and so give an error.
> +
> + switch0port10: port@10 {
> + reg = <10>;
> + label = "dsa";
> + phy-mode = "xgmii";
> + link = <&switch1port10>;
> + fixed-link {
> + speed = <10000>;
> + full-duplex;
> + };
This fixed-link node is wrong, and invalid. 10000 is not supported by
the fixed link driver, only 10, 100, and 1000. Also, it is not
required. The DSA driver should configure the link to the fastest
possible speed the port supports. You only need a fixed-link property
when you need to configure it at a lower speed. Rev B also gets this
wrong.
> + };
> + };
> +
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + switch0phy0: switch0phy0@0 {
> + reg = <0>;
> + };
I think the strapping for the PHY is such that it is at address 9.
Also, it is on the external mdio bus, not the internal mdio bus. The
6390 family has two MDIO busses. I have patches to support this, which
will appear eventually.
Andrew
^ permalink raw reply
* [PATCH v6 5/9] drm/hisilicon/hibmc: Add crtc for DE
From: Sean Paul @ 2016-11-14 17:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5826ECA6.6030900@huawei.com>
On Sat, Nov 12, 2016 at 5:19 AM, Rongrong Zou <zourongrong@huawei.com> wrote:
> ? 2016/11/11 6:14, Sean Paul ??:
>>
>> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com>
>> wrote:
>>>
>>> Add crtc funcs and helper funcs for DE.
>>>
>>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>>> ---
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 318
>>> ++++++++++++++++++++++++
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 +
>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 +
>>> 3 files changed, 326 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> index 9c1a68c..9b5d0d0 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> @@ -23,6 +23,7 @@
>>>
>>> #include "hibmc_drm_drv.h"
>>> #include "hibmc_drm_regs.h"
>>> +#include "hibmc_drm_de.h"
>>> #include "hibmc_drm_power.h"
>>
>>
>> nit: alphabetize
>
>
> ok, thanks.
>
>>
>>>
>>> /*
>>> ---------------------------------------------------------------------- */
>>
>>
>> Remove
>
>
> will do, thanks.
>
>>
>>> @@ -168,3 +169,320 @@ int hibmc_plane_init(struct hibmc_drm_device
>>> *hidev)
>>> drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
>>> return 0;
>>> }
>>> +
>>> +static void hibmc_crtc_enable(struct drm_crtc *crtc)
>>> +{
>>> + unsigned int reg;
>>> + /* power mode 0 is default. */
>>
>>
>> This comment seems to be in the wrong place
>
>
> will remove it, thanks.
>
>
>>
>>> + struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>>> +
>>> + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>>> +
>>> + /* Enable display power gate & LOCALMEM power gate*/
>>> + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> + reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>>> + reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>>> + hibmc_set_current_gate(hidev, reg);
>>> + drm_crtc_vblank_on(crtc);
>>> +}
>>> +
>>> +static void hibmc_crtc_disable(struct drm_crtc *crtc)
>>> +{
>>> + unsigned int reg;
>>> + struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>>> +
>>> + drm_crtc_vblank_off(crtc);
>>> +
>>> + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_SLEEP);
>>> +
>>> + /* Enable display power gate & LOCALMEM power gate*/
>>> + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> + reg |= HIBMC_CURR_GATE_LOCALMEM(OFF);
>>> + reg |= HIBMC_CURR_GATE_DISPLAY(OFF);
>>> + hibmc_set_current_gate(hidev, reg);
>>> +}
>>> +
>>> +static int hibmc_crtc_atomic_check(struct drm_crtc *crtc,
>>> + struct drm_crtc_state *state)
>>> +{
>>> + return 0;
>>> +}
>>
>>
>> Caller NULL-checks, no need for stub
>
>
> thanks for pointing it out.
>
>
>>
>>> +
>>> +static unsigned int format_pll_reg(void)
>>> +{
>>> + unsigned int pllreg = 0;
>>> + struct panel_pll pll = {0};
>>> +
>>> + /* Note that all PLL's have the same format. Here,
>>> + * we just use Panel PLL parameter to work out the bit
>>> + * fields in the register.On returning a 32 bit number, the value
>>> can
>>> + * be applied to any PLL in the calling function.
>>> + */
>>> + pllreg |= HIBMC_PLL_CTRL_BYPASS(OFF) &
>>> HIBMC_PLL_CTRL_BYPASS_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_POWER(ON) & HIBMC_PLL_CTRL_POWER_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_INPUT(OSC) & HIBMC_PLL_CTRL_INPUT_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_POD(pll.POD) & HIBMC_PLL_CTRL_POD_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_OD(pll.OD) & HIBMC_PLL_CTRL_OD_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_N(pll.N) & HIBMC_PLL_CTRL_N_MASK;
>>> + pllreg |= HIBMC_PLL_CTRL_M(pll.M) & HIBMC_PLL_CTRL_M_MASK;
>>> +
>>> + return pllreg;
>>> +}
>>> +
>>> +static void set_vclock_hisilicon(struct drm_device *dev, unsigned long
>>> pll)
>>> +{
>>> + unsigned long tmp0, tmp1;
>>> + struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> + /* 1. outer_bypass_n=0 */
>>> + tmp0 = readl(hidev->mmio + CRT_PLL1_HS);
>>> + tmp0 &= 0xBFFFFFFF;
>>> + writel(tmp0, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 2. pll_pd=1?inter_bypass=1 */
>>> + writel(0x21000000, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 3. config pll */
>>> + writel(pll, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 4. delay */
>>> + mdelay(1);
>>
>>
>> These should be usleep_range() see
>> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>
>
> This looks better to me. i think a 'usleep_range(1000, 2000)' is ok.
>
>>
>>> +
>>> + /* 5. pll_pd =0 */
>>> + tmp1 = pll & ~0x01000000;
>>> + writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 6. delay */
>>> + mdelay(1);
>>> +
>>> + /* 7. inter_bypass=0 */
>>> + tmp1 &= ~0x20000000;
>>> + writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> + /* 8. delay */
>>> + mdelay(1);
>>> +
>>> + /* 9. outer_bypass_n=1 */
>>> + tmp1 |= 0x40000000;
>>> + writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>
>>
>> This function is a whole lot of magic. Any chance you can pull the
>> values out into #defines?
>
>
> will do. thanks.
>
>>
>>> +}
>>> +
>>> +/* This function takes care the extra registers and bit fields required
>>> to
>>
>>
>> nit: multi-line comments have a leading /* line with the comment
>> starting on the following line
>
>
> thanks for pointing it out.
>
>>
>> applies below as well
>>
>>
>>> + *setup a mode in board.
>>
>>
>> nit: space between * and comment, ie: * setup a mode in board
>
>
> understood, thanks.
>
>
>>
>> applies to the rest of the comment too
>>
>>
>>> + *Explanation about Display Control register:
>>> + *FPGA only supports 7 predefined pixel clocks, and clock select is
>>> + *in bit 4:0 of new register 0x802a8.
>>> + */
>>> +static unsigned int display_ctrl_adjust(struct drm_device *dev,
>>> + struct drm_display_mode *mode,
>>> + unsigned int ctrl)
>>> +{
>>> + unsigned long x, y;
>>> + unsigned long pll1; /* bit[31:0] of PLL */
>>> + unsigned long pll2; /* bit[63:32] of PLL */
>>> + struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> + x = mode->hdisplay;
>>> + y = mode->vdisplay;
>>> +
>>> + /* Hisilicon has to set up a new register for PLL control
>>> + *(CRT_PLL1_HS & CRT_PLL2_HS).
>>> + */
>>> + if (x == 800 && y == 600) {
>>> + pll1 = CRT_PLL1_HS_40MHZ;
>>> + pll2 = CRT_PLL2_HS_40MHZ;
>>> + } else if (x == 1024 && y == 768) {
>>> + pll1 = CRT_PLL1_HS_65MHZ;
>>> + pll2 = CRT_PLL2_HS_65MHZ;
>>> + } else if (x == 1152 && y == 864) {
>>> + pll1 = CRT_PLL1_HS_80MHZ_1152;
>>> + pll2 = CRT_PLL2_HS_80MHZ;
>>> + } else if (x == 1280 && y == 768) {
>>> + pll1 = CRT_PLL1_HS_80MHZ;
>>> + pll2 = CRT_PLL2_HS_80MHZ;
>>> + } else if (x == 1280 && y == 720) {
>>> + pll1 = CRT_PLL1_HS_74MHZ;
>>> + pll2 = CRT_PLL2_HS_74MHZ;
>>> + } else if (x == 1280 && y == 960) {
>>> + pll1 = CRT_PLL1_HS_108MHZ;
>>> + pll2 = CRT_PLL2_HS_108MHZ;
>>> + } else if (x == 1280 && y == 1024) {
>>> + pll1 = CRT_PLL1_HS_108MHZ;
>>> + pll2 = CRT_PLL2_HS_108MHZ;
>>> + } else if (x == 1600 && y == 1200) {
>>> + pll1 = CRT_PLL1_HS_162MHZ;
>>> + pll2 = CRT_PLL2_HS_162MHZ;
>>> + } else if (x == 1920 && y == 1080) {
>>> + pll1 = CRT_PLL1_HS_148MHZ;
>>> + pll2 = CRT_PLL2_HS_148MHZ;
>>> + } else if (x == 1920 && y == 1200) {
>>> + pll1 = CRT_PLL1_HS_193MHZ;
>>> + pll2 = CRT_PLL2_HS_193MHZ;
>>> + } else /* default to VGA clock */ {
>>> + pll1 = CRT_PLL1_HS_25MHZ;
>>> + pll2 = CRT_PLL2_HS_25MHZ;
>>> + }
>>
>>
>> This seems like something that should be checked in atomic_check so
>> you can be sure the mode is supported.
>>
>> It would also be nice to pull this out into a separate function (and a
>> lookup table if you're feeling adventurous)
>
>
> a lookup table seems good, thanks.
>
>
>>
>>> +
>>> + writel(pll2, hidev->mmio + CRT_PLL2_HS);
>>> + set_vclock_hisilicon(dev, pll1);
>>> +
>>> + /* Hisilicon has to set up the top-left and bottom-right
>>> + * registers as well.
>>> + * Note that normal chip only use those two register for
>>> + * auto-centering mode.
>>> + */
>>> + writel((HIBMC_CRT_AUTO_CENTERING_TL_TOP(0) &
>>> + HIBMC_CRT_AUTO_CENTERING_TL_TOP_MSK) |
>>> + (HIBMC_CRT_AUTO_CENTERING_TL_LEFT(0) &
>>> + HIBMC_CRT_AUTO_CENTERING_TL_LEFT_MSK),
>>> + hidev->mmio + HIBMC_CRT_AUTO_CENTERING_TL);
>>> +
>>> + writel((HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM(y - 1) &
>>> + HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM_MASK) |
>>> + (HIBMC_CRT_AUTO_CENTERING_BR_RIGHT(x - 1) &
>>> + HIBMC_CRT_AUTO_CENTERING_BR_RIGHT_MASK),
>>> + hidev->mmio + HIBMC_CRT_AUTO_CENTERING_BR);
>>> +
>>> + /* Assume common fields in ctrl have been properly set before
>>> + * calling this function.
>>> + * This function only sets the extra fields in ctrl.
>>> + */
>>> +
>>> + /* Set bit 25 of display controller: Select CRT or VGA clock */
>>> + ctrl &= ~HIBMC_CRT_DISP_CTL_CRTSELECT_MASK;
>>> + ctrl &= ~HIBMC_CRT_DISP_CTL_CLOCK_PHASE_MASK;
>>> +
>>> + ctrl |= HIBMC_CRT_DISP_CTL_CRTSELECT(CRTSELECT_CRT);
>>> +
>>> + /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL, CRTSELECT, CRT);*/
>>
>>
>> What's the deal with this commented code?
>
>
> sorry, will clean up.
>
>>
>>> +
>>> + /* Set bit 14 of display controller */
>>> + /*ctrl &= FIELD_CLEAR(HIBMC_CRT_DISP_CTL, CLOCK_PHASE);*/
>>> +
>>> + /* clock_phase_polarity is 0 */
>>> + ctrl |= HIBMC_CRT_DISP_CTL_CLOCK_PHASE(PHASE_ACTIVE_HIGH);
>>> + /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL,*/
>>> + /*CLOCK_PHASE, ACTIVE_HIGH);*/
>>
>>
>> Here too...
>
>
> ditto.
>
>>
>>> +
>>> + writel(ctrl, hidev->mmio + HIBMC_CRT_DISP_CTL);
>>> +
>>> + return ctrl;
>>> +}
>>> +
>>> +static void hibmc_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>> +{
>>> + unsigned int val;
>>> + struct drm_display_mode *mode = &crtc->state->mode;
>>> + struct drm_device *dev = crtc->dev;
>>> + struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> + writel(format_pll_reg(), hidev->mmio + HIBMC_CRT_PLL_CTRL);
>>> + writel((HIBMC_CRT_HORZ_TOTAL_TOTAL(mode->htotal - 1) &
>>> + HIBMC_CRT_HORZ_TOTAL_TOTAL_MASK) |
>>> + (HIBMC_CRT_HORZ_TOTAL_DISPLAY_END(mode->hdisplay - 1) &
>>> + HIBMC_CRT_HORZ_TOTAL_DISPLAY_END_MASK),
>>
>>
>> You could probably macroize this code to make it more readable
>
>
>
> #define HIBMC_FIELD(field, value) (field(value) & filed##_MASK)
>
> writel(HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_TOTAL, mode->htotal - 1) |
> HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_DISPLAY_END, mode->hdisplay
> - 1),
> hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>
> Is above ok?
>
Seems like an improvement.
>
>
>
>>
>>> + hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>>> +
>>> + writel((HIBMC_CRT_HORZ_SYNC_WIDTH(mode->hsync_end -
>>> mode->hsync_start)
>>> + & HIBMC_CRT_HORZ_SYNC_WIDTH_MASK) |
>>> + (HIBMC_CRT_HORZ_SYNC_START(mode->hsync_start - 1)
>>> + & HIBMC_CRT_HORZ_SYNC_START_MASK),
>>> + hidev->mmio + HIBMC_CRT_HORZ_SYNC);
>>> +
>>> + writel((HIBMC_CRT_VERT_TOTAL_TOTAL(mode->vtotal - 1) &
>>> + HIBMC_CRT_VERT_TOTAL_TOTAL_MASK) |
>>> + (HIBMC_CRT_VERT_TOTAL_DISPLAY_END(mode->vdisplay - 1) &
>>> + HIBMC_CRT_VERT_TOTAL_DISPLAY_END_MASK),
>>> + hidev->mmio + HIBMC_CRT_VERT_TOTAL);
>>> +
>>> + writel((HIBMC_CRT_VERT_SYNC_HEIGHT(mode->vsync_end -
>>> mode->vsync_start)
>>> + & HIBMC_CRT_VERT_SYNC_HEIGHT_MASK) |
>>> + (HIBMC_CRT_VERT_SYNC_START(mode->vsync_start - 1) &
>>> + HIBMC_CRT_VERT_SYNC_START_MASK),
>>> + hidev->mmio + HIBMC_CRT_VERT_SYNC);
>>> +
>>> + val = HIBMC_CRT_DISP_CTL_VSYNC_PHASE(0) &
>>> + HIBMC_CRT_DISP_CTL_VSYNC_PHASE_MASK;
>>> + val |= HIBMC_CRT_DISP_CTL_HSYNC_PHASE(0) &
>>> + HIBMC_CRT_DISP_CTL_HSYNC_PHASE_MASK;
>>> + val |= HIBMC_CRT_DISP_CTL_TIMING(ENABLE);
>>> + val |= HIBMC_CRT_DISP_CTL_PLANE(ENABLE);
>>> +
>>> + display_ctrl_adjust(dev, mode, val);
>>> +}
>>> +
>>> +static void hibmc_crtc_atomic_begin(struct drm_crtc *crtc,
>>> + struct drm_crtc_state *old_state)
>>> +{
>>> + unsigned int reg;
>>> + struct drm_device *dev = crtc->dev;
>>> + struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>>> +
>>> + /* Enable display power gate & LOCALMEM power gate*/
>>> + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> + reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>>> + reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>>> + hibmc_set_current_gate(hidev, reg);
>>> +
>>> + /* We can add more initialization as needed. */
>>> +}
>>> +
>>> +static void hibmc_crtc_atomic_flush(struct drm_crtc *crtc,
>>> + struct drm_crtc_state *old_state)
>>> +
>>> +{
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&crtc->dev->event_lock, flags);
>>> + if (crtc->state->event)
>>> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>>> + crtc->state->event = NULL;
>>> +
>>> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>> +}
>>> +
>>> +/* These provide the minimum set of functions required to handle a CRTC
>>> */
>>
>>
>> nit: don't need this comment
>
>
> will delete, thanks.
>
>
>>
>>> +static const struct drm_crtc_funcs hibmc_crtc_funcs = {
>>> + .page_flip = drm_atomic_helper_page_flip,
>>> + .set_config = drm_atomic_helper_set_config,
>>> + .destroy = drm_crtc_cleanup,
>>> + .reset = drm_atomic_helper_crtc_reset,
>>> + .atomic_duplicate_state =
>>> drm_atomic_helper_crtc_duplicate_state,
>>> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> +};
>>> +
>>> +static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
>>> + .enable = hibmc_crtc_enable,
>>> + .disable = hibmc_crtc_disable,
>>> + .mode_set_nofb = hibmc_crtc_mode_set_nofb,
>>> + .atomic_check = hibmc_crtc_atomic_check,
>>> + .atomic_begin = hibmc_crtc_atomic_begin,
>>> + .atomic_flush = hibmc_crtc_atomic_flush,
>>> +};
>>> +
>>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev)
>>> +{
>>> + struct drm_device *dev = hidev->dev;
>>> + struct drm_crtc *crtc = &hidev->crtc;
>>> + struct drm_plane *plane = &hidev->plane;
>>> + int ret;
>>> +
>>> + ret = drm_crtc_init_with_planes(dev, crtc, plane,
>>> + NULL, &hibmc_crtc_funcs, NULL);
>>> + if (ret) {
>>> + DRM_ERROR("failed to init crtc.\n");
>>
>>
>> print return code
>
>
> agreed, thanks.
>
>>
>>> + return ret;
>>> + }
>>> +
>>> + drm_mode_crtc_set_gamma_size(crtc, 256);
>>
>>
>> check return code
>
>
> agreed though none of other drivers has done this,
> thanks.
>
>>
>>> + drm_crtc_helper_add(crtc, &hibmc_crtc_helper_funcs);
>>> + return 0;
>>> +}
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> index 7d96583..303cd36 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> @@ -119,6 +119,12 @@ static int hibmc_kms_init(struct hibmc_drm_device
>>> *hidev)
>>> return ret;
>>> }
>>>
>>> + ret = hibmc_crtc_init(hidev);
>>> + if (ret) {
>>> + DRM_ERROR("failed to init crtc.\n");
>>> + return ret;
>>> + }
>>
>>
>> Typically the plane is initialized internally in the crtc driver. I
>> think this is a good design pattern, and you should probably use it.
>>
>> So how about squashing this down with the plane patch and keeping the
>> plane inside hibmc_drm_de.c?
>
>
> understood after i looked at intel_display.c, this file will be merged
> with patch 4/9, and the tile will be: 'drm/hisilicon/hibmc: Add display
> engine'.
>
Great, thanks.
>>
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> index 49e39d2..5731ec2 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> @@ -46,6 +46,7 @@ struct hibmc_drm_device {
>>> /* drm */
>>> struct drm_device *dev;
>>> struct drm_plane plane;
>>
>>
>> I don't think you should be keeping track of plane here. plane is only
>> used in the crtc init, which should be addressed by the previous
>> comment.
>
>
> so allocate with devm_kzalloc(sizeof(*plane)) and remove it from
> hibmc_drm_device?
>
>>
>>
>>> + struct drm_crtc crtc;
>>
>>
>> crtc is only used in the irq handler, so you could remove this here
>> and just call drm_handle_vblank(dev, 0) in the handler.
>
>
> so allocate with devm_kzalloc(sizeof(*crtc)) and remove it from
> hibmc_drm_device, when driver unload drm_crtc_cleanup() will be
> called and finally memory will be freed before quit.
>
Yep, precisely.
Sean
>>
>>
>>> bool mode_config_initialized;
>>>
>>> /* ttm */
>>> @@ -85,6 +86,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct
>>> drm_gem_object *gem)
>>> #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>>>
>>> int hibmc_plane_init(struct hibmc_drm_device *hidev);
>>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev);
>>> int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
>>> void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
>>>
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> _______________________________________________
>> linuxarm mailing list
>> linuxarm at huawei.com
>> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>>
>> .
>>
>
>
> --
> Regards, Rongrong
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: vf610-zii-dev: Add .dts file for rev. C
From: Andrew Lunn @ 2016-11-14 17:10 UTC (permalink / raw)
To: Andrey Smirnov
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Shawn Guo,
Rob Herring, Mark Rutland, Russell King, Sascha Hauer,
Stefan Agner, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cphealy-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1479141306-15141-2-git-send-email-andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + mdio_mux_1: mdio@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + switch0: switch0@0 {
> + compatible = "marvell,mv88e6390";
Hi Andrey
The driver for this is not in net-next yet. And when it is, it will
probably be called "marvell,mv88e6190", keeping to the pattern of the
lowest product ID which supports these features.
> + port@4 {
> + reg = <4>;
> + label = "lan4";
> + };
> +
> + port@9 {
> + reg = <9>;
> + label = "lan4";
> + phy-handle = <&switch0phy0>;
> + };
> +
You have two "lan4". I would also suggest leaving port 9 out for the
moment. It needs clause 45 MDIO to talk to the PHY, which we don't
have yet. Hence it cannot find it, and so give an error.
> +
> + switch0port10: port@10 {
> + reg = <10>;
> + label = "dsa";
> + phy-mode = "xgmii";
> + link = <&switch1port10>;
> + fixed-link {
> + speed = <10000>;
> + full-duplex;
> + };
This fixed-link node is wrong, and invalid. 10000 is not supported by
the fixed link driver, only 10, 100, and 1000. Also, it is not
required. The DSA driver should configure the link to the fastest
possible speed the port supports. You only need a fixed-link property
when you need to configure it at a lower speed. Rev B also gets this
wrong.
> + };
> + };
> +
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + switch0phy0: switch0phy0@0 {
> + reg = <0>;
> + };
I think the strapping for the PHY is such that it is at address 9.
Also, it is on the external mdio bus, not the internal mdio bus. The
6390 family has two MDIO busses. I have patches to support this, which
will appear eventually.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.