Subject: [PATCH v6 1/9] drm/hisilicon/hibmc: Add hisilicon hibmc drm master driver

From: [email-masked] (Rongrong Zou)
Date: Fri, 11 Nov 2016 11:10:47 +0800

Hi Sean,

Thanks for reviewing! All comments is helpful.

? 2016/11/11 1:35, Sean Paul ??:
> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <[email-masked]> wrote:
>> Add DRM master driver for Hisilicon Hibmc SoC which used for
>> Out-of-band management. Blow is the general hardware connection,
>> both the Hibmc and the host CPU are on the same mother board.
>>
>> +----------+       +----------+
>> |          | PCIe  |  Hibmc   |
>> |host CPU( |<----->| display  |
>> |arm64,x86)|       |subsystem |
>> +----------+       +----------+
>>
>> Signed-off-by: Rongrong Zou <[email-masked]>
>> ---
>>   drivers/gpu/drm/hisilicon/Kconfig                 |   1 +
>>   drivers/gpu/drm/hisilicon/Makefile                |   1 +
>>   drivers/gpu/drm/hisilicon/hibmc/Kconfig           |   7 +
>>   drivers/gpu/drm/hisilicon/hibmc/Makefile          |   5 +
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 269 ++++++++++++++++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  35 +++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c |  85 +++++++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h |  28 +++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h  | 212 +++++++++++++++++
>>   9 files changed, 643 insertions(+)
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/Kconfig
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/Makefile
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
>>
>> diff --git a/drivers/gpu/drm/hisilicon/Kconfig b/drivers/gpu/drm/hisilicon/Kconfig
>> index 558c61b..2fd2724 100644
>> --- a/drivers/gpu/drm/hisilicon/Kconfig
>> +++ b/drivers/gpu/drm/hisilicon/Kconfig
>> @@ -2,4 +2,5 @@
>>   # hisilicon drm device configuration.
>>   # Please keep this list sorted alphabetically
>>
>> +source "drivers/gpu/drm/hisilicon/hibmc/Kconfig"
>>   source "drivers/gpu/drm/hisilicon/kirin/Kconfig"
>> diff --git a/drivers/gpu/drm/hisilicon/Makefile b/drivers/gpu/drm/hisilicon/Makefile
>> index e3f6d49..c8155bf 100644
>> --- a/drivers/gpu/drm/hisilicon/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/Makefile
>> @@ -2,4 +2,5 @@
>>   # Makefile for hisilicon drm drivers.
>>   # Please keep this list sorted alphabetically
>>
>> +obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc/
>>   obj-$(CONFIG_DRM_HISI_KIRIN) += kirin/
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> new file mode 100644
>> index 0000000..a9af90d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> @@ -0,0 +1,7 @@
>> +config DRM_HISI_HIBMC
>> +       tristate "DRM Support for Hisilicon Hibmc"
>> +       depends on DRM && PCI
>> +
>> +       help
>> +         Choose this option if you have a Hisilicon Hibmc soc chipset.
>> +         If M is selected the module will be called hibmc-drm.
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> new file mode 100644
>> index 0000000..97cf4a0
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -0,0 +1,5 @@
>> +ccflags-y := -Iinclude/drm
>> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o
>> +
>> +obj-$(CONFIG_DRM_HISI_HIBMC)   +=hibmc-drm.o
>
> nit: Improper spacing here

seems a space was missed, thanks.

