Hi, Tomasz, Thanks for the code review. > -----Original Message----- > From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] > Sent: Sunday, June 17, 2018 11:46 PM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; Sakari Ailus > <sakari.ailus@xxxxxxxxxxxxxxx>; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Toivonen, Tuukka > <tuukka.toivonen@xxxxxxxxx>; Hu, Jerry W <jerry.w.hu@xxxxxxxxx>; Zheng, > Jian Xu <jian.xu.zheng@xxxxxxxxx> > Subject: Re: [PATCH v6 03/12] intel-ipu3: mmu: Implement driver > > On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong.zhi@xxxxxxxxx> wrote: > > > > From: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > > > This driver translates IO virtual address to physical address based on > > two levels page tables. > > > > Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx> > > --- > > drivers/media/pci/intel/ipu3/ipu3-mmu.c | 560 > > ++++++++++++++++++++++++++++++++ > > drivers/media/pci/intel/ipu3/ipu3-mmu.h | 28 ++ > > 2 files changed, 588 insertions(+) > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-mmu.c > > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-mmu.h > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-mmu.c > > b/drivers/media/pci/intel/ipu3/ipu3-mmu.c > > new file mode 100644 > > index 000000000000..a4b3e1680bbb > > --- /dev/null > > +++ b/drivers/media/pci/intel/ipu3/ipu3-mmu.c > > @@ -0,0 +1,560 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 Intel Corporation. > > + * Copyright (C) 2018 Google, Inc. > > I followed wrong guide when adding this one. Could you fix it up to the > following? > > Copyright 2018 Google LLC. > Sure, will do. > [snip] > > +/** > > + * ipu3_mmu_exit() - clean up IPU3 MMU block > > + * @mmu: IPU3 MMU private data > > + */ > > +void ipu3_mmu_exit(struct ipu3_mmu_info *info) { > > + struct ipu3_mmu *mmu = to_ipu3_mmu(info); > > + > > + /* We are going to free our page tables, no more memory access. */ > > + ipu3_mmu_set_halt(mmu, true); > > + ipu3_mmu_tlb_invalidate(mmu); > > + > > + ipu3_mmu_free_page_table(mmu->l1pt); > > + vfree(mmu->l2pts); > > + ipu3_mmu_free_page_table(mmu->dummy_l2pt); > > + kfree(mmu->dummy_page); > > Should be free_page(). (Might be already included in your tree as per > https://chromium- > review.googlesource.com/c/chromiumos/third_party/kernel/+/1084522) > Yes, will add above fix to next upstream version. > > + kfree(mmu); > > +} > > + > > +void ipu3_mmu_suspend(struct ipu3_mmu_info *info) { > > + struct ipu3_mmu *mmu = to_ipu3_mmu(info); > > + > > + ipu3_mmu_set_halt(mmu, true); > > +} > > + > > +void ipu3_mmu_resume(struct ipu3_mmu_info *info) { > > + struct ipu3_mmu *mmu = to_ipu3_mmu(info); > > + u32 pteval; > > + > > + ipu3_mmu_set_halt(mmu, true); > > + > > + pteval = IPU3_ADDR2PTE(virt_to_phys(mmu->l1pt)); > > + writel(pteval, mmu->base + REG_L1_PHYS); > > + > > + ipu3_mmu_tlb_invalidate(mmu); > > + ipu3_mmu_set_halt(mmu, false); } > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-mmu.h > > b/drivers/media/pci/intel/ipu3/ipu3-mmu.h > > new file mode 100644 > > index 000000000000..4976187c18f6 > > --- /dev/null > > +++ b/drivers/media/pci/intel/ipu3/ipu3-mmu.h > > @@ -0,0 +1,28 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* Copyright (C) 2018 Intel Corporation */ > > +/* Copyright (C) 2018 Google, Inc. */ > > + > > +#ifndef __IPU3_MMU_H > > +#define __IPU3_MMU_H > > + > > +struct ipu3_mmu_info { > > + dma_addr_t aperture_start; /* First address that can be mapped > */ > > + dma_addr_t aperture_end; /* Last address that can be mapped > */ > > + unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */ > > If documenting the fields, why not use a kerneldoc comment above the > struct instead? > Ack. > Best regards, > Tomasz
