mbox series

[v12,0/7] Add support for the X1830 and fix bugs for X1000.

Message ID 20200527175635.5558-1-zhouyanjie@wanyeetech.com
Headers show
Series Add support for the X1830 and fix bugs for X1000. | expand

Message

Zhou Yanjie May 27, 2020, 5:56 p.m. UTC 
v10->v11:
Split [3/6] in v10 to [3/7] in v11 and [4/7] in v11.

v11->v12:
Use "CLK_OF_DECLARE_DRIVER" instead "CLK_OF_DECLARE",
this modification was mentioned in the comments, but
did not really exist in the patch.
Reported-by: Paul Cercueil <paul@crapouillou.net>

周琰杰 (Zhou Yanjie) (7):
  clk: Ingenic: Remove unnecessary spinlock when reading registers.
  clk: Ingenic: Adjust cgu code to make it compatible with X1830.
  dt-bindings: clock: Add documentation for X1830 bindings.
  dt-bindings: clock: Add X1830 clock bindings.
  clk: Ingenic: Add CGU driver for X1830.
  dt-bindings: clock: Add and reorder ABI for X1000.
  clk: X1000: Add FIXDIV for SSI clock of X1000.

 .../devicetree/bindings/clock/ingenic,cgu.yaml     |   2 +
 drivers/clk/ingenic/Kconfig                        |  10 +
 drivers/clk/ingenic/Makefile                       |   1 +
 drivers/clk/ingenic/cgu.c                          |  28 +-
 drivers/clk/ingenic/cgu.h                          |   4 +
 drivers/clk/ingenic/jz4725b-cgu.c                  |   4 +
 drivers/clk/ingenic/jz4740-cgu.c                   |   4 +
 drivers/clk/ingenic/jz4770-cgu.c                   |   8 +-
 drivers/clk/ingenic/jz4780-cgu.c                   |   3 +
 drivers/clk/ingenic/x1000-cgu.c                    | 118 +++++-
 drivers/clk/ingenic/x1830-cgu.c                    | 443 +++++++++++++++++++++
 include/dt-bindings/clock/x1000-cgu.h              |  64 +--
 include/dt-bindings/clock/x1830-cgu.h              |  55 +++
 13 files changed, 695 insertions(+), 49 deletions(-)
 create mode 100644 drivers/clk/ingenic/x1830-cgu.c
 create mode 100644 include/dt-bindings/clock/x1830-cgu.h

Comments

Stephen Boyd May 28, 2020, 1:12 a.m. UTC | #1 
Quoting Zhou Yanjie (2020-05-27 10:56:35)
> @@ -40,8 +43,47 @@
>  #define OPCR_SPENDN0           BIT(7)
>  #define OPCR_SPENDN1           BIT(6)
>  
> +/* bits within the USBPCR register */
> +#define USBPCR_SIDDQ           BIT(21)
> +#define USBPCR_OTG_DISABLE     BIT(20)
> +
>  static struct ingenic_cgu *cgu;
>  
> +static int x1000_usb_phy_enable(struct clk_hw *hw)
> +{
> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
> +
> +       writel(readl(reg_opcr) | OPCR_SPENDN0, reg_opcr);

Please include linux/io.h for writel/readl.

> +       writel(readl(reg_usbpcr) & ~USBPCR_OTG_DISABLE & ~USBPCR_SIDDQ, reg_usbpcr);
> +       return 0;
> +}
> +
> +static void x1000_usb_phy_disable(struct clk_hw *hw)
> +{
> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
> +
> +       writel(readl(reg_opcr) & ~OPCR_SPENDN0, reg_opcr);
> +       writel(readl(reg_usbpcr) | USBPCR_OTG_DISABLE | USBPCR_SIDDQ, reg_usbpcr);
> +}
> +
> +static int x1000_usb_phy_is_enabled(struct clk_hw *hw)
> +{
> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
> +
> +       return (readl(reg_opcr) & OPCR_SPENDN0) &&
> +               !(readl(reg_usbpcr) & USBPCR_SIDDQ) &&
> +               !(readl(reg_usbpcr) & USBPCR_OTG_DISABLE);
> +}
> +
> +static const struct clk_ops x1000_otg_phy_ops = {
> +       .enable         = x1000_usb_phy_enable,
> +       .disable        = x1000_usb_phy_disable,
> +       .is_enabled     = x1000_usb_phy_is_enabled,
> +};
> +
>  static const s8 pll_od_encoding[8] = {
>         0x0, 0x1, -1, 0x2, -1, -1, -1, 0x3,
>  };
> @@ -277,4 +377,4 @@ static void __init x1000_cgu_init(struct device_node *np)
>  
>         ingenic_cgu_register_syscore_ops(cgu);
>  }
> -CLK_OF_DECLARE(x1000_cgu, "ingenic,x1000-cgu", x1000_cgu_init);
> +CLK_OF_DECLARE_DRIVER(x1000_cgu, "ingenic,x1000-cgu", x1000_cgu_init);

Why does this change to DECLARE_DRIVER? Can you please add a comment
here in the code indicating what other driver is probing this compatible
string?
Stephen Boyd May 28, 2020, 1:13 a.m. UTC | #2 
Quoting Zhou Yanjie (2020-05-27 10:56:33)
> diff --git a/drivers/clk/ingenic/x1830-cgu.c b/drivers/clk/ingenic/x1830-cgu.c
> new file mode 100644
> index 000000000000..29a637f4a2cc
> --- /dev/null
> +++ b/drivers/clk/ingenic/x1830-cgu.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * X1830 SoC CGU driver
> + * Copyright (c) 2019 \u5468\u7430\u6770 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>

Add linux/io.h here.

> +#include <linux/of.h>
> +
> +#include <dt-bindings/clock/x1830-cgu.h>
> +
> +#include "cgu.h"
[...]
> +               return;
> +       }
> +
> +       ingenic_cgu_register_syscore_ops(cgu);
> +}
> +CLK_OF_DECLARE_DRIVER(x1830_cgu, "ingenic,x1830-cgu", x1830_cgu_init);

Same question about why this is DECLARE_DRIVER.
Zhou Yanjie May 28, 2020, 2:13 a.m. UTC | #3 
Hi Stephen,

在 2020/5/28 上午9:12, Stephen Boyd 写道:
> Quoting Zhou Yanjie (2020-05-27 10:56:35)
>> @@ -40,8 +43,47 @@
>>   #define OPCR_SPENDN0           BIT(7)
>>   #define OPCR_SPENDN1           BIT(6)
>>   
>> +/* bits within the USBPCR register */
>> +#define USBPCR_SIDDQ           BIT(21)
>> +#define USBPCR_OTG_DISABLE     BIT(20)
>> +
>>   static struct ingenic_cgu *cgu;
>>   
>> +static int x1000_usb_phy_enable(struct clk_hw *hw)
>> +{
>> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
>> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
>> +
>> +       writel(readl(reg_opcr) | OPCR_SPENDN0, reg_opcr);
> Please include linux/io.h for writel/readl.


Sure, I'll add it.


>> +       writel(readl(reg_usbpcr) & ~USBPCR_OTG_DISABLE & ~USBPCR_SIDDQ, reg_usbpcr);
>> +       return 0;
>> +}
>> +
>> +static void x1000_usb_phy_disable(struct clk_hw *hw)
>> +{
>> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
>> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
>> +
>> +       writel(readl(reg_opcr) & ~OPCR_SPENDN0, reg_opcr);
>> +       writel(readl(reg_usbpcr) | USBPCR_OTG_DISABLE | USBPCR_SIDDQ, reg_usbpcr);
>> +}
>> +
>> +static int x1000_usb_phy_is_enabled(struct clk_hw *hw)
>> +{
>> +       void __iomem *reg_opcr          = cgu->base + CGU_REG_OPCR;
>> +       void __iomem *reg_usbpcr        = cgu->base + CGU_REG_USBPCR;
>> +
>> +       return (readl(reg_opcr) & OPCR_SPENDN0) &&
>> +               !(readl(reg_usbpcr) & USBPCR_SIDDQ) &&
>> +               !(readl(reg_usbpcr) & USBPCR_OTG_DISABLE);
>> +}
>> +
>> +static const struct clk_ops x1000_otg_phy_ops = {
>> +       .enable         = x1000_usb_phy_enable,
>> +       .disable        = x1000_usb_phy_disable,
>> +       .is_enabled     = x1000_usb_phy_is_enabled,
>> +};
>> +
>>   static const s8 pll_od_encoding[8] = {
>>          0x0, 0x1, -1, 0x2, -1, -1, -1, 0x3,
>>   };
>> @@ -277,4 +377,4 @@ static void __init x1000_cgu_init(struct device_node *np)
>>   
>>          ingenic_cgu_register_syscore_ops(cgu);
>>   }
>> -CLK_OF_DECLARE(x1000_cgu, "ingenic,x1000-cgu", x1000_cgu_init);
>> +CLK_OF_DECLARE_DRIVER(x1000_cgu, "ingenic,x1000-cgu", x1000_cgu_init);
> Why does this change to DECLARE_DRIVER? Can you please add a comment
> here in the code indicating what other driver is probing this compatible
> string?


Yes, CGU has some children devices, this is useful for probing children 
devices in the case where the device node is compatible with 
"simple-mfd" (see commit 03d570e1a4dc for a reference).

Thanks and best regards!
Zhou Yanjie May 28, 2020, 2:14 a.m. UTC | #4 
Hi Stephen,

在 2020/5/28 上午9:13, Stephen Boyd 写道:
> Quoting Zhou Yanjie (2020-05-27 10:56:33)
>> diff --git a/drivers/clk/ingenic/x1830-cgu.c b/drivers/clk/ingenic/x1830-cgu.c
>> new file mode 100644
>> index 000000000000..29a637f4a2cc
>> --- /dev/null
>> +++ b/drivers/clk/ingenic/x1830-cgu.c
>> @@ -0,0 +1,443 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * X1830 SoC CGU driver
>> + * Copyright (c) 2019 \u5468\u7430\u6770 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/delay.h>
> Add linux/io.h here.


Sure.


>> +#include <linux/of.h>
>> +
>> +#include <dt-bindings/clock/x1830-cgu.h>
>> +
>> +#include "cgu.h"
> [...]
>> +               return;
>> +       }
>> +
>> +       ingenic_cgu_register_syscore_ops(cgu);
>> +}
>> +CLK_OF_DECLARE_DRIVER(x1830_cgu, "ingenic,x1830-cgu", x1830_cgu_init);
> Same question about why this is DECLARE_DRIVER.


CGU has some children devices, this is useful for probing children 
devices in the case where the device node is compatible with 
"simple-mfd" (see commit 03d570e1a4dc for a reference).

Thanks and best regards!