Re: [PATCH 16/26] agp/intel: add ValleyView AGP driver

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, Mar 22, 2012 at 02:38:58PM -0700, Jesse Barnes wrote:
> But don't bind the PCI ID yet.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/char/agp/intel-agp.c |    1 +
>  drivers/char/agp/intel-agp.h |    3 +++
>  drivers/char/agp/intel-gtt.c |   23 +++++++++++++++++++++++
>  3 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
> index 962e75d..74c2d92 100644
> --- a/drivers/char/agp/intel-agp.c
> +++ b/drivers/char/agp/intel-agp.c
> @@ -907,6 +907,7 @@ static struct pci_device_id agp_intel_pci_table[] = {
>  	ID(PCI_DEVICE_ID_INTEL_IVYBRIDGE_HB),
>  	ID(PCI_DEVICE_ID_INTEL_IVYBRIDGE_M_HB),
>  	ID(PCI_DEVICE_ID_INTEL_IVYBRIDGE_S_HB),
> +	ID(PCI_DEVICE_ID_INTEL_VALLEYVIEW_HB),
>  	{ }
>  };
>  
> diff --git a/drivers/char/agp/intel-agp.h b/drivers/char/agp/intel-agp.h
> index 5da67f1..41d9ee1 100644
> --- a/drivers/char/agp/intel-agp.h
> +++ b/drivers/char/agp/intel-agp.h
> @@ -96,6 +96,7 @@
>  #define G4x_GMCH_SIZE_VT_2M	(G4x_GMCH_SIZE_2M | G4x_GMCH_SIZE_VT_EN)
>  
>  #define GFX_FLSH_CNTL		0x2170 /* 915+ */
> +#define GFX_FLSH_CNTL_VLV	0x101008
>  
>  #define I810_DRAM_CTL		0x3000
>  #define I810_DRAM_ROW_0		0x00000001
> @@ -234,6 +235,8 @@
>  #define PCI_DEVICE_ID_INTEL_IVYBRIDGE_M_GT2_IG		0x0166
>  #define PCI_DEVICE_ID_INTEL_IVYBRIDGE_S_HB		0x0158  /* Server */
>  #define PCI_DEVICE_ID_INTEL_IVYBRIDGE_S_GT1_IG		0x015A
> +#define PCI_DEVICE_ID_INTEL_VALLEYVIEW_HB		0x0F00 /* VLV1 */
> +#define PCI_DEVICE_ID_INTEL_VALLEYVIEW_IG		0x0F30
>  
>  int intel_gmch_probe(struct pci_dev *pdev,
>  			       struct agp_bridge_data *bridge);
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 269cb02..ccc0045 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -1179,6 +1179,20 @@ static void gen6_write_entry(dma_addr_t addr, unsigned int entry,
>  	writel(addr | pte_flags, intel_private.gtt + entry);
>  }
>  
> +static void valleyview_write_entry(dma_addr_t addr, unsigned int entry,
> +				   unsigned int flags)
> +{
> +	u32 pte_flags;
> +
> +	pte_flags = GEN6_PTE_UNCACHED | I810_PTE_VALID;
> +
> +	/* gen6 has bit11-4 for physical addr bit39-32 */
The comment about gen6 should probably go.
> +	addr |= (addr >> 28) & 0xff0;
> +	writel(addr | pte_flags, intel_private.gtt + entry);
> +
> +	writel(1, intel_private.registers + GFX_FLSH_CNTL_VLV);
I think a comment here is very well deserved.

> +}
> +
>  static void gen6_cleanup(void)
>  {
>  }
> @@ -1359,6 +1373,15 @@ static const struct intel_gtt_driver sandybridge_gtt_driver = {
>  	.check_flags = gen6_check_flags,
>  	.chipset_flush = i9xx_chipset_flush,
>  };
> +static const struct intel_gtt_driver valleyview_gtt_driver = {
> +	.gen = 7,
> +	.setup = i9xx_setup,
> +	.cleanup = gen6_cleanup,
> +	.write_entry = valleyview_write_entry,
> +	.dma_mask_size = 40,
> +	.check_flags = gen6_check_flags,
> +	.chipset_flush = i9xx_chipset_flush,
> +};

I'm pretty torn about whether or not we actually want to use .gen=7
(both here and in the other patch). But if it's what you think is best,
let's do it.

do you really want gen6_check_flags? or i830_check_flags?
FWIW, I have no clue why we even need check_flags()

>  
>  /* Table to describe Intel GMCH and AGP/PCIE GART drivers.  At least one of
>   * driver and gmch_driver must be non-null, and find_gmch will determine

Assuming you consider the above:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>