>
>> +#obj-y += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> new file mode 100644
>> index 0000000..4669d42
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -0,0 +1,269 @@
>> +/* Hisilicon Hibmc SoC drm driver
>> + *
>> + * Based on the bochs drm driver.
>> + *
>> + * Copyright (c) 2016 Huawei Limited.
>> + *
>> + * Author:
>> + *     Rongrong Zou <[email-masked]>
>> + *     Rongrong Zou <[email-masked]>
>> + *     Jianhua Li <[email-masked]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/console.h>
>> +
>> +#include "hibmc_drm_drv.h"
>> +#include "hibmc_drm_regs.h"
>> +#include "hibmc_drm_power.h"
>
> nit: Alphabetize headers

agreed, thanks.

>
>> +
>> +static const struct file_operations hibmc_fops = {
>> +       .owner          = THIS_MODULE,
>> +       .open           = drm_open,
>> +       .release        = drm_release,
>> +       .unlocked_ioctl = drm_ioctl,
>> +#ifdef CONFIG_COMPAT
>
> drm_compat_ioctl is now initialized to NULL, so you can remove the #ifdef
>
understood, will remove it next version.

>> +       .compat_ioctl   = drm_compat_ioctl,
>> +#endif
>> +       .poll           = drm_poll,
>> +       .read           = drm_read,
>> +       .llseek         = no_llseek,
>> +};
>> +
>> +static int hibmc_enable_vblank(struct drm_device *dev, unsigned int pipe)
>> +{
>> +       return 0;
>> +}
>> +
>> +static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>> +{
>> +}
>> +
>> +static struct drm_driver hibmc_driver = {
>> +       .fops                   = &hibmc_fops,
>> +       .name                   = "hibmc",
>> +       .date                   = "20160828",
>> +       .desc                   = "hibmc drm driver",
>> +       .major                  = 1,
>> +       .minor                  = 0,
>> +       .get_vblank_counter     = drm_vblank_no_hw_counter,
>> +       .enable_vblank          = hibmc_enable_vblank,
>> +       .disable_vblank         = hibmc_disable_vblank,
>> +};
>> +
>> +static int hibmc_pm_suspend(struct device *dev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static int hibmc_pm_resume(struct device *dev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static const struct dev_pm_ops hibmc_pm_ops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(hibmc_pm_suspend,
>> +                               hibmc_pm_resume)
>> +};
>> +
>> +static int hibmc_hw_config(struct hibmc_drm_device *hidev)
>> +{
>> +       unsigned int reg;
>> +
>> +       /* On hardware reset, power mode 0 is default. */
>> +       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);
>> +
>> +       /* Reset the memory controller. If the memory controller
>> +        * is not reset in chip,the system might hang when sw accesses
>> +        * the memory.The memory should be resetted after
>> +        * changing the MXCLK.
>> +        */
>> +       reg = readl(hidev->mmio + HIBMC_MISC_CTRL);
>> +       reg &= ~HIBMC_MSCCTL_LOCALMEM_RESET_MASK;
>> +       reg |= HIBMC_MSCCTL_LOCALMEM_RESET(RESET);
>> +       writel(reg, hidev->mmio + HIBMC_MISC_CTRL);
>> +
>> +       reg &= ~HIBMC_MSCCTL_LOCALMEM_RESET_MASK;
>> +       reg |= HIBMC_MSCCTL_LOCALMEM_RESET(NORMAL);
>> +
>> +       writel(reg, hidev->mmio + HIBMC_MISC_CTRL);
>> +
>> +       /* We can add more initialization as needed. */
>> +
>> +       return 0;
>
> Consider using void return type here to simplify error checking in the
> caller, especially since it looks like you aren't checking the return
> code, anyways :)

Yes, you are right. i did not check the return value, so void type
is better, thanks.

