* [PATCH v2] dma: Add Xilinx AXI Video Direct Memory Access Engine driver support
From: Lars-Peter Clausen @ 2014-01-26 17:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140126135933.GD10628@intel.com>
On 01/26/2014 02:59 PM, Vinod Koul wrote:
> On Fri, Jan 24, 2014 at 02:24:27PM +0100, Lars-Peter Clausen wrote:
>> On 01/24/2014 12:16 PM, Srikanth Thokala wrote:
>>> Hi Lars,
>>>
>>> On Thu, Jan 23, 2014 at 4:55 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 01/22/2014 05:52 PM, Srikanth Thokala wrote:
>>>> [...]
>>>>> +/**
>>>>> + * xilinx_vdma_device_control - Configure DMA channel of the device
>>>>> + * @dchan: DMA Channel pointer
>>>>> + * @cmd: DMA control command
>>>>> + * @arg: Channel configuration
>>>>> + *
>>>>> + * Return: '0' on success and failure value on error
>>>>> + */
>>>>> +static int xilinx_vdma_device_control(struct dma_chan *dchan,
>>>>> + enum dma_ctrl_cmd cmd, unsigned long arg)
>>>>> +{
>>>>> + struct xilinx_vdma_chan *chan = to_xilinx_chan(dchan);
>>>>> +
>>>>> + switch (cmd) {
>>>>> + case DMA_TERMINATE_ALL:
>>>>> + xilinx_vdma_terminate_all(chan);
>>>>> + return 0;
>>>>> + case DMA_SLAVE_CONFIG:
>>>>> + return xilinx_vdma_slave_config(chan,
>>>>> + (struct xilinx_vdma_config *)arg);
>>>>
>>>> You really shouldn't be overloading the generic API with your own semantics.
>>>> DMA_SLAVE_CONFIG should take a dma_slave_config and nothing else.
>>>
>>> Ok. The driver needs few additional configuration from the slave
>>> device like Vertical
>>> Size, Horizontal Size, Stride etc., for the DMA transfers, in that case do you
>>> suggest me to define a separate dma_ctrl_cmd like the one FSLDMA_EXTERNAL_START
>>> defined for Freescale drivers?
>>
>> In my opinion it is not a good idea to have driver implement a generic API,
>> but at the same time let the driver have custom semantics for those API
>> calls. It's a bit like having a gpio driver that expects 23 and 42 as the
>> values passed to gpio_set_value instead of 0 and 1. It completely defeats
>> the purpose of a generic API, namely that you are able to write generic code
>> that makes use of the API without having to know about which implementation
>> API it is talking to. The dmaengine framework provides the
>> dmaengine_prep_interleaved_dma() function to setup two dimensional
>> transfers, e.g. take a look at sirf-dma.c or imx-dma.c.
>
> The question here i think would be waht this device supports? Is the hardware
> capable of doing interleaved transfers, then would make sense.
The hardware does 2D transfers. The parameters for a transfer are height,
width and stride. That's only a subset of what interleaved transfers can be
(xt->num_frames must be one for 2d transfers). But if I remember correctly
there has been some discussion on this in the past and the result of that
discussion was that using interleaved transfers for 2D transfers is
preferred over adding a custom API for 2D transfers.
- Lars
^ permalink raw reply
* [PATCH 1/3] mmc: add support for power-on sequencing through DT
From: Tomasz Figa @ 2014-01-26 17:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52DEBDA7.7030200@samsung.com>
On 21.01.2014 19:34, Tomasz Figa wrote:
> Hi,
>
> On 20.01.2014 04:56, Olof Johansson wrote:
>> This patch enables support for power-on sequencing of SDIO peripherals
>> through DT.
>>
>> In general, it's quite common that wifi modules and other similar
>> peripherals have several signals in addition to the SDIO interface that
>> needs wiggling before the module will power on. It's common to have a
>> reference clock, one or several power rails and one or several lines
>> for reset/enable type functions.
>>
>> The binding as written today introduces a number of reset gpios,
>> a regulator and a clock specifier. The code will handle up to 2 gpio
>> reset lines, but it's trivial to increase to more than that if needed
>> at some point.
>>
>> Implementation-wise, the MMC core has been changed to handle this during
>> host power up, before the host interface is powered on. I have not yet
>> implemented the power-down side, I wanted people to have a chance for
>> reporting back w.r.t. issues (or comments on the bindings) first.
>>
>> I have not tested the regulator portion, since the system and module
>> I'm working on doesn't need one (Samsung Chromebook with Marvell
>> 8797-based wifi). Testing of those portions (and reporting back) would
>> be appreciated.
>
> While I fully agree that this is an important problem that needs to be
> solved, I really don't think this is the right way, because:
>
> a) power-up sequence is really specific to the MMC device and often it's
> not simply a matter of switching on one regulator or one clock, e.g.
> specific time constraints need to be met.
>
> b) you can have WLAN chips in which SDIO is just one of the options to
> use as host interface, which may be also HSIC, I2C or UART. Really. See
> [1].
>
> c) this is leaking device specific details to generic host code, which
> isn't really elegant.
>
> Now, to make this a bit more constructive, [2] is a solution that I came
> up with (not perfect either), which simply adds a separate platform
> device for the low level part of the chip. I believe this is a better
> solution because:
>
> a) you can often see such WLAN/BT combo chip as a set of separate
> devices, e.g. SDIO WLAN, UART BT and a simple PMIC or management IC,
> which provides power/reset control, out of band signalling and etc. for
> the first two, so it isn't that bad to have a separate device node for
> the last one,
>
> b) you have full freedom of defining your DT binding with whatever data
> you need, any number of clocks, regulators, GPIOs and even out of band
> interrupts (IMHO the most important one).
>
> c) you can implement power-on, power-off sequences as needed for your
> particular device,
>
> d) you have full separation of device-specific data from MMC core (or
> any other subsystem simply used as a way to perform I/O to the chip).
>
> Now what's missing there is a way to signal the MMC core or any other
> transport that a device showed up and the controller should be woken up
> out of standby and scan of the bus initialized. This could be done by
> explicitly specifying the device as a subnode of the
> MMC/UART/USB(HSIC)/I2C or whatever with a link (phandle) to the power
> controller of the chip or the other way around - a link to the
> MMC/UART/... controller from the power controller node.
I've looked a bit around MMC core code and got some basic idea how
things look. I will definitely need some guidance, or at least some
opinions, from MMC guys, as some MMC core changes are unavoidable.
Now, the device-specific code is not really an issue, existing drivers
usually already have their ways of powering the chips on and off, based
on platform data. Everything needed here is to retrieve needed resources
(GPIOs, clocks, regulators) using DT, which should be trivial.
The worse part is the interaction between MMC and power controller
driver (the platform driver part of WLAN driver, if you look at brcmfmac
as an example). I believe that we need following things:
a) A way to tell the MMC controller that there is no card detection
mechanism available on given slot and it also should not be polling the
slot to check card presence. Something like a "manual card detect" that
would be triggered by another kernel entity that controls whether the
MMC device is present (i.e. WLAN driver). We already have "broken-cd"
property, but it only implies the former, wasting time on needless polling.
b) A mechanism to bind the power controller to used MMC slot. Something
like "mmc-bus = <&mmc2>;" property in device node of the power
controller and a function like of_find_mmc_controller_by_node(), which
would be an MMC counterpart of I2C's of_find_i2c_adapter_by_node(). To
avoid races, it should probably take a reference on MMC host that would
have to be dropped explicitly whenever it is not needed anymore.
c) A method to notify the MMC subsystem that card presence has changed.
We already have something like this in drivers/mmc/core/slot-gpio.c, but
used for a simple GPIO-based card detection. If the main part of
mmc_gpio_cd_irqt() could be turned into an exported helper, e.g.
mmc_force_card_detect(host) then basically we would have everything needed.
Unfortunately, I don't have more time left for today to create patches
and test them, so for now, I'd like to hear opinion of MMC maintainers
about this approach. Do you find this acceptable?
By the way, it seems like slot-gpio.c could replace a lot of custom GPIO
card detection code used in MMC host drivers, e.g. sdhci-s3c. Is there
any reason why it couldn't?
Best regards,
Tomasz
^ permalink raw reply
* [PATCH v4 07/18] watchdog: orion: Handle IRQ
From: Thomas Petazzoni @ 2014-01-26 14:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140126131445.GA14713@localhost>
Dear Ezequiel Garcia,
On Sun, 26 Jan 2014 10:14:46 -0300, Ezequiel Garcia wrote:
> > I don't quiite understand the first sentence of this commit log, and
> > the commit title looks wrong. Maybe a bad copy/paste or something?
> >
>
> Hm... yes it doesn't look right. It should read:
>
> "DT-enabled platforms, where the irqchip driver for the brigde interrupt
> controller is available, can handle the watchdog IRQ properly. Therefore,
> we request the interrupt and add a dummy handler that merely calls panic()".
Ok.
> I guess we can re-phrase it be a bit more readable.
>
> Why does th commit title looks wrong? By requesting the IRQ we are
> "handling it", no?
Right, but it looks "truncated". Maybe something like:
watchdog: orion: handle irq to avoid having to clear BRIDGE_CLAUSE
or something like that (adjust to the actual reality, I haven't
followed all the implications).
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH v2] dma: Add Xilinx AXI Video Direct Memory Access Engine driver support
From: Vinod Koul @ 2014-01-26 14:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390409565-4200-2-git-send-email-sthokal@xilinx.com>
On Wed, Jan 22, 2014 at 10:22:45PM +0530, Srikanth Thokala wrote:
> This is the driver for the AXI Video Direct Memory Access (AXI
> VDMA) core, which is a soft Xilinx IP core that provides high-
> bandwidth direct memory access between memory and AXI4-Stream
> type video target peripherals. The core provides efficient two
> dimensional DMA operations with independent asynchronous read
ok here is tha catch, do you want to support interleaved API rather?
> +* DMA client + +Required properties: +- dmas: a list of <[Video DMA device
> phandle] [Channel ID]> pairs, + where Channel ID is '0' for write/tx and
> '1' for read/rx + channel. +- dma-names: a list of DMA channel names, one
> per "dmas" entry + +Example: +++++++++ + +vdmatest_0: vdmatest at 0 { +
> compatible ="xlnx,axi-vdma-test-1.00.a"; + dmas = <&axi_vdma_0 0 +
> &axi_vdma_0 1>; + dma-names = "vdma0", "vdma1"; +} ;
Need ack from DT folks. ALso would be better to split the binding to a separate
patch
> +/**
> + * struct xilinx_vdma_chan - Driver specific VDMA channel structure
> + * @xdev: Driver specific device structure
> + * @ctrl_offset: Control registers offset
> + * @desc_offset: TX descriptor registers offset
> + * @completed_cookie: Maximum cookie completed
> + * @cookie: The current cookie
> + * @lock: Descriptor operation lock
> + * @pending_list: Descriptors waiting
> + * @active_desc: Active descriptor
> + * @done_list: Complete descriptors
> + * @common: DMA common channel
> + * @desc_pool: Descriptors pool
> + * @dev: The dma device
> + * @irq: Channel IRQ
> + * @id: Channel ID
> + * @direction: Transfer direction
> + * @num_frms: Number of frames
> + * @has_sg: Support scatter transfers
> + * @genlock: Support genlock mode
> + * @err: Channel has errors
> + * @tasklet: Cleanup work after irq
> + * @config: Device configuration info
> + * @flush_on_fsync: Flush on Frame sync
> + */
> +struct xilinx_vdma_chan {
> + struct xilinx_vdma_device *xdev;
> + u32 ctrl_offset;
> + u32 desc_offset;
> + dma_cookie_t completed_cookie;
> + dma_cookie_t cookie;
> + spinlock_t lock;
> + struct list_head pending_list;
> + struct xilinx_vdma_tx_descriptor *active_desc;
> + struct list_head done_list;
> + struct dma_chan common;
> + struct dma_pool *desc_pool;
> + struct device *dev;
> + int irq;
> + int id;
> + enum dma_transfer_direction direction;
why should channel have a direction... descriptor should have direction and not
the channel!
> +/**
> + * xilinx_vdma_free_tx_descriptor - Free transaction descriptor
> + * @chan: Driver specific VDMA channel
> + * @desc: VDMA transaction descriptor
> + */
> +static void
> +xilinx_vdma_free_tx_descriptor(struct xilinx_vdma_chan *chan,
> + struct xilinx_vdma_tx_descriptor *desc)
> +{
> + struct xilinx_vdma_tx_segment *segment, *next;
> +
> + if (!desc)
> + return;
> +
> + list_for_each_entry_safe(segment, next, &desc->segments, node) {
do you want to use _safe. Isee that this is called for cleanup while lock held,
and in other case within another _safe iterator!
> + list_del(&segment->node);
> + xilinx_vdma_free_tx_segment(chan, segment);
> + }
> +
> + kfree(desc);
> +}
> +
> +/* Required functions */
> +
> + * xilinx_vdma_do_tasklet - Schedule completion tasklet
> + * @data: Pointer to the Xilinx VDMA channel structure
> + */
> +static void xilinx_vdma_do_tasklet(unsigned long data)
> +{
> + struct xilinx_vdma_chan *chan = (struct xilinx_vdma_chan *)data;
> +
> + xilinx_vdma_chan_desc_cleanup(chan);
> +}
> +
> +/**
> + * xilinx_vdma_alloc_chan_resources - Allocate channel resources
> + * @dchan: DMA channel
> + *
> + * Return: '1' on success and failure value on error
naaah, we dont do that, pls use standard notation of 0 on success
Also API wants you to return descriptors allocated here!
> + */
> +static int xilinx_vdma_alloc_chan_resources(struct dma_chan *dchan)
> +{
> + struct xilinx_vdma_chan *chan = to_xilinx_chan(dchan);
> +
> + /* Has this channel already been allocated? */
> + if (chan->desc_pool)
> + return 1;
> +
> + /*
> + * We need the descriptor to be aligned to 64bytes
> + * for meeting Xilinx VDMA specification requirement.
> + */
> + chan->desc_pool = dma_pool_create("xilinx_vdma_desc_pool",
> + chan->dev,
> + sizeof(struct xilinx_vdma_tx_segment),
> + __alignof__(struct xilinx_vdma_tx_segment), 0);
> + if (!chan->desc_pool) {
> + dev_err(chan->dev,
> + "unable to allocate channel %d descriptor pool\n",
> + chan->id);
> + return -ENOMEM;
> + }
> +
> + tasklet_init(&chan->tasklet, xilinx_vdma_do_tasklet,
> + (unsigned long)chan);
> +
> + chan->completed_cookie = DMA_MIN_COOKIE;
> + chan->cookie = DMA_MIN_COOKIE;
Can you use virtual dma implementation to simplfy your implemenattion of lists,
cookies (driver/dma/virt-dma.c)
> + /* There is at least one descriptor free to be allocated */
???
> + return 1;
> +}
> +
> + * xilinx_vdma_tx_status - Get VDMA transaction status
> + * @dchan: DMA channel
> + * @cookie: Transaction identifier
> + * @txstate: Transaction state
> + *
> + * Return: DMA transaction status
> + */
> +static enum dma_status xilinx_vdma_tx_status(struct dma_chan *dchan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct xilinx_vdma_chan *chan = to_xilinx_chan(dchan);
> + dma_cookie_t last_used;
> + dma_cookie_t last_complete;
> +
> + xilinx_vdma_chan_desc_cleanup(chan);
> +
> + last_used = dchan->cookie;
> + last_complete = chan->completed_cookie;
> +
> + dma_set_tx_state(txstate, last_complete, last_used, 0);
> +
> + return dma_async_is_complete(cookie, last_complete, last_used);
no residue calculation?
> +}
> +
> + * xilinx_vdma_start - Start VDMA channel
> + * @chan: Driver specific VDMA channel
> + */
> +static void xilinx_vdma_start(struct xilinx_vdma_chan *chan)
> +{
> + int loop = XILINX_VDMA_LOOP_COUNT + 1;
> +
> + vdma_ctrl_set(chan, XILINX_VDMA_REG_DMACR, XILINX_VDMA_DMACR_RUNSTOP);
> +
> + /* Wait for the hardware to start */
> + while (loop--)
> + if (!(vdma_ctrl_read(chan, XILINX_VDMA_REG_DMASR) &
> + XILINX_VDMA_DMASR_HALTED))
> + break;
wouldnt do while be better than doing than increamenting loop by 1 above and
using in while!
> +
> + if (!loop) {
> + dev_err(chan->dev, "Cannot start channel %p: %x\n",
> + chan, vdma_ctrl_read(chan, XILINX_VDMA_REG_DMASR));
> +
> + chan->err = true;
> + }
> +
> + return;
> +}
> +
> +/**
> + * xilinx_vdma_prep_slave_sg - prepare a descriptor for a DMA_SLAVE transaction
> + * @dchan: DMA channel
> + * @sgl: scatterlist to transfer to/from
> + * @sg_len: number of entries in @sgl
> + * @dir: DMA direction
> + * @flags: transfer ack flags
> + * @context: unused
> + *
> + * Return: Async transaction descriptor on success and NULL on failure
> + */
> +static struct dma_async_tx_descriptor *
> +xilinx_vdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
> + unsigned int sg_len, enum dma_transfer_direction dir,
> + unsigned long flags, void *context)
okay now am worried, this is supposed to memcpy DMA so why slave-sg??
Looking at the driver overall, IMHO we need to do:
- use the virt-dma to simplfy the cookie handling and perhpasn the descrptors
too!
- Perhpas use interleaved API..?
- I dont think we should use the slave API as this seems memcpy case!
--
~Vinod
^ permalink raw reply
* [PATCH v2] dma: Add Xilinx AXI Video Direct Memory Access Engine driver support
From: Vinod Koul @ 2014-01-26 14:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E12224.9060506@metafoo.de>
On Thu, Jan 23, 2014 at 03:07:32PM +0100, Lars-Peter Clausen wrote:
> On 01/23/2014 03:00 PM, Andy Shevchenko wrote:
> > On Thu, 2014-01-23 at 14:50 +0100, Lars-Peter Clausen wrote:
> >> On 01/23/2014 02:38 PM, Shevchenko, Andriy wrote:
> >>> On Thu, 2014-01-23 at 12:25 +0100, Lars-Peter Clausen wrote:
> >>>> On 01/22/2014 05:52 PM, Srikanth Thokala wrote:
> >>>
> >>> [...]
> >>>
> >>>>> + /* Request the interrupt */
> >>>>> + chan->irq = irq_of_parse_and_map(node, 0);
> >>>>> + err = devm_request_irq(xdev->dev, chan->irq, xilinx_vdma_irq_handler,
> >>>>> + IRQF_SHARED, "xilinx-vdma-controller", chan);
> >>>>
> >>>> This is a clasic example of where to not use devm_request_irq. 'chan' is
> >>>> accessed in the interrupt handler, but if you use devm_request_irq 'chan'
> >>>> will be freed before the interrupt handler has been released, which means
> >>>> there is now a race condition where the interrupt handler can access already
> >>>> freed memory.ta
> >>>
> >>> Could you elaborate this case? As far as I understood managed resources
> >>> are a kind of stack pile. In this case you have no such condition. Where
> >>> am I wrong?
> >>
> >> The stacked stuff is only ran after the remove() function. Which means that
> >> you call dma_async_device_unregister() before the interrupt handler is
> >> freed. Another issue with the interrupt handler is a bit hidden. The driver
> >> does not call tasklet_kill in the remove function. Which it should though to
> >> make sure that the tasklet does not race against the freeing of the memory.
> >> And in order to make sure that the tasklet is not rescheduled you need to
> >> free the irq before killing the tasklet, since the interrupt handler
> >> schedules the tasklet.
> >
> > So, you mean devm_request_irq() will race in any DMA driver?
>
> Most likely yes. devm_request_irq() is race condition prone for the majority
> of device driver. You have to be really careful if you want to use it.
>
> >
> > I think the proper solution is to disable all device work in
> > the .remove() and devm will care about resources.
>
> As long as the interrupt handler is registered it can be called, the only
> proper solution is to make sure that the order in which resources are torn
> down is correct.
Wouldn't it work if we register the irq last in the probe. That wil ensure on
success the channel is always valid.
Also the tasklet is required to be killed not just in your .remove but also in
drivers .suspend handler, you dont want handler to be invoked after you returned
from your suspend
--
~Vinod
^ permalink raw reply
* [PATCH v2] dma: Add Xilinx AXI Video Direct Memory Access Engine driver support
From: Vinod Koul @ 2014-01-26 13:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E2698B.6070001@metafoo.de>
On Fri, Jan 24, 2014 at 02:24:27PM +0100, Lars-Peter Clausen wrote:
> On 01/24/2014 12:16 PM, Srikanth Thokala wrote:
> > Hi Lars,
> >
> > On Thu, Jan 23, 2014 at 4:55 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> On 01/22/2014 05:52 PM, Srikanth Thokala wrote:
> >> [...]
> >>> +/**
> >>> + * xilinx_vdma_device_control - Configure DMA channel of the device
> >>> + * @dchan: DMA Channel pointer
> >>> + * @cmd: DMA control command
> >>> + * @arg: Channel configuration
> >>> + *
> >>> + * Return: '0' on success and failure value on error
> >>> + */
> >>> +static int xilinx_vdma_device_control(struct dma_chan *dchan,
> >>> + enum dma_ctrl_cmd cmd, unsigned long arg)
> >>> +{
> >>> + struct xilinx_vdma_chan *chan = to_xilinx_chan(dchan);
> >>> +
> >>> + switch (cmd) {
> >>> + case DMA_TERMINATE_ALL:
> >>> + xilinx_vdma_terminate_all(chan);
> >>> + return 0;
> >>> + case DMA_SLAVE_CONFIG:
> >>> + return xilinx_vdma_slave_config(chan,
> >>> + (struct xilinx_vdma_config *)arg);
> >>
> >> You really shouldn't be overloading the generic API with your own semantics.
> >> DMA_SLAVE_CONFIG should take a dma_slave_config and nothing else.
> >
> > Ok. The driver needs few additional configuration from the slave
> > device like Vertical
> > Size, Horizontal Size, Stride etc., for the DMA transfers, in that case do you
> > suggest me to define a separate dma_ctrl_cmd like the one FSLDMA_EXTERNAL_START
> > defined for Freescale drivers?
>
> In my opinion it is not a good idea to have driver implement a generic API,
> but at the same time let the driver have custom semantics for those API
> calls. It's a bit like having a gpio driver that expects 23 and 42 as the
> values passed to gpio_set_value instead of 0 and 1. It completely defeats
> the purpose of a generic API, namely that you are able to write generic code
> that makes use of the API without having to know about which implementation
> API it is talking to. The dmaengine framework provides the
> dmaengine_prep_interleaved_dma() function to setup two dimensional
> transfers, e.g. take a look at sirf-dma.c or imx-dma.c.
The question here i think would be waht this device supports? Is the hardware
capable of doing interleaved transfers, then would make sense.
While we do try to get users use dma_slave_config, but there will always be
someone who have specfic params. If we can generalize then we might want to add
to the dma_slave_config as well
--
~Vinod
^ permalink raw reply
* [BUG] FL1009: xHCI host not responding to stop endpoint command.
From: Thomas Petazzoni @ 2014-01-26 13:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87sisfjeba.fsf@natisbad.org>
Dear Arnaud Ebalard,
On Thu, 23 Jan 2014 09:24:41 +0100, Arnaud Ebalard wrote:
> The various Armada-based devices I have are NAS which do not have PCIe
> slots to plug additional devices (everything is soldered). I don't know
> which device Thomas used for its tests. Just in case, I also added Willy
> in CC: who have various boards and may also have done more test with
> additional PCIe devices and CONFIG_PCI_MSI enabled on 3.13 kernel.
The device I've used to test MSI is a e1000e PCIe Intel network card.
It uses one MSI interrupt, so admittedly, the MSI testing is quite
limited for now.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH v4 08/18] watchdog: orion: Make RSTOUT register a separate resource
From: Ezequiel Garcia @ 2014-01-26 13:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E4003E.4070106@roeck-us.net>
On Sat, Jan 25, 2014 at 10:19:42AM -0800, Guenter Roeck wrote:
> On 01/22/2014 03:05 PM, Ezequiel Garcia wrote:
> > In order to support other SoC, it's required to distinguish
> > the 'control' timer register, from the 'rstout' register
> > that enables system reset on watchdog expiration.
> >
> > To prevent a compatibility break, this commit adds a fallback
> > to a hardcoded RSTOUT address.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> > .../devicetree/bindings/watchdog/marvel.txt | 6 ++-
> > arch/arm/mach-dove/include/mach/bridge-regs.h | 1 +
> > arch/arm/mach-kirkwood/include/mach/bridge-regs.h | 1 +
> > arch/arm/mach-mv78xx0/include/mach/bridge-regs.h | 1 +
> > arch/arm/mach-orion5x/include/mach/bridge-regs.h | 1 +
> > arch/arm/plat-orion/common.c | 10 +++--
> > drivers/watchdog/orion_wdt.c | 44 +++++++++++++++++++++-
> > 7 files changed, 56 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/marvel.txt
> > index 0731fbd..1544fe9 100644
> > --- a/Documentation/devicetree/bindings/watchdog/marvel.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/marvel.txt
> > @@ -3,7 +3,9 @@
> > Required Properties:
> >
> > - Compatibility : "marvell,orion-wdt"
> > -- reg : Address of the timer registers
> > +- reg : Should contain two entries: first one with the
> > + timer control address, second one with the
> > + rstout enable address.
> >
> > Optional properties:
> >
> > @@ -14,7 +16,7 @@ Example:
> >
> > wdt at 20300 {
> > compatible = "marvell,orion-wdt";
> > - reg = <0x20300 0x28>;
> > + reg = <0x20300 0x28>, <0x20108 0x4>;
> > interrupts = <3>;
> > timeout-sec = <10>;
> > status = "okay";
> > diff --git a/arch/arm/mach-dove/include/mach/bridge-regs.h b/arch/arm/mach-dove/include/mach/bridge-regs.h
> > index 5362df3..f4a5b34 100644
> > --- a/arch/arm/mach-dove/include/mach/bridge-regs.h
> > +++ b/arch/arm/mach-dove/include/mach/bridge-regs.h
> > @@ -21,6 +21,7 @@
> > #define CPU_CTRL_PCIE1_LINK 0x00000008
> >
> > #define RSTOUTn_MASK (BRIDGE_VIRT_BASE + 0x0108)
> > +#define RSTOUTn_MASK_PHYS (BRIDGE_PHYS_BASE + 0x0108)
> > #define SOFT_RESET_OUT_EN 0x00000004
> >
> > #define SYSTEM_SOFT_RESET (BRIDGE_VIRT_BASE + 0x010c)
> > diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> > index 8b9d1c9..60f6421 100644
> > --- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> > +++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> > @@ -21,6 +21,7 @@
> > #define CPU_RESET 0x00000002
> >
> > #define RSTOUTn_MASK (BRIDGE_VIRT_BASE + 0x0108)
> > +#define RSTOUTn_MASK_PHYS (BRIDGE_PHYS_BASE + 0x0108)
> > #define SOFT_RESET_OUT_EN 0x00000004
> >
> > #define SYSTEM_SOFT_RESET (BRIDGE_VIRT_BASE + 0x010c)
> > diff --git a/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h b/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
> > index 5f03484..e20d6da 100644
> > --- a/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
> > +++ b/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
> > @@ -15,6 +15,7 @@
> > #define L2_WRITETHROUGH 0x00020000
> >
> > #define RSTOUTn_MASK (BRIDGE_VIRT_BASE + 0x0108)
> > +#define RSTOUTn_MASK_PHYS (BRIDGE_PHYS_BASE + 0x0108)
> > #define SOFT_RESET_OUT_EN 0x00000004
> >
> > #define SYSTEM_SOFT_RESET (BRIDGE_VIRT_BASE + 0x010c)
> > diff --git a/arch/arm/mach-orion5x/include/mach/bridge-regs.h b/arch/arm/mach-orion5x/include/mach/bridge-regs.h
> > index f727d03..5766e3f 100644
> > --- a/arch/arm/mach-orion5x/include/mach/bridge-regs.h
> > +++ b/arch/arm/mach-orion5x/include/mach/bridge-regs.h
> > @@ -18,6 +18,7 @@
> > #define CPU_CTRL (ORION5X_BRIDGE_VIRT_BASE + 0x104)
> >
> > #define RSTOUTn_MASK (ORION5X_BRIDGE_VIRT_BASE + 0x108)
> > +#define RSTOUTn_MASK_PHYS (ORION5X_BRIDGE_PHYS_BASE + 0x108)
> >
> > #define CPU_SOFT_RESET (ORION5X_BRIDGE_VIRT_BASE + 0x10c)
> >
> > diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
> > index c66d163..3375037 100644
> > --- a/arch/arm/plat-orion/common.c
> > +++ b/arch/arm/plat-orion/common.c
> > @@ -594,14 +594,16 @@ void __init orion_spi_1_init(unsigned long mapbase)
> > /*****************************************************************************
> > * Watchdog
> > ****************************************************************************/
> > -static struct resource orion_wdt_resource =
> > - DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x28);
> > +static struct resource orion_wdt_resource[] = {
> > + DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x04),
> > + DEFINE_RES_MEM(RSTOUTn_MASK_PHYS, 0x04),
> > +};
> >
> > static struct platform_device orion_wdt_device = {
> > .name = "orion_wdt",
> > .id = -1,
> > - .num_resources = 1,
> > - .resource = &orion_wdt_resource,
> > + .num_resources = ARRAY_SIZE(orion_wdt_resource),
> > + .resource = orion_wdt_resource,
> > };
> >
> > void __init orion_wdt_init(void)
> > diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> > index f5e7b17..ba8eea9d 100644
> > --- a/drivers/watchdog/orion_wdt.c
> > +++ b/drivers/watchdog/orion_wdt.c
> > @@ -26,6 +26,12 @@
> > #include <linux/of.h>
> > #include <mach/bridge-regs.h>
> >
> > +/* RSTOUT mask register physical address for Orion5x, Kirkwood and Dove */
> > +#define ORION_RSTOUT_MASK_OFFSET 0x20108
> > +
> > +/* Internal registers can be configured at any 1 MiB aligned address */
> > +#define INTERNAL_REGS_MASK ~(SZ_1M - 1)
> > +
> > /*
> > * Watchdog timer block registers.
> > */
> > @@ -44,6 +50,7 @@ static unsigned int wdt_max_duration; /* (seconds) */
> > static struct clk *clk;
> > static unsigned int wdt_tclk;
> > static void __iomem *wdt_reg;
> > +static void __iomem *wdt_rstout;
> >
> > static int orion_wdt_ping(struct watchdog_device *wdt_dev)
> > {
> > @@ -64,14 +71,14 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
> > atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
> >
> > /* Enable reset on watchdog */
> > - atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
> > + atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
> > return 0;
> > }
> >
> > static int orion_wdt_stop(struct watchdog_device *wdt_dev)
> > {
> > /* Disable reset on watchdog */
> > - atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, 0);
> > + atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, 0);
> >
> > /* Disable watchdog timer */
> > atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0);
> > @@ -116,6 +123,33 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
> > return IRQ_HANDLED;
> > }
> >
> > +/*
> > + * The original devicetree binding for this driver specified only
> > + * one memory resource, so in order to keep DT backwards compatibility
> > + * we try to fallback to a hardcoded register address, if the resource
> > + * is missing from the devicetree.
> > + */
> > +static void __iomem *try_rstout_ioremap(struct platform_device *pdev,
> > + phys_addr_t internal_regs)
> > +{
> > + struct resource *res;
> > + phys_addr_t rstout;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (res)
> > + return devm_ioremap(&pdev->dev, res->start,
> > + resource_size(res));
> > +
> > + /* This workaround works only for "orion-wdt", DT-enabled */
> > + if (!of_device_is_compatible(pdev->dev.of_node, "marvell,orion-wdt"))
> > + return NULL;
> > +
> > + rstout = internal_regs + ORION_RSTOUT_MASK_OFFSET;
> > +
> > + WARN(1, FW_BUG "falling back to harcoded RSTOUT reg 0x%x\n", rstout);
>
> WARN seems to be a bit excessive here. Is that on purpose (sorry if that was discussed and I missed it) ?
>
> Assuming it is on purpose
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
Yes, it's on purpose. We want users to notice this and be aware they have
a broken dtb (hence the sign firmware bug).
--
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH v4 08/18] watchdog: orion: Make RSTOUT register a separate resource
From: Ezequiel Garcia @ 2014-01-26 13:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140126091753.2910a39f@skate>
On Sun, Jan 26, 2014 at 09:17:53AM +0100, Thomas Petazzoni wrote:
[..]
> > +/*
> > + * The original devicetree binding for this driver specified only
> > + * one memory resource, so in order to keep DT backwards compatibility
> > + * we try to fallback to a hardcoded register address, if the resource
> > + * is missing from the devicetree.
> > + */
> > +static void __iomem *try_rstout_ioremap(struct platform_device *pdev,
> > + phys_addr_t internal_regs)
>
> Why is it called "try" ? It actually does the mapping. So I would
> prefer the function to be named:
>
> orion_wdt_ioremap_rstout()
>
Ah, yes. This is a left over from the previous attempt.
--
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH v4 07/18] watchdog: orion: Handle IRQ
From: Ezequiel Garcia @ 2014-01-26 13:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140126090412.37f4932f@skate>
On Sun, Jan 26, 2014 at 09:04:12AM +0100, Thomas Petazzoni wrote:
> Dear Ezequiel Garcia,
>
> On Wed, 22 Jan 2014 20:05:04 -0300, Ezequiel Garcia wrote:
> > DT-enabled where an irqchip driver for the brigde interrupt controller is
> > available can handle the watchdog IRQ properly. Therefore, we request
> > the interruption and add a dummy handler that merely calls panic().
>
> I don't quiite understand the first sentence of this commit log, and
> the commit title looks wrong. Maybe a bad copy/paste or something?
>
Hm... yes it doesn't look right. It should read:
"DT-enabled platforms, where the irqchip driver for the brigde interrupt
controller is available, can handle the watchdog IRQ properly. Therefore,
we request the interrupt and add a dummy handler that merely calls panic()".
I guess we can re-phrase it be a bit more readable.
Why does th commit title looks wrong? By requesting the IRQ we are
"handling it", no?
> >
> > This is done in order to have an initial 'ack' of the interruption,
>
> interruption -> interrupt
>
Right.
--
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH] dma: fix vchan_cookie_complete() debug print
From: Vinod Koul @ 2014-01-26 11:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140120132829.GD15937@n2100.arm.linux.org.uk>
On Mon, Jan 20, 2014 at 01:28:29PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 20, 2014 at 05:33:01PM +0530, Vinod Koul wrote:
> > On Mon, Jan 20, 2014 at 11:28:22AM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Jan 20, 2014 at 03:29:17PM +0530, Vinod Koul wrote:
> > > > On Fri, Dec 06, 2013 at 04:42:09PM +0100, Jonas Jensen wrote:
> > > > > vd->tx.cookie is set zero on dma_cookie_complete(),
> > > > > save to local before printing it.
> > > > >
> > > > > Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > > dev_vdbg() could also be moved to happen earlier, what do you prefer?
> > > > This would be preferred IMHO. Also pls cc dmaengine at vger on this
> > >
> > > I prefer this version - it means that the verbose debug printk doesn't
> > > impact the completion timing when printk is expensive (eg, because its
> > > outputting via a serial port.)
> > But if you know your printk is costly, do you want to enable these?
>
> dev_vdbg() is for verbose debugging - you only enable it if you really
> need to. Even so, it should have _minimal_ impact where possible. That's
> why I prefer the first patch, because we mark the cookie as being
> complete _before_ we call the verbose debugging, which isn't going to add
> milliseconds to that.
Sure this version is better approach in that respect as it makes it debug
aognostic! Both mine and Dan's comment were trying to simlify by ignoring debug
option, but yes i do agree to you point here. So this patch will be applied!
> If you don't care about debugging, then getting rid of the dev_vdbg().
> But really, I could pull rank and say that this is *my* file, I get to
> choose how stuff should be done here - I'd prefer not to but...
That is not required!
--
~Vinod
^ permalink raw reply
* [PATCH v4 08/18] watchdog: orion: Make RSTOUT register a separate resource
From: Thomas Petazzoni @ 2014-01-26 8:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390431915-5115-9-git-send-email-ezequiel.garcia@free-electrons.com>
Dear Ezequiel Garcia,
On Wed, 22 Jan 2014 20:05:05 -0300, Ezequiel Garcia wrote:
> +/* RSTOUT mask register physical address for Orion5x, Kirkwood and Dove */
> +#define ORION_RSTOUT_MASK_OFFSET 0x20108
> +
> +/* Internal registers can be configured at any 1 MiB aligned address */
> +#define INTERNAL_REGS_MASK ~(SZ_1M - 1)
I'm not a big fan of hardcoding the internal register window size in a
driver. I would have preferred to have used the offset between the
watchdog registers and the rstout register, but since this one is
*before* the watchdog registers, it would have to be a negative offset.
Not sure how to handle this.
> +/*
> + * The original devicetree binding for this driver specified only
> + * one memory resource, so in order to keep DT backwards compatibility
> + * we try to fallback to a hardcoded register address, if the resource
> + * is missing from the devicetree.
> + */
> +static void __iomem *try_rstout_ioremap(struct platform_device *pdev,
> + phys_addr_t internal_regs)
Why is it called "try" ? It actually does the mapping. So I would
prefer the function to be named:
orion_wdt_ioremap_rstout()
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH v4 07/18] watchdog: orion: Handle IRQ
From: Thomas Petazzoni @ 2014-01-26 8:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390431915-5115-8-git-send-email-ezequiel.garcia@free-electrons.com>
Dear Ezequiel Garcia,
On Wed, 22 Jan 2014 20:05:04 -0300, Ezequiel Garcia wrote:
> DT-enabled where an irqchip driver for the brigde interrupt controller is
> available can handle the watchdog IRQ properly. Therefore, we request
> the interruption and add a dummy handler that merely calls panic().
I don't quiite understand the first sentence of this commit log, and
the commit title looks wrong. Maybe a bad copy/paste or something?
>
> This is done in order to have an initial 'ack' of the interruption,
interruption -> interrupt
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH v2 2/3] ata: sata_mv: fix disk hotplug for Armada 370/XP SoCs
From: Thomas Petazzoni @ 2014-01-26 7:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1389711007-7239-3-git-send-email-simon.guinot@sequanux.org>
Dear Simon Guinot,
On Tue, 14 Jan 2014 15:50:06 +0100, Simon Guinot wrote:
> + if (hpriv->hp_flags & MV_HP_FIX_LP_PHY_CTL) {
> + void __iomem *lp_phy_addr =
> + mv_ap_base(link->ap) + LP_PHY_CTL;
> + /*
> + * Set PHY speed according to SControl speed.
> + */
> + if ((val & 0xf0) == 0x10)
> + writelfl(0x7, lp_phy_addr);
> + else
> + writelfl(0x227, lp_phy_addr);
> + }
I think we could do a little bit better than these magical values.
The datasheet says:
* bits 12:9, PIN_PHY_GEN_RX. Value 0x0 => 1.5 Gbps, value 0x1 => 3 Gbps
* bits 8:5, PIN_PHY_GEN_TX. Value 0x0 => 1.5 Gbps, value 0x1 => 3 Gbps
* bit 2, PIN_PU_TX. Value 0x0 => Power down, value 0x1 => Power up.
* bit 1, PIN_PU_RX. Value 0x0 => Power down, value 0x1 => Power up.
* bit 0, PIN_PU_PLL. Value 0x0 => Power down, value 0x1 => Power up.
So maybe something like:
#define PIN_PHY_GEN_1_5 0
#define PIN_PHY_GEN_3 1
#define PIN_PHY_GEN_RX(gen) ((gen) << 9)
#define PIN_PHY_GEN_TX(gen) ((gen) << 5)
#define PIN_PU_TX BIT(2)
#define PIN_PU_RX BIT(1)
#define PIN_PU_PLL BIT(0)
u32 sata_gen;
if ((val & 0xf0) == 0x10)
sata_gen = PIN_PHY_GEN_1_5;
else
sata_gen = PIN_PHY_GEN_3;
writelfl(PIN_PHY_GEN_RX(sata_gen) |
PIN_PHY_GEN_TX(sata_gen) |
PIN_PU_TX | PIN_PU_RX | PIN_PU_PLL,
lp_phy_addr);
> + /*
> + * To allow disk hotplug on Armada 370/XP SoCs, the PHY speed must be
> + * updated in the LP_PHY_CTL register.
> + */
> + if (pdev->dev.of_node &&
> + of_device_is_compatible(pdev->dev.of_node,
> + "marvell,armada-370-xp-sata"))
Testing whether pdev->dev.of_node is not NULL does not seems to be
useful. A quick read of of_device_is_compatible() and the function it's
calling seem to indicate that of_device_is_compatible will return false
if the passed struct device_node * is NULL.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply
* [Q] L1_CACHE_BYTES on flush_pfn_alias function.
From: Jungseung Lee @ 2014-01-26 5:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124154321.GI19052@arm.com>
Not to flush some more bytes. In the scenario, they can *omit* to flush last 32 bytes.
L1_CACHE_BYTES = 64 (ARM v7, CA9)
asm( "mcrr p15, 0, %1, %0, c14\n"
" mcr p15, 0, %2, c7, c10, 4"
:
: "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero)
: "cc");
-----Original Message-----
From: Catalin Marinas [mailto:catalin.marinas at arm.com]
Sent: Saturday, January 25, 2014 12:43 AM
Cc: linux-arm-kernel at lists.infradead.org; linux at arm.linux.org.uk; linux-kernel at vger.kernel.org
Subject: Re: [Q] L1_CACHE_BYTES on flush_pfn_alias function.
On Fri, Jan 17, 2014 at 09:54:42AM +0000, wrote:
> Follow the mailing-list
> http://comments.gmane.org/gmane.linux.ports.arm.omap/31686
>
> >>Setting the L1 cache line size larger than it actually is should be safe.
>
> the written code expected as L1_CACHE_BYTES should be real cache line
> size has bug.
> It looks like that flush_pfn_alias function should be fixed.
Did you actually notice any problem with flushing some more bytes? It's a clean+invalidate rather than invalidate, I don't see any problem.
--
Catalin
^ permalink raw reply
* GPIO triggers kernel reboot
From: Florian Fainelli @ 2014-01-26 2:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E3E031.6040709@denx.de>
Le 25/01/2014 08:02, Heiko Schocher a ?crit :
> Hello all,
>
> is it possible to reboot current mainline kernel triggered from a
> GPIO in kernel space?
>
> If not, how can be done this, so that it has a chance to go into
> mainline?
Your GPIO could (should) be bound to a gpio-keys driver which will
deliver key presses event; which you can ultimately bind to the action
of triggering a system reboot.
Granted, this makes you cross a lot of layers, and requires user-space
to implemnt the reboot policy, but what you are asking for is both a
mechanism and policy mix, which is usually not accepted.
You are probably working with HW which won't change, but wiring a reset
button to a system reset output is usually much simpler ;)
>
> Thanks for any suggestions
>
> bye,
> Heiko
^ permalink raw reply
* [PATCH 0/4] clk: mvebu: fix clk init order
From: Ezequiel Garcia @ 2014-01-26 0:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E4367B.3090608@elopez.com.ar>
Hi Emilio,
Thanks for your help with this.
On Sat, Jan 25, 2014 at 07:11:07PM -0300, Emilio L?pez wrote:
[..]
> >
> > Ok, I'll look if using of_clk_get_parent_name will help here. But again,
> > I can see that clk-gating driver gets registered before core-clk driver.
> > There may be no code to give you the parent name at that time.
>
> After looking at some of the armada*.dtsi, I see you don't list the
> clock names on the coreclk node, so of_clk_get_parent_name may not be of
> much value after all.
>
IIRC, we faced a similar issue with the Core Divider clock and solved it by
specifying the clock names in the DT.
I meant to complete the core and gating clocks in a similar way
(providing names on the DT), but apparently (as discussed with Gregory Clement)
Mike Turquette and others are planning to remove the clock names from
the DT entirely.
Can you guys explain about this plan a bit further? Or do you think we
should specify the names on the DT for all the clocks instead?
Notice that the latter would remove lots of strings from the kernel
itself (right?)
--
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH 0/4] clk: mvebu: fix clk init order
From: Emilio López @ 2014-01-25 22:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E43046.1030105@gmail.com>
Sebastian,
El 25/01/14 18:44, Sebastian Hesselbarth escribi?:
> On 01/25/2014 10:32 PM, Emilio L?pez wrote:
>> El 25/01/14 15:19, Sebastian Hesselbarth escribi?:
>>> This patch set fixes clk init order that went upside-down with
>>> v3.14. I haven't really investigated what caused this, but I assume
>>> it is related with DT node reordering by addresses.
>>
>> The framework should be able to deal with unordered registration. I am
>> not very familiar with the mvebu driver though, do you have a valid
>> reason to require a specific order?
>
> Emilio,
>
> I rather think that everthing registered with CLK_OF_DECLARE cannot
> deal with unordered registration. The callback passed to CLK_OF_DECLARE
> has to have void as return value, so there is no way to pass errors,
> e.g. -EPROBE_DEFER, back to of_clk_init.
Indeed. What I meant is that the framework works fine if you first
register a child clock that refers to a not yet registered parent, and
then register the parent. The registration need not be strictly ordered.
> The reason for this ordering is that the clock gates depend on core
> clocks. It is always that way, so merging both init functions isn't
> that odd.
If your only dependency is the parent name, and you can use DT or
something else to get it, then you don't need to enforce an order.
>>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
>>> registered before core clocks driver. Unfortunately, we cannot
>>> return -EPROBE_DEFER in drivers initialized by clk_of_init.
>>
>> Why would you need to do so? After a quick inspection on the code, I see
>> you may have problems on mvebu_clk_gating_setup() when getting the
>> default parent clock name, but I believe you could solve it in an easier
>> way by using of_clk_get_parent_name().
>
> Ok, I'll look if using of_clk_get_parent_name will help here. But again,
> I can see that clk-gating driver gets registered before core-clk driver.
> There may be no code to give you the parent name at that time.
After looking at some of the armada*.dtsi, I see you don't list the
clock names on the coreclk node, so of_clk_get_parent_name may not be of
much value after all.
Cheers,
Emilio
^ permalink raw reply
* [PATCH 0/4] clk: mvebu: fix clk init order
From: Sebastian Hesselbarth @ 2014-01-25 21:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E42D6A.5050508@elopez.com.ar>
On 01/25/2014 10:32 PM, Emilio L?pez wrote:
> El 25/01/14 15:19, Sebastian Hesselbarth escribi?:
>> This patch set fixes clk init order that went upside-down with
>> v3.14. I haven't really investigated what caused this, but I assume
>> it is related with DT node reordering by addresses.
>
> The framework should be able to deal with unordered registration. I am
> not very familiar with the mvebu driver though, do you have a valid
> reason to require a specific order?
Emilio,
I rather think that everthing registered with CLK_OF_DECLARE cannot
deal with unordered registration. The callback passed to CLK_OF_DECLARE
has to have void as return value, so there is no way to pass errors,
e.g. -EPROBE_DEFER, back to of_clk_init.
The reason for this ordering is that the clock gates depend on core
clocks. It is always that way, so merging both init functions isn't
that odd.
>> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
>> registered before core clocks driver. Unfortunately, we cannot
>> return -EPROBE_DEFER in drivers initialized by clk_of_init.
>
> Why would you need to do so? After a quick inspection on the code, I see
> you may have problems on mvebu_clk_gating_setup() when getting the
> default parent clock name, but I believe you could solve it in an easier
> way by using of_clk_get_parent_name().
Ok, I'll look if using of_clk_get_parent_name will help here. But again,
I can see that clk-gating driver gets registered before core-clk driver.
There may be no code to give you the parent name at that time.
Sebastian
^ permalink raw reply
* [PATCH 0/4] clk: mvebu: fix clk init order
From: Emilio López @ 2014-01-25 21:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390673950-4521-1-git-send-email-sebastian.hesselbarth@gmail.com>
Hello Sebastian,
El 25/01/14 15:19, Sebastian Hesselbarth escribi?:
> This patch set fixes clk init order that went upside-down with
> v3.14. I haven't really investigated what caused this, but I assume
> it is related with DT node reordering by addresses.
The framework should be able to deal with unordered registration. I am
not very familiar with the mvebu driver though, do you have a valid
reason to require a specific order?
> Anyway, with v3.14 for MVEBU SoCs, the clock gating driver gets
> registered before core clocks driver. Unfortunately, we cannot
> return -EPROBE_DEFER in drivers initialized by clk_of_init.
Why would you need to do so? After a quick inspection on the code, I see
you may have problems on mvebu_clk_gating_setup() when getting the
default parent clock name, but I believe you could solve it in an easier
way by using of_clk_get_parent_name().
Cheers,
Emilio
^ permalink raw reply
* [PATCH 9/9] ARM: dts: omap3-gta04: Add ti, omap36xx to compatible property to avoid problems with booting.
From: Marek Belisko @ 2014-01-25 21:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390685343-11663-1-git-send-email-marek@goldelico.com>
Without that change booting leads to crash with more warnings like below:
[ 0.284454] omap_hwmod: uart4: cannot clk_get main_clk uart4_fck
[ 0.284484] omap_hwmod: uart4: cannot _init_clocks
[ 0.284484] ------------[ cut here ]------------
[ 0.284545] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/omap_hwmod.c:2543 _init+0x300/0x3e4()
[ 0.284545] omap_hwmod: uart4: couldn't init clocks
[ 0.284576] Modules linked in:
[ 0.284606] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0-next-20140124-00020-gd2aefec-dirty #26
[ 0.284637] [<c00151c0>] (unwind_backtrace) from [<c0011e20>] (show_stack+0x10/0x14)
[ 0.284667] [<c0011e20>] (show_stack) from [<c0568544>] (dump_stack+0x7c/0x94)
[ 0.284729] [<c0568544>] (dump_stack) from [<c003ff94>] (warn_slowpath_common+0x6c/0x90)
[ 0.284729] [<c003ff94>] (warn_slowpath_common) from [<c003ffe8>] (warn_slowpath_fmt+0x30/0x40)
[ 0.284759] [<c003ffe8>] (warn_slowpath_fmt) from [<c07d1be8>] (_init+0x300/0x3e4)
[ 0.284790] [<c07d1be8>] (_init) from [<c07d217c>] (__omap_hwmod_setup_all+0x40/0x8c)
[ 0.284820] [<c07d217c>] (__omap_hwmod_setup_all) from [<c0008918>] (do_one_initcall+0xe8/0x14c)
[ 0.284851] [<c0008918>] (do_one_initcall) from [<c07c5c18>] (kernel_init_freeable+0x104/0x1c8)
[ 0.284881] [<c07c5c18>] (kernel_init_freeable) from [<c0563524>] (kernel_init+0x8/0x118)
[ 0.284912] [<c0563524>] (kernel_init) from [<c000e368>] (ret_from_fork+0x14/0x2c)
[ 0.285064] ---[ end trace 63de210ad43b627d ]---
Reference:
https://lkml.org/lkml/2013/10/8/553
Signed-off-by: Marek Belisko <marek@goldelico.com>
---
arch/arm/boot/dts/omap3-gta04.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/omap3-gta04.dts b/arch/arm/boot/dts/omap3-gta04.dts
index ea1b94d..998fe1e 100644
--- a/arch/arm/boot/dts/omap3-gta04.dts
+++ b/arch/arm/boot/dts/omap3-gta04.dts
@@ -13,7 +13,7 @@
/ {
model = "OMAP3 GTA04";
- compatible = "ti,omap3-gta04", "ti,omap3";
+ compatible = "ti,omap3-gta04", "ti,omap36xx", "ti,omap3";
cpus {
cpu at 0 {
--
1.8.3.2
^ permalink raw reply related
* [PATCH 8/9] ARM: dts: omap3-gta04: Add support for magnetometer.
From: Marek Belisko @ 2014-01-25 21:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390685343-11663-1-git-send-email-marek@goldelico.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
arch/arm/boot/dts/omap3-gta04.dts | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/omap3-gta04.dts b/arch/arm/boot/dts/omap3-gta04.dts
index 151ed3b..ea1b94d 100644
--- a/arch/arm/boot/dts/omap3-gta04.dts
+++ b/arch/arm/boot/dts/omap3-gta04.dts
@@ -100,6 +100,12 @@
regulator-max-microvolt = <3150000>;
};
};
+
+ /* compass aka magnetometer */
+ hmc5843 at 1e {
+ compatible = "honeywell,hmc5843";
+ reg = <0x1e>;
+ };
};
#include "twl4030.dtsi"
--
1.8.3.2
^ permalink raw reply related
* [PATCH 7/9] ARM: dts: omap3-gta04: Add touchscreen properties.
From: Marek Belisko @ 2014-01-25 21:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390685343-11663-1-git-send-email-marek@goldelico.com>
Signed-off-by: Marek Belisko <marek@goldelico.com>
---
arch/arm/boot/dts/omap3-gta04.dts | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/omap3-gta04.dts b/arch/arm/boot/dts/omap3-gta04.dts
index f72e408..151ed3b 100644
--- a/arch/arm/boot/dts/omap3-gta04.dts
+++ b/arch/arm/boot/dts/omap3-gta04.dts
@@ -150,6 +150,16 @@
reg = <0x4>;
};
};
+
+ /* touchscreen */
+ tsc2007 at 48 {
+ compatible = "ti,tsc2007";
+ reg = <0x48>;
+ interrupt-parent = <&gpio6>;
+ interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
+ gpios = <&gpio6 0 GPIO_ACTIVE_LOW>;
+ ti,x-plate-ohms = <600>;
+ };
};
&i2c3 {
--
1.8.3.2
^ permalink raw reply related
* [PATCH 6/9] ARM: dts: omap3-gta04: Add twl4030 charger.
From: Marek Belisko @ 2014-01-25 21:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390685343-11663-1-git-send-email-marek@goldelico.com>
Signed-off-by: Marek Belisko <marek@goldelico.com>
---
arch/arm/boot/dts/omap3-gta04.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/omap3-gta04.dts b/arch/arm/boot/dts/omap3-gta04.dts
index a924a843..f72e408 100644
--- a/arch/arm/boot/dts/omap3-gta04.dts
+++ b/arch/arm/boot/dts/omap3-gta04.dts
@@ -198,3 +198,7 @@
pinctrl-0 = <&uart3_pins>;
};
+&charger {
+ bb_uvolt = <3200000>;
+ bb_uamp = <150>;
+};
--
1.8.3.2
^ permalink raw reply related
* [PATCH 5/9] ARM: dts: omap3-gta04: Add basic sound support.
From: Marek Belisko @ 2014-01-25 21:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390685343-11663-1-git-send-email-marek@goldelico.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
arch/arm/boot/dts/omap3-gta04.dts | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/boot/dts/omap3-gta04.dts b/arch/arm/boot/dts/omap3-gta04.dts
index 7e09410..a924a843 100644
--- a/arch/arm/boot/dts/omap3-gta04.dts
+++ b/arch/arm/boot/dts/omap3-gta04.dts
@@ -36,6 +36,14 @@
gpio-key,wakeup;
};
};
+
+ sound {
+ compatible = "ti,omap-twl4030";
+ ti,model = "gta04";
+
+ ti,mcbsp = <&mcbsp2>;
+ ti,codec = <&twl_audio>;
+ };
};
&omap3_pmx_core {
@@ -79,6 +87,13 @@
reg = <0x48>;
interrupts = <7>; /* SYS_NIRQ cascaded to intc */
interrupt-parent = <&intc>;
+
+ twl_audio: audio {
+ compatible = "ti,twl4030-audio";
+ codec {
+ };
+ };
+
vaux4: regulator-vaux4 {
compatible = "ti,twl4030-vaux4";
regulator-min-microvolt = <2800000>;
--
1.8.3.2
^ 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