* Re: [PATCH v3 0/7] Add pwm and IIO timer drivers for stm32
From: Jonathan Cameron @ 2016-12-03 9:32 UTC (permalink / raw)
To: Benjamin Gaignard, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
alexandre.torgue-qxv4g6HH51o, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
linux-pwm-u79uwXL29TY76Z2rM5mHXA, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: fabrice.gasnier-qxv4g6HH51o, gerald.baeza-qxv4g6HH51o,
arnaud.pouliquen-qxv4g6HH51o,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw, Benjamin Gaignard
In-Reply-To: <1480673842-20804-1-git-send-email-benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
On 02/12/16 10:17, Benjamin Gaignard wrote:
> version 3:
> - no change on mfd and pwm divers patches
> - add cross reference between bindings
> - change compatible to "st,stm32-timer-trigger"
> - fix attributes access rights
> - use string instead of int for master_mode and slave_mode
> - document device attributes in sysfs-bus-iio-timer-stm32
> - udpate DT with the new compatible
>
> version 2:
> - keep only one compatible per driver
> - use DT parameters to describe hardware block configuration:
> - pwm channels, complementary output, counter size, break input
> - triggers accepted and create by IIO timers
> - change DT to limite use of reference to the node
> - interrupt is now in IIO timer driver
> - rename stm32-mfd-timer to stm32-gptimer (for general purpose timer)
>
> The following patches enable pwm and IIO Timer features for stm32 platforms.
>
> Those two features are mixed into the registers of the same hardware block
> (named general purpose timer) which lead to introduce a multifunctions driver
> on the top of them to be able to share the registers.
>
> In stm32 14 instances of timer hardware block exist, even if they all have
> the same register mapping they could have a different number of pwm channels
> and/or different triggers capabilities. We use various parameters in DT to
> describe the differences between hardware blocks
>
> The MFD (stm32-gptimer.c) takes care of clock and register mapping
> by using regmap. stm32_gptimer_dev structure is provided to its sub-node to
> share those information.
>
> PWM driver is implemented into pwm-stm32.c. Depending of the instance we may
> have up to 4 channels, sometime with complementary outputs or 32 bits counter
> instead of 16 bits. Some hardware blocks may also have a break input function
> which allows to stop pwm depending of a level, defined in devicetree, on an
> external pin.
>
> IIO timer driver (stm32-iio-timer.c and stm32-iio-timers.h) define a list of
> hardware triggers usable by hardware blocks like ADC, DAC or other timers.
>
> The matrix of possible connections between blocks is quite complex so we use
> trigger names and is_stm32_iio_timer_trigger() function to be sure that
> triggers are valid and configure the IPs.
> Possible triggers ar listed in include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>
> At run time IIO timer hardware blocks can configure (through "master_mode"
> IIO device attribute) which internal signal (counter enable, reset,
> comparison block, etc...) is used to generate the trigger.
>
> By using "slave_mode" IIO device attribute timer can also configure on which
> event (level, rising edge) of the block is enabled.
>
> Since we can use trigger from one hardware to control an other block, we can
> use a pwm to control an other one. The following example shows how to configure
> pwm1 and pwm3 to make pwm3 generate pulse only when pwm1 pulse level is high.
>
> /sys/bus/iio/devices # ls
> iio:device0 iio:device1 trigger0 trigger1
>
> configure timer1 to use pwm1 channel 0 as output trigger
> /sys/bus/iio/devices # echo 'OC1REF' > iio\:device0/master_mode
> configure timer3 to enable only when input is high
> /sys/bus/iio/devices # echo 'gated' > iio\:device1/slave_mode
> /sys/bus/iio/devices # cat trigger0/name
> tim1_trgo
> configure timer2 to use timer1 trigger is input
> /sys/bus/iio/devices # echo "tim1_trgo" > iio\:device1/trigger/current_trigger
>
> configure pwm3 channel 0 to generate a signal with a period of 100ms and a
> duty cycle of 50%
> /sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 0 > export
> /sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 100000000 > pwm0/period
> /sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 50000000 > pwm0/duty_cycle
> /sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4# echo 1 > pwm0/enable
> here pwm3 channel 0, as expected, doesn't start because has to be triggered by
> pwm1 channel 0
>
> configure pwm1 channel 0 to generate a signal with a period of 1s and a
> duty cycle of 50%
> /sys/devices/platform/soc/40010000.gptimer1/40010000.gptimer1:pwm1@0/pwm/pwmchip0 # echo 0 > export
> /sys/devices/platform/soc/40010000.gptimer1/40010000.gptimer1:pwm1@0/pwm/pwmchip0 # echo 1000000000 > pwm0/period
> /sys/devices/platform/soc/40010000.gptimer1/40010000.gptimer1:pwm1@0/pwm/pwmchip0 # echo 500000000 > pwm0/duty_cycle
> /sys/devices/platform/soc/40010000.gptimer1/40010000.gptimer1:pwm1@0/pwm/pwmchip0 # echo 1 > pwm0/enable
> finally pwm1 starts and pwm3 only generates pulse when pwm1 signal is high
>
> An other example to use a timer as source of clock for another device.
> Here timer1 is used a source clock for pwm3:
>
> /sys/bus/iio/devices # echo 100000 > trigger0/sampling_frequency
> /sys/bus/iio/devices # echo tim1_trgo > iio\:device1/trigger/current_trigger
> /sys/bus/iio/devices # echo 'external_clock' > iio\:device1/slave_mode
> /sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 0 > export
> /sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 1000000 > pwm0/period
> /sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 500000 > pwm0/duty_cycle
> /sys/devices/platform/soc/40000400.gptimer3/40000400.gptimer3:pwm3@0/pwm/pwmchip4 # echo 1 > pwm0/enable
This is good thorough documentation. Could we have an additional
patch adding just this documentation to the tree (probably under
Documentation/mfd?). Documentation in general is a bit in flux at the
moment so we'll may want to sphixify it afterwards but plain text
is fine for now.
Jonathan
>
> Benjamin Gaignard (7):
> MFD: add bindings for stm32 general purpose timer driver
> MFD: add stm32 general purpose timer driver
> PWM: add pwm-stm32 DT bindings
> PWM: add pwm driver for stm32 plaftorm
> IIO: add bindings for stm32 timer trigger driver
> IIO: add STM32 timer trigger driver
> ARM: dts: stm32: add stm32 general purpose timer driver in DT
>
> .../ABI/testing/sysfs-bus-iio-timer-stm32 | 47 ++
> .../bindings/iio/timer/stm32-timer-trigger.txt | 39 ++
> .../bindings/mfd/stm32-general-purpose-timer.txt | 47 ++
> .../devicetree/bindings/pwm/pwm-stm32.txt | 38 ++
> arch/arm/boot/dts/stm32f429.dtsi | 333 +++++++++++++-
> arch/arm/boot/dts/stm32f469-disco.dts | 28 ++
> drivers/iio/Kconfig | 2 +-
> drivers/iio/Makefile | 1 +
> drivers/iio/timer/Kconfig | 15 +
> drivers/iio/timer/Makefile | 1 +
> drivers/iio/timer/stm32-timer-trigger.c | 477 +++++++++++++++++++++
> drivers/iio/trigger/Kconfig | 1 -
> drivers/mfd/Kconfig | 10 +
> drivers/mfd/Makefile | 2 +
> drivers/mfd/stm32-gptimer.c | 73 ++++
> drivers/pwm/Kconfig | 8 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-stm32.c | 285 ++++++++++++
> .../iio/timer/st,stm32-timer-triggers.h | 60 +++
> include/linux/iio/timer/stm32-timer-trigger.h | 16 +
> include/linux/mfd/stm32-gptimer.h | 62 +++
> 21 files changed, 1543 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-timer-stm32
> create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt
> create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> create mode 100644 drivers/iio/timer/Kconfig
> create mode 100644 drivers/iio/timer/Makefile
> create mode 100644 drivers/iio/timer/stm32-timer-trigger.c
> create mode 100644 drivers/mfd/stm32-gptimer.c
> create mode 100644 drivers/pwm/pwm-stm32.c
> create mode 100644 include/dt-bindings/iio/timer/st,stm32-timer-triggers.h
> create mode 100644 include/linux/iio/timer/stm32-timer-trigger.h
> create mode 100644 include/linux/mfd/stm32-gptimer.h
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 6/7] IIO: add STM32 IIO timer driver
From: Jonathan Cameron @ 2016-12-03 9:28 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Lee Jones, robh+dt, Mark Rutland, alexandre.torgue, devicetree,
Linux Kernel Mailing List, Thierry Reding, linux-pwm, knaack.h,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
linux-arm-kernel, Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen,
Linus Walleij, Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <CA+M3ks7ks=YsToEyLcPhPf+JfdJtekUYcihpcYLLt0h6APbcfA@mail.gmail.com>
On 29/11/16 09:46, Benjamin Gaignard wrote:
> 2016-11-27 16:42 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>> I delved into the datasheet after trying to figure this out, so I think
>> I now sort of understand your intent, but please do answer the questions
>> inline.
>>
>> On 24/11/16 15:14, Benjamin Gaignard wrote:
>>> Timers IPs can be used to generate triggers for other IPs like
>>> DAC, ADC or other timers.
>>> Each trigger may result of timer internals signals like counter enable,
>>> reset or edge, this configuration could be done through "master_mode"
>>> device attribute.
>>>
>>> A timer device could be triggered by other timers, we use the trigger
>>> name and is_stm32_iio_timer_trigger() function to distinguish them
>>> and configure IP input switch.
>> The presence of an IIO device in here was a suprise.. What is it actually for?
>
> IIO device is needed to be able to valid the input triggers, which
> aren't the same than
> those generated by the device.
> Since I use triggers name to distinguish them I have introduced
> is_stm32_iio_timer_trigger()
> function to be sure that triggers are coming for a valid hardware and
> not from a fake one
> using the same name.
>
>>
>> I think this needs some examples of usage to make it clear what the aim is.
>
> In the hardware block there is switch in input to select which trigger
> will drive the IP.
> For example that allow to start multiple pwm exactly that the same
> time or to start/stop
> it on master edges.
Hmm. OK. We need to think about how to represent this concept of a tree
of triggers - independent of having an IIO device as that is down right
misleading.
In the first instance the tree is full supported by this one driver I think?
If so lets use it as a testbed and try and put together a simple tree between
the triggers.
So the child triggers (started on the parent firing) can perhaps have a
'parent' attribute? (might be better naming possible!)
>
>>
>> I was basically expecting to see a driver instantiating one iio trigger
>> per timer that can act as a trigger. Those would each have sampling frequency
>> controls and basica enable / disable.
>
> An hardware device could have up to 5 triggers: timX_trgo, timX_ch1, timX_ch2,
> timX_ch3, timX_ch4.
On a train so I don't have the datasheet. Which of these would actually make
sense when driving an adc scan?
> Until now I have try to simplify the problem and just use timX_trgo trigger.
> I have added a "sampling_frequency" attribute on the trigger to
> control the frequence
> and I use trigger set_state function to enable disable it.
Wise move! Makes sense to build this up in baby steps if possible.
>
>>
>> I'm seeing something much more complex here so additional explanation is
>> needed.
>>>
>>> Timer may also decide on which event (edge, level) they could
>>> be activated by a trigger, this configuration is done by writing in
>>> "slave_mode" device attribute.
>> Really? Sounds like magic numbers in sysfs which is never a good idea.
>> Please document those attributes / or break them up into elements that
>> don't require magic numbers.
>
> I would like to use strings here, it is possible to use IIO_CONST_ATTR
> to describe them ?
If it's on the iio device use the iio_ext_info stuff that has support
for enums. If you need this for the trigger feel free to add equivalent
support to the core as needed.
Note that we are still evolving IIO so if we need new stuff to support
your usecase, never be afraid of proposing it! The only element
I am keen on is keeping anything that is opaque to drivers opaque
unless there is a VERY good reason to do otherwise. Mostly this
just means using access functions etc. That makes messing around
with the core internals (as still happens from time to time) a lot
less painful!
>
>>>
>>> Since triggers could also be used by DAC or ADC their names are defined
>>> in include/dt-bindings/iio/timer/st,stm32-iio-timer.h so those IPs will be able
>>> to configure themselves in valid_trigger function
>>>
>>> Trigger have a "sampling_frequency" attribute which allow to configure
>>> timer sampling frequency without using pwm interface
>>>
>>> version 2:
>>> - keep only one compatible
>> Hmm. I'm not sure I like this as such. We are actually dealing with lots
>> of instances of a hardware block with only a small amount of shared
>> infrastrcuture (which is classic mfd teritory). So to my mind we
>> should have a separate device for each.
>
> Registers mapping and offset are the same, from triggers point of view
> only the configuration of the input switch is different.
>
>>
>>> - use st,input-triggers-names and st,output-triggers-names
>>> to know which triggers are accepted and/or create by the device
>> I'm not following why we have this cascade setup?
>>
>> These are triggers, not devices in the IIO context. We need some detailed
>> description of why you have it setup like this. This would include the
>> ABI with examples of how you are using it.
>
> I had put example of usage on the cover letter, I will duplicate them
> in this commit
> message.
Ooops. I didn't ready that ;) Sorry.
>
>>
>> Basically I don't currently understand what you are doing :(
>>
>>
>> Thanks,
>>
>> Jonathan
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>> ---
>>> drivers/iio/Kconfig | 2 +-
>>> drivers/iio/Makefile | 1 +
>>> drivers/iio/timer/Kconfig | 15 +
>>> drivers/iio/timer/Makefile | 1 +
>>> drivers/iio/timer/stm32-iio-timer.c | 448 +++++++++++++++++++++
>>> drivers/iio/trigger/Kconfig | 1 -
>>> include/dt-bindings/iio/timer/st,stm32-iio-timer.h | 23 ++
>>> include/linux/iio/timer/stm32-iio-timers.h | 16 +
>>> 8 files changed, 505 insertions(+), 2 deletions(-)
>>> create mode 100644 drivers/iio/timer/Kconfig
>>> create mode 100644 drivers/iio/timer/Makefile
>>> create mode 100644 drivers/iio/timer/stm32-iio-timer.c
>>> create mode 100644 include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>> create mode 100644 include/linux/iio/timer/stm32-iio-timers.h
>>>
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 6743b18..2de2a80 100644
>>> --- a/drivers/iio/Kconfig
>>> +++ b/drivers/iio/Kconfig
>>> @@ -90,5 +90,5 @@ source "drivers/iio/potentiometer/Kconfig"
>>> source "drivers/iio/pressure/Kconfig"
>>> source "drivers/iio/proximity/Kconfig"
>>> source "drivers/iio/temperature/Kconfig"
>>> -
>>> +source "drivers/iio/timer/Kconfig"
>>> endif # IIO
>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>> index 87e4c43..b797c08 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -32,4 +32,5 @@ obj-y += potentiometer/
>>> obj-y += pressure/
>>> obj-y += proximity/
>>> obj-y += temperature/
>>> +obj-y += timer/
>>> obj-y += trigger/
>>> diff --git a/drivers/iio/timer/Kconfig b/drivers/iio/timer/Kconfig
>>> new file mode 100644
>>> index 0000000..7a73bc6
>>> --- /dev/null
>>> +++ b/drivers/iio/timer/Kconfig
>>> @@ -0,0 +1,15 @@
>>> +#
>>> +# Timers drivers
>>> +
>>> +menu "Timers"
>>> +
>>> +config IIO_STM32_TIMER
>>> + tristate "stm32 iio timer"
>>> + depends on ARCH_STM32
>>> + depends on OF
>>> + select IIO_TRIGGERED_EVENT
>>> + select MFD_STM32_GP_TIMER
>>> + help
>>> + Select this option to enable stm32 timers hardware IPs
>>> +
>>> +endmenu
>>> diff --git a/drivers/iio/timer/Makefile b/drivers/iio/timer/Makefile
>>> new file mode 100644
>>> index 0000000..a360c9f
>>> --- /dev/null
>>> +++ b/drivers/iio/timer/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_IIO_STM32_TIMER) += stm32-iio-timer.o
>>> diff --git a/drivers/iio/timer/stm32-iio-timer.c b/drivers/iio/timer/stm32-iio-timer.c
>>> new file mode 100644
>>> index 0000000..35f2687
>>> --- /dev/null
>>> +++ b/drivers/iio/timer/stm32-iio-timer.c
>>> @@ -0,0 +1,448 @@
>>> +/*
>>> + * stm32-iio-timer.c
>>> + *
>>> + * Copyright (C) STMicroelectronics 2016
>>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>>> + * License terms: GNU General Public License (GPL), version 2
>>> + */
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/iio/timer/stm32-iio-timers.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/triggered_event.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/mfd/stm32-gptimer.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#define DRIVER_NAME "stm32-iio-timer"
>>> +
>>> +struct stm32_iio_timer_dev {
>>> + struct device *dev;
>>> + struct regmap *regmap;
>>> + struct clk *clk;
>>> + int irq;
>>> + bool own_timer;
>>> + unsigned int sampling_frequency;
>>> + struct iio_trigger *active_trigger;
>>> +};
>>> +
>>> +static ssize_t _store_frequency(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct iio_trigger *trig = to_iio_trigger(dev);
>>> + struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>>> + unsigned int freq;
>>> + int ret;
>>> +
>>> + ret = kstrtouint(buf, 10, &freq);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + stm32->sampling_frequency = freq;
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static ssize_t _read_frequency(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_trigger *trig = to_iio_trigger(dev);
>>> + struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>>> + unsigned long long freq = stm32->sampling_frequency;
>>> + u32 psc, arr, cr1;
>>> +
>>> + regmap_read(stm32->regmap, TIM_CR1, &cr1);
>>> + regmap_read(stm32->regmap, TIM_PSC, &psc);
>>> + regmap_read(stm32->regmap, TIM_ARR, &arr);
>>> +
>>> + if (psc && arr && (cr1 & TIM_CR1_CEN)) {
>>> + freq = (unsigned long long)clk_get_rate(stm32->clk);
>>> + do_div(freq, psc);
>>> + do_div(freq, arr);
>>> + }
>>> +
>>> + return sprintf(buf, "%d\n", (unsigned int)freq);
>>> +}
>>> +
>>> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
>>> + _read_frequency,
>>> + _store_frequency);
>>> +
>>> +static struct attribute *stm32_trigger_attrs[] = {
>>> + &iio_dev_attr_sampling_frequency.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group stm32_trigger_attr_group = {
>>> + .attrs = stm32_trigger_attrs,
>>> +};
>>> +
>>> +static const struct attribute_group *stm32_trigger_attr_groups[] = {
>>> + &stm32_trigger_attr_group,
>>> + NULL,
>>> +};
>>> +
>>> +static
>>> +ssize_t _show_master_mode(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>>> + u32 cr2;
>>> +
>>> + regmap_read(stm32->regmap, TIM_CR2, &cr2);
>>> +
>>> + return snprintf(buf, PAGE_SIZE, "%d\n", (cr2 >> 4) & 0x7);
>>> +}
>>> +
>>> +static
>>> +ssize_t _store_master_mode(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>>> + u8 mode;
>>> + int ret;
>>> +
>>> + ret = kstrtou8(buf, 10, &mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (mode > 0x7)
>>> + return -EINVAL;
>>> +
>>> + regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, mode << 4);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(master_mode, S_IRUGO | S_IWUSR,
>>> + _show_master_mode,
>>> + _store_master_mode,
>>> + 0);
>>> +
>>> +static
>>> +ssize_t _show_slave_mode(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>>> + u32 smcr;
>>> +
>>> + regmap_read(stm32->regmap, TIM_SMCR, &smcr);
>>> +
>>> + return snprintf(buf, PAGE_SIZE, "%d\n", smcr & 0x3);
>>> +}
>>> +
>>> +static
>>> +ssize_t _store_slave_mode(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
>>> + u8 mode;
>>> + int ret;
>>> +
>>> + ret = kstrtou8(buf, 10, &mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (mode > 0x7)
>>> + return -EINVAL;
>> How is something called slave mode going to be fed a number between 0 and 7?
>> Rule of thumb is no magic numbers in sysfs and right now this is looking
>> rather cryptic to say the least!
>
> I would like to use strings here, it is possible to use IIO_CONST_ATTR
> to describe them ?
> In documentation slave modes are describe that this:
> 000: Slave mode disabled - if CEN = ‘1’ then the prescaler is clocked
> directly by the internal clock.
> 001: Encoder mode 1 - Counter counts up/down on TI2FP1 edge depending
> on TI1FP2 level.
> 010: Encoder mode 2 - Counter counts up/down on TI1FP2 edge depending
> on TI2FP1 level.
> 011: Encoder mode 3 - Counter counts up/down on both TI1FP1 and TI2FP2
> edges depending on the level of the other input.
> 100: Reset Mode - Rising edge of the selected trigger input (TRGI)
> reinitializes the counter and generates an update of the registers.
> 101: Gated Mode - The counter clock is enabled when the trigger input
> (TRGI) is high.
> The counter stops (but is not reset) as soon as the trigger becomes low.
> Both start and stop of the counter are controlled.
> 110: Trigger Mode - The counter starts at a rising edge of the trigger
> TRGI (but it is notreset).
> Only the start of the counter is controlled.
> 111: External Clock Mode 1 - Rising edges of the selected trigger
> (TRGI) clock the counter.
>
>>> +
>>> + regmap_update_bits(stm32->regmap, TIM_SMCR, TIM_SMCR_SMS, mode);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(slave_mode, S_IRUGO | S_IWUSR,
>> There is an iritating move (in terms of noise it's generating) to use values
>> directly instead fo these defines. Still if you don't fix it here I'll only
>> get a patch 'fixing' it soon after...
>
> I will fix at in version 3
>
>>
>>> + _show_slave_mode,
>>> + _store_slave_mode,
>>> + 0);
>>> +
>>> +static struct attribute *stm32_timer_attrs[] = {
>>> + &iio_dev_attr_master_mode.dev_attr.attr,
>>> + &iio_dev_attr_slave_mode.dev_attr.attr,
>> New ABI so must be documented under Documentation/ABI/testing/sysfs-bus-iio-*
>
> OK
>
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group stm32_timer_attr_group = {
>>> + .attrs = stm32_timer_attrs,
>>> +};
>>> +
>>> +static int stm32_timer_start(struct stm32_iio_timer_dev *stm32)
>>> +{
>>> + unsigned long long prd, div;
>>> + int prescaler = 0;
>>> + u32 max_arr = 0xFFFF, cr1;
>>> +
>>> + if (stm32->sampling_frequency == 0)
>>> + return 0;
>>> +
>>> + /* Period and prescaler values depends of clock rate */
>>> + div = (unsigned long long)clk_get_rate(stm32->clk);
>>> +
>>> + do_div(div, stm32->sampling_frequency);
>>> +
>>> + prd = div;
>>> +
>>> + while (div > max_arr) {
>>> + prescaler++;
>>> + div = prd;
>>> + do_div(div, (prescaler + 1));
>>> + }
>>> + prd = div;
>>> +
>>> + if (prescaler > MAX_TIM_PSC) {
>>> + dev_err(stm32->dev, "prescaler exceeds the maximum value\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Check that we own the timer */
>>> + regmap_read(stm32->regmap, TIM_CR1, &cr1);
>>> + if ((cr1 & TIM_CR1_CEN) && !stm32->own_timer)
>>> + return -EBUSY;
>>> +
>>> + if (!stm32->own_timer) {
>>> + stm32->own_timer = true;
>>> + clk_enable(stm32->clk);
>>> + }
>>> +
>>> + regmap_write(stm32->regmap, TIM_PSC, prescaler);
>>> + regmap_write(stm32->regmap, TIM_ARR, prd - 1);
>>> + regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
>>> +
>>> + /* Force master mode to update mode */
>>> + regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, 0x20);
>>> +
>>> + /* Make sure that registers are updated */
>>> + regmap_update_bits(stm32->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>>> +
>>> + /* Enable interrupt */
>>> + regmap_write(stm32->regmap, TIM_SR, 0);
>>> + regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, TIM_DIER_UIE);
>>> +
>>> + /* Enable controller */
>>> + regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int stm32_timer_stop(struct stm32_iio_timer_dev *stm32)
>>> +{
>>> + if (!stm32->own_timer)
>>> + return 0;
>>> +
>>> + /* Stop timer */
>>> + regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, 0);
>>> + regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>>> + regmap_write(stm32->regmap, TIM_PSC, 0);
>>> + regmap_write(stm32->regmap, TIM_ARR, 0);
>>> +
>>> + clk_disable(stm32->clk);
>>> +
>>> + stm32->own_timer = false;
>>> + stm32->active_trigger = NULL;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int stm32_set_trigger_state(struct iio_trigger *trig, bool state)
>>> +{
>>> + struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
>>> +
>>> + stm32->active_trigger = trig;
>>> +
>>> + if (state)
>>> + return stm32_timer_start(stm32);
>>> + else
>>> + return stm32_timer_stop(stm32);
>>> +}
>>> +
>>> +static irqreturn_t stm32_timer_irq_handler(int irq, void *private)
>>> +{
>>> + struct stm32_iio_timer_dev *stm32 = private;
>>> + u32 sr;
>>> +
>>> + regmap_read(stm32->regmap, TIM_SR, &sr);
>>> + regmap_write(stm32->regmap, TIM_SR, 0);
>>> +
>>> + if ((sr & TIM_SR_UIF) && stm32->active_trigger)
>>> + iio_trigger_poll(stm32->active_trigger);
>> This is acting like a trigger cascading off another trigger?
>
> Not only a trigger but ADC or DAC too.
>
>>
>> Normally this interrupt handler would be directly associated with the
>> trigger hardware - in this case the timer.
>
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static const struct iio_trigger_ops timer_trigger_ops = {
>>> + .owner = THIS_MODULE,
>>> + .set_trigger_state = stm32_set_trigger_state,
>>> +};
>>> +
>>> +static int stm32_setup_iio_triggers(struct stm32_iio_timer_dev *stm32)
>>> +{
>>> + int ret;
>>> + struct property *p;
>>> + const char *cur = NULL;
>>> +
>>> + p = of_find_property(stm32->dev->of_node,
>>> + "st,output-triggers-names", NULL);
>>> +
>>> + while ((cur = of_prop_next_string(p, cur)) != NULL) {
>>> + struct iio_trigger *trig;
>>> +
>>> + trig = devm_iio_trigger_alloc(stm32->dev, "%s", cur);
>>> + if (!trig)
>>> + return -ENOMEM;
>>> +
>>> + trig->dev.parent = stm32->dev->parent;
>>> + trig->ops = &timer_trigger_ops;
>>> + trig->dev.groups = stm32_trigger_attr_groups;
>>> + iio_trigger_set_drvdata(trig, stm32);
>>> +
>>> + ret = devm_iio_trigger_register(stm32->dev, trig);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * is_stm32_iio_timer_trigger
>>> + * @trig: trigger to be checked
>>> + *
>>> + * return true if the trigger is a valid stm32 iio timer trigger
>>> + * either return false
>>> + */
>>> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig)
>>> +{
>>> + return (trig->ops == &timer_trigger_ops);
>>> +}
>>> +EXPORT_SYMBOL(is_stm32_iio_timer_trigger);
>>> +
>>> +static int stm32_validate_trigger(struct iio_dev *indio_dev,
>>> + struct iio_trigger *trig)
>>> +{
>>> + struct stm32_iio_timer_dev *dev = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + if (!is_stm32_iio_timer_trigger(trig))
>>> + return -EINVAL;
>>> +
>>> + ret = of_property_match_string(dev->dev->of_node,
>>> + "st,input-triggers-names",
>>> + trig->name);
>>> +
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + regmap_update_bits(dev->regmap, TIM_SMCR, TIM_SMCR_TS, ret << 4);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct iio_info stm32_trigger_info = {
>>> + .driver_module = THIS_MODULE,
>>> + .validate_trigger = stm32_validate_trigger,
>>> + .attrs = &stm32_timer_attr_group,
>>> +};
>>> +
>>> +static struct stm32_iio_timer_dev *stm32_setup_iio_device(struct device *dev)
>>> +{
>>> + struct iio_dev *indio_dev;
>>> + int ret;
>>> +
>>> + indio_dev = devm_iio_device_alloc(dev, sizeof(struct stm32_iio_timer_dev));
>>> + if (!indio_dev)
>>> + return NULL;
>> This is 'unusual'. Why does a trigger driver need an iio_dev at all?
>
> Trigger doesn't need it but for configuring the input switch when
> validating the triggers I need a device
As suggested above, lets pull this trigger cascade clear of involving devices
at all. It's nice functionality to have anyway. Once we've figured it
out for this driver, I'd like to generalize it as I think the same stuff could
be used to do clean setup of oscilloscope sampling approaches for more
complex sensor setups...
>
>>> +
>>> + indio_dev->name = dev_name(dev);
>>> + indio_dev->dev.parent = dev;
>>> + indio_dev->info = &stm32_trigger_info;
>>> + indio_dev->modes = INDIO_EVENT_TRIGGERED;
>>> + indio_dev->num_channels = 0;
>>> + indio_dev->dev.of_node = dev->of_node;
>>> +
>>> + ret = iio_triggered_event_setup(indio_dev,
>>> + NULL,
>>> + stm32_timer_irq_handler);
>> So the iio_dev exists to provide the ability to fire this interrupt from
>> another trigger? Why do you want to do this?
>
> I need interrupt because I use set_trigger_state() to enable/disable
> the sampling frequency.
> As far I understand and test set_trigger_state() is only called when
> indio_dev->modes = INDIO_EVENT_TRIGGERED
> and iio_triggered_event_setup have been called to create register an
> irq handler.
> I just need irq declaration for this last condition, I don't need the
> irq to fire in kernel to drive other hardware block.
>
> If I could use set_trigger_state() without calling using
> iio_triggered_event_setup() I should remove all
> irq code from the driver.
>
> One possible solution would be to add calls to set_trigger_state() in
> iio_trigger_write_current() function
> at the same level than iio_trigger_detach_poll_func() or
> iio_trigger_attach_poll_func() calls:
I fear this may introduce race conditions in drivers that assume this stuff
can't change whilst the trigger is enabled.
Bit too risky a change to my mind.
If you need to add functions to explicitly do such a trigger enable, then
feel free to propose them. I never have a problem with adding core
functionality if that is the best way to solve a particular issue.
(subject to standard questions of maintainability and insisting they have
good documentation - do as I say, not as I did years ago ;)
>
> if (indio_dev->modes = INDIO_DIRECT_MODE && oldtrig->ops->set_trigger_state)
> oldtrig->ops->set_trigger_state(oldtrig, false);
>
> if (indio_dev->modes = INDIO_DIRECT_MODE &&
> indio_dev->trig->ops->set_trigger_state)
> indio_dev->trig->ops->set_trigger_state(indio_dev->trig, true);
>
> I'm to new in IIO framework to understand if that it possible or not
>
>>> + if (ret)
>>> + return NULL;
>>> +
>>> + ret = devm_iio_device_register(dev, indio_dev);
>>> + if (ret) {
>>> + iio_triggered_event_cleanup(indio_dev);
>>> + return NULL;
>>> + }
>>> +
>>> + return iio_priv(indio_dev);
>>> +}
>>> +
>>> +static int stm32_iio_timer_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct stm32_iio_timer_dev *stm32;
>>> + struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
>>> + int ret;
>>> +
>>> + stm32 = stm32_setup_iio_device(dev);
>>> + if (!stm32)
>>> + return -ENOMEM;
>>> +
>>> + stm32->dev = dev;
>>> + stm32->regmap = mfd->regmap;
>>> + stm32->clk = mfd->clk;
>>> +
>>> + stm32->irq = platform_get_irq(pdev, 0);
>>> + if (stm32->irq < 0)
>>> + return -EINVAL;
>>> +
>>> + ret = devm_request_irq(stm32->dev, stm32->irq,
>>> + stm32_timer_irq_handler, IRQF_SHARED,
>>> + "iiotimer_event", stm32);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = stm32_setup_iio_triggers(stm32);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + platform_set_drvdata(pdev, stm32);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int stm32_iio_timer_remove(struct platform_device *pdev)
>>> +{
>>> + struct stm32_iio_timer_dev *stm32 = platform_get_drvdata(pdev);
>>> +
>>> + iio_triggered_event_cleanup((struct iio_dev *)stm32);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id stm32_trig_of_match[] = {
>>> + {
>>> + .compatible = "st,stm32-iio-timer",
>>> + },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
>>> +
>>> +static struct platform_driver stm32_iio_timer_driver = {
>>> + .probe = stm32_iio_timer_probe,
>>> + .remove = stm32_iio_timer_remove,
>>> + .driver = {
>>> + .name = DRIVER_NAME,
>>> + .of_match_table = stm32_trig_of_match,
>>> + },
>>> +};
>>> +module_platform_driver(stm32_iio_timer_driver);
>>> +
>>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>>> +MODULE_DESCRIPTION("STMicroelectronics STM32 iio timer driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
>>> index 809b2e7..f2af4fe 100644
>>> --- a/drivers/iio/trigger/Kconfig
>>> +++ b/drivers/iio/trigger/Kconfig
>>> @@ -46,5 +46,4 @@ config IIO_SYSFS_TRIGGER
>>>
>>> To compile this driver as a module, choose M here: the
>>> module will be called iio-trig-sysfs.
>>> -
>> Clear this out...
>>> endmenu
>>> diff --git a/include/dt-bindings/iio/timer/st,stm32-iio-timer.h b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>> new file mode 100644
>>> index 0000000..d39bf16
>>> --- /dev/null
>>> +++ b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>> @@ -0,0 +1,23 @@
>>> +/*
>>> + * st,stm32-iio-timer.h
>>> + *
>>> + * Copyright (C) STMicroelectronics 2016
>>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>>> + * License terms: GNU General Public License (GPL), version 2
>>> + */
>>> +
>>> +#ifndef _DT_BINDINGS_IIO_TIMER_H_
>>> +#define _DT_BINDINGS_IIO_TIMER_H_
>>> +
>>> +#define TIM1_TRGO "tim1_trgo"
>>> +#define TIM2_TRGO "tim2_trgo"
>>> +#define TIM3_TRGO "tim3_trgo"
>>> +#define TIM4_TRGO "tim4_trgo"
>>> +#define TIM5_TRGO "tim5_trgo"
>>> +#define TIM6_TRGO "tim6_trgo"
>>> +#define TIM7_TRGO "tim7_trgo"
>>> +#define TIM8_TRGO "tim8_trgo"
>>> +#define TIM9_TRGO "tim9_trgo"
>>> +#define TIM12_TRGO "tim12_trgo"
>>> +
>>> +#endif
>>> diff --git a/include/linux/iio/timer/stm32-iio-timers.h b/include/linux/iio/timer/stm32-iio-timers.h
>>> new file mode 100644
>>> index 0000000..5d1b86c
>>> --- /dev/null
>>> +++ b/include/linux/iio/timer/stm32-iio-timers.h
>>> @@ -0,0 +1,16 @@
>>> +/*
>>> + * stm32-iio-timers.h
>>> + *
>>> + * Copyright (C) STMicroelectronics 2016
>>> + * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>>> + * License terms: GNU General Public License (GPL), version 2
>>> + */
>>> +
>>> +#ifndef _STM32_IIO_TIMERS_H_
>>> +#define _STM32_IIO_TIMERS_H_
>>> +
>>> +#include <dt-bindings/iio/timer/st,stm32-iio-timer.h>
>>> +
>>> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig);
>>> +
>>> +#endif
>>>
>>
^ permalink raw reply
* Re: [PATCH v3 00/13] net: ethernet: ti: cpts: update and fixes
From: Richard Cochran @ 2016-12-03 9:22 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N,
Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA, Murali Karicheri, Wingman Kwok,
Thomas Gleixner
In-Reply-To: <20161202203023.25526-1-grygorii.strashko-l0cyMroinI0@public.gmane.org>
On Fri, Dec 02, 2016 at 02:30:10PM -0600, Grygorii Strashko wrote:
> It is preparation series intended to clean up and optimize TI CPTS driver to
> facilitate further integration with other TI's SoCs like Keystone 2.
>
> Changes in v3:
> - patches reordered: fixes and small updates moved first
> - added comments in code about cpts->cc_mult
> - conversation range (maxsec) limited to 10sec
On net-next:
$ git am ~/grygorii.strashko
Applying: net: ethernet: ti: cpts: switch to readl/writel_relaxed()
Applying: net: ethernet: ti: allow cpts to be built separately
error: patch failed: drivers/net/ethernet/ti/cpsw.c:1963
error: drivers/net/ethernet/ti/cpsw.c: patch does not apply
Patch failed at 0002 net: ethernet: ti: allow cpts to be built separately
Also, you have the order of the SOB tags wrong. The author's SOB goes
first.
Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/7] MFD: add bindings for stm32 general purpose timer driver
From: Jonathan Cameron @ 2016-12-03 9:14 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Lee Jones, robh+dt, Mark Rutland, alexandre.torgue, devicetree,
Linux Kernel Mailing List, Thierry Reding, linux-pwm, knaack.h,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
linux-arm-kernel, Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen,
Linus Walleij, Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <CA+M3ks4Y4FnJGqwyH0oitrxvzRnKNHA261wqfEOSfc1aA4am4g@mail.gmail.com>
On 29/11/16 08:48, Benjamin Gaignard wrote:
> 2016-11-27 16:41 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>> On 27/11/16 14:10, Jonathan Cameron wrote:
>>> On 24/11/16 15:14, Benjamin Gaignard wrote:
>>>> Add bindings information for stm32 general purpose timer
>>>>
>>>> version 2:
>>>> - rename stm32-mfd-timer to stm32-gptimer
>>>> - only keep one compatible string
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>>>> ---
>>>> .../bindings/mfd/stm32-general-purpose-timer.txt | 43 ++++++++++++++++++++++
>>>> 1 file changed, 43 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>>> new file mode 100644
>>>> index 0000000..2f10e67
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>>> @@ -0,0 +1,43 @@
>>>> +STM32 general purpose timer driver
>>>> +
>>>> +Required parameters:
>>>> +- compatible: must be "st,stm32-gptimer"
>>>> +
>>>> +- reg: Physical base address and length of the controller's
>>>> + registers.
>>>> +- clock-names: Set to "clk_int".
>>>> +- clocks: Phandle to the clock used by the timer module.
>>>> + For Clk properties, please refer to ../clock/clock-bindings.txt
>>>> +
>>>> +Optional parameters:
>>>> +- resets: Phandle to the parent reset controller.
>>>> + See ..reset/st,stm32-rcc.txt
>>>> +
>>>> +Optional subnodes:
>>>> +- pwm: See ../pwm/pwm-stm32.txt
>>>> +- iiotimer: See ../iio/timer/stm32-iio-timer.txt
>>> Naming issue here. Can't mention IIO as that's a linux subsystem and all
>>> bindings must be independent of OS.
>>>
>>> Perhaps adc-trigger-timer?
>>>> +
>>>> +Example:
>>>> + gptimer1: gptimer1@40010000 {
>>>> + compatible = "st,stm32-gptimer";
>>>> + reg = <0x40010000 0x400>;
>>>> + clocks = <&rcc 0 160>;
>>>> + clock-names = "clk_int";
>>>> +
>>>> + pwm1@0 {
>>>> + compatible = "st,stm32-pwm";
>>>> + st,pwm-num-chan = <4>;
>>>> + st,breakinput;
>>>> + st,complementary;
>>>> + };
>>>> +
>>>> + iiotimer1@0 {
>>>> + compatible = "st,stm32-iio-timer";
>>> Again, avoid the use of iio in here (same issue you had with mfd in the previous
>>> version).
>>>> + interrupts = <27>;
>>>> + st,input-triggers-names = TIM5_TRGO,
>>> Docs for these should be introduced before they are used in an example.
>>> Same for the PWM ones above. Expand the detail fo the example as you add
>>> the other elements.
>>
>> I've just dived into the datasheet for these timers.
>> http://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf
>>
>
> I really appreciate that you do this effort, thanks.
>
>>
>> I think you need a binding that describes the capabilities of each of the timers
>> explicitly. Down to the level of whether there is a repetition counter or not.
>> Each should exists as a separate entity in device tree.
>>
>> They then have an existence as timers separate to the description of what they
>> are used for.
>>
>> Here the only way we are saying they exist is by their use which doesn't feel
>> right to me.
>>
>> So I think you need to move back to what you had in the first place. The key
>> thing is that ever timer needs describing fully. They are different enough
>> that for example the datasheet doesn't even try to describe them in one section.
>> (it has 4 separate chapters covering different sets of these hardware blocks).
>> The naming isn't really based on index, we are talking different hardware
>> that the datasheet authors have decided not to give different names to!
>
> Even if the hardware are named differently in the documentation they
> all share the
> same registers definitions and mapping but configurations are
> different for each device.
>
>>
>> If they'd called them
>> advanced timers
>> generic timers
>> basic timers
>> really basic timers meant for driving the DAC (6 and 7)
>>
>> We'd all have been quite happy with different compatible strings giving away
>> what they can do.
>
> 4 compatible strings will not be enough to describe devices
> configuration, for example
> in the documentation general purpose timers could have a 16 or 32 bit
> counter, for 1 to 4
> pwm channels and triggers (accepted or generated by the device) are
> different for each device.
>
> DAC could be drive by timers 2, 4, 5, 6, 7 and 8.
> ADC could be driver by 32 triggers
My point was more about the fact that though the naming appears to (and kind
of does describe an index) these devices are really as different as for example
different part numbers would imply on a range of ADCs (say the multitude supported
by the max1363 driver - all of which are very nearly register compatible)
Hence I'd be less quick to dismiss the option of a number of compatible strings
rather than the wealth of description you'll otherwise have to put in the device
tree.
>
>> What you have here is far too specific to what you are trying to do with them
>> right now.
>>
>> These things are separately capable of timing capture (which is I guess where
>> the IIO device later comes in).
>>
>> So my expectation is that we end up potentially instantiating:
>>
>> 1) An MFD to handle the shared elements of the timers.
>> 2) Up to 12ish timers each with separate existence as a device in the driver model
>> and in device tree.
>> (nasty corner cases here are using timers as perscalers for other timers - I'd be
>> tempted to leave that for now)
>> Note that each of these devices has a different register set I think? Any shared
>> bits are handled via the mfd above (if we even need that MFD which I'm starting
>> to doubt looking at the datasheet).
>>
>
> pwm and trigger share the same registers but not the same bits.
> With regmap write functions I don't have sharing problems.
>
>> 3) Up to N pwms again with there own existence in the device model. These don't
>> do much other than wrap the timer and stick it in output mode.
>> 4) Up to N iio triggers - this is basically using the timer as a periodic interrupt
>> (though without the interrupt having visibility to the kernel) which fires off
>> sampling on associated ADCs.
>> 5) Up to N iio capture devices for all channels that support timing capture.
>> Note there is also hardware encoder capture support in these which should be
>> correctly handled as well. This comes back to an ancient discussion on the
>> TI ecap units which have similar capabilities (driver never went anywhere but
>> I think that was because the author left TI).
>>
>> Certainly for the IIO devices these should no be bound up into one instance
>> as you have done here.
>>
>> Anyhow, I fear that right now this discussion is missing the key ingredient
>> that the hardware is horrendously variable in it's capabilities and really
>> is 4-5 different types of hardware that just happen to share a few bits of
>> their offsets in their register maps.
>
> Hardware really share the same registers mapping that why I have wrote
> one only driver
> per framework. Only the configurations are different
One driver is fine, but the difference to my mind are sufficient that
we may need to use compatible strings for the various options. Worth
a go at trying to fully describe them first though!
>
>>
>> So after all that I'm almost more confused than I was at the start!
>>
>> Jonathan
>>
>>
>>>> + TIM2_TRGO,
>>>> + TIM4_TRGO,
>>>> + TIM3_TRGO;
>>>> + st,output-triggers-names = TIM1_TRGO;
>>>> + };
>>>> + };
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>
>
^ permalink raw reply
* Re: [PATCH] iio: misc: add a generic regulator driver
From: Jonathan Cameron @ 2016-12-03 9:11 UTC (permalink / raw)
To: Lars-Peter Clausen, Bartosz Golaszewski
Cc: Hartmut Knaack, Peter Meerwald-Stadler, Rob Herring, Mark Rutland,
linux-iio-u79uwXL29TY76Z2rM5mHXA, linux-devicetree, LKML,
Kevin Hilman, Patrick Titiano, Neil Armstrong, Liam Girdwood,
Mark Brown
In-Reply-To: <d4c7f1ca-49d4-7abe-bfc9-e1728f62a9fb-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
On 30/11/16 10:10, Lars-Peter Clausen wrote:
> On 11/29/2016 04:35 PM, Bartosz Golaszewski wrote:
>> 2016-11-29 16:30 GMT+01:00 Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>:
>>> On 11/29/2016 04:22 PM, Bartosz Golaszewski wrote:
>>> [...]
>>>> diff --git a/Documentation/devicetree/bindings/iio/misc/iio-regulator.txt b/Documentation/devicetree/bindings/iio/misc/iio-regulator.txt
>>>> new file mode 100644
>>>> index 0000000..147458f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/misc/iio-regulator.txt
>>>> @@ -0,0 +1,18 @@
>>>> +Industrial IO regulator device driver
>>>> +-------------------------------------
>>>> +
>>>> +This document describes the bindings for the iio-regulator - a dummy device
>>>> +driver representing a physical regulator within the iio framework.
>>>
>>> No bindings for drivers, only for hardware. So this wont work.
>>>
>>
>> What about exporting regulator attributes analogous to the one in this
>> patch from the iio-core when a *-supply property is specified for a
>> node?
>
> The problem with exposing direct control to the regulator is that it allows
> to modify the hardware state without the drivers knowledge. If you
> power-cycle a device all previous configuration that has been written to the
> device is reset. The device driver needs to be aware of this otherwise its
> assumed state and the actual device state can divert which will result in
> undefined behavior. Also access to the device will fail unexpectedly when
> the regulator is turned off. So I think generally the driver should
> explicitly control the regulator, power-up when needed, power-down when not.
I agree with what Lars has said.
There 'may' be some argument to ultimately have a bridge driver from
regulators to IIO. That would be for cases where the divide between a regulator
and a DAC is blurred. However it would still have to play nicely with the
regulator framework and any other devices registered on that regulator.
Ultimately the ideal in that case would then be to describe what the DAC is
actually being used to do but that's a more complex issue!
That doesn't seem to be what you are targeting here.
What it sounds like you need is to have the hardware well enough described that
the standard runtime power management can disable the regulator just fine when
it is not in use. This may mean improving the power management in the relevant
drivers.
Jonathan
p.s. If ever proposing to do something 'unusual' with a regulator you should
bring in the regulator framework maintainers in the cc list.
>
> - Lars
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
From: Marek Vasut @ 2016-12-03 2:49 UTC (permalink / raw)
To: Masahiro Yamada, Rob Herring
Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Kernel Mailing List, Boris Brezillon, Brian Norris,
Richard Weinberger, David Woodhouse, Cyrille Pitchen,
Mark Rutland, Dinh Nguyen, Alan Tull, Chin Liang See
In-Reply-To: <CAK7LNAQHnH=On=+7fzenu_v6rB71y9hYuAZi5oinZFu-tfAdjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 12/03/2016 03:41 AM, Masahiro Yamada wrote:
> Hi Rob,
Hi!
> 2016-12-03 1:26 GMT+09:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>
>>>
>>>
>>> (Plan A)
>>> "denali,socfpga-nand" (for Altera SOCFPGA variant)
>>> "denali,uniphier-nand-v1" (for old Socionext UniPhier family variant)
>>> "denali,uniphier-nand-v2" (for new Socionext UniPhier family variant)
>>>
>>> (Plan B)
>>> "altera,denali-nand" (for Altera SOCFPGA variant)
>>> "socionext,denali-nand-v5a" (for old Socionext UniPhier family variant)
>>> "socionext,denali-nand-v5b" (for new Socionext UniPhier family variant)
>
>> Let the Altera folks worry about their stuff. At least for soft IP in
>> FPGA, it's a bit of a special case. The old string can remain as bad
>> as it is.
>
>
> Hmm, I am not sure if this IP would fit in FPGA
> (to use it along with NIOS-II?)
>
> (even if it happened, nothing of this IP would be customizable on users' side.
> When buying the IP, SoC vendors submit a list of desired features.
> Denali (now Cadence) generates the RTL according to the configuration sheet.
> The function is fixed at this point. So, generic compatible would be
> useless anyway.)
>
>
> If we are talking about SOCFPGA,
> SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
> It consists of two parts:
> [1] SOC part (Cortex-A9 + various hard-wired peripherals such UART,
> USB, SD, NAND, ...)
> [2] FPGA part (User design logic)
>
> The Denali NAND controller is included in [1].
> So, as far as we talk about the Denali on SOCFPGA,
> it is as hard-wired as Intel, Socionext's ones.
That's correct, the Denali NAND IP in altera socfpga is a hardware
block. You can make it available to the fabric too, but by default
it's used by the ARM part of the chip, so for this discussion, you
can forget that the FPGA part exists altogether.
I would be in favor of plan B, since it seems to be the more often
taken approach. A nice example is ci-hdrc:
$ git grep compatible drivers/usb/chipidea/
>> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
>> The fact that it is denali is part of the documentation.
>>
>
> Let me think about this.
>
> Socionext bought two version of Denali IP,
> and we are now re-using the newer one (v5b) for several SoCs.
> Socionext has some more product lines other than Uniphier SoC family,
> perhaps wider re-use might happen in the future.
>
> At first, I included "uniphier" in compatible, but I am still wondering
> if such a specific string is good or not.
>
> Also, comments from Altera engineers are appreciated.
Adding a few more on Cc
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 39/39] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
From: Masahiro Yamada @ 2016-12-03 2:41 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree@vger.kernel.org, Boris Brezillon,
Richard Weinberger, Linux Kernel Mailing List, Marek Vasut,
linux-mtd@lists.infradead.org, Cyrille Pitchen, Brian Norris,
David Woodhouse, Dinh Nguyen
In-Reply-To: <CAL_Jsq+H-JcAtDV3NCfptCWrmTgkQkmZFTj=Do8HhX4b=At9RA@mail.gmail.com>
Hi Rob,
2016-12-03 1:26 GMT+09:00 Rob Herring <robh@kernel.org>:
>>
>>
>> (Plan A)
>> "denali,socfpga-nand" (for Altera SOCFPGA variant)
>> "denali,uniphier-nand-v1" (for old Socionext UniPhier family variant)
>> "denali,uniphier-nand-v2" (for new Socionext UniPhier family variant)
>>
>> (Plan B)
>> "altera,denali-nand" (for Altera SOCFPGA variant)
>> "socionext,denali-nand-v5a" (for old Socionext UniPhier family variant)
>> "socionext,denali-nand-v5b" (for new Socionext UniPhier family variant)
> Let the Altera folks worry about their stuff. At least for soft IP in
> FPGA, it's a bit of a special case. The old string can remain as bad
> as it is.
Hmm, I am not sure if this IP would fit in FPGA
(to use it along with NIOS-II?)
(even if it happened, nothing of this IP would be customizable on users' side.
When buying the IP, SoC vendors submit a list of desired features.
Denali (now Cadence) generates the RTL according to the configuration sheet.
The function is fixed at this point. So, generic compatible would be
useless anyway.)
If we are talking about SOCFPGA,
SOCFPGA is not only FPGA. Rather "SOC" + "FPGA".
It consists of two parts:
[1] SOC part (Cortex-A9 + various hard-wired peripherals such UART,
USB, SD, NAND, ...)
[2] FPGA part (User design logic)
The Denali NAND controller is included in [1].
So, as far as we talk about the Denali on SOCFPGA,
it is as hard-wired as Intel, Socionext's ones.
> I simply would do "socionext,uniphier-v5b-nand" (and v5a).
> The fact that it is denali is part of the documentation.
>
Let me think about this.
Socionext bought two version of Denali IP,
and we are now re-using the newer one (v5b) for several SoCs.
Socionext has some more product lines other than Uniphier SoC family,
perhaps wider re-use might happen in the future.
At first, I included "uniphier" in compatible, but I am still wondering
if such a specific string is good or not.
Also, comments from Altera engineers are appreciated.
--
Best Regards
Masahiro Yamada
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* Re: [PATCHv4 11/15] clk: ti: clockdomain: add clock provider support to clockdomains
From: Tony Lindgren @ 2016-12-03 0:18 UTC (permalink / raw)
To: Michael Turquette
Cc: Stephen Boyd, Tero Kristo, linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring
In-Reply-To: <148072274164.32158.6012452044533845688@resonance>
* Michael Turquette <mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [161202 15:52]:
> Quoting Tony Lindgren (2016-12-02 15:12:40)
> > * Michael Turquette <mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> [161202 14:34]:
> > > Quoting Tony Lindgren (2016-10-28 16:54:48)
> > > > * Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> [161028 16:37]:
> > > > > On 10/28, Tony Lindgren wrote:
> > > > > > * Tero Kristo <t-kristo-l0cyMroinI0@public.gmane.org> [161028 00:43]:
> > > > > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > > > > I suppose a PRCM is
> > > > > > > > like an MFD that has clocks and resets under it? On other
> > > > > > > > platforms we've combined that all into one node and just had
> > > > > > > > #clock-cells and #reset-cells in that node. Is there any reason
> > > > > > > > we can't do that here?
> > > > > > >
> > > > > > > For OMAPs, there are typically multiple instances of the PRCM around; OMAP4
> > > > > > > for example has:
> > > > > > >
> > > > > > > cm1 @ 0x4a004000 (clocks + clockdomains)
> > > > > > > cm2 @ 0x4a008000 (clocks + clockdomains)
> > > > > > > prm @ 0x4a306000 (few clocks + resets + power state handling)
> > > > > > > scrm @ 0x4a30a000 (few external clocks + plenty of misc stuff)
> > > > > > >
> > > > > > > These instances are also under different power/voltage domains which means
> > > > > > > their PM behavior is different.
> > > > > > >
> > > > > > > The idea behind having a clockdomain as a provider was mostly to have the
> > > > > > > topology visible : prcm-instance -> clockdomain -> clocks
> > > > > >
> > > > > > Yeah that's needed to get the interconnect hierarchy right for
> > > > > > genpd :)
> > > > > >
> > > > > > > ... but basically I think it would be possible to drop the clockdomain
> > > > > > > representation and just mark the prcm-instance as a clock provider. Tony,
> > > > > > > any thoughts on that?
> > > > > >
> > > > > > No let's not drop the clockdomains as those will be needed when we
> > > > > > move things into proper hierarchy within the interconnect instances.
> > > > > > This will then help with getting things right with genpd.
> > > > > >
> > > > > > In the long run we just want to specify clockdomain and the offset of
> > > > > > the clock instance within the clockdomain in the dts files.
> > > > > >
> > > > >
> > > > > Sorry, I have very little idea how OMAP hardware works. Do you
> > > > > mean that you will have different nodes for each clockdomain so
> > > > > that genpd can map 1:1 to the node in dts? But in hardware
> > > > > there's a prcm that allows us to control many clock domains
> > > > > through register read/writes? How is the interconnect involved?
> > > >
> > > > There are multiple clockdomains, at least one for each interconnect
> > > > instance. Once a clockdomain is idle, the related interconnect can
> > > > idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
> > > >
> > > > There's more info in for example omap4 TRM section "3.4.1 Device
> > > > Power-Management Layout" that shows the voltage/power/clock domains.
> > > > The interconnect instances are mostly named there too looking at
> > > > the L4/L3 naming.
> > >
> > > I'm confused on two points:
> > >
> > > 1) why are the clkdm's acting as clock providers? I've always hated the
> > > name "clock domain" since those bits are for managing module state, not
> > > clock state. The PRM, CM1 and CM2 provide the clocks, not the
> > > clockdomains.
> >
> > The clock domains have multiple clock inputs that are routed to multiple
> > child clocks. So it is a clock :)
> >
> > See for example omap4430 TRM "3.6.4 CD_WKUP Clock Domain" on page
> > 393 in my revision here.
> >
> > On that page "Figure 3-48" shows CD_WKUP with the four input clocks.
> > And then "Table 3-84. CD_WKUP Control and Status Parameters" shows
> > the CD_WKUP clock domain specific registers. These registers show
> > the status, I think they are all read-only registers. Then CD_WKUP
> > has multiple child clocks with configurable registers.
> >
> > From hardware register point of view, each clock domain has:
> >
> > - Read-only clockdomain status registers in the beginning of
> > the address space
> >
> > - Multiple similar clock instances register instances each
> > mapping to a specific interconnect target module
> >
> > These are documented in "3.11.16.1 WKUP_CM Register Summary".
>
> Oh, this is because you are treating the MODULEMODE bits like gate
> clocks. I never really figured out if this was the best way to model
> those bits since they do more than control a line toggling at a rate.
> For instance this bit will affect the master/slave IDLE protocol between
> the module and the PRCM.
Yes seems like there is some negotiation going on there with the
target module. But from practical point of view the CLKCTRL
register is the gate for a module functional clock.
> > From hardware point of view, we ideally want to map interconnect
> > target modules to the clock instance offset from the clock domain
> > for that interconnect segment. For example gptimer1 clocks would
> > be just:
> >
> > clocks = <&cd_wkup 0x40>;
> >
> > > 2) why aren't the clock domains modeled as genpds with their associated
> > > devices attached to them? Note that it is possible to "nest" genpd
> > > objects. This would also allow for the "Clockdomain Dependency"
> > > relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
> > > Dependency in the OMAP4 TRM).
> >
> > Clock domains only route clocks to child clocks. Power domains
> > are different registers. The power domains map roughly to
> > interconnect instances, there we have registers to disable the
> > whole interconnect when idle.
>
> I'm not talking about power islands at all, but the genpd object in
> Linux. For instance, if we treat each clock domain like a clock
> provider, how could the functional dependency between clkdm_A and
> clkdm_B be asserted?
To me it seems that some output of a clockdomain is just a input
of another clockdomain? So it's just the usual parent child
relationship once we treat a clockdomain just as a clock. Tero
probably has some input here.
> There is certainly no API for that in the clock framework, but for genpd
> your runtime_pm_get() callback for clkdm_A could call runtime_pm_get
> against clkdm_B, which would satisfy the requirement. See section
> 3.1.1.1.7 Clock Domain Dependency in the OMAP4 TRM, version AB.
To me it seems the API is just clk_get() :) Do you have some
specific example we can use to check? My guess is that the
TRM "Clock Domain Dependency" is just the usual parent child
relationship between clocks that are the clockdomains..
If there is something more magical there certainly that should
be considered though.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RESEND PATCH v2 0/7] drm/vc4: VEC (SDTV) output support
From: Eric Anholt @ 2016-12-02 23:59 UTC (permalink / raw)
To: Boris Brezillon, David Airlie, Daniel Vetter, dri-devel
Cc: Mark Rutland, devicetree, Ian Campbell, Florian Fainelli,
Pawel Moll, Scott Branden, Stephen Warren, Ray Jui, Lee Jones,
Rob Herring, bcm-kernel-feedback-list, linux-rpi-kernel,
Kumar Gala, linux-arm-kernel
In-Reply-To: <1480686493-4813-1-git-send-email-boris.brezillon@free-electrons.com>
[-- Attachment #1.1: Type: text/plain, Size: 396 bytes --]
Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> Sorry for the noise, but I forgot to Cc the DT maintainers.
>
> Here is the 2nd version of the VC4/VEC series.
>
> We still miss the two clock patches mentioned by Eric in the first
> version to make the encoder work no matter the setting applied by the
> bootloader.
I'm happy with the series and I'm just waiting for the DT ack.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCHv4 11/15] clk: ti: clockdomain: add clock provider support to clockdomains
From: Michael Turquette @ 2016-12-02 23:52 UTC (permalink / raw)
To: Tony Lindgren
Cc: devicetree, Stephen Boyd, Tero Kristo, Rob Herring, linux-omap,
linux-clk, linux-arm-kernel
In-Reply-To: <20161202231240.GH4705@atomide.com>
Quoting Tony Lindgren (2016-12-02 15:12:40)
> * Michael Turquette <mturquette@baylibre.com> [161202 14:34]:
> > Quoting Tony Lindgren (2016-10-28 16:54:48)
> > > * Stephen Boyd <sboyd@codeaurora.org> [161028 16:37]:
> > > > On 10/28, Tony Lindgren wrote:
> > > > > * Tero Kristo <t-kristo@ti.com> [161028 00:43]:
> > > > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > > > I suppose a PRCM is
> > > > > > > like an MFD that has clocks and resets under it? On other
> > > > > > > platforms we've combined that all into one node and just had
> > > > > > > #clock-cells and #reset-cells in that node. Is there any reason
> > > > > > > we can't do that here?
> > > > > >
> > > > > > For OMAPs, there are typically multiple instances of the PRCM around; OMAP4
> > > > > > for example has:
> > > > > >
> > > > > > cm1 @ 0x4a004000 (clocks + clockdomains)
> > > > > > cm2 @ 0x4a008000 (clocks + clockdomains)
> > > > > > prm @ 0x4a306000 (few clocks + resets + power state handling)
> > > > > > scrm @ 0x4a30a000 (few external clocks + plenty of misc stuff)
> > > > > >
> > > > > > These instances are also under different power/voltage domains which means
> > > > > > their PM behavior is different.
> > > > > >
> > > > > > The idea behind having a clockdomain as a provider was mostly to have the
> > > > > > topology visible : prcm-instance -> clockdomain -> clocks
> > > > >
> > > > > Yeah that's needed to get the interconnect hierarchy right for
> > > > > genpd :)
> > > > >
> > > > > > ... but basically I think it would be possible to drop the clockdomain
> > > > > > representation and just mark the prcm-instance as a clock provider. Tony,
> > > > > > any thoughts on that?
> > > > >
> > > > > No let's not drop the clockdomains as those will be needed when we
> > > > > move things into proper hierarchy within the interconnect instances.
> > > > > This will then help with getting things right with genpd.
> > > > >
> > > > > In the long run we just want to specify clockdomain and the offset of
> > > > > the clock instance within the clockdomain in the dts files.
> > > > >
> > > >
> > > > Sorry, I have very little idea how OMAP hardware works. Do you
> > > > mean that you will have different nodes for each clockdomain so
> > > > that genpd can map 1:1 to the node in dts? But in hardware
> > > > there's a prcm that allows us to control many clock domains
> > > > through register read/writes? How is the interconnect involved?
> > >
> > > There are multiple clockdomains, at least one for each interconnect
> > > instance. Once a clockdomain is idle, the related interconnect can
> > > idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
> > >
> > > There's more info in for example omap4 TRM section "3.4.1 Device
> > > Power-Management Layout" that shows the voltage/power/clock domains.
> > > The interconnect instances are mostly named there too looking at
> > > the L4/L3 naming.
> >
> > I'm confused on two points:
> >
> > 1) why are the clkdm's acting as clock providers? I've always hated the
> > name "clock domain" since those bits are for managing module state, not
> > clock state. The PRM, CM1 and CM2 provide the clocks, not the
> > clockdomains.
>
> The clock domains have multiple clock inputs that are routed to multiple
> child clocks. So it is a clock :)
>
> See for example omap4430 TRM "3.6.4 CD_WKUP Clock Domain" on page
> 393 in my revision here.
>
> On that page "Figure 3-48" shows CD_WKUP with the four input clocks.
> And then "Table 3-84. CD_WKUP Control and Status Parameters" shows
> the CD_WKUP clock domain specific registers. These registers show
> the status, I think they are all read-only registers. Then CD_WKUP
> has multiple child clocks with configurable registers.
>
> From hardware register point of view, each clock domain has:
>
> - Read-only clockdomain status registers in the beginning of
> the address space
>
> - Multiple similar clock instances register instances each
> mapping to a specific interconnect target module
>
> These are documented in "3.11.16.1 WKUP_CM Register Summary".
Oh, this is because you are treating the MODULEMODE bits like gate
clocks. I never really figured out if this was the best way to model
those bits since they do more than control a line toggling at a rate.
For instance this bit will affect the master/slave IDLE protocol between
the module and the PRCM.
>
> From hardware point of view, we ideally want to map interconnect
> target modules to the clock instance offset from the clock domain
> for that interconnect segment. For example gptimer1 clocks would
> be just:
>
> clocks = <&cd_wkup 0x40>;
>
> > 2) why aren't the clock domains modeled as genpds with their associated
> > devices attached to them? Note that it is possible to "nest" genpd
> > objects. This would also allow for the "Clockdomain Dependency"
> > relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
> > Dependency in the OMAP4 TRM).
>
> Clock domains only route clocks to child clocks. Power domains
> are different registers. The power domains map roughly to
> interconnect instances, there we have registers to disable the
> whole interconnect when idle.
I'm not talking about power islands at all, but the genpd object in
Linux. For instance, if we treat each clock domain like a clock
provider, how could the functional dependency between clkdm_A and
clkdm_B be asserted?
There is certainly no API for that in the clock framework, but for genpd
your runtime_pm_get() callback for clkdm_A could call runtime_pm_get
against clkdm_B, which would satisfy the requirement. See section
3.1.1.1.7 Clock Domain Dependency in the OMAP4 TRM, version AB.
Regards,
Mike
>
> Regards,
>
> Tony
^ permalink raw reply
* Re: [PATCH v2 0/7] stmmac: dwmac-meson8b: configurable RGMII TX delay
From: Martin Blumenstingl @ 2016-12-02 23:52 UTC (permalink / raw)
To: David Miller
Cc: linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
khilman-rdvid1DuHRBWk0Htik3J/w, mark.rutland-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
alexandre.torgue-qxv4g6HH51o, peppe.cavallaro-qxv4g6HH51o,
will.deacon-5wv7dgnIgG8, catalin.marinas-5wv7dgnIgG8,
carlo-KA+7E9HrN00dnm+yROfE0A, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <20161127.203324.1634866862730391239.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Mon, Nov 28, 2016 at 2:33 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> Date: Fri, 25 Nov 2016 14:01:49 +0100
>
>> Currently the dwmac-meson8b stmmac glue driver uses a hardcoded 1/4
>> cycle TX clock delay. This seems to work fine for many boards (for
>> example Odroid-C2 or Amlogic's reference boards) but there are some
>> others where TX traffic is simply broken.
>> There are probably multiple reasons why it's working on some boards
>> while it's broken on others:
>> - some of Amlogic's reference boards are using a Micrel PHY
>> - hardware circuit design
>> - maybe more...
>
> The ARM arch file changes do not apply cleanly to net-next, you probably
> want to merge them via the ARM tree instead of mine, and respin this series
> to be without the .dts file changes.
done, v3 contains only the net-next changes while the dts changes can
be found here: [0]
Regards,
Martin
[0] http://lists.infradead.org/pipermail/linux-amlogic/2016-December/001836.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 5/5] ARM64: dts: amlogic: add the ethernet TX delay configuration
From: Martin Blumenstingl @ 2016-12-02 23:47 UTC (permalink / raw)
To: linux-amlogic, khilman, carlo
Cc: mark.rutland, devicetree, Martin Blumenstingl, catalin.marinas,
will.deacon, robh+dt, linux-arm-kernel
In-Reply-To: <20161202234739.22929-1-martin.blumenstingl@googlemail.com>
This adds the amlogic,tx-delay-ns with the old (hardcoded) default value
of 2ns to all boards which are using an RGMII ethernet PHY.
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 2 ++
arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi | 2 ++
arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi | 2 ++
arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p230.dts | 2 ++
arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts | 2 ++
arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts | 2 ++
6 files changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
index cbaf024..fdade07 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
@@ -161,6 +161,8 @@
snps,reset-delays-us = <0 10000 1000000>;
snps,reset-active-low;
+ amlogic,tx-delay-ns = <2>;
+
phy-mode = "rgmii";
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
index 2abc553..8172e12 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
@@ -152,6 +152,8 @@
snps,reset-delays-us = <0 10000 1000000>;
snps,reset-active-low;
+ amlogic,tx-delay-ns = <2>;
+
phy-mode = "rgmii";
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi
index a0e92e3..ab49712 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi
@@ -131,6 +131,8 @@
snps,reset-delays-us = <0 10000 1000000>;
snps,reset-active-low;
+ amlogic,tx-delay-ns = <2>;
+
phy-mode = "rgmii";
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p230.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p230.dts
index f66939c..7fd11c6 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p230.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p230.dts
@@ -64,6 +64,8 @@
snps,reset-delays-us = <0 10000 1000000>;
snps,reset-active-low;
+ amlogic,tx-delay-ns = <2>;
+
/* External PHY is in RGMII */
phy-mode = "rgmii";
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts b/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
index f859d75..cf9b960 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
@@ -156,6 +156,8 @@
snps,reset-delays-us = <0 10000 1000000>;
snps,reset-active-low;
+ amlogic,tx-delay-ns = <2>;
+
/* External PHY is in RGMII */
phy-mode = "rgmii";
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts b/arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts
index 5dbc660..e428e29 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts
@@ -64,6 +64,8 @@
snps,reset-delays-us = <0 10000 1000000>;
snps,reset-active-low;
+ amlogic,tx-delay-ns = <2>;
+
/* External PHY is in RGMII */
phy-mode = "rgmii";
};
--
2.10.2
^ permalink raw reply related
* [PATCH 4/5] ARM64: dts: meson-gxbb-vega-s95: add reset for the ethernet PHY
From: Martin Blumenstingl @ 2016-12-02 23:47 UTC (permalink / raw)
To: linux-amlogic, khilman, carlo
Cc: mark.rutland, devicetree, Martin Blumenstingl, catalin.marinas,
will.deacon, robh+dt, linux-arm-kernel
In-Reply-To: <20161202234739.22929-1-martin.blumenstingl@googlemail.com>
This resets the ethernet PHY during boot to get the PHY into a "clean"
state. While here also specify the phy-handle of the ethmac node to
make the PHY configuration similar to the one we have on GXL devices.
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi
index e59ad30..a0e92e3 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi
@@ -113,10 +113,25 @@
pinctrl-names = "default";
};
+&mdio0 {
+ ethernet_phy0: ethernet-phy@0 {
+ compatible = "ethernet-phy-id001c.c916", "ethernet-phy-ieee802.3-c22";
+ reg = <0>;
+ };
+};
+
ðmac {
status = "okay";
pinctrl-0 = <ð_rgmii_pins>;
pinctrl-names = "default";
+
+ phy-handle = <ðernet_phy0>;
+
+ snps,reset-gpio = <&gpio GPIOZ_14 0>;
+ snps,reset-delays-us = <0 10000 1000000>;
+ snps,reset-active-low;
+
+ phy-mode = "rgmii";
};
&usb0_phy {
--
2.10.2
^ permalink raw reply related
* [PATCH 3/5] ARM64: dts: meson-gxbb-p20x: add reset for the ethernet PHY
From: Martin Blumenstingl @ 2016-12-02 23:47 UTC (permalink / raw)
To: linux-amlogic, khilman, carlo
Cc: mark.rutland, devicetree, Martin Blumenstingl, catalin.marinas,
will.deacon, robh+dt, linux-arm-kernel
In-Reply-To: <20161202234739.22929-1-martin.blumenstingl@googlemail.com>
This resets the ethernet PHY during boot to get the PHY into a "clean"
state. While here also specify the phy-handle of the ethmac node to
make the PHY configuration similar to the one we have on GXL devices.
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
index 203be28..2abc553 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
@@ -134,10 +134,25 @@
pinctrl-names = "default";
};
+&mdio0 {
+ ethernet_phy0: ethernet-phy@0 {
+ compatible = "ethernet-phy-ieee802.3-c22";
+ reg = <0>;
+ };
+};
+
ðmac {
status = "okay";
pinctrl-0 = <ð_rgmii_pins>;
pinctrl-names = "default";
+
+ phy-handle = <ðernet_phy0>;
+
+ snps,reset-gpio = <&gpio GPIOZ_14 0>;
+ snps,reset-delays-us = <0 10000 1000000>;
+ snps,reset-active-low;
+
+ phy-mode = "rgmii";
};
&ir {
--
2.10.2
^ permalink raw reply related
* [PATCH 2/5] ARM64: dts: meson-gxbb-odroidc2: add reset for the ethernet PHY
From: Martin Blumenstingl @ 2016-12-02 23:47 UTC (permalink / raw)
To: linux-amlogic, khilman, carlo
Cc: mark.rutland, devicetree, Martin Blumenstingl, catalin.marinas,
will.deacon, robh+dt, linux-arm-kernel
In-Reply-To: <20161202234739.22929-1-martin.blumenstingl@googlemail.com>
This resets the ethernet PHY during boot to get the PHY into a "clean"
state. While here also specify the phy-handle of the ethmac node to
make the PHY configuration similar to the one we have on GXL devices.
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
index 238fbea..cbaf024 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
@@ -143,10 +143,25 @@
pinctrl-names = "default";
};
+&mdio0 {
+ ethernet_phy0: ethernet-phy@0 {
+ compatible = "ethernet-phy-id001c.c916", "ethernet-phy-ieee802.3-c22";
+ reg = <0>;
+ };
+};
+
ðmac {
status = "okay";
pinctrl-0 = <ð_rgmii_pins>;
pinctrl-names = "default";
+
+ phy-handle = <ðernet_phy0>;
+
+ snps,reset-gpio = <&gpio GPIOZ_14 0>;
+ snps,reset-delays-us = <0 10000 1000000>;
+ snps,reset-active-low;
+
+ phy-mode = "rgmii";
};
&ir {
--
2.10.2
^ permalink raw reply related
* [PATCH 1/5] ARM64: dts: meson-gx: move the MDIO node to meson-gx
From: Martin Blumenstingl @ 2016-12-02 23:47 UTC (permalink / raw)
To: linux-amlogic, khilman, carlo
Cc: mark.rutland, devicetree, Martin Blumenstingl, catalin.marinas,
will.deacon, robh+dt, linux-arm-kernel
In-Reply-To: <20161202234739.22929-1-martin.blumenstingl@googlemail.com>
stmmac's MDIO bus is currently only defined in meson-gxl.dtsi. Move it
up to meson-gx to allow us to keep the stmmac configuration for
meson-gxbb similar to the configuration on meson-gxl.
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 6 ++++++
arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 6 ------
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 47ab306..a2c3ca6 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -371,6 +371,12 @@
interrupt-names = "macirq";
phy-mode = "rgmii";
status = "disabled";
+
+ mdio0: mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "snps,dwmac-mdio";
+ };
};
apb: apb@d0000000 {
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index 9f89b99..d338f9b 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -57,12 +57,6 @@
<&clkc CLKID_FCLK_DIV2>,
<&clkc CLKID_MPLL2>;
clock-names = "stmmaceth", "clkin0", "clkin1";
-
- mdio0: mdio {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "snps,dwmac-mdio";
- };
};
&aobus {
--
2.10.2
^ permalink raw reply related
* [PATCH 0/5] meson-gx: reset RGMII PHYs and configure TX delay
From: Martin Blumenstingl @ 2016-12-02 23:47 UTC (permalink / raw)
To: linux-amlogic, khilman, carlo
Cc: mark.rutland, devicetree, Martin Blumenstingl, catalin.marinas,
will.deacon, robh+dt, linux-arm-kernel
This partially fixes the 1000Mbit/s ethernet TX throughput issues (on
networks which are not affected by the EEE problem, as reported here:
[1]).
The actual problem for the TX throughput issues was that the TX delay
was applied twice:
- once "accidentally" by the PHY (this was fixed with [2])
- once by the MAC because there was a hardcoded TX delay (of 2ns),
this will be configurable with the changes from [0]
These are the dts changes which belong to my other series (in v2
these patches were part of the other series, upon request of the
net maintainers I have split the .dts changes into their own series so
we are able to take both through different trees):
"[PATCH net-next v3 0/2] stmmac: dwmac-meson8b: configurable
RGMII TX delay": [0].
Thus this series depends on the ACK for the binding changes in the
other series!
I based these changes on my other series "[PATCH v2 0/2] GXL and GXM
SCPI improvements": [3]
[0] http://lists.infradead.org/pipermail/linux-amlogic/2016-December/001834.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/001607.html
[2] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/001707.html
[3] http://lists.infradead.org/pipermail/linux-amlogic/2016-December/001831.html
Martin Blumenstingl (5):
ARM64: dts: meson-gx: move the MDIO node to meson-gx
ARM64: dts: meson-gxbb-odroidc2: add reset for the ethernet PHY
ARM64: dts: meson-gxbb-p20x: add reset for the ethernet PHY
ARM64: dts: meson-gxbb-vega-s95: add reset for the ethernet PHY
ARM64: dts: amlogic: add the ethernet TX delay configuration
arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 6 ++++++
arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 17 +++++++++++++++++
arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi | 17 +++++++++++++++++
arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi | 17 +++++++++++++++++
arch/arm64/boot/dts/amlogic/meson-gxl-s905d-p230.dts | 2 ++
arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 6 ------
arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts | 2 ++
arch/arm64/boot/dts/amlogic/meson-gxm-s912-q200.dts | 2 ++
8 files changed, 63 insertions(+), 6 deletions(-)
--
2.10.2
^ permalink raw reply
* [PATCH net-next v3 2/2] net: stmmac: dwmac-meson8b: make the RGMII TX delay configurable
From: Martin Blumenstingl @ 2016-12-02 23:32 UTC (permalink / raw)
To: davem, netdev, devicetree, linux-amlogic, robh+dt, mark.rutland,
carlo, khilman
Cc: peppe.cavallaro, alexandre.torgue, linux-arm-kernel,
Martin Blumenstingl
In-Reply-To: <20161202233238.17561-1-martin.blumenstingl@googlemail.com>
Prior to this patch we were using a hardcoded RGMII TX clock delay of
2ns (= 1/4 cycle of the 125MHz RGMII TX clock). This value works for
many boards, but unfortunately not for all (due to the way the actual
circuit is designed, sometimes because the TX delay is enabled in the
PHY, etc.). Making the TX delay on the MAC side configurable allows us
to support all possible hardware combinations.
This allows fixing a compatibility issue on some boards, where the
RTL8211F PHY is configured to generate the TX delay. We can now turn
off the TX delay in the MAC, because otherwise we would be applying the
delay twice (which results in non-working TX traffic).
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
index 250e4ce..dad31b0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
@@ -35,10 +35,6 @@
#define PRG_ETH0_TXDLY_SHIFT 5
#define PRG_ETH0_TXDLY_MASK GENMASK(6, 5)
-#define PRG_ETH0_TXDLY_OFF (0x0 << PRG_ETH0_TXDLY_SHIFT)
-#define PRG_ETH0_TXDLY_QUARTER (0x1 << PRG_ETH0_TXDLY_SHIFT)
-#define PRG_ETH0_TXDLY_HALF (0x2 << PRG_ETH0_TXDLY_SHIFT)
-#define PRG_ETH0_TXDLY_THREE_QUARTERS (0x3 << PRG_ETH0_TXDLY_SHIFT)
/* divider for the result of m250_sel */
#define PRG_ETH0_CLK_M250_DIV_SHIFT 7
@@ -69,6 +65,8 @@ struct meson8b_dwmac {
struct clk_divider m25_div;
struct clk *m25_div_clk;
+
+ u32 tx_delay_ns;
};
static void meson8b_dwmac_mask_bits(struct meson8b_dwmac *dwmac, u32 reg,
@@ -179,6 +177,7 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
{
int ret;
unsigned long clk_rate;
+ u8 tx_dly_val;
switch (dwmac->phy_mode) {
case PHY_INTERFACE_MODE_RGMII:
@@ -196,9 +195,13 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac)
meson8b_dwmac_mask_bits(dwmac, PRG_ETH0,
PRG_ETH0_INVERTED_RMII_CLK, 0);
- /* TX clock delay - all known boards use a 1/4 cycle delay */
+ /* TX clock delay in ns = "8ns / 4 * tx_dly_val" (where
+ * 8ns are exactly one cycle of the 125MHz RGMII TX clock):
+ * 0ns = 0x0, 2ns = 0x1, 4ns = 0x2, 6ns = 0x3
+ */
+ tx_dly_val = dwmac->tx_delay_ns >> 1;
meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TXDLY_MASK,
- PRG_ETH0_TXDLY_QUARTER);
+ tx_dly_val << PRG_ETH0_TXDLY_SHIFT);
break;
case PHY_INTERFACE_MODE_RMII:
@@ -277,6 +280,12 @@ static int meson8b_dwmac_probe(struct platform_device *pdev)
if (dwmac->phy_mode < 0) {
dev_err(&pdev->dev, "missing phy-mode property\n");
return -EINVAL;
+ } else if (dwmac->phy_mode != PHY_INTERFACE_MODE_RMII) {
+ /* ignore errors as this is an optional property - by default
+ * we assume a TX delay of 0ns.
+ */
+ of_property_read_u32(pdev->dev.of_node, "amlogic,tx-delay-ns",
+ &dwmac->tx_delay_ns);
}
ret = meson8b_init_clk(dwmac);
--
2.10.2
^ permalink raw reply related
* [PATCH net-next v3 1/2] net: dt-bindings: add RGMII TX delay configuration to meson8b-dwmac
From: Martin Blumenstingl @ 2016-12-02 23:32 UTC (permalink / raw)
To: davem, netdev, devicetree, linux-amlogic, robh+dt, mark.rutland,
carlo, khilman
Cc: peppe.cavallaro, alexandre.torgue, linux-arm-kernel,
Martin Blumenstingl
In-Reply-To: <20161202233238.17561-1-martin.blumenstingl@googlemail.com>
This allows configuring the RGMII TX clock delay. The RGMII clock is
generated by underlying hardware of the the Meson 8b / GXBB DWMAC glue.
The configuration depends on the actual hardware (no delay may be
needed due to the design of the actual circuit, the PHY might add this
delay, etc.).
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
Documentation/devicetree/bindings/net/meson-dwmac.txt | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/meson-dwmac.txt b/Documentation/devicetree/bindings/net/meson-dwmac.txt
index 89e62dd..f8bc540 100644
--- a/Documentation/devicetree/bindings/net/meson-dwmac.txt
+++ b/Documentation/devicetree/bindings/net/meson-dwmac.txt
@@ -25,6 +25,20 @@ Required properties on Meson8b and newer:
- "clkin0" - first parent clock of the internal mux
- "clkin1" - second parent clock of the internal mux
+Optional properties on Meson8b and newer:
+- amlogic,tx-delay-ns: The internal RGMII TX clock delay (provided
+ by this driver) in nanoseconds. Allowed values
+ are: 0ns, 2ns, 4ns, 6ns.
+ This must be configured when the phy-mode is
+ "rgmii" (typically a value of 2ns is used in
+ this case).
+ When phy-mode is set to "rgmii-id" or
+ "rgmii-txid" the TX clock delay is already
+ provided by the PHY. In that case this
+ property should be set to 0ns (which disables
+ the TX clock delay in the MAC to prevent the
+ clock from going off because both PHY and MAC
+ are adding a delay).
Example for Meson6:
--
2.10.2
^ permalink raw reply related
* [PATCH net-next v3 0/2] stmmac: dwmac-meson8b: configurable RGMII TX delay
From: Martin Blumenstingl @ 2016-12-02 23:32 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
carlo-KA+7E9HrN00dnm+yROfE0A, khilman-rdvid1DuHRBWk0Htik3J/w
Cc: peppe.cavallaro-qxv4g6HH51o, alexandre.torgue-qxv4g6HH51o,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Martin Blumenstingl
In-Reply-To: <20161125130156.17879-1-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Currently the dwmac-meson8b stmmac glue driver uses a hardcoded 1/4
cycle (= 2ns) TX clock delay. This seems to work fine for many boards
(for example Odroid-C2 or Amlogic's reference boards) but there are
some others where TX traffic is simply broken.
There are probably multiple reasons why it's working on some boards
while it's broken on others:
- some of Amlogic's reference boards are using a Micrel PHY
- hardware circuit design
- maybe more...
iperf3 results on my Mecool BB2 board (Meson GXM, RTL8211F PHY) with
TX clock delay disabled on the MAC (as it's enabled in the PHY driver).
TX throughput was virtually zero before:
$ iperf3 -c 192.168.1.100 -R
Connecting to host 192.168.1.100, port 5201
Reverse mode, remote host 192.168.1.100 is sending
[ 4] local 192.168.1.206 port 52828 connected to 192.168.1.100 port 5201
[ ID] Interval Transfer Bandwidth
[ 4] 0.00-1.00 sec 108 MBytes 901 Mbits/sec
[ 4] 1.00-2.00 sec 94.2 MBytes 791 Mbits/sec
[ 4] 2.00-3.00 sec 96.5 MBytes 810 Mbits/sec
[ 4] 3.00-4.00 sec 96.2 MBytes 808 Mbits/sec
[ 4] 4.00-5.00 sec 96.6 MBytes 810 Mbits/sec
[ 4] 5.00-6.00 sec 96.5 MBytes 810 Mbits/sec
[ 4] 6.00-7.00 sec 96.6 MBytes 810 Mbits/sec
[ 4] 7.00-8.00 sec 96.5 MBytes 809 Mbits/sec
[ 4] 8.00-9.00 sec 105 MBytes 884 Mbits/sec
[ 4] 9.00-10.00 sec 111 MBytes 934 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bandwidth Retr
[ 4] 0.00-10.00 sec 1000 MBytes 839 Mbits/sec 0 sender
[ 4] 0.00-10.00 sec 998 MBytes 837 Mbits/sec receiver
iperf Done.
$ iperf3 -c 192.168.1.100
Connecting to host 192.168.1.100, port 5201
[ 4] local 192.168.1.206 port 52832 connected to 192.168.1.100 port 5201
[ ID] Interval Transfer Bandwidth Retr Cwnd
[ 4] 0.00-1.01 sec 99.5 MBytes 829 Mbits/sec 117 139 KBytes
[ 4] 1.01-2.00 sec 105 MBytes 884 Mbits/sec 129 70.7 KBytes
[ 4] 2.00-3.01 sec 107 MBytes 889 Mbits/sec 106 187 KBytes
[ 4] 3.01-4.01 sec 105 MBytes 878 Mbits/sec 92 143 KBytes
[ 4] 4.01-5.00 sec 105 MBytes 882 Mbits/sec 140 129 KBytes
[ 4] 5.00-6.01 sec 106 MBytes 883 Mbits/sec 115 195 KBytes
[ 4] 6.01-7.00 sec 102 MBytes 863 Mbits/sec 133 70.7 KBytes
[ 4] 7.00-8.01 sec 106 MBytes 884 Mbits/sec 143 97.6 KBytes
[ 4] 8.01-9.01 sec 104 MBytes 875 Mbits/sec 124 107 KBytes
[ 4] 9.01-10.01 sec 105 MBytes 876 Mbits/sec 90 139 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bandwidth Retr
[ 4] 0.00-10.01 sec 1.02 GBytes 874 Mbits/sec 1189 sender
[ 4] 0.00-10.01 sec 1.02 GBytes 873 Mbits/sec receiver
iperf Done.
I get similar TX throughput on my Meson GXBB "MXQ Pro+" board when I
disable the PHY's TX-delay and configure a 4ms TX-delay on the MAC.
So changes to at least the RTL8211F PHY driver are needed to get it
working properly in all situations.
Changes since v2:
- moved all .dts patches (3-7) to a separate series
- removed the default 2ns TX delay when phy-mode RGMII is specified
- (rebased against current net-next)
Changes since v1:
- renamed the devicetree property "amlogic,tx-delay" to
"amlogic,tx-delay-ns", which makes the .dts easier to read as we can
simply specify human-readable values instead of having "preprocessor
defines and calculation in human brain". Thanks to Andrew Lunn for
the suggestion!
- improved documentation to indicate when the MAC TX-delay should be
configured and how to use the PHY's TX-delay
- changed the default TX-delay in the dwmac-meson8b driver from 2ns
to 0ms when any of the rgmii-*id modes are used (the 2ns default
value still applies for phy-mode "rgmii")
- added patches to properly reset the PHY on Meson GXBB devices and to
use a similar configuration than the one we use on Meson GXL devices
(by passing a phy-handle to stmmac and defining the PHY in the mdio0
bus - patch 3-6)
- add the "amlogic,tx-delay-ns" property to all boards which are using
the RGMII PHY (patch 7)
Martin Blumenstingl (2):
net: dt-bindings: add RGMII TX delay configuration to meson8b-dwmac
net: stmmac: dwmac-meson8b: make the RGMII TX delay configurable
.../devicetree/bindings/net/meson-dwmac.txt | 14 ++++++++++++++
drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 21 +++++++++++++++------
2 files changed, 29 insertions(+), 6 deletions(-)
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv4 11/15] clk: ti: clockdomain: add clock provider support to clockdomains
From: Tony Lindgren @ 2016-12-02 23:12 UTC (permalink / raw)
To: Michael Turquette
Cc: Stephen Boyd, Tero Kristo, linux-omap, linux-clk,
linux-arm-kernel, devicetree, Rob Herring
In-Reply-To: <148071802182.32158.11479184984645880313@resonance>
* Michael Turquette <mturquette@baylibre.com> [161202 14:34]:
> Quoting Tony Lindgren (2016-10-28 16:54:48)
> > * Stephen Boyd <sboyd@codeaurora.org> [161028 16:37]:
> > > On 10/28, Tony Lindgren wrote:
> > > > * Tero Kristo <t-kristo@ti.com> [161028 00:43]:
> > > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > > I suppose a PRCM is
> > > > > > like an MFD that has clocks and resets under it? On other
> > > > > > platforms we've combined that all into one node and just had
> > > > > > #clock-cells and #reset-cells in that node. Is there any reason
> > > > > > we can't do that here?
> > > > >
> > > > > For OMAPs, there are typically multiple instances of the PRCM around; OMAP4
> > > > > for example has:
> > > > >
> > > > > cm1 @ 0x4a004000 (clocks + clockdomains)
> > > > > cm2 @ 0x4a008000 (clocks + clockdomains)
> > > > > prm @ 0x4a306000 (few clocks + resets + power state handling)
> > > > > scrm @ 0x4a30a000 (few external clocks + plenty of misc stuff)
> > > > >
> > > > > These instances are also under different power/voltage domains which means
> > > > > their PM behavior is different.
> > > > >
> > > > > The idea behind having a clockdomain as a provider was mostly to have the
> > > > > topology visible : prcm-instance -> clockdomain -> clocks
> > > >
> > > > Yeah that's needed to get the interconnect hierarchy right for
> > > > genpd :)
> > > >
> > > > > ... but basically I think it would be possible to drop the clockdomain
> > > > > representation and just mark the prcm-instance as a clock provider. Tony,
> > > > > any thoughts on that?
> > > >
> > > > No let's not drop the clockdomains as those will be needed when we
> > > > move things into proper hierarchy within the interconnect instances.
> > > > This will then help with getting things right with genpd.
> > > >
> > > > In the long run we just want to specify clockdomain and the offset of
> > > > the clock instance within the clockdomain in the dts files.
> > > >
> > >
> > > Sorry, I have very little idea how OMAP hardware works. Do you
> > > mean that you will have different nodes for each clockdomain so
> > > that genpd can map 1:1 to the node in dts? But in hardware
> > > there's a prcm that allows us to control many clock domains
> > > through register read/writes? How is the interconnect involved?
> >
> > There are multiple clockdomains, at least one for each interconnect
> > instance. Once a clockdomain is idle, the related interconnect can
> > idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
> >
> > There's more info in for example omap4 TRM section "3.4.1 Device
> > Power-Management Layout" that shows the voltage/power/clock domains.
> > The interconnect instances are mostly named there too looking at
> > the L4/L3 naming.
>
> I'm confused on two points:
>
> 1) why are the clkdm's acting as clock providers? I've always hated the
> name "clock domain" since those bits are for managing module state, not
> clock state. The PRM, CM1 and CM2 provide the clocks, not the
> clockdomains.
The clock domains have multiple clock inputs that are routed to multiple
child clocks. So it is a clock :)
See for example omap4430 TRM "3.6.4 CD_WKUP Clock Domain" on page
393 in my revision here.
On that page "Figure 3-48" shows CD_WKUP with the four input clocks.
And then "Table 3-84. CD_WKUP Control and Status Parameters" shows
the CD_WKUP clock domain specific registers. These registers show
the status, I think they are all read-only registers. Then CD_WKUP
has multiple child clocks with configurable registers.
>From hardware register point of view, each clock domain has:
- Read-only clockdomain status registers in the beginning of
the address space
- Multiple similar clock instances register instances each
mapping to a specific interconnect target module
These are documented in "3.11.16.1 WKUP_CM Register Summary".
>From hardware point of view, we ideally want to map interconnect
target modules to the clock instance offset from the clock domain
for that interconnect segment. For example gptimer1 clocks would
be just:
clocks = <&cd_wkup 0x40>;
> 2) why aren't the clock domains modeled as genpds with their associated
> devices attached to them? Note that it is possible to "nest" genpd
> objects. This would also allow for the "Clockdomain Dependency"
> relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
> Dependency in the OMAP4 TRM).
Clock domains only route clocks to child clocks. Power domains
are different registers. The power domains map roughly to
interconnect instances, there we have registers to disable the
whole interconnect when idle.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH 2/2] HID: i2c-hid: support regulator power on/off
From: Dmitry Torokhov @ 2016-12-02 23:00 UTC (permalink / raw)
To: Brian Norris
Cc: Jiri Kosina, Benjamin Tissoires, Caesar Wang, linux-rockchip,
Rob Herring, linux-input, devicetree, linux-kernel, Mark Rutland,
Doug Anderson
In-Reply-To: <1480717140-14558-2-git-send-email-briannorris@chromium.org>
On Fri, Dec 02, 2016 at 02:19:00PM -0800, Brian Norris wrote:
> On some boards, we need to enable a regulator before using the HID, and
> it's also nice to save power in suspend by disabling it. Support an
> optional "vdd-supply" and a companion initialization delay.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: linux-input@vger.kernel.org
> ---
> v2:
> * support compatible property for wacom, with specific "vdd-supply"
> * name
> * support the 100ms delay needed for this digitizer
> * target regulator support only at specific device
>
> v3:
> * drop Wacom specifics and allow this to be used generically
> * add "init-delay-ms" property support
>
> v4:
> * use devm_regulator_get() (with a 'dummy' regulator for most cases)
> instead of _optional() version, to make code less conditional (Dmitry)
> * fix but where 'init_delay_ms' wasn't getting assigned properly
> * disable regulator in probe() failure path
> ---
> drivers/hid/i2c-hid/i2c-hid.c | 44 +++++++++++++++++++++++++++++++++++++++++--
> include/linux/i2c/i2c-hid.h | 6 ++++++
> 2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index b3ec4f2de875..5c6e037613d7 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -38,6 +38,7 @@
> #include <linux/acpi.h>
> #include <linux/of.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
>
> #include <linux/i2c/i2c-hid.h>
>
> @@ -937,6 +938,10 @@ static int i2c_hid_of_probe(struct i2c_client *client,
> }
> pdata->hid_descriptor_address = val;
>
> + ret = of_property_read_u32(dev->of_node, "init-delay-ms", &val);
> + if (!ret)
> + pdata->init_delay_ms = val;
> +
> return 0;
> }
>
> @@ -983,6 +988,15 @@ static int i2c_hid_probe(struct i2c_client *client,
> ihid->pdata = *platform_data;
> }
>
> + ihid->pdata.supply = devm_regulator_get(&client->dev, "vdd");
Oh, haveni't noticed that the rest of the driver does not use devm.
Well, I'll leave it up to hid-i2c folks to decide if they are OK with
mixing up the managed and non-managed resources, it seems safe to me in
this case.
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCHv4 11/15] clk: ti: clockdomain: add clock provider support to clockdomains
From: Michael Turquette @ 2016-12-02 22:33 UTC (permalink / raw)
To: Tony Lindgren, Stephen Boyd
Cc: Tero Kristo, linux-omap, linux-clk, linux-arm-kernel, devicetree,
Rob Herring
In-Reply-To: <20161028235448.57squdf6cu2wxxvm@atomide.com>
Quoting Tony Lindgren (2016-10-28 16:54:48)
> * Stephen Boyd <sboyd@codeaurora.org> [161028 16:37]:
> > On 10/28, Tony Lindgren wrote:
> > > * Tero Kristo <t-kristo@ti.com> [161028 00:43]:
> > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > I suppose a PRCM is
> > > > > like an MFD that has clocks and resets under it? On other
> > > > > platforms we've combined that all into one node and just had
> > > > > #clock-cells and #reset-cells in that node. Is there any reason
> > > > > we can't do that here?
> > > >
> > > > For OMAPs, there are typically multiple instances of the PRCM around; OMAP4
> > > > for example has:
> > > >
> > > > cm1 @ 0x4a004000 (clocks + clockdomains)
> > > > cm2 @ 0x4a008000 (clocks + clockdomains)
> > > > prm @ 0x4a306000 (few clocks + resets + power state handling)
> > > > scrm @ 0x4a30a000 (few external clocks + plenty of misc stuff)
> > > >
> > > > These instances are also under different power/voltage domains which means
> > > > their PM behavior is different.
> > > >
> > > > The idea behind having a clockdomain as a provider was mostly to have the
> > > > topology visible : prcm-instance -> clockdomain -> clocks
> > >
> > > Yeah that's needed to get the interconnect hierarchy right for
> > > genpd :)
> > >
> > > > ... but basically I think it would be possible to drop the clockdomain
> > > > representation and just mark the prcm-instance as a clock provider. Tony,
> > > > any thoughts on that?
> > >
> > > No let's not drop the clockdomains as those will be needed when we
> > > move things into proper hierarchy within the interconnect instances.
> > > This will then help with getting things right with genpd.
> > >
> > > In the long run we just want to specify clockdomain and the offset of
> > > the clock instance within the clockdomain in the dts files.
> > >
> >
> > Sorry, I have very little idea how OMAP hardware works. Do you
> > mean that you will have different nodes for each clockdomain so
> > that genpd can map 1:1 to the node in dts? But in hardware
> > there's a prcm that allows us to control many clock domains
> > through register read/writes? How is the interconnect involved?
>
> There are multiple clockdomains, at least one for each interconnect
> instance. Once a clockdomain is idle, the related interconnect can
> idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
>
> There's more info in for example omap4 TRM section "3.4.1 Device
> Power-Management Layout" that shows the voltage/power/clock domains.
> The interconnect instances are mostly named there too looking at
> the L4/L3 naming.
I'm confused on two points:
1) why are the clkdm's acting as clock providers? I've always hated the
name "clock domain" since those bits are for managing module state, not
clock state. The PRM, CM1 and CM2 provide the clocks, not the
clockdomains.
2) why aren't the clock domains modeled as genpds with their associated
devices attached to them? Note that it is possible to "nest" genpd
objects. This would also allow for the "Clockdomain Dependency"
relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
Dependency in the OMAP4 TRM).
Regards,
Mike
>
> Regards,
>
> Tony
^ permalink raw reply
* [PATCH 2/2] HID: i2c-hid: support regulator power on/off
From: Brian Norris @ 2016-12-02 22:19 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
Dmitry Torokhov, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Doug Anderson, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Rob Herring, linux-input-u79uwXL29TY76Z2rM5mHXA, Caesar Wang
In-Reply-To: <1480717140-14558-1-git-send-email-briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
On some boards, we need to enable a regulator before using the HID, and
it's also nice to save power in suspend by disabling it. Support an
optional "vdd-supply" and a companion initialization delay.
Signed-off-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Caesar Wang <wxt-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Cc: Jiri Kosina <jikos-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
v2:
* support compatible property for wacom, with specific "vdd-supply"
* name
* support the 100ms delay needed for this digitizer
* target regulator support only at specific device
v3:
* drop Wacom specifics and allow this to be used generically
* add "init-delay-ms" property support
v4:
* use devm_regulator_get() (with a 'dummy' regulator for most cases)
instead of _optional() version, to make code less conditional (Dmitry)
* fix but where 'init_delay_ms' wasn't getting assigned properly
* disable regulator in probe() failure path
---
drivers/hid/i2c-hid/i2c-hid.c | 44 +++++++++++++++++++++++++++++++++++++++++--
include/linux/i2c/i2c-hid.h | 6 ++++++
2 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index b3ec4f2de875..5c6e037613d7 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -38,6 +38,7 @@
#include <linux/acpi.h>
#include <linux/of.h>
#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
#include <linux/i2c/i2c-hid.h>
@@ -937,6 +938,10 @@ static int i2c_hid_of_probe(struct i2c_client *client,
}
pdata->hid_descriptor_address = val;
+ ret = of_property_read_u32(dev->of_node, "init-delay-ms", &val);
+ if (!ret)
+ pdata->init_delay_ms = val;
+
return 0;
}
@@ -983,6 +988,15 @@ static int i2c_hid_probe(struct i2c_client *client,
ihid->pdata = *platform_data;
}
+ ihid->pdata.supply = devm_regulator_get(&client->dev, "vdd");
+ if (IS_ERR(ihid->pdata.supply)) {
+ ret = PTR_ERR(ihid->pdata.supply);
+ if (ret != -EPROBE_DEFER)
+ dev_err(&client->dev, "Failed to get regulator: %d\n",
+ ret);
+ return ret;
+ }
+
if (client->irq > 0) {
ihid->irq = client->irq;
} else if (ACPI_COMPANION(&client->dev)) {
@@ -1000,6 +1014,15 @@ static int i2c_hid_probe(struct i2c_client *client,
}
}
+ ret = regulator_enable(ihid->pdata.supply);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to enable regulator: %d\n",
+ ret);
+ goto err;
+ }
+ if (ihid->pdata.init_delay_ms)
+ msleep(ihid->pdata.init_delay_ms);
+
i2c_set_clientdata(client, ihid);
ihid->client = client;
@@ -1015,7 +1038,7 @@ static int i2c_hid_probe(struct i2c_client *client,
* real computation later. */
ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
if (ret < 0)
- goto err;
+ goto err_regulator;
pm_runtime_get_noresume(&client->dev);
pm_runtime_set_active(&client->dev);
@@ -1070,6 +1093,9 @@ static int i2c_hid_probe(struct i2c_client *client,
pm_runtime_put_noidle(&client->dev);
pm_runtime_disable(&client->dev);
+err_regulator:
+ regulator_disable(ihid->pdata.supply);
+
err:
if (ihid->desc)
gpiod_put(ihid->desc);
@@ -1100,6 +1126,8 @@ static int i2c_hid_remove(struct i2c_client *client)
if (ihid->desc)
gpiod_put(ihid->desc);
+ regulator_disable(ihid->pdata.supply);
+
kfree(ihid);
acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));
@@ -1152,6 +1180,11 @@ static int i2c_hid_suspend(struct device *dev)
else
hid_warn(hid, "Failed to enable irq wake: %d\n",
wake_status);
+ } else {
+ ret = regulator_disable(ihid->pdata.supply);
+ if (ret < 0)
+ hid_warn(hid, "Failed to disable supply: %d\n",
+ ret);
}
return 0;
@@ -1165,7 +1198,14 @@ static int i2c_hid_resume(struct device *dev)
struct hid_device *hid = ihid->hid;
int wake_status;
- if (device_may_wakeup(&client->dev) && ihid->irq_wake_enabled) {
+ if (!device_may_wakeup(&client->dev)) {
+ ret = regulator_enable(ihid->pdata.supply);
+ if (ret < 0)
+ hid_warn(hid, "Failed to enable supply: %d\n",
+ ret);
+ if (ihid->pdata.init_delay_ms)
+ msleep(ihid->pdata.init_delay_ms);
+ } else if (ihid->irq_wake_enabled) {
wake_status = disable_irq_wake(ihid->irq);
if (!wake_status)
ihid->irq_wake_enabled = false;
diff --git a/include/linux/i2c/i2c-hid.h b/include/linux/i2c/i2c-hid.h
index 7aa901d92058..97688cde4a91 100644
--- a/include/linux/i2c/i2c-hid.h
+++ b/include/linux/i2c/i2c-hid.h
@@ -14,9 +14,13 @@
#include <linux/types.h>
+struct regulator;
+
/**
* struct i2chid_platform_data - used by hid over i2c implementation.
* @hid_descriptor_address: i2c register where the HID descriptor is stored.
+ * @supply: regulator for powering on the device.
+ * @init_delay_ms: delay after powering on before device is usable.
*
* Note that it is the responsibility of the platform driver (or the acpi 5.0
* driver, or the flattened device tree) to setup the irq related to the gpio in
@@ -31,6 +35,8 @@
*/
struct i2c_hid_platform_data {
u16 hid_descriptor_address;
+ struct regulator *supply;
+ int init_delay_ms;
};
#endif /* __LINUX_I2C_HID_H */
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 1/2] devicetree: i2c-hid: Add regulator support
From: Brian Norris @ 2016-12-02 22:18 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
Dmitry Torokhov, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Doug Anderson, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Rob Herring, linux-input-u79uwXL29TY76Z2rM5mHXA, Caesar Wang
From: Caesar Wang <wxt-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Document a "vdd-supply" and an initialization delay. Can be used for
powering on/off a HID.
Signed-off-by: Caesar Wang <wxt-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Jiri Kosina <jikos-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
v2:
* add compatible property for wacom, per Rob's request
* name the regulator property specifically (VDD)
v3:
* remove wacom property, per Benjamin's request
* add delay property
v4: no change
---
Documentation/devicetree/bindings/input/hid-over-i2c.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index 488edcb264c4..1ea290167652 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
@@ -17,6 +17,11 @@ Required properties:
- interrupt-parent: the phandle for the interrupt controller
- interrupts: interrupt line
+Optional properties:
+- vdd-supply: phandle of the regulator that provides the supply voltage.
+- init-delay-ms: time required by the device after power-on before it is ready
+ for communication.
+
Example:
i2c-hid-dev@2c {
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox