Re: [PATCH v9 2/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Stephen,

On 2016/2/26 7:42, Stephen Boyd wrote:
>> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
>> new file mode 100644
>> index 000..50e00e7
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/reset.c
>> +
>> +int hisi_reset_init(struct device_node *np)
>> +{
>> +struct hisi_reset_controller *rstc;
>> +
>> +rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
>> +if (!rstc)
>> +return -ENOMEM;
>> +
>> +rstc->membase = of_iomap(np, 0);
> 
> Any reason why we can't pass the platform device here and map the
> register space with platform device APIs?
> 
This function can be called by other clock drivers except clk-hi3519. Some
clock drivers may not be registered as platform drivers. Moreover this function
may be called early even before platform_bus_init.

Thank you!

Regards,
Jiancheng.

Re: [PATCH v9 2/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Stephen,
Thank you for your advice.  I'll fixed them in next version.

Regards,

Jiancheng.

On 2016/2/26 7:42, Stephen Boyd wrote:
> On 02/22, Jiancheng Xue wrote:
>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt 
>> b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> new file mode 100644
>> index 000..2d23950
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> @@ -0,0 +1,46 @@
>> +Example: CRG nodes
>> +CRG: clock-reset-controller@1201 {
>> +compatible = "hisilicon,hi3519-crg";
> 
> Indentation is off here.
> 
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@1211 {
>> +compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xe4 0>;
>> +};
>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> index e434854..296371f 100644
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
>> @@ -1,3 +1,11 @@
>> +config COMMON_CLK_HI3519
>> +tristate "Hi3519 Clock Driver"
>> +depends on ARCH_HISI || COMPILE_TEST
>> +select RESET_HISI
>> +default y
> 
> default ARCH_HISI
> 
>> +help
>> +  Build the clock driver for hi3519.
>> +
>> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
>> new file mode 100644
>> index 000..50e00e7
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/reset.c
>> +
>> +int hisi_reset_init(struct device_node *np)
>> +{
>> +struct hisi_reset_controller *rstc;
>> +
>> +rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
>> +if (!rstc)
>> +return -ENOMEM;
>> +
>> +rstc->membase = of_iomap(np, 0);
> 
> Any reason why we can't pass the platform device here and map the
> register space with platform device APIs?
> 
>> +if (!rstc->membase)
>> +return -EINVAL;
>> +
>> +spin_lock_init(&rstc->lock);
>> +
>> +rstc->rcdev.owner = THIS_MODULE;
>> +rstc->rcdev.ops = &hisi_reset_ops;
>> +rstc->rcdev.of_node = np;
>> +rstc->rcdev.of_reset_n_cells = 2;
>> +rstc->rcdev.of_xlate = hisi_reset_of_xlate;
>> +
>> +return reset_controller_register(&rstc->rcdev);
>> +}
>> +EXPORT_SYMBOL(hisi_reset_init);
> 
> Why not GPL?
> 

Re: [PATCH v9 2/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi all,

I'll fix this warning in next version.

Other comments will be appreciated!

Thank you!

On 2016/2/22 18:01, kbuild test robot wrote:
> Hi Jiancheng,
> 
> [auto build test WARNING on clk/clk-next]
> [also build test WARNING on v4.5-rc5 next-20160222]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improving the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Jiancheng-Xue/ARM-hisi-Add-initial-support-including-clock-driver-for-Hi3519-soc/20160222-160736
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
> config: xtensa-allyesconfig (attached as .config)
> reproduce:
> wget 
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
>  -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=xtensa 
> 
> All warnings (new ones prefixed by >>):
> 
> warning: (COMMON_CLK_HI3519) selects RESET_HISI which has unmet direct 
> dependencies (COMMON_CLK && ARCH_HISI)
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
> 

Re: [RESEND PATCH v8 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Mike,
   Thank you very much for your comments.

On 2016/2/17 8:46, Michael Turquette wrote:
> Hello Jiancheng Xue,
> 
> Quoting Jiancheng Xue (2016-02-04 18:31:07)
>> diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
>> index 74dba31..3f57b09 100644
>> --- a/drivers/clk/hisilicon/Makefile
>> +++ b/drivers/clk/hisilicon/Makefile
>> @@ -4,8 +4,10 @@
>>  
>>  obj-y  += clk.o clkgate-separated.o clkdivider-hi6220.o
>>  
>> +obj-$(CONFIG_RESET_CONTROLLER) += reset.o
> 
> Do you really want to build reset.o for all hisi SoCs?
> 

This reset controller driver will be just used in some of hisilicon SOCs.
I'll add a specific config item for it like CONFIG_RESET_HISI. The config
item will be selected by default in SOCs needing this driver.

I'll also fix other issues in next version. Thank you!

Regards,
Jiancheng.

Re: [PATCH v5] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[xuejiancheng]

Hi Ezequiel,

Thank you very much for your comments. I read them carefully.
All of your suggestions are very good and helpful. I will correct
in next version.

Regards,

Jiancheng

.
On 2016/1/29 22:45, Ezequiel Garcia wrote:
> Hi Jiancheng,
>  
> Driver looks mostly good. Few comments below.
> 
> On 29 Jan 04:03 PM, Jiancheng Xue wrote:
>> add hisilicon spi-nor flash controller driver
>>
> 

Re: [PATCH v7 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Paul Bolle，
   Thank you for your reply.

On 2016/1/26 9:17, Paul Bolle wrote:
> On ma, 2016-01-25 at 11:01 +0800, Jiancheng Xue wrote:
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
> 
>> +config COMMON_CLK_HI3519
>> +bool "Hi3519 Clock Driver"
>> +depends on ARCH_HISI
>> +default y
>> +help
>> +  Build the clock driver for hi3519.
> 
>> --- a/drivers/clk/hisilicon/Makefile
>> +++ b/drivers/clk/hisilicon/Makefile
> 
>> +obj-$(CONFIG_COMMON_CLK_HI3519) += clk-hi3519.o
> 
> If I parsed the above correctly clk-hi3519.o can only be built-in,
> right?
> 
Yes. You are right.

But this clock driver should be able to be compiled as a module.
Even though it is preferred to be built-in.

I'll fix it in next version. Thank you.

Regards,

Jiancheng

>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/clk-hi3519.c
> 
>> +#include 
> 
> So is this include actually needed?
> 
>> +static int hi3519_clk_probe(struct platform_device *pdev)
>> +{
>> +   struct device_node *np = pdev->dev.of_node;
>> +   struct hisi_clock_data *clk_data;
>> +
>> +   clk_data = hisi_clk_init(np, HI3519_NR_CLKS);
>> +   if (!clk_data)
>> +   return -ENODEV;
>> +
>> +   hisi_clk_register_fixed_rate(hi3519_fixed_rate_clks,
>> +   
>> ARRAY_SIZE(hi3519_fixed_rate_clks),
>> +clk_data);
>> +   hisi_clk_register_mux(hi3519_mux_clks,
>> ARRAY_SIZE(hi3519_mux_clks),
>> +   clk_data);
>> +   hisi_clk_register_gate(hi3519_gate_clks,
>> +   ARRAY_SIZE(hi3519_gate_clks), clk_data);
>> +
>> +   return hisi_reset_init(np);
>> +}
> 
> (evolution 3.16.5 makes replying to code quite a challenge.)
> 
>> +static const struct of_device_id hi3519_clk_match_table[] = {
>> +{ .compatible = "hisilicon,hi3519-crg" },
>> +{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, hi3519_clk_match_table);
> 
> Last time I checked MODULE_DEVICE_TABLE is preprocessed away for built
> -in code.
> 
>> +static void __exit hi3519_clk_exit(void)
>> +{
>> +platform_driver_unregister(&hi3519_clk_driver);
>> +}
>> +module_exit(hi3519_clk_exit);
> 
> Not needed for built-in only code.
> 
>> +MODULE_DESCRIPTION("HiSilicon Hi3519 Clock Driver");
> 
> Ditto.
> 
>> --- a/drivers/clk/hisilicon/clk.c
>> +++ b/drivers/clk/hisilicon/clk.c
> 
>> +EXPORT_SYMBOL(hisi_clk_init);
> 
> What module uses this export?
>  
>> +EXPORT_SYMBOL(hisi_clk_register_fixed_rate);
> 
> Ditto.
> 
>> +EXPORT_SYMBOL(hisi_clk_register_fixed_factor);
> 
> Ditto.
>  
>> +EXPORT_SYMBOL(hisi_clk_register_mux);
> 
> Ditto.
> 
>> +EXPORT_SYMBOL(hisi_clk_register_divider);
> 
> Ditto.
> 
>> +EXPORT_SYMBOL(hisi_clk_register_gate);
> 
> Ditto.
> 
>> +EXPORT_SYMBOL(hisi_clk_register_gate_sep);
> 
> Ditto.
> 
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/reset.c
> 
>> +int hisi_reset_init(struct device_node *np)
>> +{
>> +[...]
>> +}
>> +EXPORT_SYMBOL(hisi_reset_init);
> 
> Ditto.
> 
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/reset.h
> 
>> +#ifdef CONFIG_RESET_CONTROLLER
>> +int hisi_reset_init(struct device_node *np);
>> +#else
>> +static inline int hisi_reset_init(struct device_node *np)
>> +{
>> +return 0;
>> +}
>> +#endif
> 
> Thanks,
> 
> 
> Paul Bolle
> 
> .
> 

Re: [PATCH v7 4/6] ARM: debug: add hi3519 debug uart

[xuejiancheng]

On 2016/1/25 11:01, Jiancheng Xue wrote:
> add hi3519 debug uart
> 
> Signed-off-by: Jiancheng Xue 
> ---
>  arch/arm/Kconfig.debug | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index c6b6175..b02a7c1 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -260,6 +260,14 @@ choice
> Say Y here if you want kernel low-level debugging support
> on Cortina Gemini based platforms.
>  
> + config DEBUG_HI3519_UART
> + bool "Hisilicon HI3519 Debug UART"
> + depends on ARCH_HISI
> + select DEBUG_UART_PL01X
> + help
> +   Say Y here if you want kernel low-level debugging support
> +   on HI3519 UART.
> +
>   config DEBUG_HI3620_UART
>   bool "Hisilicon HI3620 Debug UART"
>   depends on ARCH_HI3xxx
> @@ -1451,6 +1459,7 @@ config DEBUG_UART_PHYS
>   default 0x11002000 if DEBUG_MT8127_UART0
>   default 0x11006000 if DEBUG_MT6589_UART0
>   default 0x11009000 if DEBUG_MT8135_UART3
> + default 0x1210 if DEBUG_HI3519_UART
>   default 0x1600 if DEBUG_INTEGRATOR
>   default 0x18000300 if DEBUG_BCM_5301X
>   default 0x1801 if DEBUG_SIRFATLAS7_UART0
> @@ -1514,6 +1523,7 @@ config DEBUG_UART_PHYS
>   default 0xfcb0 if DEBUG_HI3620_UART
>   default 0xfd883000 if DEBUG_ALPINE_UART0
>   default 0xfe80 if ARCH_IOP32X
> + default 0xfef0 if DEBUG_HI3519_UART

Sorry. This line is put in a wrong place by my mistake. I'll correct it in next 
version.

>   default 0xff69 if DEBUG_RK32_UART2
>   default 0xffc02000 if DEBUG_SOCFPGA_UART0
>   default 0xffc02100 if DEBUG_SOCFPGA_UART1
> 

Re: [PATCH v5 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

On 2016/1/20 14:38, Tomeu Vizoso wrote:
> On 19 January 2016 at 19:20, Rob Herring  wrote:
>> On Fri, Jan 15, 2016 at 1:57 AM, xuejiancheng  
>> wrote:
>>>
>>> On 2016/1/14 21:16, xuejiancheng wrote:
>>>> Hi Mike,
>>>>
>>>> On 2016/1/14 2:57, Michael Turquette wrote:
>>>>> Quoting xuejiancheng (2016-01-12 19:03:01)
>>>>>> Hi Stephen,
>>>>>>Thank you very much for your reply.
>>>>>>
>>>>>> On 2016/1/13 6:12, Stephen Boyd wrote:
>>>>>>> On 01/08, Jiancheng Xue wrote:
>>>>>>>> diff --git a/drivers/clk/hisilicon/Kconfig 
>>>>>>>> b/drivers/clk/hisilicon/Kconfig
>>>>>>>> index e434854..b6baebf 100644
>>>>>>>> --- a/drivers/clk/hisilicon/Kconfig
>>>>>>>> +++ b/drivers/clk/hisilicon/Kconfig
>>>>>>>> @@ -1,3 +1,10 @@
>>>>>>>> +config COMMON_CLK_HI3519
>>>>>>>> +tristate "Clock Driver for Hi3519"
>>>>>>>
>>>>>>> It looks like this has to be bool. Otherwise it needs to be a
>>>>>>> platform driver and the hisilicon APIs need to be exported and
>>>>>>> lose their __init markings.
>>>>>>>
>>>>>> Yes,it's a problem. I will fix it in next version. Thank you.
>>>>>
>>>>> The best solution would be to make this clock driver a real platform
>>>>> driver.
>>>>>
>>>> Now the work clock of the clocksource timer-sp804 is provided by this 
>>>> driver. So
>>>> it need to be registered early by CLK_OF_DECLARE. If the timer clock is 
>>>> treated
>>>> as a fixed-clock provider, this driver can be implemented as a platform 
>>>> driver.
>>>> Then the crg device must be registered before other clock consumer 
>>>> devices.Accordingly
>>>> the crg device node must be written above all other clock consumer devices 
>>>> node in dts files.
>>>> I think it is also a dependence.
>>>>
>>>> Can you help me understand why it is better to make this driver a platform 
>>>> driver?
>>>> Thank you very much!
>>>>
>>> arch_initcall(customize_machine)
>>> -->of_platform_populate
>>>-->of_platform_bus_create
>>>  -->of_amba_device_create
>>> -->amba_device_add
>>>-->amba_get_enable_pclk
>>> The call sequence above shows that the clock of the amba device must be 
>>> registered before
>>> amba_device_add. The clock of "arm,pl011" uart is registered in the probe 
>>> function of the
>>> platform driver "hi3519-crg". So the platform device "hi3519-crg" must be 
>>> created before
>>> the amba device "arm,pl011" uart.
>>
>> It is a problem, but Tomeu had a fix to support deferred probes here.
>> That was part of the on-demand probing series, but maybe it needs to
>> be applied separately if we are moving clock drivers to platform
>> drivers.
> 
> Hi,
> 
> Marek Szyprowski has kindly taken those two patches as part of a series of 
> him:
> 
> http://lkml.kernel.org/g/1450868368-5650-1-git-send-email-m.szyprow...@samsung.com
> 
> I think it would be great if you could test them and report.
> 
Hi Tomeu,

I have applied the patch "https://lkml.org/lkml/2015/12/23/105"; and tested on 
my hi3519-demb board.
It works even if the apb_pclk is registered later than the amba-pl011 device 
being registered.

But I think it is a problem if amba_read_periphid() returns -ENOMEM or -ENODEV 
when apb_pclk is available.
In this condition，amba_match() returns a non zero value which means the driver 
and device matches
and the amba_probe() will be called, but amba_device->periphid remains as 0. 
Then amba_lookup() called in
amba_probe() will return a null id pointer.The null pointer will be passed to 
amba_driver->probe() and
this may cause a segment fault.

Regards,

Jiancheng

> Thanks,
> 
> Tomeu
> 
> .
> 

Re: [PATCH v4 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Mike，
   I am sorry. I have to correct my answer about using CLK_OF_DECLARE.  

On 2016/1/5 15:21, xuejiancheng wrote:
> Hi Mike,
>Happy new year to you!
>Thank you for taking time to reply.
> 
> On 2015/12/31 8:23, Michael Turquette wrote:
>> Hello Jiancheng Xue,
>>
>> Quoting Jiancheng Xue (2015-12-29 17:43:47)
>>> The CRG(Clock and Reset Generator) block provides clock
>>> and reset signals for other modules in hi3519 soc.
>>>
>>> Signed-off-by: Jiancheng Xue 
>>> ---
>>>  .../devicetree/bindings/clock/hi3519-crg.txt   |  46 +++
>>>  drivers/clk/hisilicon/Kconfig  |   7 +
>>>  drivers/clk/hisilicon/Makefile |   2 +
>>>  drivers/clk/hisilicon/clk-hi3519.c | 103 ++
>>>  drivers/clk/hisilicon/reset.c  | 149 
>>> +
>>>  drivers/clk/hisilicon/reset.h  |  32 +
>>>  include/dt-bindings/clock/hi3519-clock.h   |  43 ++
>>>  7 files changed, 382 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>>  create mode 100644 drivers/clk/hisilicon/clk-hi3519.c
>>>  create mode 100644 drivers/clk/hisilicon/reset.c
>>>  create mode 100644 drivers/clk/hisilicon/reset.h
>>>  create mode 100644 include/dt-bindings/clock/hi3519-clock.h
>>
>> Please keep Philipp Zabel Cc'd for reset-related patches. I've added
>> him to Cc.
>>
> OK.
> 
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt 
>>> b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>> new file mode 100644
>>> index 000..2d23950
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>> @@ -0,0 +1,46 @@
>>> +* Hisilicon Hi3519 Clock and Reset Generator(CRG)
>>> +
>>> +The Hi3519 CRG module provides clock and reset signals to various
>>> +controllers within the SoC.
>>> +
>>> +This binding uses the following bindings:
>>> +Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> +Documentation/devicetree/bindings/reset/reset.txt
>>> +
>>> +Required Properties:
>>> +
>>> +- compatible: should be one of the following.
>>> +  - "hisilicon,hi3519-crg" - controller compatible with Hi3519 SoC.
>>> +
>>> +- reg: physical base address of the controller and length of memory mapped
>>> +  region.
>>> +
>>> +- #clock-cells: should be 1.
>>> +
>>> +Each clock is assigned an identifier and client nodes use this identifier
>>> +to specify the clock which they consume.
>>> +
>>> +All these identifier could be found in .
>>> +
>>> +- #reset-cells: should be 2.
>>> +
>>> +A reset signal can be controlled by writing a bit register in the CRG 
>>> module.
>>> +The reset specifier consists of two cells. The first cell represents the
>>> +register offset relative to the base address. The second cell represents 
>>> the
>>> +bit index in the register.
>>> +
>>> +Example: CRG nodes
>>> +CRG: clock-reset-controller@1201 {
>>> +   compatible = "hisilicon,hi3519-crg";
>>> +reg = <0x1201 0x1>;
>>> +#clock-cells = <1>;
>>> +#reset-cells = <2>;
>>> +};
>>> +
>>> +Example: consumer nodes
>>> +i2c0: i2c@1211 {
>>> +   compatible = "hisilicon,hi3519-i2c";
>>> +reg = <0x1211 0x1000>;
>>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>>> +resets = <&CRG 0xe4 0>;
>>> +};
>>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>>> index e434854..b6baebf 100644
>>> --- a/drivers/clk/hisilicon/Kconfig
>>> +++ b/drivers/clk/hisilicon/Kconfig
>>> @@ -1,3 +1,10 @@
>>> +config COMMON_CLK_HI3519
>>> +   tristate "Clock Driver for Hi3519"
>>> +   depends on ARCH_HISI
>>> +   default y
>>> +   help
>>> + Build the clock driver for hi3519.
>>> +
>>>  config COMMON_CLK_HI6220
>>> bool "Hi6220 Clock Driver"
>>> depends on ARCH_HISI || COMPILE_TEST
>>> diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
>>

Re: [PATCH v4 5/6] mfd: dt-bindings: add device tree bindings for Hi3519 sysctrl

[xuejiancheng]

On 2016/1/5 18:12, Philipp Zabel wrote:
> Am Mittwoch, den 30.12.2015, 09:43 +0800 schrieb Jiancheng Xue:
>> Add device tree bindings for Hi3519 system controller.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  Documentation/devicetree/bindings/mfd/hi3519.txt | 14 ++
>>  1 file changed, 14 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/hi3519.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/hi3519.txt 
>> b/Documentation/devicetree/bindings/mfd/hi3519.txt
>> new file mode 100644
>> index 000..2536edc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/hi3519.txt
>> @@ -0,0 +1,14 @@
>> +* Hisilicon Hi3519 System Controller Block
>> +
>> +This bindings use the following binding:
>> +Dcumentation/devicetree/bindings/clock/clock-bindings.txt
> 
> Typo: "Documentation"
> - but I don't see the clock bindings being used here at all.
> Maybe just drop this sentence?
> 

Sorry about this mistake. Thank you very much.

>> +
>> +Required properties:
>> +- compatible: "hisilicon,hi3519-sysctrl".
>> +- reg: the register region of this block
>> +
>> +Examples:
>> +sysctrl: system-controller@1201 {
>> +compatible = "hisilicon,hi3519-sysctrl", "syscon";
>> +reg = <0x1201 0x1000>;
>> +};
> 
> regards
> Philipp
> 
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Philipp,
   Thank you very much for your quick reply.

On 2016/1/5 18:12, Philipp Zabel wrote:
> H Jiancheng,
> 
> Am Mittwoch, den 30.12.2015, 09:43 +0800 schrieb Jiancheng Xue:
>> The CRG(Clock and Reset Generator) block provides clock
>> and reset signals for other modules in hi3519 soc.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/clock/hi3519-crg.txt   |  46 +++
>>  drivers/clk/hisilicon/Kconfig  |   7 +
>>  drivers/clk/hisilicon/Makefile |   2 +
>>  drivers/clk/hisilicon/clk-hi3519.c | 103 ++
>>  drivers/clk/hisilicon/reset.c  | 149 
>> +
>>  drivers/clk/hisilicon/reset.h  |  32 +
>>  include/dt-bindings/clock/hi3519-clock.h   |  43 ++
>>  7 files changed, 382 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>  create mode 100644 drivers/clk/hisilicon/clk-hi3519.c
>>  create mode 100644 drivers/clk/hisilicon/reset.c
>>  create mode 100644 drivers/clk/hisilicon/reset.h
>>  create mode 100644 include/dt-bindings/clock/hi3519-clock.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt 
>> b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> new file mode 100644
>> index 000..2d23950
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> @@ -0,0 +1,46 @@
>> +* Hisilicon Hi3519 Clock and Reset Generator(CRG)
>> +
>> +The Hi3519 CRG module provides clock and reset signals to various
>> +controllers within the SoC.
>> +
>> +This binding uses the following bindings:
>> +Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +Documentation/devicetree/bindings/reset/reset.txt
>> +
>> +Required Properties:
>> +
>> +- compatible: should be one of the following.
>> +  - "hisilicon,hi3519-crg" - controller compatible with Hi3519 SoC.
>> +
>> +- reg: physical base address of the controller and length of memory mapped
>> +  region.
>> +
>> +- #clock-cells: should be 1.
>> +
>> +Each clock is assigned an identifier and client nodes use this identifier
>> +to specify the clock which they consume.
>> +
>> +All these identifier could be found in .
>> +
>> +- #reset-cells: should be 2.
>> +
>> +A reset signal can be controlled by writing a bit register in the CRG 
>> module.
>> +The reset specifier consists of two cells. The first cell represents the
>> +register offset relative to the base address. The second cell represents the
>> +bit index in the register.
> 
> Are the resets controlled by single bits spread around the register
> space? If so, I'm fine with this binding.
> 

Yes, you are right.

>> +Example: CRG nodes
>> +CRG: clock-reset-controller@1201 {
>> +compatible = "hisilicon,hi3519-crg";
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@1211 {
>> +compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xe4 0>;
>> +};
>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> index e434854..b6baebf 100644
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
>> @@ -1,3 +1,10 @@
>> +config COMMON_CLK_HI3519
>> +tristate "Clock Driver for Hi3519"
>> +depends on ARCH_HISI
>> +default y
>> +help
>> +  Build the clock driver for hi3519.
>> +
>>  config COMMON_CLK_HI6220
>>  bool "Hi6220 Clock Driver"
>>  depends on ARCH_HISI || COMPILE_TEST
>> diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
>> index 74dba31..3f57b09 100644
>> --- a/drivers/clk/hisilicon/Makefile
>> +++ b/drivers/clk/hisilicon/Makefile
>> @@ -4,8 +4,10 @@
>>  
>>  obj-y   += clk.o clkgate-separated.o clkdivider-hi6220.o
>>  
>> +obj-$(CONFIG_RESET_CONTROLLER)  += reset.o
>>  obj-$(CONFIG_ARCH_HI3xxx)   += clk-hi3620.o
>>  obj-$(CONFIG_ARCH_HIP04)+= clk-hip04.o
>>  obj-$(CONFIG_ARCH_HIX5HD2)  += clk-hix5hd2.o
>>  obj-$(CONFIG_COMMON_CLK_HI6220) += clk-hi6220.o
>>  obj-$(CONFIG_STUB_CLK_HI6220)   += clk-hi6220-stub.o
>> +obj-$(CONFIG_COMMON_CLK_HI3519) += clk-hi3519.o
>> diff --git a/drivers/clk/hisilicon/clk-hi3519.c 
>> b/drivers/clk/hisilicon/clk-hi3519.c
>> new file mode 100644
>> index 000..e220234
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/clk-hi3519.c
>> @@ -0,0 +1,103 @@
>> +/*
>> + * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHO

Re: [PATCH v4 1/6] clk: hisilicon: add CRG driver for hi3519 soc

[xuejiancheng]

Hi Mike,
   Happy new year to you!
   Thank you for taking time to reply.

On 2015/12/31 8:23, Michael Turquette wrote:
> Hello Jiancheng Xue,
> 
> Quoting Jiancheng Xue (2015-12-29 17:43:47)
>> The CRG(Clock and Reset Generator) block provides clock
>> and reset signals for other modules in hi3519 soc.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/clock/hi3519-crg.txt   |  46 +++
>>  drivers/clk/hisilicon/Kconfig  |   7 +
>>  drivers/clk/hisilicon/Makefile |   2 +
>>  drivers/clk/hisilicon/clk-hi3519.c | 103 ++
>>  drivers/clk/hisilicon/reset.c  | 149 
>> +
>>  drivers/clk/hisilicon/reset.h  |  32 +
>>  include/dt-bindings/clock/hi3519-clock.h   |  43 ++
>>  7 files changed, 382 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>  create mode 100644 drivers/clk/hisilicon/clk-hi3519.c
>>  create mode 100644 drivers/clk/hisilicon/reset.c
>>  create mode 100644 drivers/clk/hisilicon/reset.h
>>  create mode 100644 include/dt-bindings/clock/hi3519-clock.h
> 
> Please keep Philipp Zabel Cc'd for reset-related patches. I've added
> him to Cc.
> 
OK.

>>
>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt 
>> b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> new file mode 100644
>> index 000..2d23950
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> @@ -0,0 +1,46 @@
>> +* Hisilicon Hi3519 Clock and Reset Generator(CRG)
>> +
>> +The Hi3519 CRG module provides clock and reset signals to various
>> +controllers within the SoC.
>> +
>> +This binding uses the following bindings:
>> +Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +Documentation/devicetree/bindings/reset/reset.txt
>> +
>> +Required Properties:
>> +
>> +- compatible: should be one of the following.
>> +  - "hisilicon,hi3519-crg" - controller compatible with Hi3519 SoC.
>> +
>> +- reg: physical base address of the controller and length of memory mapped
>> +  region.
>> +
>> +- #clock-cells: should be 1.
>> +
>> +Each clock is assigned an identifier and client nodes use this identifier
>> +to specify the clock which they consume.
>> +
>> +All these identifier could be found in .
>> +
>> +- #reset-cells: should be 2.
>> +
>> +A reset signal can be controlled by writing a bit register in the CRG 
>> module.
>> +The reset specifier consists of two cells. The first cell represents the
>> +register offset relative to the base address. The second cell represents the
>> +bit index in the register.
>> +
>> +Example: CRG nodes
>> +CRG: clock-reset-controller@1201 {
>> +   compatible = "hisilicon,hi3519-crg";
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@1211 {
>> +   compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xe4 0>;
>> +};
>> diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
>> index e434854..b6baebf 100644
>> --- a/drivers/clk/hisilicon/Kconfig
>> +++ b/drivers/clk/hisilicon/Kconfig
>> @@ -1,3 +1,10 @@
>> +config COMMON_CLK_HI3519
>> +   tristate "Clock Driver for Hi3519"
>> +   depends on ARCH_HISI
>> +   default y
>> +   help
>> + Build the clock driver for hi3519.
>> +
>>  config COMMON_CLK_HI6220
>> bool "Hi6220 Clock Driver"
>> depends on ARCH_HISI || COMPILE_TEST
>> diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
>> index 74dba31..3f57b09 100644
>> --- a/drivers/clk/hisilicon/Makefile
>> +++ b/drivers/clk/hisilicon/Makefile
>> @@ -4,8 +4,10 @@
>>  
>>  obj-y  += clk.o clkgate-separated.o clkdivider-hi6220.o
>>  
>> +obj-$(CONFIG_RESET_CONTROLLER) += reset.o
>>  obj-$(CONFIG_ARCH_HI3xxx)  += clk-hi3620.o
>>  obj-$(CONFIG_ARCH_HIP04)   += clk-hip04.o
>>  obj-$(CONFIG_ARCH_HIX5HD2) += clk-hix5hd2.o
>>  obj-$(CONFIG_COMMON_CLK_HI6220)+= clk-hi6220.o
>>  obj-$(CONFIG_STUB_CLK_HI6220)  += clk-hi6220-stub.o
>> +obj-$(CONFIG_COMMON_CLK_HI3519)+= clk-hi3519.o
>> diff --git a/drivers/clk/hisilicon/clk-hi3519.c 
>> b/drivers/clk/hisilicon/clk-hi3519.c
>> new file mode 100644
>> index 000..e220234
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/clk-hi3519.c
>> @@ -0,0 +1,103 @@
>> +/*
>> + * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT AN

Re: [PATCH] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[xuejiancheng]

Hi Rob,
   Happy new year to you! Thank you for your patience.

On 2016/1/1 6:26, Rob Herring wrote:
> On Wed, Dec 30, 2015 at 10:26:11AM +0800, Jiancheng Xue wrote:
>> add hisilicon spi-nor flash controller driver
>>
>> Signed-off-by: Binquan Peng 
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/spi/spi-hisi-sfc.txt   |  24 +
>>  drivers/mtd/spi-nor/Kconfig|   7 +
>>  drivers/mtd/spi-nor/Makefile   |   1 +
>>  drivers/mtd/spi-nor/hisi-sfc.c | 505 
>> +
>>  4 files changed, 537 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-hisi-sfc.txt
>>  create mode 100644 drivers/mtd/spi-nor/hisi-sfc.c
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-hisi-sfc.txt 
>> b/Documentation/devicetree/bindings/spi/spi-hisi-sfc.txt
>> new file mode 100644
>> index 000..170885a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/spi-hisi-sfc.txt
>> @@ -0,0 +1,24 @@
>> +HiSilicon SPI-NOR Flash Controller
>> +
>> +Required properties:
>> +- compatible : Should be "hisilicon,hisi-sfc".
>> +- address-cells: number of cells required to define a chip select
>> +address on the SPI bus. Should be set to 1. See spi-bus.txt.
>> +- size-cells:Should be 0.
> 
> How about some consistency in the spacing around the ':'.

OK. I'll correct it in next version. Thank you.

> 
>> +- reg : Offset and length of the register set for the controller device.
>> +- reg-names: Must include the following two entries:"control","memory".
>^ ^
> Spaces needed.

OK

> 
>> +- clocks: handle to spi-nor flash controller clock.
>> +
>> +Example:
>> +spi-nor-controller@1400 {
>> +compatible = "hisilicon,hisi-sfc";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reg = <0x1000 0x1000>, <0x1400 0x100>;
>> +reg-names = "control", "memory";
>> +clocks = <&clock HI3519_FMC_CLK>;
>> +spi-nor@0 {
>> +compatible = "jedec,spi-nor";
>> +reg = <0>;
>> +};
>> +};
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 2fe2a7e..7fe1564 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -30,6 +30,12 @@ config SPI_FSL_QUADSPI
>>This controller does not support generic SPI. It only supports
>>SPI NOR.
>>  
>> +config SPI_HISI_SFC
>> +tristate "Hisilicon SPI-NOR Flash Controller(SFC)"
>> +depends on ARCH_HISI
>> +help
>> +  This enables support for hisilicon SPI-NOR flash controller.
>> +
>>  config SPI_NXP_SPIFI
>>  tristate "NXP SPI Flash Interface (SPIFI)"
>>  depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
>> @@ -41,4 +47,5 @@ config SPI_NXP_SPIFI
>>Flash. Enable this option if you have a device with a SPIFI
>>controller and want to access the Flash as a mtd device.
>>  
>> +
> 
> Drop this spurious change.

OK. Thank you. I'll pay more attention to this later.

> 
>>  endif # MTD_SPI_NOR
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index e5e..8cea3c5 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -1,3 +1,4 @@
>>  obj-$(CONFIG_MTD_SPI_NOR)   += spi-nor.o
>>  obj-$(CONFIG_SPI_FSL_QUADSPI)   += fsl-quadspi.o
>> +obj-$(CONFIG_SPI_HISI_SFC)  += hisi-sfc.o
>>  obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o
>> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
>> new file mode 100644
>> index 000..fd9649a
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/hisi-sfc.c
>> @@ -0,0 +1,505 @@
>> +/* HiSilicon SPI Nor Flash Controller Driver
>> + *
>> + * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see .
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> Some say to alphabetize includes. I don't care so much, but at least 
> group subdirectories.

Sorry about that. I'll reorder them.

> 
> Rob
> 
> .
> 

Jiancheng

.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ple

Re: [PATCH v3 7/7] ARM: dts: add dts files for Hi3519

[xuejiancheng]

Hi Rob,

On 2015/12/12 0:28, Rob Herring wrote:
> On Fri, Dec 11, 2015 at 03:45:21PM +0800, Jiancheng Xue wrote:
>> add dts files for Hi3519
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  arch/arm/boot/dts/Makefile|   2 +
>>  arch/arm/boot/dts/hi3519-demb.dts |  42 +++
>>  arch/arm/boot/dts/hi3519.dtsi | 142 
>> ++
>>  3 files changed, 186 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/hi3519-demb.dts
>>  create mode 100644 arch/arm/boot/dts/hi3519.dtsi
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 30bbc37..1ff3ed9 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -135,6 +135,8 @@ dtb-$(CONFIG_ARCH_EXYNOS5) += \
>>  exynos5800-peach-pi.dtb
>>  dtb-$(CONFIG_ARCH_HI3xxx) += \
>>  hi3620-hi4511.dtb
>> +dtb-$(CONFIG_ARCH_HISI) += \
>> +hi3519-demb.dtb
>>  dtb-$(CONFIG_ARCH_HIX5HD2) += \
>>  hisi-x5hd2-dkb.dtb
>>  dtb-$(CONFIG_ARCH_HIGHBANK) += \
>> diff --git a/arch/arm/boot/dts/hi3519-demb.dts 
>> b/arch/arm/boot/dts/hi3519-demb.dts
>> new file mode 100644
>> index 000..6991ab6
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hi3519-demb.dts
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + *
>> + */
>> +
>> +/dts-v1/;
>> +#include "hi3519.dtsi"
>> +
>> +/ {
>> +model = "HiSilicon HI3519 DEMO Board";
>> +compatible = "hisilicon,hi3519";
>> +
>> +aliases {
>> +serial0 = &uart0;
>> +};
>> +
>> +memory {
>> +device_type = "memory";
>> +reg = <0x8000 0x4000>;
>> +};
>> +};
>> +
>> +&uart0 {
>> +status = "okay";
>> +};
>> +
>> +&dual_timer0 {
>> +status = "okay";
>> +};
>> diff --git a/arch/arm/boot/dts/hi3519.dtsi b/arch/arm/boot/dts/hi3519.dtsi
>> new file mode 100644
>> index 000..50b736e
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hi3519.dtsi
>> @@ -0,0 +1,142 @@
>> +/*
>> + * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + *
>> + */
>> +
>> +#include "skeleton.dtsi"
> 
> Don't include skeleton.dtsi. We've decided it was a bad idea.

OK.

> 
>> +#include 
>> +/ {
>> +cpus {
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +cpu@0 {
> 
> Single core system? Add the other cpu nodes if not. Adding them doesn't 
> have to be in sync with SMP kernel support.

Hi3519 includes an A7 core and an A17 core. But it will run in AMP mode.
The A17 core is reserved for customers for special use. We can treat the
system as a single core system here.

> 
>> +device_type = "cpu";
>> +compatible = "arm,cortex-a7";
>> +reg = <0>;
>> +};
>> +};
>> +
>> +gic: interrupt-controller@1030 {
>> +compatible = "arm,cortex-a7-gic";
>> +#interrupt-cells = <3>;
>> +interrupt-controller;
>> +reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
>> +};
>> +
>> +soc {
>> +#address-cells = <1>;
>> +#size-cells = <1>;
>> +compatible = "simple-bus";
>> +interrupt-parent = <&gic>;
>> +ranges;
> 
> Looks like everything is in 0x12xx range, so you should add actual 
> translation here if that's the case.

Other device nodes will be added later which are not in 0x12xx range.

> 
>> +
>> +amba {
> 
> Is this actually a separate bus in the physical design of the chip?
> 
>> +#address-cells = <1>;
>> +

Re: [PATCH v3 3/7] ARM: hisi: add dt_machine definition for Hi3519

[xuejiancheng]

On 2015/12/11 23:21, Rob Herring wrote:
> On Fri, Dec 11, 2015 at 03:45:17PM +0800, Jiancheng Xue wrote:
>> add dt_machine definition for hi3519.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  arch/arm/mach-hisi/hisilicon.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm/mach-hisi/hisilicon.c b/arch/arm/mach-hisi/hisilicon.c
>> index 8cc6215..010d8a2 100644
>> --- a/arch/arm/mach-hisi/hisilicon.c
>> +++ b/arch/arm/mach-hisi/hisilicon.c
>> @@ -81,3 +81,12 @@ static const char *const hip01_compat[] __initconst = {
>>  DT_MACHINE_START(HIP01, "Hisilicon HIP01 (Flattened Device Tree)")
>>  .dt_compat  = hip01_compat,
>>  MACHINE_END
>> +
>> +static const char *const hi3519_compat[] __initconst = {
>> +"hisilicon,hi3519",
>> +NULL,
>> +};
> 
> You should just have 1 mach desc with multiple compatible strings to 
> match against, not 1 mach desc per compatible string.

Yes, you're right. But Hi3519 is a soc in a new family. It doesn't belong to
any other existing mach descs. And more hi3519 compatible boards will be added
in this mach desc afterwards.
Can I do it like this now, and combine them at a proper time?

Or just add a more generic mach desc, then other socs like hix5hd2/hip01/hip04 
can use this?
static const char *const hisilicon_compat[] __initconst = {
"hisilicon,hi3519",
NULL,
};

DT_MACHINE_START(HISILICON_DT, "Hisilicon")
.dt_compat  = hisilicon_compat,
MACHINE_END



> Rob
> 
>> +
>> +DT_MACHINE_START(HI3519_DT, "Hisilicon Hi3519")
>> +.dt_compat  = hi3519_compat,
>> +MACHINE_END
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 2/7] clk: hisilicon: add dt-binding document for Hi3519 CRG

[xuejiancheng]

On 2015/12/11 23:19, Rob Herring wrote:
> On Fri, Dec 11, 2015 at 03:45:16PM +0800, Jiancheng Xue wrote:
>> add dt-binding document for Hi3519 CRG block
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/clock/hi3519-crg.txt   | 46 
>> ++
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-crg.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-crg.txt 
>> b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> new file mode 100644
>> index 000..e0d30a4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/hi3519-crg.txt
>> @@ -0,0 +1,46 @@
>> +* Hisilicon Hi3519 Clock and Reset Generator(CRG)
>> +
>> +The Hi3519 CRG module provides clock and reset signals to various
>> +controllers within the SoC.
>> +
>> +This binding uses the following bindings:
>> +Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +Documentation/devicetree/bindings/reset/reset.txt
>> +
>> +Required Properties:
>> +
>> +- compatible: should be one of the following.
>> +  - "hisilicon,hi3519-crg" - controller compatible with Hi3519 SoC.
>> +
>> +- reg: physical base address of the controller and length of memory mapped
>> +  region.
>> +
>> +- #clock-cells: should be 1.
>> +
>> +Each clock is assigned an identifier and client nodes use this identifier
>> +to specify the clock which they consume.
>> +
>> +All these identifier could be found in .
> 
> This header should be part of this patch.

Because the header is also depended on by the clock driver. It's also a problem
if I separate it from the clock driver patch.

Is it OK if I put this binding file into the clock driver patch?
> 
>> +
>> +- #reset-cells: should be 2.
>> +
>> +A reset signal can be controlled by writing a bit register in the CRG 
>> module.
>> +The reset specifier consists of two cells. The first cell represents the
>> +register offset relative to the base address. The second cell represents the
>> +bit index in the register.
>> +
>> +Example: CRG nodes
>> +CRG: clock-reset-controller {
> 
> clock-reset-controller@1201

OK.

> 
>> +compatible = "hisilicon,hi3519-crg";
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@0x1211 {
> 
> Drop '0x'

OK.

> 
>> +compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xE4 0>;
> 
> lowercase hex please

OK.

> 
>> +};
>> -- 
>> 1.9.1
>>
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

On 2015/12/10 16:04, Arnd Bergmann wrote:
> On Thursday 10 December 2015 14:32:05 xuejiancheng wrote:
>>>
>>> This is not what I meant. You have to use "syscon" as the most generic
>>> "compatible" value here, but should add a machine specific string
>>> as a more specific one. "hisilicon,sysctrl" is not appropriate because
>>> it does not identify the IP block uniquely, you can only use that
>>> in combination with another more specific string.
>>
>> OK. I will use "hisilicon,hi3519-syscon" and "syscon" as the compatible value
>> for the sysctrl node in hi3519.dtsi.
>>
> 
> Why hisilicon,hi3519-syscon instead of hisilicon,hi3519-sysctrl?

> Is this not called a sysctrl at all in the datasheet then?

It is OK to use hisilicon,hi3519-sysctrl.

I thought syscon was more commonly used as abbr. of system-controller in the 
kernel code.

>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

On 2015/12/9 23:31, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 11:54:51 xuejiancheng wrote:
>> On 2015/12/7 14:37, xuejiancheng wrote:
>>>
>>> On 2015/12/4 18:49, Arnd Bergmann wrote:
>>>> On Friday 04 December 2015 10:27:58 xuejiancheng wrote:
>>>>>>
>>>> Maybe split out the sysctrl binding from
>>>> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt, as it has
>>>> you already have a couple of those, and it's not clear how they relate
>>>> to one another.
>>>>
>>>> If we introduce a string for all hip04 compatible sysctrl devices, we 
>>>> should
>>>> document that and use it consistently, so hi3519 becomes
>>>>
>>>>  compatible = "hisilicon,hi3519-sysctrl", "hisilicon,hip04-sysctrl", 
>>>> "hisilicon,sysctrl";
>>>>
>>>> but I'd clarify in the binding documentation that "hisilicon,sysctrl" 
>>>> should
>>>> only be used for hip04 and hi3519 but not the others.
>>>>
>>>> As this seems to be a standard part, we can also think about making a
>>>> high-level driver for in in drivers/soc rather than relying on the syscon
>>>> driver which we tend to use more for one-off devices with random register
>>>> layouts.
>>>>
>>>Sorry. I didn't understand your meaning well and maybe I gave you a 
>>> wrong description.
>>> Please allow me to clarify it again.
>>>The "sysctrl" nodes here is just used for the "reboot" function. It is 
>>> corresponding to
>>> the driver "drivers/power/reset/hisi-reboot.c". The compatible string in 
>>> the driver is
>>> "hisilicon,sysctrl".
>>>The layout of this block is also different from the one in HiP04.
>>
>> I'll use "syscon" as the compatible value for sysctrl node and 
>> "syscon-reboot" for a new reboot node.
>>
>>
> 
> This is not what I meant. You have to use "syscon" as the most generic
> "compatible" value here, but should add a machine specific string
> as a more specific one. "hisilicon,sysctrl" is not appropriate because
> it does not identify the IP block uniquely, you can only use that
> in combination with another more specific string.

OK. I will use "hisilicon,hi3519-syscon" and "syscon" as the compatible value
for the sysctrl node in hi3519.dtsi.

Thank you!

> 
> That way, we have to option to create a high-level driver for the IP
> block later if it turns out that we need some more generic functionality
> that is provided by those registers.
> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/9] ARM: hisi: enable Hi3519 soc

[xuejiancheng]

On 2015/12/9 23:32, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 11:03:20 xuejiancheng wrote:
>>>>
>>>> I think we should come up with a way to handle this in general for
>>>> ARCH_HISI. It's not problem to have a couple of sub-options, but I'd
>>>> rather not have one for each SoC because I'm sure that hisilicon has
>>>> made dozens or possibly hundreds of ARM based SoCs that belong into
>>>> a couple of families.
>>>
>>> Agree with you.
>>>
>>>>
>>>> The individual selection of IP blocks is not that important, because
>>>> those tend to just be generic device drivers that we can enable on
>>>> any platform using the defconfig files.
>>>>
>>>> You said that ARCH_HI3519 and HIP04 have an identical system controller,
>>>> but it's different for Hi36xx, correct?
>>>
>>> No. The system controller of HI3519 is also different from HIP04. Maybe I 
>>> gave you
>>> wrong descriptions. Sorry about that.
>>>
>>>>
>>>> So maybe we can generalize the HIP04 option to include all chips with
>>>> that system controller as they appear to share a common ancestry regardless
>>>> of the target market?
>>>>
>>>
>>> I agree that we generalize some options regardless of the product line and 
>>> target market.
>>>
>>>> The Hi35xx family includes some rather older chips as well based on ARM9
>>>> etc, correct? Are they closely related to the new one as well, or do they
>>>> just share the name?
>>>
>>> Yes. It's correct. They may share some IP blocks. But they may be very 
>>> different
>>> from the new one for the arch code. I also don't think it's a good idea to 
>>> make
>>> them share the same name.
>>
>> I will use ARCH_HISI instead of ARCH_HI3519.
>>
>>
> 
> Do you mean you want to remove the other options as well?
> 
> We should do this consistently at least within the Kconfig file.

I think it is ideal if we can do that.  But I won't change it in HI3519 soc 
patch.

I will just use ARCH_HISI for HI3519 this time.

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[xuejiancheng]

On 2015/12/7 17:36, Arnd Bergmann wrote:
> On Monday 07 December 2015 16:01:03 xuejiancheng wrote:
>> On 2015/12/4 18:56, Arnd Bergmann wrote:
>>> On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
>>>> Hi Arnd,
>>>>
>>>> On 2015/12/3 17:44, Arnd Bergmann wrote:
>>>>> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
>>>>>> +#ifndef __DTS_HI3519_CLOCK_H
>>>>>> +#define __DTS_HI3519_CLOCK_H
>>>>>
>>>>> Please try to avoid adding headers like this if you can at all.
>>>>>
>>>>> I might ask you to merge the header file in one merge window
>>>>> otherwise and submit the platform code one kernel later, as they
>>>>> tendn to cause us needless dependencies otherwise.
>>>>>
>>>>
>>>> Sorry. In v1, Rob suggested putting binding doc and header files in
>>>> a separate patch. The clock driver indeed depends on the header.
>>>>
>>>> I will put the header and the clock driver in a patch, and keep the
>>>> binding doc in another patch.
>>>
>>> Having split patches is better, I was really commenting on the fact
>>> that ideally you would not have a header file at all. If we merge
>>> the header through arm-soc, then you won't be able to merge the
>>> clk driver easily, and if you merge the header through the clk
>>> maintainer, I'm can't take your dts files.
>>
>> Thank you for your comments. Because the clocks in the crg module have
>> different types and random layouts. If this header file is removed,
>> the clock driver and the dts files will get very complicated.
>>
>> Could you help me acknowledge it if I put the header file and clock driver
>> in a patch?
>>
>> Could you give me some suggestions If I want to keep this header file?
> 
> If this is another clock controller that has a random register layout,
> then adding the header file is the least problematic solution indeed.

Is it OK if I put the header file and the clock driver in a patch?

If it's not OK, could you tell me how should I separate the patches?

Thank you.

> 
>>>>> Where do those numbers come from? They are not consecutive, so it sounds
>>>>> like they are directly from the data sheet and won't be needed in the 
>>>>> driver.
>>>>> If that's true, just use the numbers directly, as you do for everything
>>>>> else.
>>>>
>>>> The numbers are defined by myself, not directly from the data sheet. Some 
>>>> numbers
>>>> are reserved for device nodes which will be added later. So they are not 
>>>> consecutive now.
>>>
>>> I don't understand. How do you decide which numbers to reserve? If the
>>> numbers are completely arbitrary and you have no idea what other clocks
>>> there are, you can simply have consecutive numbers here and do the driver
>>> accordingly.
>>
>> The clocks are divided into several groups according to their types. The 
>> clocks in
>> a group are expected to have consecutive numbers. So some numbers are 
>> reserved for
>> every group in this file, just like in some existing headers. Other clocks 
>> will be
>> added when other peripherals drivers are submitted. They will use the 
>> reserve numbers
>> and be inserted into existing groups.
>>
>> Of course it is not required to reserve numbers for later added clocks.
> 
> Are you able to enumerate all the clocks then? If all clocks that are
> supported by this clock controllers have names in the data sheet, I
> would prefer to just list them all now rather than only enter the ones
> we already need, to avoid having future merge conflicts.
> 
> Then we just need to add code to support those clocks when we need them,
> but that can be done in parallel to adding the DT nodes and the driver,
> rather than strongly serializing the patch flow on the header file patches.
> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

On 2015/12/7 14:37, xuejiancheng wrote:
> 
> On 2015/12/4 18:49, Arnd Bergmann wrote:
>> On Friday 04 December 2015 10:27:58 xuejiancheng wrote:
>>>>
>>>>> +sysctrl: system-controller@1202 {
>>>>> +compatible = "hisilicon,sysctrl";
>>>>> +reg = <0x1202 0x1000>;
>>>>> +reboot-offset = <0x4>;
>>>>> +};
>>>>
>>>> Is this one identical to the one in hip04?
>>>>
>>>> If not, pick a new unique compatible string
>>>
>>> Yes. It's compatible with the one in hip04.
>>
>> Ok, we should add a compatible string for that then, as the hip04 apparently
>> has a slightly different layout from hip01.
>>
>> Can you add a separate patch to clarify the existing hisilicon,sysctrl nodes?
>>
>> It look like hi3620 accidentally uses the same string as hip04 already
>> but is actually a bit different.
>>
>> Maybe split out the sysctrl binding from
>> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt, as it has
>> you already have a couple of those, and it's not clear how they relate
>> to one another.
>>
>> If we introduce a string for all hip04 compatible sysctrl devices, we should
>> document that and use it consistently, so hi3519 becomes
>>
>>  compatible = "hisilicon,hi3519-sysctrl", "hisilicon,hip04-sysctrl", 
>> "hisilicon,sysctrl";
>>
>> but I'd clarify in the binding documentation that "hisilicon,sysctrl" should
>> only be used for hip04 and hi3519 but not the others.
>>
>> As this seems to be a standard part, we can also think about making a
>> high-level driver for in in drivers/soc rather than relying on the syscon
>> driver which we tend to use more for one-off devices with random register
>> layouts.
>>
>Sorry. I didn't understand your meaning well and maybe I gave you a wrong 
> description.
> Please allow me to clarify it again.
>The "sysctrl" nodes here is just used for the "reboot" function. It is 
> corresponding to
> the driver "drivers/power/reset/hisi-reboot.c". The compatible string in the 
> driver is
> "hisilicon,sysctrl".
>The layout of this block is also different from the one in HiP04.

I'll use "syscon" as the compatible value for sysctrl node and "syscon-reboot" 
for a new reboot node.

Thank you.

> 
>>>>> +
>>>>> +crg: crg@1201 {
>>>>> +compatible = "hisilicon,hi3519-crg";
>>>>
>>>>
>>>> what is a "crg"? Is there a standard name for these?
>>>
>>> The "crg" means Clock and Reset Generator. It's a block which supplies clock
>>> and reset signals for other modules in the chip. I used "hi3519-clock"
>>> in last version patch. Rob Herring suggested that it's better to use 
>>> "hi3519-crg"
>>> as the compatible string if it's a whole block.
>>>
>>> what about writing like this？
>>>
>>> crg: clock-reset-controller@1201 {
>>> compatible = "hisilicon,hi3519-crg";
>>> }
>>>
>>
>> Ok, that's better.
>>
>>  Arnd
>>
>> .
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/9] ARM: hisi: enable Hi3519 soc

[xuejiancheng]

On 2015/12/8 9:55, xuejiancheng wrote:
> 
> 
> On 2015/12/7 17:46, Arnd Bergmann wrote:
>> On Monday 07 December 2015 14:58:14 xuejiancheng wrote:
>>> On 2015/12/5 5:54, Arnd Bergmann wrote:
>>>> On Friday 04 December 2015 12:07:58 xuejiancheng wrote:
>>>>> On 2015/12/3 17:40, Arnd Bergmann wrote:
>>>>>> On Thursday 03 December 2015 10:42:45 Jiancheng Xue wrote:
>>>>>>> --- a/arch/arm/mach-hisi/Kconfig
>>>>>>> +++ b/arch/arm/mach-hisi/Kconfig
>>>>>>> @@ -12,6 +12,14 @@ if ARCH_HISI
>>>>>>>  
>>>>>>>  menu "Hisilicon platform type"
>>>>>>>  
>>>>>>> +config ARCH_HI3519
>>>>>>> +   bool "Hisilicon Hi3519 Soc" if ARCH_MULTI_V7
>>>>>>> +   select HAVE_ARM_ARCH_TIMER
>>>>>>> +   select ARCH_HAS_RESET_CONTROLLER
>>>>>>> +
>>>>>>> +   help
>>>>>>> + Support for Hisilicon Hi3519 Soc
>>>>>>> +
>>>>>>>  config ARCH_HI3xxx
>>>>>>> bool "Hisilicon Hi36xx family" if ARCH_MULTI_V7
>>>>>>> select CACHE_L2X0
>>>>>>
>>>>>> Do those need to be separate? I would just extend the Hi36xx
>>>>>> to cover all Hi3xxx, if nothing in the platform code really
>>>>>> depends on this.
>>>>>
>>>>> For HI3519, there is really no special platform code. But HI35xx and 
>>>>> HI36xx soc families
>>>>> belong to different product lines in hisilicon. HI35xx family also 
>>>>> composes of various
>>>>> architectures socs(single core, smp and big-little). So I think it may be 
>>>>> clear to have
>>>>> separate arch definitions.
>>>>>
>>>>> Could you give me more suggestions about this?  Thank you!
>>>>
>>>> For the most part, you already need to enable the device drivers for the
>>>> specific components on each chip, and the per-soc top-level options here
>>>> don't actually control the compilation of any particular code.
>>>>
>>>> This is slightly different for some of the older platforms that for 
>>>> historic
>>>> reasons need fine-grained options. You could probably just make the device
>>>> drivers depend on "ARCH_HISI || COMPILE_TEST" in general, but some level
>>>> of classification is ok, in particular when the chips are not related at 
>>>> all.
>>>>
>>>> In this case, my impression is that while HI3519 and HI36xx are made
>>>> by different business units, there is still a noticeable amount of shared
>>>> IP in them (e.g. the "sysctrl" node that seems to be shared with some of
>>>> the other chips as well), so grouping them together should make sense.
>>>
>>> HI35xx and HI36xx are designed totally independently, including IP 
>>> selection.
>>> The relation between HI35xx and HI36xx is just like the one between HI36xx
>>> and HIP0x. In another word, HI35xx and HI36xx are not related except they 
>>> all
>>> belong to hisilicon. So I don't think it's proper to group them together.
>>>
>>> Is it OK if I drop ARCH_HI3519 and use ARCH_HISI directly?
>>
>> I think we should come up with a way to handle this in general for
>> ARCH_HISI. It's not problem to have a couple of sub-options, but I'd
>> rather not have one for each SoC because I'm sure that hisilicon has
>> made dozens or possibly hundreds of ARM based SoCs that belong into
>> a couple of families.
> 
> Agree with you.
> 
>>
>> The individual selection of IP blocks is not that important, because
>> those tend to just be generic device drivers that we can enable on
>> any platform using the defconfig files.
>>
>> You said that ARCH_HI3519 and HIP04 have an identical system controller,
>> but it's different for Hi36xx, correct?
> 
> No. The system controller of HI3519 is also different from HIP04. Maybe I 
> gave you
> wrong descriptions. Sorry about that.
> 
>>
>> So maybe we can generalize the HIP04 option to include all chips with
>> that system controller as they appear to share a common ancestry regardless
>> of the target market?
>>
> 
> I agree that we generalize some options regardless of the product line and 
> target market.
> 
>> The Hi35xx family includes some rather older chips as well based on ARM9
>> etc, correct? Are they closely related to the new one as well, or do they
>> just share the name?
> 
> Yes. It's correct. They may share some IP blocks. But they may be very 
> different
> from the new one for the arch code. I also don't think it's a good idea to 
> make
> them share the same name.

I will use ARCH_HISI instead of ARCH_HI3519.

> 
>>
>>  Arnd
>>
>> .
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/9] ARM: hisi: enable Hi3519 soc

[xuejiancheng]

On 2015/12/7 17:46, Arnd Bergmann wrote:
> On Monday 07 December 2015 14:58:14 xuejiancheng wrote:
>> On 2015/12/5 5:54, Arnd Bergmann wrote:
>>> On Friday 04 December 2015 12:07:58 xuejiancheng wrote:
>>>> On 2015/12/3 17:40, Arnd Bergmann wrote:
>>>>> On Thursday 03 December 2015 10:42:45 Jiancheng Xue wrote:
>>>>>> --- a/arch/arm/mach-hisi/Kconfig
>>>>>> +++ b/arch/arm/mach-hisi/Kconfig
>>>>>> @@ -12,6 +12,14 @@ if ARCH_HISI
>>>>>>  
>>>>>>  menu "Hisilicon platform type"
>>>>>>  
>>>>>> +config ARCH_HI3519
>>>>>> +   bool "Hisilicon Hi3519 Soc" if ARCH_MULTI_V7
>>>>>> +   select HAVE_ARM_ARCH_TIMER
>>>>>> +   select ARCH_HAS_RESET_CONTROLLER
>>>>>> +
>>>>>> +   help
>>>>>> + Support for Hisilicon Hi3519 Soc
>>>>>> +
>>>>>>  config ARCH_HI3xxx
>>>>>> bool "Hisilicon Hi36xx family" if ARCH_MULTI_V7
>>>>>> select CACHE_L2X0
>>>>>
>>>>> Do those need to be separate? I would just extend the Hi36xx
>>>>> to cover all Hi3xxx, if nothing in the platform code really
>>>>> depends on this.
>>>>
>>>> For HI3519, there is really no special platform code. But HI35xx and 
>>>> HI36xx soc families
>>>> belong to different product lines in hisilicon. HI35xx family also 
>>>> composes of various
>>>> architectures socs(single core, smp and big-little). So I think it may be 
>>>> clear to have
>>>> separate arch definitions.
>>>>
>>>> Could you give me more suggestions about this?  Thank you!
>>>
>>> For the most part, you already need to enable the device drivers for the
>>> specific components on each chip, and the per-soc top-level options here
>>> don't actually control the compilation of any particular code.
>>>
>>> This is slightly different for some of the older platforms that for historic
>>> reasons need fine-grained options. You could probably just make the device
>>> drivers depend on "ARCH_HISI || COMPILE_TEST" in general, but some level
>>> of classification is ok, in particular when the chips are not related at 
>>> all.
>>>
>>> In this case, my impression is that while HI3519 and HI36xx are made
>>> by different business units, there is still a noticeable amount of shared
>>> IP in them (e.g. the "sysctrl" node that seems to be shared with some of
>>> the other chips as well), so grouping them together should make sense.
>>
>> HI35xx and HI36xx are designed totally independently, including IP selection.
>> The relation between HI35xx and HI36xx is just like the one between HI36xx
>> and HIP0x. In another word, HI35xx and HI36xx are not related except they all
>> belong to hisilicon. So I don't think it's proper to group them together.
>>
>> Is it OK if I drop ARCH_HI3519 and use ARCH_HISI directly?
> 
> I think we should come up with a way to handle this in general for
> ARCH_HISI. It's not problem to have a couple of sub-options, but I'd
> rather not have one for each SoC because I'm sure that hisilicon has
> made dozens or possibly hundreds of ARM based SoCs that belong into
> a couple of families.

Agree with you.

> 
> The individual selection of IP blocks is not that important, because
> those tend to just be generic device drivers that we can enable on
> any platform using the defconfig files.
> 
> You said that ARCH_HI3519 and HIP04 have an identical system controller,
> but it's different for Hi36xx, correct?

No. The system controller of HI3519 is also different from HIP04. Maybe I gave 
you
wrong descriptions. Sorry about that.

> 
> So maybe we can generalize the HIP04 option to include all chips with
> that system controller as they appear to share a common ancestry regardless
> of the target market?
> 

I agree that we generalize some options regardless of the product line and 
target market.

> The Hi35xx family includes some rather older chips as well based on ARM9
> etc, correct? Are they closely related to the new one as well, or do they
> just share the name?

Yes. It's correct. They may share some IP blocks. But they may be very different
from the new one for the arch code. I also don't think it's a good idea to make
them share the same name.

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[xuejiancheng]

On 2015/12/7 17:36, Arnd Bergmann wrote:
> On Monday 07 December 2015 16:01:03 xuejiancheng wrote:
>> On 2015/12/4 18:56, Arnd Bergmann wrote:
>>> On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
>>>> Hi Arnd,
>>>>
>>>> On 2015/12/3 17:44, Arnd Bergmann wrote:
>>>>> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
>>>>>> +#ifndef __DTS_HI3519_CLOCK_H
>>>>>> +#define __DTS_HI3519_CLOCK_H
>>>>>
>>>>> Please try to avoid adding headers like this if you can at all.
>>>>>
>>>>> I might ask you to merge the header file in one merge window
>>>>> otherwise and submit the platform code one kernel later, as they
>>>>> tendn to cause us needless dependencies otherwise.
>>>>>
>>>>
>>>> Sorry. In v1, Rob suggested putting binding doc and header files in
>>>> a separate patch. The clock driver indeed depends on the header.
>>>>
>>>> I will put the header and the clock driver in a patch, and keep the
>>>> binding doc in another patch.
>>>
>>> Having split patches is better, I was really commenting on the fact
>>> that ideally you would not have a header file at all. If we merge
>>> the header through arm-soc, then you won't be able to merge the
>>> clk driver easily, and if you merge the header through the clk
>>> maintainer, I'm can't take your dts files.
>>
>> Thank you for your comments. Because the clocks in the crg module have
>> different types and random layouts. If this header file is removed,
>> the clock driver and the dts files will get very complicated.
>>
>> Could you help me acknowledge it if I put the header file and clock driver
>> in a patch?
>>
>> Could you give me some suggestions If I want to keep this header file?
> 
> If this is another clock controller that has a random register layout,
> then adding the header file is the least problematic solution indeed.
> 
>>>>> Where do those numbers come from? They are not consecutive, so it sounds
>>>>> like they are directly from the data sheet and won't be needed in the 
>>>>> driver.
>>>>> If that's true, just use the numbers directly, as you do for everything
>>>>> else.
>>>>
>>>> The numbers are defined by myself, not directly from the data sheet. Some 
>>>> numbers
>>>> are reserved for device nodes which will be added later. So they are not 
>>>> consecutive now.
>>>
>>> I don't understand. How do you decide which numbers to reserve? If the
>>> numbers are completely arbitrary and you have no idea what other clocks
>>> there are, you can simply have consecutive numbers here and do the driver
>>> accordingly.
>>
>> The clocks are divided into several groups according to their types. The 
>> clocks in
>> a group are expected to have consecutive numbers. So some numbers are 
>> reserved for
>> every group in this file, just like in some existing headers. Other clocks 
>> will be
>> added when other peripherals drivers are submitted. They will use the 
>> reserve numbers
>> and be inserted into existing groups.
>>
>> Of course it is not required to reserve numbers for later added clocks.
> 
> Are you able to enumerate all the clocks then? If all clocks that are
> supported by this clock controllers have names in the data sheet, I
> would prefer to just list them all now rather than only enter the ones
> we already need, to avoid having future merge conflicts.
> 
> Then we just need to add code to support those clocks when we need them,
> but that can be done in parallel to adding the DT nodes and the driver,
> rather than strongly serializing the patch flow on the header file patches.

OK. I'll enumerate all the clocks in the data sheet in next version patch.

Thank you.

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[xuejiancheng]

On 2015/12/4 18:56, Arnd Bergmann wrote:
> On Friday 04 December 2015 11:21:28 xuejiancheng wrote:
>> Hi Arnd,
>>
>> On 2015/12/3 17:44, Arnd Bergmann wrote:
>>> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
>>>> +#ifndef __DTS_HI3519_CLOCK_H
>>>> +#define __DTS_HI3519_CLOCK_H
>>>
>>> Please try to avoid adding headers like this if you can at all.
>>>
>>> I might ask you to merge the header file in one merge window
>>> otherwise and submit the platform code one kernel later, as they
>>> tendn to cause us needless dependencies otherwise.
>>>
>>
>> Sorry. In v1, Rob suggested putting binding doc and header files in
>> a separate patch. The clock driver indeed depends on the header.
>>
>> I will put the header and the clock driver in a patch, and keep the
>> binding doc in another patch.
> 
> Having split patches is better, I was really commenting on the fact
> that ideally you would not have a header file at all. If we merge
> the header through arm-soc, then you won't be able to merge the
> clk driver easily, and if you merge the header through the clk
> maintainer, I'm can't take your dts files.

Thank you for your comments. Because the clocks in the crg module have
different types and random layouts. If this header file is removed,
the clock driver and the dts files will get very complicated.

Could you help me acknowledge it if I put the header file and clock driver
in a patch?

Could you give me some suggestions If I want to keep this header file?

> 
>>>> +/* fixed rate */
>>>> +#define HI3519_FIXED_400M  1
>>>> +#define HI3519_FIXED_200M  2
>>>> +#define HI3519_FIXED_125M  3
>>>> +#define HI3519_FIXED_150M  4
>>>> +#define HI3519_FIXED_75M   5
>>>> +#define HI3519_FIXED_300M  6
>>>> +#define HI3519_FIXED_50M   7
>>>> +#define HI3519_FIXED_24M   8
>>>> +#define HI3519_FIXED_3M9
>>>> +
>>>> +/* mux clocks */
>>>> +#define HI3519_FMC_MUX 32
>>>> +#define HI3519_I2C_MUX 33
>>>> +#define HI3519_UART_MUX34
>>>> +#define HI3519_SYSAXI_MUX  35
>>>> +
>>>> +/*fixed factor clocks*/
>>>> +#define HI3519_SYSAPB_CLK  64
>>>> +
>>>> +/* gate clocks */
>>>> +#define HI3519_FMC_CLK 129
>>>> +#define HI3519_UART0_CLK   153
>>>> +#define HI3519_UART1_CLK   154
>>>> +#define HI3519_UART2_CLK   155
>>>> +#define HI3519_UART3_CLK   156
>>>> +#define HI3519_UART4_CLK   157
>>>
>>> Where do those numbers come from? They are not consecutive, so it sounds
>>> like they are directly from the data sheet and won't be needed in the 
>>> driver.
>>> If that's true, just use the numbers directly, as you do for everything
>>> else.
>>
>> The numbers are defined by myself, not directly from the data sheet. Some 
>> numbers
>> are reserved for device nodes which will be added later. So they are not 
>> consecutive now.
> 
> I don't understand. How do you decide which numbers to reserve? If the
> numbers are completely arbitrary and you have no idea what other clocks
> there are, you can simply have consecutive numbers here and do the driver
> accordingly.

The clocks are divided into several groups according to their types. The clocks 
in
a group are expected to have consecutive numbers. So some numbers are reserved 
for
every group in this file, just like in some existing headers. Other clocks will 
be
added when other peripherals drivers are submitted. They will use the reserve 
numbers
and be inserted into existing groups.

Of course it is not required to reserve numbers for later added clocks.

> 
> If the numbers actually have a real meaning, then you either don't need them
> at all, or you could just put all numbers in there that you would eventually 
> need.

The numbers have no hardware meaning actually.

> 
>>>> +#define HI3519_NR_CLKS 256
>>>> +#define HI3519_NR_RSTS 256
>>>>
>>> These seem to not be needed at all.
>>
>> These are used in drivers/clk/hisilicon/clk-hi3519.c.
> 
> Then move them there. Anything that is not needed by *both* the driver and 
> the dts files doesn't belong in here.


OK. I will move them into the driver code.

> 
>   Arnd
> 
> .
> 

Many thanks.

Jiancheng

.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/9] ARM: hisi: enable Hi3519 soc

[xuejiancheng]

Hi Arnd,

On 2015/12/5 5:54, Arnd Bergmann wrote:
> On Friday 04 December 2015 12:07:58 xuejiancheng wrote:
>> On 2015/12/3 17:40, Arnd Bergmann wrote:
>>> On Thursday 03 December 2015 10:42:45 Jiancheng Xue wrote:
>>>> --- a/arch/arm/mach-hisi/Kconfig
>>>> +++ b/arch/arm/mach-hisi/Kconfig
>>>> @@ -12,6 +12,14 @@ if ARCH_HISI
>>>>  
>>>>  menu "Hisilicon platform type"
>>>>  
>>>> +config ARCH_HI3519
>>>> +   bool "Hisilicon Hi3519 Soc" if ARCH_MULTI_V7
>>>> +   select HAVE_ARM_ARCH_TIMER
>>>> +   select ARCH_HAS_RESET_CONTROLLER
>>>> +
>>>> +   help
>>>> + Support for Hisilicon Hi3519 Soc
>>>> +
>>>>  config ARCH_HI3xxx
>>>> bool "Hisilicon Hi36xx family" if ARCH_MULTI_V7
>>>> select CACHE_L2X0
>>>
>>> Do those need to be separate? I would just extend the Hi36xx
>>> to cover all Hi3xxx, if nothing in the platform code really
>>> depends on this.
>>
>> For HI3519, there is really no special platform code. But HI35xx and HI36xx 
>> soc families
>> belong to different product lines in hisilicon. HI35xx family also composes 
>> of various
>> architectures socs(single core, smp and big-little). So I think it may be 
>> clear to have
>> separate arch definitions.
>>
>> Could you give me more suggestions about this?  Thank you!
> 
> For the most part, you already need to enable the device drivers for the
> specific components on each chip, and the per-soc top-level options here
> don't actually control the compilation of any particular code.
> 
> This is slightly different for some of the older platforms that for historic
> reasons need fine-grained options. You could probably just make the device
> drivers depend on "ARCH_HISI || COMPILE_TEST" in general, but some level
> of classification is ok, in particular when the chips are not related at all.
> 
> In this case, my impression is that while HI3519 and HI36xx are made
> by different business units, there is still a noticeable amount of shared
> IP in them (e.g. the "sysctrl" node that seems to be shared with some of
> the other chips as well), so grouping them together should make sense.

HI35xx and HI36xx are designed totally independently, including IP selection.
The relation between HI35xx and HI36xx is just like the one between HI36xx
and HIP0x. In another word, HI35xx and HI36xx are not related except they all
belong to hisilicon. So I don't think it's proper to group them together.

Is it OK if I drop ARCH_HI3519 and use ARCH_HISI directly?

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

On 2015/12/4 18:49, Arnd Bergmann wrote:
> On Friday 04 December 2015 10:27:58 xuejiancheng wrote:
>>>
>>>> +sysctrl: system-controller@1202 {
>>>> +compatible = "hisilicon,sysctrl";
>>>> +reg = <0x1202 0x1000>;
>>>> +reboot-offset = <0x4>;
>>>> +};
>>>
>>> Is this one identical to the one in hip04?
>>>
>>> If not, pick a new unique compatible string
>>
>> Yes. It's compatible with the one in hip04.
> 
> Ok, we should add a compatible string for that then, as the hip04 apparently
> has a slightly different layout from hip01.
> 
> Can you add a separate patch to clarify the existing hisilicon,sysctrl nodes?
> 
> It look like hi3620 accidentally uses the same string as hip04 already
> but is actually a bit different.
> 
> Maybe split out the sysctrl binding from
> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt, as it has
> you already have a couple of those, and it's not clear how they relate
> to one another.
> 
> If we introduce a string for all hip04 compatible sysctrl devices, we should
> document that and use it consistently, so hi3519 becomes
> 
>   compatible = "hisilicon,hi3519-sysctrl", "hisilicon,hip04-sysctrl", 
> "hisilicon,sysctrl";
> 
> but I'd clarify in the binding documentation that "hisilicon,sysctrl" should
> only be used for hip04 and hi3519 but not the others.
> 
> As this seems to be a standard part, we can also think about making a
> high-level driver for in in drivers/soc rather than relying on the syscon
> driver which we tend to use more for one-off devices with random register
> layouts.
> 
   Sorry. I didn't understand your meaning well and maybe I gave you a wrong 
description.
Please allow me to clarify it again.
   The "sysctrl" nodes here is just used for the "reboot" function. It is 
corresponding to
the driver "drivers/power/reset/hisi-reboot.c". The compatible string in the 
driver is
"hisilicon,sysctrl".
   The layout of this block is also different from the one in HiP04.

>>>> +
>>>> +crg: crg@1201 {
>>>> +compatible = "hisilicon,hi3519-crg";
>>>
>>>
>>> what is a "crg"? Is there a standard name for these?
>>
>> The "crg" means Clock and Reset Generator. It's a block which supplies clock
>> and reset signals for other modules in the chip. I used "hi3519-clock"
>> in last version patch. Rob Herring suggested that it's better to use 
>> "hi3519-crg"
>> as the compatible string if it's a whole block.
>>
>> what about writing like this？
>>
>> crg: clock-reset-controller@1201 {
>> compatible = "hisilicon,hi3519-crg";
>> }
>>
> 
> Ok, that's better.
> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/9] ARM: hisi: enable Hi3519 soc

[xuejiancheng]

Hi Arnd,

On 2015/12/3 17:40, Arnd Bergmann wrote:
> On Thursday 03 December 2015 10:42:45 Jiancheng Xue wrote:
>> --- a/arch/arm/mach-hisi/Kconfig
>> +++ b/arch/arm/mach-hisi/Kconfig
>> @@ -12,6 +12,14 @@ if ARCH_HISI
>>  
>>  menu "Hisilicon platform type"
>>  
>> +config ARCH_HI3519
>> +   bool "Hisilicon Hi3519 Soc" if ARCH_MULTI_V7
>> +   select HAVE_ARM_ARCH_TIMER
>> +   select ARCH_HAS_RESET_CONTROLLER
>> +
>> +   help
>> + Support for Hisilicon Hi3519 Soc
>> +
>>  config ARCH_HI3xxx
>> bool "Hisilicon Hi36xx family" if ARCH_MULTI_V7
>> select CACHE_L2X0
> 
> Do those need to be separate? I would just extend the Hi36xx
> to cover all Hi3xxx, if nothing in the platform code really
> depends on this.

For HI3519, there is really no special platform code. But HI35xx and HI36xx soc 
families
belong to different product lines in hisilicon. HI35xx family also composes of 
various
architectures socs(single core, smp and big-little). So I think it may be clear 
to have
separate arch definitions.

Could you give me more suggestions about this?  Thank you!

>> +
>> +static const char *const hi3519_compat[] __initconst = {
>> +   "hisilicon,hi3519",
>> +   NULL,
>> +};
>> +
>> +DT_MACHINE_START(HI3519_DT, "Hisilicon Hi3519 (Flattened Device Tree)")
>> +   .dt_compat  = hi3519_compat,
>> +MACHINE_END
> 
> Also drop the "(Flattened Device Tree)" part of the name, we don't
> support any other kind anyway.

OK.

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 9/9] dmaengine: Kconfig: rename ARCH_HI3xxx to ARCH_HI36xx

[xuejiancheng]

Hi Arnd,

On 2015/12/3 17:41, Arnd Bergmann wrote:
> On Thursday 03 December 2015 10:49:37 Jiancheng Xue wrote:
>> Rename ARCH_HI3xxx to ARCH_HI36xx.
>>
>> Signed-off-by: Jiancheng Xue 
> 
> Maybe just change it to 'depends on ARCH_HISI'? That would make it
> possible to merge the change independently.

OK. Agree with you!

> 
> This also needs a better changelog text.

OK. I'll try to make the changelog text more detailed.

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/9] clk: hi3519: add dt-binding document and header file

[xuejiancheng]

Hi Arnd,

On 2015/12/3 17:44, Arnd Bergmann wrote:
> On Thursday 03 December 2015 10:39:24 Jiancheng Xue wrote:
>> +#ifndef __DTS_HI3519_CLOCK_H
>> +#define __DTS_HI3519_CLOCK_H
> 
> Please try to avoid adding headers like this if you can at all.
> 
> I might ask you to merge the header file in one merge window
> otherwise and submit the platform code one kernel later, as they
> tendn to cause us needless dependencies otherwise.
> 

Sorry. In v1, Rob suggested putting binding doc and header files in
a separate patch. The clock driver indeed depends on the header.

I will put the header and the clock driver in a patch, and keep the
binding doc in another patch.

> 
>> +/* fixed rate */
>> +#define HI3519_FIXED_400M  1
>> +#define HI3519_FIXED_200M  2
>> +#define HI3519_FIXED_125M  3
>> +#define HI3519_FIXED_150M  4
>> +#define HI3519_FIXED_75M   5
>> +#define HI3519_FIXED_300M  6
>> +#define HI3519_FIXED_50M   7
>> +#define HI3519_FIXED_24M   8
>> +#define HI3519_FIXED_3M9
>> +
>> +/* mux clocks */
>> +#define HI3519_FMC_MUX 32
>> +#define HI3519_I2C_MUX 33
>> +#define HI3519_UART_MUX34
>> +#define HI3519_SYSAXI_MUX  35
>> +
>> +/*fixed factor clocks*/
>> +#define HI3519_SYSAPB_CLK  64
>> +
>> +/* gate clocks */
>> +#define HI3519_FMC_CLK 129
>> +#define HI3519_UART0_CLK   153
>> +#define HI3519_UART1_CLK   154
>> +#define HI3519_UART2_CLK   155
>> +#define HI3519_UART3_CLK   156
>> +#define HI3519_UART4_CLK   157
> 
> Where do those numbers come from? They are not consecutive, so it sounds
> like they are directly from the data sheet and won't be needed in the driver.
> If that's true, just use the numbers directly, as you do for everything
> else.

The numbers are defined by myself, not directly from the data sheet. Some 
numbers
are reserved for device nodes which will be added later. So they are not 
consecutive now.

> 
>> +#define HI3519_NR_CLKS 256
>> +#define HI3519_NR_RSTS 256
>>
> These seem to not be needed at all.

These are used in drivers/clk/hisilicon/clk-hi3519.c.

> 
>   Arnd
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/9] ARM: dts: add dts files for hi3519-demb board

[xuejiancheng]

Hi Arnd,
   Thank you very much for all your comments.

On 2015/12/3 17:36, Arnd Bergmann wrote:
> On Thursday 03 December 2015 10:44:28 Jiancheng Xue wrote:
> 
>> +
>> +/dts-v1/;
>> +#include "hi3519.dtsi"
>> +
>> +/ {
>> +model = "HiSilicon HI3519 DEMO Board";
>> +compatible = "hisilicon,hi3519";
>> +
>> +chosen {
>> +bootargs = "mem=64M console=ttyAMA0,115200 early_printk \
>> +root=/dev/mtdblock2 rootfstype=jffs2 \
>> +mtdparts=hi_sfc:1M(boot),4M(kernel),11M(rootfs)";
>> +};
> 
> Most of the arguments should be dropped and replaced with the respective
> DT properties in this file:
> 
> mem:  /memory (you have that already, but the size seems wrong)
> console:  /chosen/stdout-path
> early_printk: just drop this, maybe use "earlycon")
> root: this one is fine
> rootfstype:   should not be needed
> mtdparts: use nodes below the MTD device
> 

This chosen node is just for debug. The real parameters will be set at boot 
stage.
I will drop it.

>> +
>> +#include "skeleton.dtsi"
>> +#include 
>> +/ {
>> +aliases {
>> +serial0 = &uart0;
>> +};
> 
> Move this into the .dts file.

OK. Thank you.

> 
>> +
>> +uart0: uart@1210 {
> 
> rename to serial@1210

OK.

> 
>> +dual_timer1: dual_timer@12001000 {
>> +compatible = "arm,sp804", "arm,primecell";
>> +interrupts = <0 66 4>, <0 67 4>;
>> +reg = <0x12001000 0x1000>;
>> +clocks = <&crg HI3519_FIXED_3M>;
>> +status = "disable";
>> +};
> 
> rename to timer@12001000

OK.

> 
>> +sysctrl: system-controller@1202 {
>> +compatible = "hisilicon,sysctrl";
>> +reg = <0x1202 0x1000>;
>> +reboot-offset = <0x4>;
>> +};
> 
> Is this one identical to the one in hip04?
> 
> If not, pick a new unique compatible string

Yes. It's compatible with the one in hip04.

> 
>> +
>> +crg: crg@1201 {
>> +compatible = "hisilicon,hi3519-crg";
> 
> 
> what is a "crg"? Is there a standard name for these?

The "crg" means Clock and Reset Generator. It's a block which supplies clock
and reset signals for other modules in the chip. I used "hi3519-clock"
in last version patch. Rob Herring suggested that it's better to use 
"hi3519-crg"
as the compatible string if it's a whole block.

what about writing like this？

crg: clock-reset-controller@1201 {
compatible = "hisilicon,hi3519-crg";
}

> 
>   Arnd
> 
> .
> 

Jiancheng
.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/5] ARM: hisi: enable Hi3519 soc

[xuejiancheng]

Hi Kevin,
   Thank you for your suggestions.

On 2015/12/1 8:02, Kevin Hilman wrote:
> Jiancheng Xue  writes:
> 
>> Hi3519 SOC is mainly used for ip camera and sport dv
>> solutions.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  arch/arm/mach-hisi/Kconfig | 9 +
>>  arch/arm/mach-hisi/hisilicon.c | 9 +
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/mach-hisi/Kconfig b/arch/arm/mach-hisi/Kconfig
>> index 83061ad..6bb822c 100644
>> --- a/arch/arm/mach-hisi/Kconfig
>> +++ b/arch/arm/mach-hisi/Kconfig
>> @@ -48,6 +48,15 @@ config ARCH_HIX5HD2
>>  select PINCTRL_SINGLE
>>  help
>>Support for Hisilicon HIX5HD2 SoC family
>> +
>> +config ARCH_HI3519
> 
> Please keep these sorted alphabetically.

Sorry about that. I will correct it in next version.

> 
> Speaking of which, there is already an existing ARCH_HI3xxx entry.
> Should this be included in that family?   If not, maybe the HI3xxx
> should be renamed HI36xx ?

ARCH_HI3xxx just represents Hi36xx mobile soc family. Hi3519 is not included.
It's good advice to rename HI3xxx to HI36xx.

> Kevin
> 
> .
> 

Many thanks,

Jiacheng

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/5] clk: hi3519: add CRG driver for hisilicon hi3519 soc

[xuejiancheng]

Hello Rob,

Thanks for your suggestions!

On 2015/12/1 4:35, Rob Herring wrote:
> On Sat, Nov 28, 2015 at 03:13:26PM +0800, Jiancheng Xue wrote:
>> The CRG(Clock and Reset Generator) module provides
>> clock and reset signals for other modules in hi3519 soc.
>>
>> Signed-off-by: Jiancheng Xue 
>> ---
>>  .../devicetree/bindings/clock/hi3519-clock.txt |  46 +++
>>  drivers/clk/hisilicon/Makefile |   1 +
>>  drivers/clk/hisilicon/clk-hi3519.c | 130 ++
>>  drivers/clk/hisilicon/reset.c  | 149 
>> +
>>  drivers/clk/hisilicon/reset.h  |  25 
>>  include/dt-bindings/clock/hi3519-clock.h   |  78 +++
> 
> It is preferred to put binding doc and header in separate patch.

I will put them in separate patch in next version. Thank you!

> 
>>  6 files changed, 429 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/hi3519-clock.txt
>>  create mode 100644 drivers/clk/hisilicon/clk-hi3519.c
>>  create mode 100644 drivers/clk/hisilicon/reset.c
>>  create mode 100644 drivers/clk/hisilicon/reset.h
>>  create mode 100644 include/dt-bindings/clock/hi3519-clock.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/hi3519-clock.txt 
>> b/Documentation/devicetree/bindings/clock/hi3519-clock.txt
>> new file mode 100644
>> index 000..9fea878
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/hi3519-clock.txt
>> @@ -0,0 +1,46 @@
>> +* Hisilicon Hi3519 Clock and Reset Generator(CRG)
>> +
>> +The Hi3519 CRG module provides clock and reset signals to various
>> +controllers within the SoC.
>> +
>> +This binding uses the following bindings:
>> +Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +Documentation/devicetree/bindings/reset/reset.txt
>> +
>> +Required Properties:
>> +
>> +- compatible: should be one of the following.
>> +  - "hisilicon,hi3519-clock" - controller compatible with Hi3519 SoC.
> 
> Use -crg rather than -clock if that is the block name.

I agreed with you! It is proper to use -crg.

> 
>> +
>> +- reg: physical base address of the controller and length of memory mapped
>> +  region.
>> +
>> +- #clock-cells: should be 1.
>> +
>> +Each clock is assigned an identifier and client nodes use this identifier
>> +to specify the clock which they consume.
>> +
>> +All these identifier could be found in .
>> +
>> +- #reset-cells: should be 2.
>> +
>> +A reset signal can be controlled by writing a bit register in the CRG 
>> module.
>> +The reset specifier consists of two cells. The first cell represents the
>> +register offset relative to the base address. The second cell represents the
>> +bit index in the register.
>> +
>> +Example: CRG nodes
>> +CRG: clock-reset-controller {
>> +compatible = "hisilicon,hi3519-clock";
>> +reg = <0x1201 0x1>;
>> +#clock-cells = <1>;
>> +#reset-cells = <2>;
>> +};
>> +
>> +Example: consumer nodes
>> +i2c0: i2c@0x1211 {
>> +compatible = "hisilicon,hi3519-i2c";
>> +reg = <0x1211 0x1000>;
>> +clocks = <&CRG HI3519_I2C0_RST>;*/
>> +resets = <&CRG 0xE4 0>;
>> +};
> 
> 
>> diff --git a/drivers/clk/hisilicon/reset.h b/drivers/clk/hisilicon/reset.h
>> new file mode 100644
>> index 000..74bea4e
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/reset.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see .
>> + */
>> +
>> +#ifndef __HISI_RESET_H
>> +#define __HISI_RESET_H
>> +
>> +#include 
>> +
>> +int __init hisi_reset_init(struct device_node *np, int nr_rsts);
>> +
>> +#endif  /* __HISI_RESET_H */
>> diff --git a/include/dt-bindings/clock/hi3519-clock.h 
>> b/include/dt-bindings/clock/hi3519-clock.h
>> new file mode 100644
>> index 000..2e08666
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/hi3519-clock.h
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Copyright (c) 2015 HiSilicon Technologies Co., Ltd.
>> + *
>> + * 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.
>> + *
>> + * This prog