Re: [PATCH v3 1/8] drm/i915/skl: Add support to load SKL CSR firmware

Animesh Manna
2015-04-13T12:11:55Z

On 04/13/2015 04:33 PM, Imre Deak wrote:
> On ma, 2015-04-13 at 15:54 +0530, Animesh Manna wrote:
>> From: "A.Sunil Kamath" <sunil.kamath@intel.com>
>>
>> Display Context Save and Restore support is needed for
>> various SKL Display C states like DC5, DC6.
>>
>> This implementation is added based on first version of DMC CSR program
>> that we received from h/w team.
>>
>> Here we are using request_firmware based design.
>> Finally this firmware should end up in linux-firmware tree.
>>
>> For SKL platform its mandatory to ensure that we load this
>> csr program before enabling DC states like DC5/DC6.
>>
>> As CSR program gets reset on various conditions, we should ensure
>> to load it during boot and in future change to be added to load
>> this system resume sequence too.
>>
>> v1: Initial relese as RFC patch
>>
>> v2: Design change as per Daniel, Damien and Shobit's review comments
>> request firmware method followed.
>>
>> v3: Some optimization and functional changes.
>> Pulled register defines into drivers/gpu/drm/i915/i915_reg.h
>> Used kmemdup to allocate and duplicate firmware content.
>> Ensured to free allocated buffer.
>>
>> v4: Modified as per review comments from Satheesh and Daniel
>> Removed temporary buffer.
>> Optimized number of writes by replacing I915_WRITE with I915_WRITE64.
>>
>> v5:
>> Modified as per review comemnts from Damien.
>> - Changed name for functions and firmware.
>> - Introduced HAS_CSR.
>> - Reverted back previous change and used csr_buf with u8 size.
>> - Using cpu_to_be64 for endianness change.
>>
>> Modified as per review comments from Imre.
>> - Modified registers and macro names to be a bit closer to bspec terminology
>> and the existing register naming in the driver.
>> - Early return for non SKL platforms in intel_load_csr_program function.
>> - Added locking around CSR program load function as it may be called
>> concurrently during system/runtime resume.
>> - Releasing the fw before loading the program for consistency
>> - Handled error path during f/w load.
>>
>> v6: Modified as per review comments from Imre.
>> - Corrected out_freecsr sequence.
>>
>> v7: Modified as per review comments from Imre.
>> Fail loading fw if fw->size%8!=0.
>>
>> v8: Rebase to latest.
>>
>> v9: Rebase on top of -nightly (Damien)
>>
>> v10: Enabled support for dmc firmware ver 1.0.
>> According to ver 1.0 in a single binary package all the firmware's that are
>> required for different stepping's of the product will be stored. The package
>> contains the css header, followed by the package header and the actual dmc
>> firmwares. Package header contains the firmware/stepping mapping table and
>> the corresponding firmware offsets to the individual binaries, within the
>> package. Each individual program binary contains the header and the payload
>> sections whose size is specified in the header section. This changes are done
>> to extract the specific firmaware from the package. (Animesh)
>>
>> v11: Modified as per review comemnts from Imre.
>> - Added code comment from bpec for header structure elements.
>> - Added __packed to avoid structure padding.
>> - Added helper functions for stepping and substepping info.
>> - Added code comment for CSR_MAX_FW_SIZE.
>> - Disabled BXT firmware loading, will be enabled with dmc 1.0 support.
>> - Changed skl_stepping_info based on bspec, earlier used from config DB.
>> - Removed duplicate call of cpu_to_be* from intel_csr_load_program function.
>> - Used cpu_to_be32 instead of cpu_to_be64 as firmware binary in dword aligned.
>> - Added sanity check for header length.
>> - Added sanity check for mmio address got from firmware binary.
>> - kmalloc done separately for dmc header and dmc firmware. (Animesh)
>>
>> v12: Modified as per review comemnts from Imre.
>> - Corrected the typo error in skl stepping info structure.
>> - Added out-of-bound access for skl_stepping_info.
>> - Sanity check for mmio address modified.
>> - Sanity check added for stepping and substeppig.
>> - Modified the intel_dmc_info structure, cache only the required header info. (Animesh)
>>
>> v13: clarify firmware load error message.
>> The reason for a firmware loading failure can be obscure if the driver
>> is built-in. Provide an explanation to the user about the likely reason for
>> the failure and how to resolve it. (Imre)
>>
>> v14: Suggested by Jani.
>> - fix s/I915/CONFIG_DRM_I915/ typo
>> - add fw_path to the firmware object instead of using a static ptr (Jani)
>>
>> v15:
>> 1) Changed the firmware name as dmc_gen9.bin, everytime for a new firmware version a symbolic link
>> with same name will help not to build kernel again.
>> 2) Changes done as per review comments from Imre.
>> - Error check removed for intel_csr_ucode_init.
>> - Moved csr-specific data structure to intel_csr.h and optimization done on structure definition.
>> - fw->data used directly for parsing the header info & memory allocation
>> only done separately for payload. (Animesh)
>>
>> v16:
>> No need for out_regs label in i915_driver_load(), so removed it. (Animesh)
>>
>> Issue: VIZ-2569
>> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>> drivers/gpu/drm/i915/Makefile | 3 +-
>> drivers/gpu/drm/i915/i915_dma.c | 11 +-
>> drivers/gpu/drm/i915/i915_drv.c | 20 ++++
>> drivers/gpu/drm/i915/i915_drv.h | 17 +++
>> drivers/gpu/drm/i915/intel_csr.c | 234 +++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_csr.h | 164 +++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_drv.h | 5 +
>> 7 files changed, 451 insertions(+), 3 deletions(-)
>> create mode 100644 drivers/gpu/drm/i915/intel_csr.c
>> create mode 100644 drivers/gpu/drm/i915/intel_csr.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index a69002e..5238deb 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -12,7 +12,8 @@ i915-y := i915_drv.o \
>> i915_suspend.o \
>> i915_sysfs.o \
>> intel_pm.o \
>> - intel_runtime_pm.o
>> + intel_runtime_pm.o \
>> + intel_csr.o
>>
>> i915-$(CONFIG_COMPAT) += i915_ioc32.o
>> i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index e44116f..a238889 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -816,6 +816,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>> spin_lock_init(&dev_priv->mmio_flip_lock);
>> mutex_init(&dev_priv->dpio_lock);
>> mutex_init(&dev_priv->modeset_restore_lock);
>> + mutex_init(&dev_priv->csr_lock);
>>
>> intel_pm_setup(dev);
>>
>> @@ -861,9 +862,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>
>> intel_uncore_init(dev);
>>
>> + /* Load CSR Firmware for SKL */
>> + intel_csr_ucode_init(dev);
>> +
>> ret = i915_gem_gtt_init(dev);
>> if (ret)
>> - goto out_regs;
>> + goto out_freecsr;
>>
>> /* WARNING: Apparently we must kick fbdev drivers before vgacon,
>> * otherwise the vga fbdev driver falls over. */
>> @@ -1033,7 +1037,8 @@ out_mtrrfree:
>> io_mapping_free(dev_priv->gtt.mappable);
>> out_gtt:
>> i915_global_gtt_cleanup(dev);
>> -out_regs:
>> +out_freecsr:
>> + intel_csr_ucode_fini(dev);
>> intel_uncore_fini(dev);
>> pci_iounmap(dev->pdev, dev_priv->regs);
>> put_bridge:
>> @@ -1113,6 +1118,8 @@ int i915_driver_unload(struct drm_device *dev)
>> mutex_unlock(&dev->struct_mutex);
>> i915_gem_cleanup_stolen(dev);
>>
>> + intel_csr_ucode_fini(dev);
>> +
>> intel_teardown_gmbus(dev);
>> intel_teardown_mchbar(dev);
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index c3fdbb0..acd0e2b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -556,6 +556,26 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
>> cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work);
>> }
>>
>> +void i915_firmware_load_error_print(const char *fw_path, int err)
>> +{
>> + DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err);
>> +
>> + /*
>> + * If the reason is not known assume -ENOENT since that's the most
>> + * usual failure mode.
>> + */
>> + if (!err)
>> + err = -ENOENT;
>> +
>> + if (!(IS_BUILTIN(CONFIG_DRM_I915) && err == -ENOENT))
>> + return;
>> +
>> + DRM_ERROR(
>> + "The driver is built-in, so to load the firmware you need to\n"
>> + "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n"
>> + "in your initrd/initramfs image.\n");
>> +}
>> +
>> static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
>> {
>> struct drm_device *dev = dev_priv->dev;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 47be4a5..90e47a9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -667,6 +667,15 @@ struct intel_uncore {
>> #define for_each_fw_domain(domain__, dev_priv__, i__) \
>> for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
>>
>> +struct intel_csr {
>> + const char *fw_path;
>> + __be32 *dmc_payload;
>> + uint32_t dmc_fw_size;
>> + uint32_t mmio_count;
>> + uint32_t mmioaddr[8];
>> + uint32_t mmiodata[8];
>> +};
>> +
>> #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
>> func(is_mobile) sep \
>> func(is_i85x) sep \
>> @@ -1573,6 +1582,11 @@ struct drm_i915_private {
>>
>> struct i915_virtual_gpu vgpu;
>>
>> + struct intel_csr csr;
>> +
>> + /* Display CSR-related protection */
>> + struct mutex csr_lock;
>> +
>> struct intel_gmbus gmbus[GMBUS_NUM_PINS];
>>
>> /** gmbus_mutex protects against concurrent usage of the single hw gmbus
>> @@ -2425,6 +2439,8 @@ struct drm_i915_cmd_table {
>> #define HAS_RC6(dev) (INTEL_INFO(dev)->gen >= 6)
>> #define HAS_RC6p(dev) (INTEL_INFO(dev)->gen == 6 || IS_IVYBRIDGE(dev))
>>
>> +#define HAS_CSR(dev) (IS_SKYLAKE(dev))
>> +
>> #define INTEL_PCH_DEVICE_ID_MASK 0xff00
>> #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00
>> #define INTEL_PCH_CPT_DEVICE_ID_TYPE 0x1c00
>> @@ -2515,6 +2531,7 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
>> extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
>> int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>> void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
>> +void i915_firmware_load_error_print(const char *fw_path, int err);
>>
>> /* i915_irq.c */
>> void i915_queue_hangcheck(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> new file mode 100644
>> index 0000000..ab9b16b
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -0,0 +1,234 @@
>> +/*
>> + * Copyright © 2014 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +#include <linux/firmware.h>
>> +#include "i915_drv.h"
>> +#include "i915_reg.h"
>> +#include "intel_csr.h"
>> +
>> +static struct stepping_info skl_stepping_info[] = {
> Nitpick: this should be const.
>
>> + {'A', '0'}, {'B', '0'}, {'C', '0'},
>> + {'D', '0'}, {'E', '0'}, {'F', '0'},
>> + {'G', '0'}, {'H', '0'}, {'I', '0'}
>> +};
>> +
>> +static char intel_get_stepping(struct drm_device *dev)
>> +{
>> + if (IS_SKYLAKE(dev) && (dev->pdev->revision <
>> + ARRAY_SIZE(skl_stepping_info)))
>> + return skl_stepping_info[dev->pdev->revision].stepping;
> Nitpick: at this point it's just
> if (dev->pdev->revision < 9)
> return 'A' + dev->pdev->revision;
>
> w/o the need for skl_stepping_info.

If we have different firmwares for different substepping for example A0,A1,A3 then stepping will
be same but substepping will be different and I assume revision_id also will be different. And
if we want to put some logic like ('A' + dev->pdev->revision) then we can do it but should be
based on bspec, Why I am telling because if later we get some table and not able to derive
a common logic then again may need this kind of lookup table.

And using lookuo table comes with advantage
- to extend support for future platform only have to add new structure or change existing
structure to incorporate any future changes in stepping info which got from bspec.
- time complexity will be less.

Yes agree space might be an issue. Suggest me trade off between time and space.

>
>
>> + else
>> + return -ENODATA;
>> +}
>> +
>> +static char intel_get_substepping(struct drm_device *dev)
>> +{
>> + if (IS_SKYLAKE(dev) && (dev->pdev->revision <
>> + ARRAY_SIZE(skl_stepping_info)))
>> + return skl_stepping_info[dev->pdev->revision].substepping;
> and this one is simply
> return 0;

For now all values are 0 for skl, but implementation done thinking of supporting future changes.
We may have multiple firmware for different substepping.

>> + else
>> + return -ENODATA;
>> +}
> Missing newline.
>
>> +void intel_csr_load_program(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + __be32 *payload = dev_priv->csr.dmc_payload;
>> + uint32_t i, fw_size;
>> +
>> + if (!IS_GEN9(dev)) {
>> + DRM_ERROR("No CSR support available for this platform\n");
>> + return;
>> + }
>> +
>> + mutex_lock(&dev_priv->csr_lock);
>> + /* fw_size is dwords, converting it to bytes and nearest 8 multiple. */
> The above comment is stale now.

Agree.

>
>> + fw_size = dev_priv->csr.dmc_fw_size;
>> + for (i = 0; i < fw_size; i++)
>> + I915_WRITE(CSR_PROGRAM_BASE + i * 4,
>> + (u32 __force)payload[i]);
>> +
>> + for (i = 0; i < dev_priv->csr.mmio_count; i++) {
>> + I915_WRITE(dev_priv->csr.mmioaddr[i],
>> + dev_priv->csr.mmiodata[i]);
>> + }
>> + mutex_unlock(&dev_priv->csr_lock);
>> +}
>> +
>> +static void finish_csr_load(const struct firmware *fw, void *context)
>> +{
>> + struct drm_i915_private *dev_priv = context;
>> + struct drm_device *dev = dev_priv->dev;
>> + struct intel_css_header *css_header;
>> + struct intel_package_header *package_header;
>> + struct intel_dmc_header *dmc_header;
>> + struct intel_csr *csr = &dev_priv->csr;
>> + char stepping = intel_get_stepping(dev);
>> + char substepping = intel_get_substepping(dev);
>> + uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>> + uint32_t i;
>> + __be32 *dmc_payload;
>> +
>> + if (!fw) {
>> + i915_firmware_load_error_print(csr->fw_path, 0);
>> + goto out;
>> + }
>> +
>> + if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
>> + DRM_ERROR("Unknown stepping info, firmware loading failed\n");
>> + goto out;
>> + }
>> +
>> + /* Extract CSS Header information*/
>> + css_header = (struct intel_css_header *)fw->data;
>> + if (sizeof(struct intel_css_header) !=
>> + (css_header->header_len * 4)) {
>> + DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
>> + (css_header->header_len * 4));
>> + goto out;
>> + }
>> + readcount += sizeof(struct intel_css_header);
>> +
>> + /* Extract Package Header information*/
>> + package_header = (struct intel_package_header *)
>> + &fw->data[readcount];
>> + if (sizeof(struct intel_package_header) !=
>> + (package_header->header_len * 4)) {
>> + DRM_ERROR("Firmware has wrong package header length %u bytes\n",
>> + (package_header->header_len * 4));
>> + goto out;
>> + }
>> + readcount += sizeof(struct intel_package_header);
>> +
>> + /* Search for dmc_offset to find firware binary. */
>> + for (i = 0; i < package_header->num_entries; i++) {
>> + if (package_header->fw_info[i].substepping == '*' &&
>> + stepping == package_header->fw_info[i].stepping) {
>> + dmc_offset = package_header->fw_info[i].offset;
>> + break;
>> + } else if (stepping == package_header->fw_info[i].stepping &&
>> + substepping == package_header->fw_info[i].substepping) {
>> + dmc_offset = package_header->fw_info[i].offset;
>> + break;
>> + } else if (package_header->fw_info[i].stepping == '*' &&
>> + package_header->fw_info[i].substepping == '*')
>> + dmc_offset = package_header->fw_info[i].offset;
>> + }
>> + if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
>> + DRM_ERROR("Firmware not supported for %c stepping\n", stepping);
>> + goto out;
>> + }
>> + readcount += dmc_offset;
>> +
>> + /* Extract dmc_header information. */
>> + dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
>> + if (sizeof(struct intel_dmc_header) != (dmc_header->header_len)) {
>> + DRM_ERROR("Firmware has wrong dmc header length %u bytes\n",
>> + (dmc_header->header_len));
>> + goto out;
>> + }
>> + readcount += sizeof(struct intel_dmc_header);
>> +
>> + /* Cache the dmc header info. */
>> + if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
>> + DRM_ERROR("Firmware has wrong mmio count %u\n",
>> + dmc_header->mmio_count);
>> + goto out;
>> + }
>> + csr->mmio_count = dmc_header->mmio_count;
>> + for (i = 0; i < dmc_header->mmio_count; i++) {
>> + if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE &&
>> + dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {
>> + DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
>> + dmc_header->mmioaddr[i]);
>> + goto out;
>> + }
>> + csr->mmioaddr[i] = dmc_header->mmioaddr[i];
>> + csr->mmiodata[i] = dmc_header->mmiodata[i];
>> + }
>> +
>> + /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
>> + nbytes = dmc_header->fw_size * 4;
>> + if (nbytes > CSR_MAX_FW_SIZE) {
>> + DRM_ERROR("CSR firmware too big (%u) bytes\n", nbytes);
>> + goto out;
>> + }
>> + csr->dmc_fw_size = dmc_header->fw_size;
>> +
>> + csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
>> + if (!csr->dmc_payload) {
>> + DRM_ERROR("Memory allocation failed for dmc payload\n");
>> + goto out;
>> + }
>> +
>> + dmc_payload = csr->dmc_payload;
>> + for (i = 0; i < dmc_header->fw_size; i++) {
>> + uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
>> + /*
>> + * The firmware payload is an array of 32 bit words stored in
>> + * little-endian format in the firmware image and programmed
>> + * as 32 bit big-endian format to memory.
>> + */
>> + dmc_payload[i] = cpu_to_be32(*tmp);
>> + }
>> +
>> + /* load csr program during system boot, as needed for DC states */
>> + intel_csr_load_program(dev);
>> +out:
>> + release_firmware(fw);
>> +}
>> +
>> +void intel_csr_ucode_init(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct intel_csr *csr = &dev_priv->csr;
>> + int ret;
>> +
>> + if (!HAS_CSR(dev))
>> + return;
>> +
>> + if (IS_SKYLAKE(dev))
>> + csr->fw_path = I915_CSR_SKL;
>> + else {
>> + DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
>> + return;
>> + }
>> +
>> + /* CSR supported for platform, load firmware */
>> + ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
>> + &dev_priv->dev->pdev->dev,
>> + GFP_KERNEL, dev_priv,
>> + finish_csr_load);
>> + if (ret)
>> + i915_firmware_load_error_print(csr->fw_path, ret);
>> +
>> +}
>> +
>> +void intel_csr_ucode_fini(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + if (!HAS_CSR(dev))
>> + return;
>> +
>> + kfree(dev_priv->csr.dmc_payload);
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_csr.h b/drivers/gpu/drm/i915/intel_csr.h
>> new file mode 100644
>> index 0000000..c2a5a53
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_csr.h
>> @@ -0,0 +1,164 @@
>> +/*
>> + * Copyright © 2014 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#ifndef _INTEL_CSR_H_
>> +#define _INTEL_CSR_H_
>> +
>> +#define I915_CSR_SKL "i915/dmc_gen9.bin"
>> +
>> +MODULE_FIRMWARE(I915_CSR_SKL);
>> +
>> +/*
>> +* SKL CSR registers for DC5 and DC6
>> +*/
>> +#define CSR_PROGRAM_BASE 0x80000
>> +#define CSR_HEADER_OFFSET 128
>> +#define CSR_SSP_BASE_ADDR_GEN9 0x00002FC0
>> +#define CSR_HTP_ADDR_SKL 0x00500034
>> +#define CSR_SSP_BASE 0x8F074
>> +#define CSR_HTP_SKL 0x8F004
>> +#define CSR_LAST_WRITE 0x8F034
>> +#define CSR_LAST_WRITE_VALUE 0xc003b400
>> +/* MMIO address range for CSR program (0x80000 - 0x82FFF) */
>> +#define CSR_MAX_FW_SIZE 0x2FFF
>> +#define CSR_DEFAULT_FW_OFFSET 0xFFFFFFFF
>> +#define CSR_MMIO_START_RANGE 0x80000
>> +#define CSR_MMIO_END_RANGE 0x8FFFF
>> +
>> +struct intel_css_header {
>> + /* 0x09 for DMC */
>> + uint32_t module_type;
>> +
>> + /* Includes the DMC specific header in dwords */
>> + uint32_t header_len;
>> +
>> + /* always value would be 0x10000 */
>> + uint32_t header_ver;
>> +
>> + /* Not used */
>> + uint32_t module_id;
>> +
>> + /* Not used */
>> + uint32_t module_vendor;
>> +
>> + /* in YYYYMMDD format */
>> + uint32_t date;
>> +
>> + /* Size in dwords (CSS_Headerlen + PackageHeaderLen + dmc FWsLen)/4 */
>> + uint32_t size;
>> +
>> + /* Not used */
>> + uint32_t key_size;
>> +
>> + /* Not used */
>> + uint32_t modulus_size;
>> +
>> + /* Not used */
>> + uint32_t exponent_size;
>> +
>> + /* Not used */
>> + uint32_t reserved1[12];
>> +
>> + /* Major Minor */
>> + uint32_t version;
>> +
>> + /* Not used */
>> + uint32_t reserved2[8];
>> +
>> + /* Not used */
>> + uint32_t kernel_header_info;
>> +} __packed;
>> +
>> +struct intel_fw_info {
>> + uint16_t reserved1;
>> +
>> + /* Stepping (A, B, C, ..., *). * is a wildcard */
>> + char stepping;
>> +
>> + /* Sub-stepping (0, 1, ..., *). * is a wildcard */
>> + char substepping;
>> +
>> + uint32_t offset;
>> + uint32_t reserved2;
>> +} __packed;
>> +
>> +struct intel_package_header {
>> + /* DMC container header length in dwords */
>> + unsigned char header_len;
>> +
>> + /* always value would be 0x01 */
>> + unsigned char header_ver;
>> +
>> + unsigned char reserved[10];
>> +
>> + /* Number of valid entries in the FWInfo array below */
>> + uint32_t num_entries;
>> +
>> + struct intel_fw_info fw_info[20];
>> +} __packed;
>> +
>> +struct intel_dmc_header {
>> + /* always value would be 0x40403E3E */
>> + uint32_t signature;
>> +
>> + /* DMC binary header length */
>> + unsigned char header_len;
>> +
>> + /* 0x01 */
>> + unsigned char header_ver;
>> +
>> + /* Reserved */
>> + uint16_t dmcc_ver;
>> +
>> + /* Major, Minor */
>> + uint32_t project;
>> +
>> + /* Firmware program size (excluding header) in dwords */
>> + uint32_t fw_size;
>> +
>> + /* Major Minor version */
>> + uint32_t fw_version;
>> +
>> + /* Number of valid MMIO cycles present. */
>> + uint32_t mmio_count;
>> +
>> + /* MMIO address */
>> + uint32_t mmioaddr[8];
>> +
>> + /* MMIO data */
>> + uint32_t mmiodata[8];
>> +
>> + /* FW filename */
>> + unsigned char dfile[32];
>> +
>> + uint32_t reserved1[2];
>> +} __packed;
>> +
>> +struct stepping_info {
>> + char stepping;
>> + char substepping;
>> +} __packed;
> stepping_info doesn't need to be packed.

To avoid structure padding used __packed which will save some memory.

>
> What I meant is to add all these to intel_csr.c. I don't think there is
> any reason for a new header file unless you need to share something
> among multiple .c modules.
>
> --Imre

intel_runtime_pm.c is using few macro which is defined in this header file in later patches.
Then we can move all macro definition to i915_drv.h or else can put few macro in intel_csr.c
and few in i915_drv.h. Give me your suggestion.

Regards,
Animesh

>
>> +
>> +
>> +#endif /* _INTEL_CSR_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 7a0aa24..f3a2d88 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1062,6 +1062,11 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>> unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>> struct drm_i915_gem_object *obj);
>>
>> +/* intel_csr.c */
>> +void intel_csr_ucode_init(struct drm_device *dev);
>> +void intel_csr_load_program(struct drm_device *dev);
>> +void intel_csr_ucode_fini(struct drm_device *dev);
>> +
>> /* intel_dp.c */
>> void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
>> bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,