>
>> +}
>> +
>> +static int hibmc_hw_map(struct hibmc_drm_device *hidev)
>> +{
>> +       struct drm_device *dev = hidev->dev;
>> +       struct pci_dev *pdev = dev->pdev;
>> +       resource_size_t addr, size, ioaddr, iosize;
>> +
>> +       ioaddr = pci_resource_start(pdev, 1);
>> +       iosize = MB(2);
>> +
>> +       hidev->mmio = ioremap_nocache(ioaddr, iosize);
>
> Use devm_ioremap_nocache to avoid managing the resource directly

agreed, thanks

>
>> +
>
> nit: extra space
>
>> +       if (!hidev->mmio) {
>> +               DRM_ERROR("Cannot map mmio region\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       addr = pci_resource_start(pdev, 0);
>> +       size = MB(32);
>
> Pull size and iosize out into #defines with descriptive names

agreed, thanks

>
>> +
>> +       hidev->fb_map = ioremap(addr, size);
>
> Use devm_ioremap to avoid managing the resource directly.

agreed, thanks

>
>> +       if (!hidev->fb_map) {
>> +               DRM_ERROR("Cannot map framebuffer\n");
>> +               return -ENOMEM;
>> +       }
>> +       hidev->fb_base = addr;
>> +       hidev->fb_size = size;
>> +
>> +       return 0;
>> +}
>> +
>> +static void hibmc_hw_fini(struct hibmc_drm_device *hidev)
>> +{
>> +       if (hidev->mmio)
>> +               iounmap(hidev->mmio);
>> +       if (hidev->fb_map)
>> +               iounmap(hidev->fb_map);
>> +}
>
> You don't need this function if you use the devm variants above

yes, it seems more simple :)

>
>> +
>> +static int hibmc_hw_init(struct hibmc_drm_device *hidev)
>> +{
>> +       int ret;
>> +
>> +       ret = hibmc_hw_map(hidev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       hibmc_hw_config(hidev);
>> +
>> +       return 0;
>> +}
>> +
>> +static int hibmc_unload(struct drm_device *dev)
>> +{
>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>> +
>> +       hibmc_hw_fini(hidev);
>> +       dev->dev_private = NULL;
>> +       return 0;
>> +}
>> +
>> +static int hibmc_load(struct drm_device *dev, unsigned long flags)
>
> flags isn't used anywhere?

Initially, hibmc_load is assigned to ".load", so second parameter is reserved.
In fact it is not used.

>
>> +{
>> +       struct hibmc_drm_device *hidev;
>> +       int ret;
>> +
>> +       hidev = devm_kzalloc(dev->dev, sizeof(*hidev), GFP_KERNEL);
>> +       if (!hidev)
>
> Print error here?

applied, thanks.

>
>> +               return -ENOMEM;
>> +       dev->dev_private = hidev;
>> +       hidev->dev = dev;
>> +
>> +       ret = hibmc_hw_init(hidev);
>> +       if (ret)
>> +               goto err;
>> +
>> +       return 0;
>> +
>> +err:
>> +       hibmc_unload(dev);
>> +       DRM_ERROR("failed to initialize drm driver.\n");
>
> Print the return value

ditto

>
>> +       return ret;
>> +}
>> +
>> +static int hibmc_pci_probe(struct pci_dev *pdev,
>> +                          const struct pci_device_id *ent)
>> +{
>> +       struct drm_device *dev;
>> +       int ret;
>> +
>> +       dev = drm_dev_alloc(&hibmc_driver, &pdev->dev);
>> +       if (!dev)
>
> Print error here

ditto

>
>> +               return -ENOMEM;
>> +
>> +       dev->pdev = pdev;
>> +       pci_set_drvdata(pdev, dev);
>> +
>> +       ret = pci_enable_device(pdev);
>> +       if (ret)
>
> and here, and in other failure paths

ditto

>
>> +               goto err_free;
>> +
>> +       ret = hibmc_load(dev, 0);
>> +       if (ret)
>> +               goto err_disable;
>> +
>> +       ret = drm_dev_register(dev, 0);
>> +       if (ret)
>> +               goto err_unload;
>> +
>> +       return 0;
>> +
>> +err_unload:
>> +       hibmc_unload(dev);
>> +err_disable:
>> +       pci_disable_device(pdev);
>> +err_free:
>> +       drm_dev_unref(dev);
>> +
>> +       return ret;
>> +}
>> +
>> +static void hibmc_pci_remove(struct pci_dev *pdev)
>> +{
>> +       struct drm_device *dev = pci_get_drvdata(pdev);
>> +
>> +       drm_dev_unregister(dev);
>> +       hibmc_unload(dev);
>> +       drm_dev_unref(dev);
>> +}
>> +
>> +static struct pci_device_id hibmc_pci_table[] = {
>> +       {0x19e5, 0x1711, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
>> +       {0,}
>> +};
>> +
>> +static struct pci_driver hibmc_pci_driver = {
>> +       .name =         "hibmc-drm",
>> +       .id_table =     hibmc_pci_table,
>> +       .probe =        hibmc_pci_probe,
>> +       .remove =       hibmc_pci_remove,
>> +       .driver.pm =    &hibmc_pm_ops,
>> +};
>> +
>> +static int __init hibmc_init(void)
>> +{
>> +       return pci_register_driver(&hibmc_pci_driver);
>> +}
>> +
>> +static void __exit hibmc_exit(void)
>> +{
>> +       return pci_unregister_driver(&hibmc_pci_driver);
>> +}
>> +
>> +module_init(hibmc_init);
>> +module_exit(hibmc_exit);
>> +
>> +MODULE_DEVICE_TABLE(pci, hibmc_pci_table);
>> +MODULE_AUTHOR("RongrongZou <[email-masked]>");
>> +MODULE_DESCRIPTION("DRM Driver for Hisilicon Hibmc");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> new file mode 100644
>> index 0000000..0037341
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -0,0 +1,35 @@
>> +/* Hisilicon Hibmc SoC drm driver
>> + *
>> + * Based on the bochs drm driver.
>> + *
>> + * Copyright (c) 2016 Huawei Limited.
>> + *
>> + * Author:
>> + *     Rongrong Zou <[email-masked]>
>> + *     Rongrong Zou <[email-masked]>
>> + *     Jianhua Li <[email-masked]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#ifndef HIBMC_DRM_DRV_H
>> +#define HIBMC_DRM_DRV_H
>> +
>> +#include <drm/drmP.h>
>> +
>> +struct hibmc_drm_device {
>
> nit: Calling this hibmc_drm_priv would probably make things easier to
> read. When I read hibmc_drm_device, it makes me think that it's an
> extension of drm_device, which this isn't.

okay, will replace hibmc_drm_device with hibmc_drm_priv, thanks.

>
>
>> +       /* hw */
>> +       void __iomem   *mmio;
>> +       void __iomem   *fb_map;
>> +       unsigned long  fb_base;
>> +       unsigned long  fb_size;
>> +
>> +       /* drm */
>> +       struct drm_device  *dev;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c
>> new file mode 100644
>> index 0000000..1036542
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c
>
> I don't think you need a new file for these functions. Just stash them
> in hibmc_drm_drv.c

okay, thanks.

>
>> @@ -0,0 +1,85 @@
>> +/* Hisilicon Hibmc SoC drm driver
>> + *
>> + * Based on the bochs drm driver.
>> + *
>> + * Copyright (c) 2016 Huawei Limited.
>> + *
>> + * Author:
>> + *     Rongrong Zou <[email-masked]>
>> + *     Rongrong Zou <[email-masked]>
>> + *     Jianhua Li <[email-masked]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#include "hibmc_drm_drv.h"
>> +#include "hibmc_drm_regs.h"
>> +
>> +/*
>> + * It can operate in one of three modes: 0, 1 or Sleep.
>> + */
>> +void hibmc_set_power_mode(struct hibmc_drm_device *hidev,
>> +                         unsigned int power_mode)
>> +{
>> +       unsigned int control_value = 0;
>> +       void __iomem   *mmio = hidev->mmio;
>> +
>> +       if (power_mode > HIBMC_PW_MODE_CTL_MODE_SLEEP)
>> +               return;
>> +
>> +       control_value = readl(mmio + HIBMC_POWER_MODE_CTRL);
>> +       control_value &= ~HIBMC_PW_MODE_CTL_MODE_MASK;
>> +
>> +       control_value |= HIBMC_PW_MODE_CTL_MODE(power_mode) &
>> +                        HIBMC_PW_MODE_CTL_MODE_MASK;
>> +
>> +    /* Set up other fields in Power Control Register */
>> +       if (power_mode == HIBMC_PW_MODE_CTL_MODE_SLEEP) {
>> +               control_value &= ~HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
>
> You do this in both branches of the conditional

sounds good to me, thanks :)

>
>> +               control_value |= HIBMC_PW_MODE_CTL_OSC_INPUT(0) &
>> +                                HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
>> +       } else {
>> +               control_value &= ~HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
>> +               control_value |= HIBMC_PW_MODE_CTL_OSC_INPUT(1) &
>> +                                HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;
>> +       }
>
> I think you could simplify this by adding a new local.
>
> unsigned int input = 1;
>
> if (power_mode == HIBMC_PW_MODE_CTL_MODE_SLEEP)
>          input = 0;
>
> control_value = readl(mmio + HIBMC_POWER_MODE_CTRL);
> control_value &= ~(HIBMC_PW_MODE_CTL_MODE_MASK |
>                     HIBMC_PW_MODE_CTL_OSC_INPUT_MASK);
> control_value |= HIBMC_PW_MODE_CTL_MODE(power_mode) &
>                   HIBMC_PW_MODE_CTL_MODE_MASK;
> control_value |= HIBMC_PW_MODE_CTL_OSC_INPUT(input) &
>                   HIBMC_PW_MODE_CTL_OSC_INPUT_MASK;

agreed.

>
>
>> +       /* Program new power mode. */
>> +       writel(control_value, mmio + HIBMC_POWER_MODE_CTRL);
>> +}
>> +
>> +static unsigned int hibmc_get_power_mode(struct hibmc_drm_device *hidev)
>> +{
>> +       void __iomem   *mmio = hidev->mmio;
>> +
>> +       return (readl(mmio + HIBMC_POWER_MODE_CTRL) &
>> +               HIBMC_PW_MODE_CTL_MODE_MASK) >> HIBMC_PW_MODE_CTL_MODE_SHIFT;
>
> nit: You're doing a lot of work in the return statement here.

so i need to define an extra variable.

>
>> +}
>> +
>> +void hibmc_set_current_gate(struct hibmc_drm_device *hidev, unsigned int gate)
>> +{
>> +       unsigned int gate_reg;
>> +       unsigned int mode;
>> +       void __iomem   *mmio = hidev->mmio;
>> +
>> +       /* Get current power mode. */
>
> nit: try to avoid comments that don't add value

okay, thanks.

>
>> +       mode = hibmc_get_power_mode(hidev);
>> +
>> +       switch (mode) {
>> +       case HIBMC_PW_MODE_CTL_MODE_MODE0:
>> +               gate_reg = HIBMC_MODE0_GATE;
>> +               break;
>> +
>> +       case HIBMC_PW_MODE_CTL_MODE_MODE1:
>> +               gate_reg = HIBMC_MODE1_GATE;
>> +               break;
>> +
>> +       default:
>> +               gate_reg = HIBMC_MODE0_GATE;
>> +               break;
>> +       }
>> +       writel(gate, mmio + gate_reg);
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h
>> new file mode 100644
>> index 0000000..e20e1aa
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h
>> @@ -0,0 +1,28 @@
>> +/* Hisilicon Hibmc SoC drm driver
>> + *
>> + * Based on the bochs drm driver.
>> + *
>> + * Copyright (c) 2016 Huawei Limited.
>> + *
>> + * Author:
>> + *     Rongrong Zou <[email-masked]>
>> + *     Rongrong Zou <[email-masked]>
>> + *     Jianhua Li <[email-masked]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#ifndef HIBMC_DRM_POWER_H
>> +#define HIBMC_DRM_POWER_H
>> +
>> +#include "hibmc_drm_drv.h"
>> +
>> +void hibmc_set_power_mode(struct hibmc_drm_device *hidev,
>> +                         unsigned int power_mode);
>> +void hibmc_set_current_gate(struct hibmc_drm_device *hidev,
>> +                           unsigned int gate);
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
>> new file mode 100644
>> index 0000000..9c804ca
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h
>> @@ -0,0 +1,212 @@
>> +/* Hisilicon Hibmc SoC drm driver
>> + *
>> + * Based on the bochs drm driver.
>> + *
>> + * Copyright (c) 2016 Huawei Limited.
>> + *
>> + * Author:
>> + *     Rongrong Zou <[email-masked]>
>> + *     Rongrong Zou <[email-masked]>
>> + *     Jianhua Li <[email-masked]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#ifndef HIBMC_DRM_HW_H
>> +#define HIBMC_DRM_HW_H
>> +
>> +#define OFF 0
>> +#define ON  1
>> +#define DISABLE               0
>> +#define ENABLE                1
>
> These are not good names. I think you can probably hardcode the 0's
> and 1's in the code instead of these. However, if you want to use
> them, at least add a HIBMC_ prefix

I like hardcode here, thanks.

>
>> +
>> +/* register definition */
>> +#define HIBMC_MISC_CTRL                                0x4
>> +
>> +#define HIBMC_MSCCTL_LOCALMEM_RESET(x)         ((x) << 6)
>> +#define HIBMC_MSCCTL_LOCALMEM_RESET_MASK       0x40
>> +
>> +#define RESET                0
>> +#define NORMAL               1
>
> Same naming nit here. Add a prefix

ditto.

>
>> +
>> +#define HIBMC_CURRENT_GATE                     0x000040
>> +#define HIBMC_CURR_GATE_DISPLAY(x)             ((x) << 2)
>> +#define HIBMC_CURR_GATE_DISPLAY_MASK           0x4
>> +
>> +#define HIBMC_CURR_GATE_LOCALMEM(x)            ((x) << 1)
>> +#define HIBMC_CURR_GATE_LOCALMEM_MASK          0x2
>> +
>> +#define HIBMC_MODE0_GATE                       0x000044
>> +#define HIBMC_MODE1_GATE                       0x000048
>> +#define HIBMC_POWER_MODE_CTRL                  0x00004C
>> +
>> +#define HIBMC_PW_MODE_CTL_OSC_INPUT(x)         ((x) << 3)
>> +#define HIBMC_PW_MODE_CTL_OSC_INPUT_MASK       0x8
>> +
>> +#define HIBMC_PW_MODE_CTL_MODE(x)              ((x) << 0)
>> +#define HIBMC_PW_MODE_CTL_MODE_MASK            0x03
>> +#define HIBMC_PW_MODE_CTL_MODE_SHIFT           0
>> +
>> +#define HIBMC_PW_MODE_CTL_MODE_MODE0           0
>> +#define HIBMC_PW_MODE_CTL_MODE_MODE1           1
>> +#define HIBMC_PW_MODE_CTL_MODE_SLEEP           2
>> +
>> +#define HIBMC_PANEL_PLL_CTRL                   0x00005C
>> +#define HIBMC_CRT_PLL_CTRL                     0x000060
>> +
>> +#define HIBMC_PLL_CTRL_BYPASS(x)               ((x) << 18)
>> +#define HIBMC_PLL_CTRL_BYPASS_MASK             0x40000
>> +
>> +#define HIBMC_PLL_CTRL_POWER(x)                        ((x) << 17)
>> +#define HIBMC_PLL_CTRL_POWER_MASK              0x20000
>> +
>> +#define HIBMC_PLL_CTRL_INPUT(x)                        ((x) << 16)
>> +#define HIBMC_PLL_CTRL_INPUT_MASK              0x10000
>> +
>> +#define OSC                                    0
>
> Naming

ditto.

>
>> +#define TESTCLK                                        1
>
> This doesn't seem to be used?

will remove it in next version.

>
>> +
>> +#define HIBMC_PLL_CTRL_POD(x)                  ((x) << 14)
>> +#define HIBMC_PLL_CTRL_POD_MASK                        0xC000
>> +
>> +#define HIBMC_PLL_CTRL_OD(x)                   ((x) << 12)
>> +#define HIBMC_PLL_CTRL_OD_MASK                 0x3000
>> +
>> +#define HIBMC_PLL_CTRL_N(x)                    ((x) << 8)
>> +#define HIBMC_PLL_CTRL_N_MASK                  0xF00
>> +
>> +#define HIBMC_PLL_CTRL_M(x)                    ((x) << 0)
>> +#define HIBMC_PLL_CTRL_M_MASK                  0xFF
>> +
>> +#define HIBMC_CRT_DISP_CTL                     0x80200
>> +
>> +#define HIBMC_CRT_DISP_CTL_CRTSELECT(x)                ((x) << 25)
>> +#define HIBMC_CRT_DISP_CTL_CRTSELECT_MASK      0x2000000
>> +
>> +#define CRTSELECT_VGA                0
>> +#define CRTSELECT_CRT                1
>> +
>> +#define HIBMC_CRT_DISP_CTL_CLOCK_PHASE(x)      ((x) << 14)
>> +#define HIBMC_CRT_DISP_CTL_CLOCK_PHASE_MASK    0x4000
>> +
>> +#define PHASE_ACTIVE_HIGH      0
>> +#define PHASE_ACTIVE_LOW       1
>> +
>> +#define HIBMC_CRT_DISP_CTL_VSYNC_PHASE(x)      ((x) << 13)
>> +#define HIBMC_CRT_DISP_CTL_VSYNC_PHASE_MASK    0x2000
>> +
>> +#define HIBMC_CRT_DISP_CTL_HSYNC_PHASE(x)      ((x) << 12)
>> +#define HIBMC_CRT_DISP_CTL_HSYNC_PHASE_MASK    0x1000
>> +
>> +#define HIBMC_CRT_DISP_CTL_TIMING(x)           ((x) << 8)
>> +#define HIBMC_CRT_DISP_CTL_TIMING_MASK         0x100
>> +
>> +#define HIBMC_CRT_DISP_CTL_PLANE(x)            ((x) << 2)
>> +#define HIBMC_CRT_DISP_CTL_PLANE_MASK          4
>> +
>> +#define HIBMC_CRT_DISP_CTL_FORMAT(x)           ((x) << 0)
>> +#define HIBMC_CRT_DISP_CTL_FORMAT_MASK         0x03
>> +
>> +#define HIBMC_CRT_FB_ADDRESS                   0x080204
>> +
>> +#define HIBMC_CRT_FB_WIDTH                     0x080208
>> +#define HIBMC_CRT_FB_WIDTH_WIDTH(x)            ((x) << 16)
>> +#define HIBMC_CRT_FB_WIDTH_WIDTH_MASK          0x3FFF0000
>> +#define HIBMC_CRT_FB_WIDTH_OFFS(x)             ((x) << 0)
>> +#define HIBMC_CRT_FB_WIDTH_OFFS_MASK           0x3FFF
>> +
>> +#define HIBMC_CRT_HORZ_TOTAL                   0x08020C
>> +#define HIBMC_CRT_HORZ_TOTAL_TOTAL(x)          ((x) << 16)
>> +#define HIBMC_CRT_HORZ_TOTAL_TOTAL_MASK                0xFFF0000
>> +
>> +#define HIBMC_CRT_HORZ_TOTAL_DISPLAY_END(x)    ((x) << 0)
>> +#define HIBMC_CRT_HORZ_TOTAL_DISPLAY_END_MASK  0xFFF
>> +
>> +#define HIBMC_CRT_HORZ_SYNC                    0x080210
>> +#define HIBMC_CRT_HORZ_SYNC_WIDTH(x)           ((x) << 16)
>> +#define HIBMC_CRT_HORZ_SYNC_WIDTH_MASK         0xFF0000
>> +
>> +#define HIBMC_CRT_HORZ_SYNC_START(x)           ((x) << 0)
>> +#define HIBMC_CRT_HORZ_SYNC_START_MASK         0xFFF
>> +
>> +#define HIBMC_CRT_VERT_TOTAL                   0x080214
>> +#define HIBMC_CRT_VERT_TOTAL_TOTAL(x)          ((x) << 16)
>> +#define HIBMC_CRT_VERT_TOTAL_TOTAL_MASK                0x7FFF0000
>> +
>> +#define HIBMC_CRT_VERT_TOTAL_DISPLAY_END(x)    ((x) << 0)
>> +#define HIBMC_CRT_VERT_TOTAL_DISPLAY_END_MASK  0x7FF
>> +
>> +#define HIBMC_CRT_VERT_SYNC                    0x080218
>> +#define HIBMC_CRT_VERT_SYNC_HEIGHT(x)          ((x) << 16)
>> +#define HIBMC_CRT_VERT_SYNC_HEIGHT_MASK                0x3F0000
>> +
>> +#define HIBMC_CRT_VERT_SYNC_START(x)           ((x) << 0)
>> +#define HIBMC_CRT_VERT_SYNC_START_MASK         0x7FF
>> +
>> +/* Auto Centering */
>> +#define HIBMC_CRT_AUTO_CENTERING_TL            0x080280
>> +#define HIBMC_CRT_AUTO_CENTERING_TL_TOP(x)     ((x) << 16)
>> +#define HIBMC_CRT_AUTO_CENTERING_TL_TOP_MSK    0x7FF0000
>> +
>> +#define HIBMC_CRT_AUTO_CENTERING_TL_LEFT(x)    ((x) << 0)
>> +#define HIBMC_CRT_AUTO_CENTERING_TL_LEFT_MSK   0x7FF
>> +
>> +#define HIBMC_CRT_AUTO_CENTERING_BR            0x080284
>> +#define HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM(x)  ((x) << 16)
>> +#define HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM_MASK        0x7FF0000
>> +
>> +#define HIBMC_CRT_AUTO_CENTERING_BR_RIGHT(x)   ((x) << 0)
>> +#define HIBMC_CRT_AUTO_CENTERING_BR_RIGHT_MASK 0x7FF
>> +
>> +/* register to control panel output */
>> +#define DISPLAY_CONTROL_HISILE                 0x80288
>> +
>> +#define HIBMC_RAW_INTERRUPT                    0x80290
>> +#define HIBMC_RAW_INTERRUPT_VBLANK(x)          ((x) << 2)
>> +#define HIBMC_RAW_INTERRUPT_VBLANK_MASK                0x4
>> +
>> +#define HIBMC_RAW_INTERRUPT_EN                 0x80298
>> +#define HIBMC_RAW_INTERRUPT_EN_VBLANK(x)       ((x) << 2)
>> +#define HIBMC_RAW_INTERRUPT_EN_VBLANK_MASK     0x4
>> +
>> +/* register and values for PLL control */
>> +#define CRT_PLL1_HS                            0x802a8
>> +#define CRT_PLL1_HS_25MHZ                      0x23d40f02
>> +#define CRT_PLL1_HS_40MHZ                      0x23940801
>> +#define CRT_PLL1_HS_65MHZ                      0x23940d01
>> +#define CRT_PLL1_HS_78MHZ                      0x23540F82
>> +#define CRT_PLL1_HS_74MHZ                      0x23941dc2
>> +#define CRT_PLL1_HS_80MHZ                      0x23941001
>> +#define CRT_PLL1_HS_80MHZ_1152                 0x23540fc2
>> +#define CRT_PLL1_HS_108MHZ                     0x23b41b01
>> +#define CRT_PLL1_HS_162MHZ                     0x23480681
>> +#define CRT_PLL1_HS_148MHZ                     0x23541dc2
>> +#define CRT_PLL1_HS_193MHZ                     0x234807c1
>> +
>> +#define CRT_PLL2_HS                            0x802ac
>> +#define CRT_PLL2_HS_25MHZ                      0x206B851E
>> +#define CRT_PLL2_HS_40MHZ                      0x30000000
>> +#define CRT_PLL2_HS_65MHZ                      0x40000000
>> +#define CRT_PLL2_HS_78MHZ                      0x50E147AE
>> +#define CRT_PLL2_HS_74MHZ                      0x602B6AE7
>> +#define CRT_PLL2_HS_80MHZ                      0x70000000
>> +#define CRT_PLL2_HS_108MHZ                     0x80000000
>> +#define CRT_PLL2_HS_162MHZ                     0xA0000000
>> +#define CRT_PLL2_HS_148MHZ                     0xB0CCCCCD
>> +#define CRT_PLL2_HS_193MHZ                     0xC0872B02
>> +
>> +/* Global macros */
>> +#define RGB(r, g, b) \
>
> Not used anywhere?

will remove it in next version.

>
>> +( \
>> +       (unsigned long)(((r) << 16) | ((g) << 8) | (b)) \
>> +)
>> +
>> +#define PADDING(align, data) (((data) + (align) - 1) & (~((align) - 1)))
>> +
>
> This is only used in hibmc_drm_de.c, move it in there. It might also
> be nice to make it a type-checked function while you're at it.

agreed, thanks.

>
>
>> +#define MB(x) ((x) << 20)
>> +
>
> This is only used in 2 places, I think you can just hardcode the value
> in their respective #defines

agreed, thanks.

Regards,
Rongrong.

>
>> +#endif
>> --
>> 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
>
> .
>