| Message ID | 1377782873-31931-1-git-send-email-iivanov@mm-sol.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Kumar, On Thu, 2013-08-29 at 10:28 -0500, Kumar Gala wrote: > On Aug 29, 2013, at 8:27 AM, Ivan T. Ivanov wrote: > > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > > > The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and > > provide input and output FIFO's for it. I2C controller can operate > > as master with supported bus speeds of 100Kbps and 400Kbps. > > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > --- > > Documentation/devicetree/bindings/i2c/i2c-qup.txt | 99 +++++++++++++++++++++ > > 1 file changed, 99 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qup.txt > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-qup.txt b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > > new file mode 100644 > > index 0000000..c682726 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > > @@ -0,0 +1,99 @@ > > +Qualcomm Universal Periferial (QUP) I2C controller > > [nit: remove extra spaces before ':'] ok. > > > + > > +Required properties: > > + - compatible : should be "qcom,i2c-qup" > > + - reg : Offset and length of the register region for the device > > + - interrupts : core interrupt > > + > > + - pinctrl-names: Should contain only one value - "default". > > + - pinctrl-0: Should specify pin control group used for this controller. > > + > > + - clocks : phandles to clock instances of the device tree nodes > > + - clock-names : > > + "core" : Allow access to FIFO buffers and registers > > + "iface" : Clock used by QUP interface > > + > > + - #address-cells : should be <1> Address cells for I2C device address > > + - #size-cells : should be <0> I2C addresses have no size component. > > + > > +Optional properties : > > + - Child nodes conforming to i2c bus binding > > + - clock-frequency : Desired I2C bus clock frequency in Hz. If > > + not set thedefault frequency is 100kHz > > + - qcom,src-freq : Frequency of the source clocking this bus in Hz. > > + Divider value is set based on soruce-frequency and > > + desired I2C bus frequency. If this value is not > > + provided, the source clock is assumed to be running > > + at 19.2 MHz. > > I'd spell out frequency instead of 'freq' to be consistent with 'clock-frequency' ok. > > Is the frequency of the 'iface' clock? Can we not use clk_get_rate? It is for 'core' clock. I think that for higher I2C bus frequencies, 'core' clock have to be higher, but I am not sure what is relation. > > > + > > +Aliases: An alias may optionally be used to bind the I2C controller > > +to bus number. Aliases are of the form i2c<n> where <n> is an integer > > +representing the bus number to use. > > + > > +Example: > > + > > + aliases { > > + i2c0 = &i2c_A; > > + i2c1 = &i2c_B; > > + i2c2 = &i2c_C; > > + }; > > What is the purpose here? Define on which I2C bus this controller operate. I2C client drivers usually do i2c_get_adapter(bus_number) before its registration. This is for drivers before invention of of_i2c_register_devices(), I believe. Thanks, Ivan > > > + > > + i2c_A: i2c@f9967000 { > > + compatible = "qcom,i2c-qup"; > > + reg = <0Xf9967000 0x1000>; > > + interrupts = <0 105 0>; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&i2c0_data>; > > + > > + clocks = <&core>, <&iface>; > > + clock-names = "core", "iface"; > > + > > + clock-frequency = <100000>; > > + qcom,src-freq = <50000000>; > > + status = "disabled"; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + dummy@60 { > > + compatible = "dummy"; > > + reg = <0x60>; > > + }; > > + }; > > + > > + i2c_B: i2c@f9923000 { > > + compatible = "qcom,i2c-qup"; > > + reg = <0xf9923000 0x1000>; > > + interrupts = <0 95 0>; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&i2c1_data>; > > + > > + clocks = <&core>, <&iface>; > > + clock-names = "core", "iface"; > > + > > + clock-frequency = <100000>; > > + qcom,src-freq = <19200000>; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + > > + i2c_C: i2c@f9924000 { > > + compatible = "qcom,i2c-qup"; > > + reg = <0xf9924000 0x1000>; > > + interrupts = <0 96 0>; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&i2c5_data>; > > + > > + clocks = <&core>, <&iface>; > > + clock-names = "core", "iface"; > > + > > + clock-frequency = <100000>; > > + qcom,src-freq = <50000000>; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > -- > > 1.7.9.5 > > > - k > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kumar, On Fri, 2013-08-30 at 10:21 -0500, Kumar Gala wrote: > On Aug 29, 2013, at 12:26 PM, Ivan T. Ivanov wrote: <snip> > >>> + > >>> +Optional properties : > >>> + - Child nodes conforming to i2c bus binding > >>> + - clock-frequency : Desired I2C bus clock frequency in Hz. If > >>> + not set thedefault frequency is 100kHz > >>> + - qcom,src-freq : Frequency of the source clocking this bus in Hz. > >>> + Divider value is set based on soruce-frequency and > >>> + desired I2C bus frequency. If this value is not > >>> + provided, the source clock is assumed to be running > >>> + at 19.2 MHz. > >> > >> I'd spell out frequency instead of 'freq' to be consistent with 'clock-frequency' > > > > ok. > > > >> > >> Is the frequency of the 'iface' clock? Can we not use clk_get_rate? > > > > It is for 'core' clock. I think that for higher I2C bus frequencies, > > 'core' clock have to be higher, but I am not sure what is relation. > > Ok, can we use clk_get_rate on the 'core' clk to get its frequency instead of needing a DT prop for it? Probably I didn't explain it well. The 'core' clock have to be accelerated before higher bus frequencies could be achieved. > > > > >> > >>> + > >>> +Aliases: An alias may optionally be used to bind the I2C controller > >>> +to bus number. Aliases are of the form i2c<n> where <n> is an integer > >>> +representing the bus number to use. > >>> + > >>> +Example: > >>> + > >>> + aliases { > >>> + i2c0 = &i2c_A; > >>> + i2c1 = &i2c_B; > >>> + i2c2 = &i2c_C; > >>> + }; > >> > >> What is the purpose here? > > > > Define on which I2C bus this controller operate. I2C client > > drivers usually do i2c_get_adapter(bus_number) before its > > registration. This is for drivers before invention of > > of_i2c_register_devices(), I believe. > > Since this is for upstream why dont we use of_i2c_register_devices() and remove this stuff related to aliases. Adapter driver already is using of_i2c_register_devices(). Also OF helper function will/or is already part of i2c_register_adapter(). Attempt here was to make it compatible with older i2c client drivers. Regards, Ivan > > - k > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Hmm. Fixing b0rked LKML address; that might explain why I am not seeing Kumar's replies.] On Tue, Sep 10, 2013 at 03:08:57PM +0300, Ivan T. Ivanov wrote: > Hi Kumar, > > On Fri, 2013-08-30 at 10:21 -0500, Kumar Gala wrote: > > On Aug 29, 2013, at 12:26 PM, Ivan T. Ivanov wrote: > > >>> + > > >>> +Optional properties : > > >>> + - Child nodes conforming to i2c bus binding > > >>> + - clock-frequency : Desired I2C bus clock frequency in Hz. If > > >>> + not set thedefault frequency is 100kHz > > >>> + - qcom,src-freq : Frequency of the source clocking this bus in Hz. > > >>> + Divider value is set based on soruce-frequency and > > >>> + desired I2C bus frequency. If this value is not > > >>> + provided, the source clock is assumed to be running > > >>> + at 19.2 MHz. > > >> > > >> I'd spell out frequency instead of 'freq' to be consistent with 'clock-frequency' > > > > > > ok. > > > > > >> > > >> Is the frequency of the 'iface' clock? Can we not use clk_get_rate? > > > > > > It is for 'core' clock. I think that for higher I2C bus frequencies, > > > 'core' clock have to be higher, but I am not sure what is relation. > > > > Ok, can we use clk_get_rate on the 'core' clk to get its frequency instead of needing a DT prop for it? > > Probably I didn't explain it well. The 'core' clock have to be > accelerated before higher bus frequencies could be achieved. I think what Kumar is suggesting is that the QUP driver not do an explicit clk_set_rate() at all (which AFAICT is what's currently being done to set the consuming clock to the rate specified in 'qcom,src-freq'), but instead assume that the consuming clock has already been setup properly. Then the driver just uses clk_get_rate() and clock-frequency to calculate how to setup any internal dividers. > > >>> +Aliases: An alias may optionally be used to bind the I2C controller > > >>> +to bus number. Aliases are of the form i2c<n> where <n> is an integer > > >>> +representing the bus number to use. > > >>> + > > >>> +Example: > > >>> + > > >>> + aliases { > > >>> + i2c0 = &i2c_A; > > >>> + i2c1 = &i2c_B; > > >>> + i2c2 = &i2c_C; > > >>> + }; > > >> > > >> What is the purpose here? > > > > > > Define on which I2C bus this controller operate. I2C client > > > drivers usually do i2c_get_adapter(bus_number) before its > > > registration. This is for drivers before invention of > > > of_i2c_register_devices(), I believe. > > > > Since this is for upstream why dont we use of_i2c_register_devices() and remove this stuff related to aliases. > > Adapter driver already is using of_i2c_register_devices(). Also OF > helper function will/or is already part of i2c_register_adapter(). > Attempt here was to make it compatible with older i2c client drivers. I agree with Kumar on removing this. If we decide it is something worth keeping, logic to support it doesn't belong in the QUP driver, but in the i2c core.
Hi, On Tue, 2013-09-10 at 08:59 -0500, Josh Cartwright wrote: > [Hmm. Fixing b0rked LKML address; that might explain why I am not > seeing Kumar's replies.] > Yes, sorry about this. > On Tue, Sep 10, 2013 at 03:08:57PM +0300, Ivan T. Ivanov wrote: > > Hi Kumar, > > > > On Fri, 2013-08-30 at 10:21 -0500, Kumar Gala wrote: > > > On Aug 29, 2013, at 12:26 PM, Ivan T. Ivanov wrote: > > > >>> + > > > >>> +Optional properties : > > > >>> + - Child nodes conforming to i2c bus binding > > > >>> + - clock-frequency : Desired I2C bus clock frequency in Hz. If > > > >>> + not set thedefault frequency is 100kHz > > > >>> + - qcom,src-freq : Frequency of the source clocking this bus in Hz. > > > >>> + Divider value is set based on soruce-frequency and > > > >>> + desired I2C bus frequency. If this value is not > > > >>> + provided, the source clock is assumed to be running > > > >>> + at 19.2 MHz. > > > >> > > > >> I'd spell out frequency instead of 'freq' to be consistent with 'clock-frequency' > > > > > > > > ok. > > > > > > > >> > > > >> Is the frequency of the 'iface' clock? Can we not use clk_get_rate? > > > > > > > > It is for 'core' clock. I think that for higher I2C bus frequencies, > > > > 'core' clock have to be higher, but I am not sure what is relation. > > > > > > Ok, can we use clk_get_rate on the 'core' clk to get its frequency instead of needing a DT prop for it? > > > > Probably I didn't explain it well. The 'core' clock have to be > > accelerated before higher bus frequencies could be achieved. > > I think what Kumar is suggesting is that the QUP driver not do an > explicit clk_set_rate() at all (which AFAICT is what's currently being > done to set the consuming clock to the rate specified in > 'qcom,src-freq'), but instead assume that the consuming clock has > already been setup properly. Then the driver just uses clk_get_rate() > and clock-frequency to calculate how to setup any internal dividers. Yes, I think I got it, but the point is that clock is not already set and it is driver responsibility to do that. > > > > >>> +Aliases: An alias may optionally be used to bind the I2C controller > > > >>> +to bus number. Aliases are of the form i2c<n> where <n> is an integer > > > >>> +representing the bus number to use. > > > >>> + > > > >>> +Example: > > > >>> + > > > >>> + aliases { > > > >>> + i2c0 = &i2c_A; > > > >>> + i2c1 = &i2c_B; > > > >>> + i2c2 = &i2c_C; > > > >>> + }; > > > >> > > > >> What is the purpose here? > > > > > > > > Define on which I2C bus this controller operate. I2C client > > > > drivers usually do i2c_get_adapter(bus_number) before its > > > > registration. This is for drivers before invention of > > > > of_i2c_register_devices(), I believe. > > > > > > Since this is for upstream why dont we use of_i2c_register_devices() and remove this stuff related to aliases. > > > > Adapter driver already is using of_i2c_register_devices(). Also OF > > helper function will/or is already part of i2c_register_adapter(). > > Attempt here was to make it compatible with older i2c client drivers. > > I agree with Kumar on removing this. If we decide it is something worth > keeping, logic to support it doesn't belong in the QUP driver, but in > the i2c core. I don't have strong objection here, will remove aliases support. Regards, Ivan > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 29, 2013 at 02:27:52PM +0100, Ivan T. Ivanov wrote: > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and > provide input and output FIFO's for it. I2C controller can operate > as master with supported bus speeds of 100Kbps and 400Kbps. > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > --- > Documentation/devicetree/bindings/i2c/i2c-qup.txt | 99 +++++++++++++++++++++ > 1 file changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qup.txt > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-qup.txt b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > new file mode 100644 > index 0000000..c682726 > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > @@ -0,0 +1,99 @@ > +Qualcomm Universal Periferial (QUP) I2C controller > + > +Required properties: > + - compatible : should be "qcom,i2c-qup" > + - reg : Offset and length of the register region for the device > + - interrupts : core interrupt How about the following: - interrupts: interrupt-specifier for the core interrupt. > + > + - pinctrl-names: Should contain only one value - "default". > + - pinctrl-0: Should specify pin control group used for this controller. > + > + - clocks : phandles to clock instances of the device tree nodes Clocks aren't just phandles, they have a clock-specifier component. This should probably be something like: - clocks: a list of phandle + clock-specifier pairs for each entry in clock-names > + - clock-names : > + "core" : Allow access to FIFO buffers and registers Huh? That description doesn't seem to descripe the hardware. > + "iface" : Clock used by QUP interface Which interface? The slave interface the CPUs access, or the interface to the I2C devices? Are these the only clock inputs to the device? Is there a regulator input that might need to be specified? > + > + - #address-cells : should be <1> Address cells for I2C device address > + - #size-cells : should be <0> I2C addresses have no size component. > + > +Optional properties : > + - Child nodes conforming to i2c bus binding > + - clock-frequency : Desired I2C bus clock frequency in Hz. If > + not set thedefault frequency is 100kHz Why is this necessary? > + - qcom,src-freq : Frequency of the source clocking this bus in Hz. > + Divider value is set based on soruce-frequency and > + desired I2C bus frequency. If this value is not > + provided, the source clock is assumed to be running > + at 19.2 MHz. This looks like it should be a clock input. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Thu, 2013-09-12 at 17:28 +0100, Mark Rutland wrote: > On Thu, Aug 29, 2013 at 02:27:52PM +0100, Ivan T. Ivanov wrote: > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > > > The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and > > provide input and output FIFO's for it. I2C controller can operate > > as master with supported bus speeds of 100Kbps and 400Kbps. > > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > --- > > Documentation/devicetree/bindings/i2c/i2c-qup.txt | 99 +++++++++++++++++++++ > > 1 file changed, 99 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qup.txt > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-qup.txt b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > > new file mode 100644 > > index 0000000..c682726 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > > @@ -0,0 +1,99 @@ > > +Qualcomm Universal Periferial (QUP) I2C controller > > + > > +Required properties: > > + - compatible : should be "qcom,i2c-qup" > > + - reg : Offset and length of the register region for the device > > + - interrupts : core interrupt > > How about the following: > > - interrupts: interrupt-specifier for the core interrupt. Ok. > > > + > > + - pinctrl-names: Should contain only one value - "default". > > + - pinctrl-0: Should specify pin control group used for this controller. > > + > > + - clocks : phandles to clock instances of the device tree nodes > > Clocks aren't just phandles, they have a clock-specifier component. This > should probably be something like: > > - clocks: a list of phandle + clock-specifier pairs for each entry in > clock-names Right now MSM clocks [1] are just phandles, I should add #clock-cells = <0>. > > > + - clock-names : > > + "core" : Allow access to FIFO buffers and registers > > Huh? That description doesn't seem to descripe the hardware. How about: Allow access to I2C core FIFO buffers and registers. > > > + "iface" : Clock used by QUP interface > > Which interface? The slave interface the CPUs access, or the interface > to the I2C devices? My understanding for this is that QUP is wrapping I2C core. QUP is interface to I2C core. > > Are these the only clock inputs to the device? Yep. > > Is there a regulator input that might need to be specified? > No. Power management is done via clock gating. > > + > > + - #address-cells : should be <1> Address cells for I2C device address > > + - #size-cells : should be <0> I2C addresses have no size component. > > + > > +Optional properties : > > + - Child nodes conforming to i2c bus binding > > + - clock-frequency : Desired I2C bus clock frequency in Hz. If > > + not set thedefault frequency is 100kHz > > Why is this necessary? This is how I2C bus frequency could be specified. It is standard I2C property. Not sure that I get the question. > > > + - qcom,src-freq : Frequency of the source clocking this bus in Hz. > > + Divider value is set based on soruce-frequency and > > + desired I2C bus frequency. If this value is not > > + provided, the source clock is assumed to be running > > + at 19.2 MHz. > > This looks like it should be a clock input. Thanks, I will change it. Regard, Ivan [1] https://lkml.org/lkml/2013/7/24/729 > > Thanks, > Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 13, 2013 at 10:13:15AM +0100, Ivan T. Ivanov wrote: > Hi Mark, > > On Thu, 2013-09-12 at 17:28 +0100, Mark Rutland wrote: > > On Thu, Aug 29, 2013 at 02:27:52PM +0100, Ivan T. Ivanov wrote: > > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > > > > > The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and > > > provide input and output FIFO's for it. I2C controller can operate > > > as master with supported bus speeds of 100Kbps and 400Kbps. > > > > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > > --- > > > Documentation/devicetree/bindings/i2c/i2c-qup.txt | 99 +++++++++++++++++++++ > > > 1 file changed, 99 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qup.txt > > > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-qup.txt b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > > > new file mode 100644 > > > index 0000000..c682726 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > > > @@ -0,0 +1,99 @@ > > > +Qualcomm Universal Periferial (QUP) I2C controller s/Periferial/Peripheral/ ? > > > + > > > +Required properties: > > > + - compatible : should be "qcom,i2c-qup" > > > + - reg : Offset and length of the register region for the device > > > + - interrupts : core interrupt > > > > How about the following: > > > > - interrupts: interrupt-specifier for the core interrupt. > > Ok. > > > > > > + > > > + - pinctrl-names: Should contain only one value - "default". > > > + - pinctrl-0: Should specify pin control group used for this controller. > > > + > > > + - clocks : phandles to clock instances of the device tree nodes > > > > Clocks aren't just phandles, they have a clock-specifier component. This > > should probably be something like: > > > > - clocks: a list of phandle + clock-specifier pairs for each entry in > > clock-names > > Right now MSM clocks [1] are just phandles, I should add #clock-cells = <0>. I see no reason to strongly tie the binding to a particular set of clocks when it's easy to make it generic. What if a new platform uses QUP, but its clocks have a number of clock-cells? As clock-specifiers may be zero cells, describing this as a phandle + clock-specifier pair means this will work with any clock provider following the common clock bindings. > > > > > > + - clock-names : > > > + "core" : Allow access to FIFO buffers and registers > > > > Huh? That description doesn't seem to descripe the hardware. > > How about: Allow access to I2C core FIFO buffers and registers. My only concern here is the words "allow access", as that seems odd in the context of a hardware description. It seems most bindings don't describe what the clocks do as that's defined in the manual for a piece of hardware anyway, so unfortunately I don't have a good example. I think something like the below would be more in keeping with what's expected: "core": Clock controlling the FIFO buffers and registers > > > > > > + "iface" : Clock used by QUP interface > > > > Which interface? The slave interface the CPUs access, or the interface > > to the I2C devices? > > > My understanding for this is that QUP is wrapping I2C core. QUP is > interface to I2C core. Ok. > > > > > Are these the only clock inputs to the device? > > Yep. Ok. > > > > > Is there a regulator input that might need to be specified? > > > > No. Power management is done via clock gating. Ok. > > > > + > > > + - #address-cells : should be <1> Address cells for I2C device address > > > + - #size-cells : should be <0> I2C addresses have no size component. > > > + > > > +Optional properties : > > > + - Child nodes conforming to i2c bus binding > > > + - clock-frequency : Desired I2C bus clock frequency in Hz. If > > > + not set thedefault frequency is 100kHz > > > > Why is this necessary? > > This is how I2C bus frequency could be specified. It is standard > I2C property. Not sure that I get the question. Not all i2c bindings have this, but I see many do. Never mind. > > > > > > + - qcom,src-freq : Frequency of the source clocking this bus in Hz. > > > + Divider value is set based on soruce-frequency and > > > + desired I2C bus frequency. If this value is not > > > + provided, the source clock is assumed to be running > > > + at 19.2 MHz. > > > > This looks like it should be a clock input. > > Thanks, I will change it. Great! Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Mon, 2013-09-16 at 14:32 +0100, Mark Rutland wrote: > On Fri, Sep 13, 2013 at 10:13:15AM +0100, Ivan T. Ivanov wrote: > > Hi Mark, > > > > On Thu, 2013-09-12 at 17:28 +0100, Mark Rutland wrote: > > > On Thu, Aug 29, 2013 at 02:27:52PM +0100, Ivan T. Ivanov wrote: > > > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > > > > > > > The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and > > > > provide input and output FIFO's for it. I2C controller can operate > > > > as master with supported bus speeds of 100Kbps and 400Kbps. > > > > > > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > > > --- > > > > Documentation/devicetree/bindings/i2c/i2c-qup.txt | 99 +++++++++++++++++++++ > > > > 1 file changed, 99 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qup.txt > > > > > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-qup.txt b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > > > > new file mode 100644 > > > > index 0000000..c682726 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-qup.txt > > > > @@ -0,0 +1,99 @@ > > > > +Qualcomm Universal Periferial (QUP) I2C controller > > s/Periferial/Peripheral/ ? Thanks. > > > > > + > > > > +Required properties: > > > > + - compatible : should be "qcom,i2c-qup" > > > > + - reg : Offset and length of the register region for the device > > > > + - interrupts : core interrupt > > > > > > How about the following: > > > > > > - interrupts: interrupt-specifier for the core interrupt. > > > > Ok. > > > > > > > > > + > > > > + - pinctrl-names: Should contain only one value - "default". > > > > + - pinctrl-0: Should specify pin control group used for this controller. > > > > + > > > > + - clocks : phandles to clock instances of the device tree nodes > > > > > > Clocks aren't just phandles, they have a clock-specifier component. This > > > should probably be something like: > > > > > > - clocks: a list of phandle + clock-specifier pairs for each entry in > > > clock-names > > > > Right now MSM clocks [1] are just phandles, I should add #clock-cells = <0>. > > I see no reason to strongly tie the binding to a particular set of > clocks when it's easy to make it generic. What if a new platform uses > QUP, but its clocks have a number of clock-cells? As clock-specifiers > may be zero cells, describing this as a phandle + clock-specifier pair > means this will work with any clock provider following the common clock > bindings. I doubt that any other platform will use Qualcomm specific peripheral, but I see you point. Will change this. > > > > > > > > > > + - clock-names : > > > > + "core" : Allow access to FIFO buffers and registers > > > > > > Huh? That description doesn't seem to descripe the hardware. > > > > How about: Allow access to I2C core FIFO buffers and registers. > > My only concern here is the words "allow access", as that seems odd in > the context of a hardware description. > > It seems most bindings don't describe what the clocks do as that's > defined in the manual for a piece of hardware anyway, so unfortunately I > don't have a good example. > > I think something like the below would be more in keeping with what's > expected: > > "core": Clock controlling the FIFO buffers and registers Ok, thanks will reword. > > > > > > > > > > + "iface" : Clock used by QUP interface > > > > > > Which interface? The slave interface the CPUs access, or the interface > > > to the I2C devices? > > > > > > My understanding for this is that QUP is wrapping I2C core. QUP is > > interface to I2C core. > > Ok. > > > > > > > > > Are these the only clock inputs to the device? > > > > Yep. > > Ok. > > > > > > > > > Is there a regulator input that might need to be specified? > > > > > > > No. Power management is done via clock gating. > > Ok. > > > > > > > + > > > > + - #address-cells : should be <1> Address cells for I2C device address > > > > + - #size-cells : should be <0> I2C addresses have no size component. > > > > + > > > > +Optional properties : > > > > + - Child nodes conforming to i2c bus binding > > > > + - clock-frequency : Desired I2C bus clock frequency in Hz. If > > > > + not set thedefault frequency is 100kHz > > > > > > Why is this necessary? > > > > This is how I2C bus frequency could be specified. It is standard > > I2C property. Not sure that I get the question. > > Not all i2c bindings have this, but I see many do. Never mind. > > > > > > > > > > + - qcom,src-freq : Frequency of the source clocking this bus in Hz. > > > > + Divider value is set based on soruce-frequency and > > > > + desired I2C bus frequency. If this value is not > > > > + provided, the source clock is assumed to be running > > > > + at 19.2 MHz. > > > > > > This looks like it should be a clock input. > > > > Thanks, I will change it. > > Great! Regards, Ivan > > Cheers, > Mark. > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > I agree with Kumar on removing this. If we decide it is something worth > > keeping, logic to support it doesn't belong in the QUP driver, but in > > the i2c core. > > I don't have strong objection here, will remove aliases support. When resubmitting, please test against v3.12-rcX. of_i2c_register_devices() is in the core meanwhile and alias support for of is in the core for a while, too. Check i2c_add_adapter().
Hi, On Wed, 2013-09-25 at 18:06 +0200, Wolfram Sang wrote: > > > I agree with Kumar on removing this. If we decide it is something worth > > > keeping, logic to support it doesn't belong in the QUP driver, but in > > > the i2c core. > > > > I don't have strong objection here, will remove aliases support. > > When resubmitting, please test against v3.12-rcX. > of_i2c_register_devices() is in the core meanwhile and alias support for > of is in the core for a while, too. Check i2c_add_adapter(). Thanks, I will. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/i2c/i2c-qup.txt b/Documentation/devicetree/bindings/i2c/i2c-qup.txt new file mode 100644 index 0000000..c682726 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/i2c-qup.txt @@ -0,0 +1,99 @@ +Qualcomm Universal Periferial (QUP) I2C controller + +Required properties: + - compatible : should be "qcom,i2c-qup" + - reg : Offset and length of the register region for the device + - interrupts : core interrupt + + - pinctrl-names: Should contain only one value - "default". + - pinctrl-0: Should specify pin control group used for this controller. + + - clocks : phandles to clock instances of the device tree nodes + - clock-names : + "core" : Allow access to FIFO buffers and registers + "iface" : Clock used by QUP interface + + - #address-cells : should be <1> Address cells for I2C device address + - #size-cells : should be <0> I2C addresses have no size component. + +Optional properties : + - Child nodes conforming to i2c bus binding + - clock-frequency : Desired I2C bus clock frequency in Hz. If + not set thedefault frequency is 100kHz + - qcom,src-freq : Frequency of the source clocking this bus in Hz. + Divider value is set based on soruce-frequency and + desired I2C bus frequency. If this value is not + provided, the source clock is assumed to be running + at 19.2 MHz. + +Aliases: An alias may optionally be used to bind the I2C controller +to bus number. Aliases are of the form i2c<n> where <n> is an integer +representing the bus number to use. + +Example: + + aliases { + i2c0 = &i2c_A; + i2c1 = &i2c_B; + i2c2 = &i2c_C; + }; + + i2c_A: i2c@f9967000 { + compatible = "qcom,i2c-qup"; + reg = <0Xf9967000 0x1000>; + interrupts = <0 105 0>; + + pinctrl-names = "default"; + pinctrl-0 = <&i2c0_data>; + + clocks = <&core>, <&iface>; + clock-names = "core", "iface"; + + clock-frequency = <100000>; + qcom,src-freq = <50000000>; + status = "disabled"; + + #address-cells = <1>; + #size-cells = <0>; + + dummy@60 { + compatible = "dummy"; + reg = <0x60>; + }; + }; + + i2c_B: i2c@f9923000 { + compatible = "qcom,i2c-qup"; + reg = <0xf9923000 0x1000>; + interrupts = <0 95 0>; + + pinctrl-names = "default"; + pinctrl-0 = <&i2c1_data>; + + clocks = <&core>, <&iface>; + clock-names = "core", "iface"; + + clock-frequency = <100000>; + qcom,src-freq = <19200000>; + + #address-cells = <1>; + #size-cells = <0>; + }; + + i2c_C: i2c@f9924000 { + compatible = "qcom,i2c-qup"; + reg = <0xf9924000 0x1000>; + interrupts = <0 96 0>; + + pinctrl-names = "default"; + pinctrl-0 = <&i2c5_data>; + + clocks = <&core>, <&iface>; + clock-names = "core", "iface"; + + clock-frequency = <100000>; + qcom,src-freq = <50000000>; + + #address-cells = <1>; + #size-cells = <0>; + };