Re: [PATCH 3/4] arm: add basic support for Mediatek MT6589 boards

[Heiko Stübner]

Hi Gregory,

Am Donnerstag, 10. April 2014, 08:46:03 schrieb Gregory CLEMENT:
> On 10/04/2014 08:29, Matthias Brugger wrote:
> > 2014-04-09 22:26 GMT+02:00 Gregory CLEMENT <gregory.clement@free-
electrons.com>:
> >> Hi Matthias,
> >> 
> >> On 09/04/2014 19:45, Matthias Brugger wrote:
> >>> diff --git a/arch/arm/boot/dts/mtk6589.dtsi
> >>> b/arch/arm/boot/dts/mtk6589.dtsi new file mode 100644
> >>> index 0000000..6dbb74f
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/mtk6589.dtsi
> >>> @@ -0,0 +1,105 @@
> >>> +/*
> >>> + * Copyright (c) 2014 MundoReader S.L.
> >>> + * Author: Matthias Brugger <matthias.bgg@xxxxxxxxx>
> >>> + *
> >>> + * 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.
> >>> + */
> >>> +
> >>> +#include <dt-bindings/interrupt-controller/irq.h>
> >>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> >>> +#include "skeleton.dtsi"
> >>> +
> >>> +/ {
> >>> +     compatible = "mediatek,mt6589";
> >>> +     interrupt-parent = <&gic>;
> >>> +
> >>> +     cpus {
> >>> +             #address-cells = <1>;
> >>> +             #size-cells = <0>;
> >>> +
> >>> +             cpu@0 {
> >>> +                     device_type = "cpu";
> >>> +                     compatible = "arm,cortex-a7";
> >>> +                     next-level-cache = <&L2>;
> >>> +                     reg = <0x0>;
> >>> +             };
> >>> +             cpu@1 {
> >>> +                     device_type = "cpu";
> >>> +                     compatible = "arm,cortex-a7";
> >>> +                     next-level-cache = <&L2>;
> >>> +                     reg = <0x1>;
> >>> +             };
> >>> +             cpu@2 {
> >>> +                     device_type = "cpu";
> >>> +                     compatible = "arm,cortex-a7";
> >>> +                     next-level-cache = <&L2>;
> >>> +                     reg = <0x2>;
> >>> +             };
> >>> +             cpu@3 {
> >>> +                     device_type = "cpu";
> >>> +                     compatible = "arm,cortex-a7";
> >>> +                     next-level-cache = <&L2>;
> >>> +                     reg = <0x3>;
> >>> +             };
> >>> +
> >>> +     };
> >>> +
> >>> +     clocks {
> >>> +             #address-cells = <1>;
> >>> +             #size-cells = <1>;
> >>> +             ranges;
> >>> +
> >>> +             system_clk: system_clk {
> >>> +                     compatible = "fixed-clock";
> >>> +                     clock-frequency = <13000000>;
> >>> +                     #clock-cells = <0>;
> >>> +                     clock-output-names = "system_clk";
> >>> +             };
> >> 
> >> Is it really a fixed clock without any parent, or do you
> >> declare it as a fixed clock because you don't have any clock
> >> common framework support yet?
> > 
> > I don't have any common clock framework support yet.
> 
> So maybe you should provide one (even a very simple one).
> 
> Pretending a clock is a fixed clock and ignoring its parents
> will be problematic when you will add the common clock framework
> support because the device tree is supposed to be stable and you won't
> be able to change it then.

personally I think the danger of introducing unsuitable bindings/precedents 
for a clock controller before it is fully understood is higher.

I'd just rename the clock above to something like dummy130m like sunxi also 
does for example, to reflect its unfinished state and when the clock controller 
is present later on change the supplying clock of the timer.

Or maybe I'm overlooking something here :-)


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