Re: [PATCH] clk: provide public clk_is_enabled function
On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote: > > On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote: > > On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote: > > > To determine if a clk has been previously enabled, provide a public > > > clk_is_enabled function. This is especially helpful to check the state > > > of clk-gate without actually changing the state of the gate. > > I wonder what you want to do with the return value. > > > > When doing > > > > if (clk_is_enabled(someclk)) > > do_something(); > > > > you cannot in general know if the clock is still on when you start to > > do_something. > > Hi Uwe > > At least in the use case Sebastian needs it for, we don't need an "in > general" solution. It is used early boot time to see if the boot > loader left the clock running. Wait, unless I'm missing something, the clk_is_enabled() call _won't_ determine whether the clock is enabled in hardware (whether the boot loader created or left this condition), instead it only determines whether clk_enable() was called previously and thus the clock _shall_ be enabled. AFAIK the kernel's CCF support is "self contained" and does not consider any data or state that was "inherited" from boot staged before the kernel. That's why the "disable unused" step disables everything that wasn't acquired _in the kernel_ regardless of what the boot loader may have done or what is enabled at reset. > The other user of the clock is the > ethernet driver, which we know cannot change it yet, because driver > probing has not started yet. I understand that the situation here is, that the ethernet driver hasn't probed yet, but the clock driver did. You are in early setup code and want to (check and) fetch data from the hardware which the ethernet driver later needs. What's wrong with an explicit enable/disable around the data acquisition? This allows to reliably fetch and store the information (into the device tree data?) for later use, and is neutral to the enable counter. If the clock was enabled before, it remains enabled. If the clock was disabled before, it will get disabled again. In any case clock only may be turned off after you have grabbed the data, the data is only taken when it's available and valid. This approach of explicit enable/disable around data access only breaks if the ethernet driver won't acquire its related clock appropriately, which would be a bug in the ethernet driver that should be easy to fix. (Strictly speaking it's not the data gathering that breaks, but the already broken network driver which accesses the hardware without acquiring the clock, and not finds the clock disabled.) virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V2 1/2] powerpc: mpc512x_dma: add support for data transfers between memory and i/o memory
dma_chan_to_mpc_dma_chan(chan);
> + struct dma_slave_config *cfg = (void *)arg;
> +
> + switch (cmd) {
> + case DMA_SLAVE_CONFIG:
> + if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES &&
> + cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
> + return -EINVAL;
> +
> + if (cfg->direction == DMA_DEV_TO_MEM) {
> + mchan->per_paddr = cfg->src_addr;
> + mchan->tcd_nunits = cfg->src_maxburst;
> + } else {
> + mchan->per_paddr = cfg->dst_addr;
> + mchan->tcd_nunits = cfg->dst_maxburst;
> + }
> +
> + return 0;
> + default:
> + return -ENOSYS;
> + }
> +
> + return -EINVAL;
> +}
> +
> static int mpc_dma_probe(struct platform_device *op)
> {
> struct device_node *dn = op->dev.of_node;
> @@ -725,9 +855,12 @@ static int mpc_dma_probe(struct platform_device *op)
> dma->device_issue_pending = mpc_dma_issue_pending;
> dma->device_tx_status = mpc_dma_tx_status;
> dma->device_prep_dma_memcpy = mpc_dma_prep_memcpy;
> + dma->device_prep_slave_sg = mpc_dma_prep_slave_sg;
> + dma->device_control = mpc_dma_device_control;
>
> INIT_LIST_HEAD(&dma->channels);
> dma_cap_set(DMA_MEMCPY, dma->cap_mask);
> + dma_cap_set(DMA_SLAVE, dma->cap_mask);
>
> for (i = 0; i < dma->chancnt; i++) {
> mchan = &mdma->channels[i];
> --
> 1.7.11.3
>
> ___
> Linuxppc-dev mailing list
> linuxppc-...@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [V2 2/2] powerpc/512x: add LocalPlus Bus FIFO device driver
PBFIFO_REG_FIFO_ALARM, 0x03e4);
> + } else {
> + out_be16(lpbfifo.regs + LPBFIFO_REG_FIFO_CONTROL, 0x0);
> + out_be32(lpbfifo.regs + LPBFIFO_REG_FIFO_ALARM, 0x03fc);
> + }
Can you use symbolic names for the flag bits, and decimal numbers
for the watermarks? This way they would match with the comment,
or the comment may become obsolete when numbers aren't
obfuscated.
> +
> + /* Start address is a physical address of the region
> + * which belongs to the device on localplus bus */
> + out_be32(lpbfifo.regs + LPBFIFO_REG_START_ADDRESS, req->bus_phys);
> +
> + /* Configure chip select, transfer direction,
> + * address increment option and bytes per transfer option */
> + bits = (req->cs & 0x7) << 24;
> + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_READ)
> + bits |= 3 << 16; /* read mode bit and flush bit */
> + if (no_incr)
> + bits |= 1 << 8;
> + bits |= bpt & 0x3f;
> + out_be32(lpbfifo.regs + LPBFIFO_REG_CONTROL, bits);
> +
> + /* Unmask irqs */
> + bits = 0x0201; /* set error irq & master enabled bit */
> + if (req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE)
> + bits |= 0x0100; /* set completion irq too */
> + out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, bits);
> +
> + /* 4. Set packet size and kick FIFO off */
> + bits = req->size;
> + bits |= (1<<31); /* set restart bit */
> + out_be32(lpbfifo.regs + LPBFIFO_REG_PACKET_SIZE, bits);
here you start the SCLPC transfer ...
> +
> +
> + /* 5. Then kick DMA off */
> + cookie = dma_tx->tx_submit(dma_tx);
> + if (dma_submit_error(cookie)) {
> + pr_err("DMA tx_submit failed\n");
> + e = -ENOSPC;
> + goto err_dma_submit;
> + }
... and here you setup the DMA job -- isn't this too late?
(Please note that I haven't re-checked the "functional
description" section in the DMA controller's chapter.)
> +
> + return 0;
> +
> + err_dma_submit:
> +
> + err_dma_prep:
> + dma_unmap_single(dma_dev->dev, sg_dma_address(&sg), req->size, dir);
> +
> + err_dma_map:
> + sg_dma_address(&sg) = 0;
> + dma_release_channel(lpbfifo.chan);
> +
> + err_align:
> + return e;
> +}
> +
> +int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req)
> +{
> + unsigned long flags;
> + int result = 0;
> +
> + if (!lpbfifo.regs)
> + return -ENODEV;
> +
> + spin_lock_irqsave(&lpbfifo.lock, flags);
> +
> + /* A transfer is in progress
> + * if the req pointer is already set */
> + if (lpbfifo.req) {
> + spin_unlock_irqrestore(&lpbfifo.lock, flags);
> + return -EBUSY;
> + }
> +
> + /* (128 kBytes - 4 Bytes) is a maximum packet size
> + * that LPB FIFO and DMA controller can handle together
> + * while exchanging DEFAULT_WORDS_PER_TRANSFER = 1
> + * per hardware request */
Could you state what the individual limits are?
I guess the SCLPC can transfer gigabytes, the FIFO depth either
should not matter or is dramatically lower than 128KB (1K?).
Is the DMA controller the limiting element, and how so? Does the
limit not depend on the biter and citer and width specs? Is the
limit mentioned here (in the LPB related code) assuming knowledge
of the DMA driver's internals?
> + if (req->size > 131068) {
> + spin_unlock_irqrestore(&lpbfifo.lock, flags);
> + return -ENOSPC;
> + }
> +
> + /* Setup the transfer */
> + lpbfifo.im_last = 0;
> + lpbfifo.req = req;
> +
> + result = mpc512x_lpbfifo_kick(req);
> + if (result != 0)
> + lpbfifo.req = NULL; /* Set the FIFO as idle */
> +
> + spin_unlock_irqrestore(&lpbfifo.lock, flags);
> +
> + return result;
> +}
> +EXPORT_SYMBOL(mpc512x_lpbfifo_submit);
> +
> +static int mpc512x_lpbfifo_probe(struct platform_device *pdev)
> +{
> + struct resource *r = &lpbfifo.res;
> + int e = 0, rc = -ENOMEM;
> +
> + if (of_address_to_resource(pdev->dev.of_node, 0, r)) {
> + e = -ENODEV;
> + goto err_res;
> + }
> +
> + if (!request_mem_region(r->start, resource_size(r), DRV_NAME)) {
> + e = -EBUSY;
> + goto err_res;
> + }
> +
> + lpbfifo.irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> + if (!lpbfifo.irq) {
> + e = -ENODEV;
> + goto err_res;
> + }
> +
> + lpbfifo.regs = ioremap(r->start, resource_
Re: [V2 1/2] powerpc: mpc512x_dma: add support for data transfers between memory and i/o memory
[ adding Lars to Cc: since we talk about OF-DMA ]
I put your DMA part of the series into a local test setup, and
slightly modified it in the suggested ways. This may not suffice
for a Tested-By attribute, since it did not test your LPB part
and did not use your original patch. But it verified that the
DMA part can be made operational and others can build on it. :)
We shall combine the parts which currently are floating around.
So that others aren't supposed to pick up pieces but instead can
use a series and get something consistent.
Here are the parts that I'm aware of:
- Anatolij's work that was motivated by SD card support, the
mxcmmc(4) part went mainline, the DMA part did not (patchwork
2368581, 2368591)
- Lars' work on OF support that was motivated by a different
platform but is useful for the MPC512x as well (2331091)
- your Alex' work that is motivated by SCLPC support is currently
under review (while the DMA part appears to re-use Anatolij's
approach?)
- my local work that's picking up Anatolij's previous work and
includes items which are unrelated to DMA yet are helpful here
(MPC512x common clock support, preprocessor use in DTS builds)
So I suggest that I create an RFC series which combines those
parts that I consider "DMA related", which you can in turn put
your SCLPC support onto, or pick it up and fold it into yours.
The latter may be more appropriate since announcing something via
OF is only useful after implementing the support (here: slave
S/G in the DMA engine).
On Fri, Jul 12, 2013 at 14:15 +0400, Alexander Popov wrote:
>
> 2013/7/10 Gerhard Sittig :
>
> >> @@ -256,7 +256,9 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
> >>
> >> prev->tcd->dlast_sga = mdesc->tcd_paddr;
> >> prev->tcd->e_sg = 1;
> >> - mdesc->tcd->start = 1;
> >> + /* only start explicitly on MDDRC channel */
> >> + if (cid == 32)
> >> + mdesc->tcd->start = 1;
> >>
> >> prev = mdesc;
> >> }
> >> @@ -268,7 +270,15 @@ static void mpc_dma_execute(struct mpc_dma_chan
> >> *mchan)
> >>
> >> if (first != prev)
> >> mdma->tcd[cid].e_sg = 1;
> >> - out_8(&mdma->regs->dmassrt, cid);
> >> +
> >> + switch (cid) {
> >> + case 26:
> >> + out_8(&mdma->regs->dmaserq, cid);
> >> + break;
> >> + case 32:
> >> + out_8(&mdma->regs->dmassrt, cid);
> >> + break;
> >> + }
> >> }
> >>
> >> /* Handle interrupt on one half of DMA controller (32 channels) */
>
> > This part of the change looks suspicious to me. The logic
> > changes from always starting the transfer in software to only
> > starting from software for memory or having hardware request the
> > start for LPB controller requests, while _all_other_ channels
> > remain without setup of the start condition.
> >
> > Either make sure that only the new source (LPB) gets handled
> > specially, or
>
> I agree, I'll use this way.
I'd prefer the other approach since it lasts longer. Upon second
thought it turns out that "memory transfers" are special since
they are software initiated, while "everything else" is hardware
driven (requester lines used).
The change is simple and handles all future DMA channel use as
well. Just say 'default' instead of 'case 26'. See my series
that I'll submit later.
> > In addition, try to get rid of the magic numbers. Use symbolic
> > identifiers instead (regardless of whether other floating patches
> > used magic numbers as well).
>
> Ok.
This gets addressed as well by what I have here (CPP use in DTS
nodes and driver code).
> >> + if (!IS_ALIGNED(sg_dma_address(sg), 4))
> >> + return NULL;
> >> +
> >> + if (direction == DMA_DEV_TO_MEM) {
> >> + tcd->saddr = mchan->per_paddr;
> >> + tcd->daddr = sg_dma_address(sg);
> >> + tcd->soff = 0;
> >> + tcd->doff = 4;
> >> + } else if (direction == DMA_MEM_TO_DEV) {
> >> + tcd->saddr = sg_dma_address(sg);
> >> + tcd->daddr = mchan->per_paddr;
> >> + tcd->soff = 4;
> >> + tcd->doff = 0;
> >> + } els
Re: [V2 1/2] powerpc: mpc512x_dma: add support for data transfers between memory and i/o memory
On Fri, Jul 12, 2013 at 14:14 +0200, Gerhard Sittig wrote:
>
> We shall combine the parts which currently are floating around.
> So that others aren't supposed to pick up pieces but instead can
> use a series and get something consistent.
>
> Here are the parts that I'm aware of:
> - Anatolij's work that was motivated by SD card support, the
> mxcmmc(4) part went mainline, the DMA part did not (patchwork
> 2368581, 2368591)
> - Lars' work on OF support that was motivated by a different
> platform but is useful for the MPC512x as well (2331091)
> - your Alex' work that is motivated by SCLPC support is currently
> under review (while the DMA part appears to re-use Anatolij's
> approach?)
> - my local work that's picking up Anatolij's previous work and
> includes items which are unrelated to DMA yet are helpful here
> (MPC512x common clock support, preprocessor use in DTS builds)
>
> So I suggest that I create an RFC series which combines those
> parts that I consider "DMA related", which you can in turn put
> your SCLPC support onto, or pick it up and fold it into yours.
> The latter may be more appropriate since announcing something via
> OF is only useful after implementing the support (here: slave
> S/G in the DMA engine).
for the record: the series was just sent out
From: Gerhard Sittig
To: linuxppc-...@lists.ozlabs.org,
devicetree-disc...@lists.ozlabs.org,
Alexander Popov
Cc: Dan Williams ,
Vinod Koul ,
Lars-Peter Clausen ,
Arnd Bergmann ,
Anatolij Gustschin ,
Gerhard Sittig
Subject: [PATCH RFC 0/8] MPC512x DMA slave s/g support, OF DMA lookup
Date: Fri, 12 Jul 2013 17:26:13 +0200
Message-Id: <1373642781-32631-1-git-send-email-...@denx.de>
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 5/5] input: pxa27x-keypad: add device tree support
On Wed, Jun 19, 2013 at 16:38 +0800, Chao Xie wrote: > > On Wed, Jun 19, 2013 at 4:22 PM, Marek Vasut wrote: > > > >> Signed-off-by: Chao Xie > >> [ ... ] > >> +++ b/Documentation/devicetree/bindings/input/pxa27x-keypad.txt > >> @@ -0,0 +1,60 @@ > >> +* Marvell PXA Keypad controller > >> + > >> +Required Properties > >> +- compatible : should be "marvell,pxa27x-keypad" > >> +- reg : Address and length of the register set for the device > >> +- interrupts : The interrupt for the keypad controller > >> +- marvell,debounce-interval : How long time the key will be > > > > Is there no generic prop name for this debounce interval? > > > I searched at drivers/input and Documents. > Two drivers use "debounce-interval", gpio-keys.c and stmpe-keypad.c. > They describe the meanings of "debounce-interval" at its own document file. > Some other drivers uses "xxx,debounce-delay-ms" or "debounce-delay-ms" > So it seems that there is no generic prop name for this debounce interval. Actually there is, but under a different (more user friendly) name: See the 'debounce-delay-ms' property in Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt which gets referenced in the matrix_keypad_parse_dt() routine in the drivers/input/keyboard/matrix_keypad.c source file. Ah, your last sentence mentions that fact. But when you introduce DT support into an existing driver which previously used platform data, then there is no problem with backwards compatibility in .dts files. So I suggest to go with the "debounce-delay-ms" name since it better reflects to the .dts author (hardware engineer) which unit the number is supposed to be specified in. Note that I've recently worked on extending the matrix keypad input driver (doc improvements, software polling, binary encoded column selection), but haven't submitted the patch series yet since it's perfectly operational on the target which motivated the extension but wasn't tested yet on any other platform or matrix setup -- I currently lack access to an ARM based board with either lots of accessible GPIOs to connect a matrix to, or some matrix already built into the board, but more importantly lack free resources for this very driver extension. Alternatively I might send an RFC series since the current state isn't good enough for v1. Just ask if you'd like to test or review what I have come up with so far (it builds but wasn't run-tested). virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V2 2/2] powerpc/512x: add LocalPlus Bus FIFO device driver
On Fri, Jul 12, 2013 at 14:42 +0400, Alexander Popov wrote:
>
> 2013/7/10 Gerhard Sittig :
> > On Wed, Jul 10, 2013 at 14:21 +0400, Alexander Popov wrote:
> >>
> >> +
> >> +struct mpc512x_lpbfifo_request {
> >> + unsigned int cs;
> >> + phys_addr_t bus_phys; /* physical address of some device on lpb */
> >> + void *ram_virt; /* virtual address of some region in ram */
> >> +
> >> + /* Details of transfer */
> >> + u32 size;
> >> + enum lpb_dev_portsize portsize;
> >> + enum mpc512x_lpbfifo_req_dir dir;
> >> +
> >> + /* Call when the transfer is finished */
> >> + void (*callback)(struct mpc512x_lpbfifo_request *);
> >> +};
> >> +
> >> +extern int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req);
> >> +
> >> #endif /* __ASM_POWERPC_MPC5121_H__ */
>
> > Needs the mpc512x_lpbfifo_request structure be part of the
> > official API? Could it be desirable to hide it behind a
> > "fill-in" routine? Which BTW could auto-determine CS numbers and
> > port width associated with a chip select from the XLB and LPB
> > register set?
>
> 1. As I wrote at the beginning of the file, the driver design
> is based on mpc52xx_lpbfifo driver written by Grant Likely.
>
> 2. CS and port width are attributes of some device on LPBus
> which participates DMA data transfer. It is the driver of this device
> who knows them.
I don't think so (I disagree with the "driver knows physical bus
attributes" part).
Bus width, endianess, timing in access, etc are all properties of
a chip select. And the chip select is associated with an address
range. See the CS part of the LPB controller, and the LAW part
of the XLB. Given a physical address, everything else can get
determined from inspecting the SoC's registers.
Just think what the SoC does: "Someone", probably the CPU or a
bus master like DMA, accesses an address, which happens to be
within an address window, which happens to be connected to some
bus and maybe map to a chip select, which happens to use the chip
select's configuration for the activity on the associated bus --
upon memory access nobody needs to explicitly know whether SRAM
or DRAM or IMMR or LPB aka EMB is involved, it's all hidden
within the XLB logic. I feel that the SCLPC job description is
an exception, and the need to specify a CS index is the unusual
case.
Drivers actually _need_not_ know the CS number or bus width of
what's attached to the EMB. Those parameters usually get setup
within boot loaders and the kernel need not care. Linux drivers
merely map an address range and "just access memory that happens
to have some arbitrary address". Drivers need not care whether
the bus is internal or external nor whether the bus has several
chip selects nor how these might be configured. The recent
addition to re-configure a chip select already was a special case
of differing configuration during runtime (when the firmware gets
sent, and when the firmware is used and communicated to), and is
by no means the normal case.
> It fills mpc512x_lpbfifo_request as a client side of that API.
>
> > Who's using the submit routine? I might have missed the "client
> > side" of that API in the series.
>
> Please look at https://patchwork.kernel.org/patch/2511951/
This answers how the current implementation is, not necessarily
how the API should or might be.
But I wasn't requesting immediate change, just was pointing at
potential for improvement.
> >> +#define LPBFIFO_REG_PACKET_SIZE (0x00)
> >> +#define LPBFIFO_REG_START_ADDRESS(0x04)
> >> +#define LPBFIFO_REG_CONTROL (0x08)
> >> +#define LPBFIFO_REG_ENABLE (0x0C)
> >> +#define LPBFIFO_REG_STATUS (0x14)
> >> +#define LPBFIFO_REG_BYTES_DONE (0x18)
> >> +#define LPBFIFO_REG_EMB_SHARE_COUNTER(0x1C)
> >> +#define LPBFIFO_REG_EMB_PAUSE_CONTROL(0x20)
> >> +#define LPBFIFO_REG_FIFO_DATA(0x40)
> >> +#define LPBFIFO_REG_FIFO_STATUS (0x44)
> >> +#define LPBFIFO_REG_FIFO_CONTROL (0x48)
> >> +#define LPBFIFO_REG_FIFO_ALARM (0x4C)
>
> > Should this not be a struct? Since using member field names
> > allows for compile time checks of data types, which is highly
> > desirable with registers of arbitrarily differing size.
>
> I've read
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2010-January/079476.html
> Which way should I use?
The way Grant puts it, there are valid reasons for both
approaches and things may tu
Re: [PATCH RFC v3 2/5] dma: mpc512x: add support for peripheral transfers
mchan->per_paddr = cfg->src_addr;
> + mchan->tcd_nunits = cfg->src_maxburst;
> + } else {
> + mchan->per_paddr = cfg->dst_addr;
> + mchan->tcd_nunits = cfg->dst_maxburst;
> + }
> +
> + spin_unlock_irqrestore(&mchan->lock, flags);
> +
> + return 0;
> + default:
> + return -ENOSYS;
> + }
> +
> + return -EINVAL;
> +}
> +
and here
I think it's unfortunate to attribute the "will access
peripheral" to the channel instead of the transfer job, and to
set the flag from within the device control callback, and to
nevery clear the flag (what will happen if a channel gets freed
and reallocated by some other client?).
I think that the peripheral access is an attribute of the
transfer job, and should be setup in the prep routines (both set
and cleared, depending on what gets setup). This would be more
robust and more readable (read: maintainable) in my eyes.
> static int mpc_dma_probe(struct platform_device *op)
> {
> struct device_node *dn = op->dev.of_node;
> @@ -733,9 +899,12 @@ static int mpc_dma_probe(struct platform_device *op)
> dma->device_issue_pending = mpc_dma_issue_pending;
> dma->device_tx_status = mpc_dma_tx_status;
> dma->device_prep_dma_memcpy = mpc_dma_prep_memcpy;
> + dma->device_prep_slave_sg = mpc_dma_prep_slave_sg;
> + dma->device_control = mpc_dma_device_control;
>
> INIT_LIST_HEAD(&dma->channels);
> dma_cap_set(DMA_MEMCPY, dma->cap_mask);
> + dma_cap_set(DMA_SLAVE, dma->cap_mask);
>
> for (i = 0; i < dma->chancnt; i++) {
> mchan = &mdma->channels[i];
> --
> 1.7.11.3
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v3 1/5] dma: mpc512x: reorder mpc8308 specific instructions
On Wed, Jul 31, 2013 at 11:20 +0400, Alexander Popov wrote:
>
> From: Gerhard Sittig
>
> Concentrate the test and the specific code for MPC8308
> in the 'if' branch and handle MPC512x in the 'else' branch.
>
> This modification only reorders instructions but doesn't change behaviour.
>
> Signed-off-by: Alexander Popov
> ---
> drivers/dma/mpc512x_dma.c | 42 +-
> 1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index 2d95673..b8881de 100644
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
> @@ -50,9 +50,17 @@
> #define MPC_DMA_DESCRIPTORS 64
>
> /* Macro definitions */
> -#define MPC_DMA_CHANNELS 64
> #define MPC_DMA_TCD_OFFSET 0x1000
>
> +/*
> + * Maximum channel counts for individual hardware variants
> + * and the maximum channel count over all supported controllers,
> + * used for data structure size
> + */
> +#define MPC8308_DMACHAN_MAX 16
> +#define MPC512x_DMACHAN_MAX 64
> +#define MPC_DMA_CHANNELS 64
> +
> /* Arbitration mode of group and channel */
> #define MPC_DMA_DMACR_EDCG (1 << 31)
> #define MPC_DMA_DMACR_ERGA (1 << 3)
nit: That's not what I wrote. Please make sure to either cite
properly or to properly mark changes as such. Don't spread false
information, please. You are free to change what I submitted,
but you should not pretend that I wrote what has become of the
code after you have modified it. Please fix the attribution.
Just to clarify: The defines here appear to be more appropriate
than the initial enums, after it turned out that we need not
handle indiviudal channels in special ways, and really only need
these three numbers (one of them being the maximum of the
others). But regardless of what you have changed, you should
clearly state the fact.
> @@ -716,10 +724,10 @@ static int mpc_dma_probe(struct platform_device *op)
>
> dma = &mdma->dma;
> dma->dev = dev;
> - if (!mdma->is_mpc8308)
> - dma->chancnt = MPC_DMA_CHANNELS;
> + if (mdma->is_mpc8308)
> + dma->chancnt = MPC8308_DMACHAN_MAX;
> else
> - dma->chancnt = 16; /* MPC8308 DMA has only 16 channels */
> + dma->chancnt = MPC512x_DMACHAN_MAX;
> dma->device_alloc_chan_resources = mpc_dma_alloc_chan_resources;
> dma->device_free_chan_resources = mpc_dma_free_chan_resources;
> dma->device_issue_pending = mpc_dma_issue_pending;
> @@ -753,7 +761,19 @@ static int mpc_dma_probe(struct platform_device *op)
>* - Round-robin group arbitration,
>* - Round-robin channel arbitration.
>*/
> - if (!mdma->is_mpc8308) {
> + if (mdma->is_mpc8308) {
> + /* MPC8308 has 16 channels and lacks some registers */
> + out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_ERCA);
> +
> + /* enable snooping */
> + out_be32(&mdma->regs->dmagpor, MPC_DMA_DMAGPOR_SNOOP_ENABLE);
> + /* Disable error interrupts */
> + out_be32(&mdma->regs->dmaeeil, 0);
> +
> + /* Clear interrupts status */
> + out_be32(&mdma->regs->dmaintl, 0x);
> + out_be32(&mdma->regs->dmaerrl, 0x);
> + } else {
> out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_EDCG |
> MPC_DMA_DMACR_ERGA |
> MPC_DMA_DMACR_ERCA);
>
> @@ -774,18 +794,6 @@ static int mpc_dma_probe(struct platform_device *op)
> /* Route interrupts to IPIC */
> out_be32(&mdma->regs->dmaihsa, 0);
> out_be32(&mdma->regs->dmailsa, 0);
> - } else {
> - /* MPC8308 has 16 channels and lacks some registers */
> - out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_ERCA);
> -
> - /* enable snooping */
> - out_be32(&mdma->regs->dmagpor, MPC_DMA_DMAGPOR_SNOOP_ENABLE);
> - /* Disable error interrupts */
> - out_be32(&mdma->regs->dmaeeil, 0);
> -
> - /* Clear interrupts status */
> - out_be32(&mdma->regs->dmaintl, 0x);
> - out_be32(&mdma->regs->dmaerrl, 0x);
> }
>
> /* Register DMA engine */
> --
> 1.7.11.3
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] OF: Clear detach flag on attach
On Tue, Nov 05, 2013 at 19:50 +0200, Pantelis Antoniou wrote: > > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -1641,6 +1641,7 @@ int of_attach_node(struct device_node *np) > np->allnext = of_allnodes; > np->parent->child = np; > of_allnodes = np; > + of_node_clear_flag(np, OF_DETACHED); > raw_spin_unlock_irqrestore(&devtree_lock, flags); > > of_add_proc_dt_entry(np); Does this add a call to a routine which only gets introduced in a subsequent patch (2/5)? If so, it would break builds during the series, and thus would hinder bisection. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: mcp23s08: Trivial: Fixed coding style issues
On Wed, Mar 12, 2014 at 14:53 +0100, Linus Walleij wrote: > > On Fri, Mar 7, 2014 at 12:25 AM, Gary Servin wrote: > > > This coding style issue was detected using the checkpatch.pl script > > > > Signed-off-by: Gary Servin > > Sometimes the compiler is just too forgiving :-/ > Thanks a lot for fixing this, patch applied. I think the code had no syntax or language issue before. The cleanup really was about style exclusively (which still is a good thing). 'sizeof' is an operator, very much like a unary minus or unary ampersand which neither require parentheses. So either of "sizeof(x)" as well as "sizeof x" are legal with regard to the C language. It's just that the community prefers "sizeof(x)" for the improved readability. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 0/3] clk: CCF clock primitives + custom IO accessors
On Fri, Feb 28, 2014 at 15:34 -0800, Soren Brinkmann wrote:
>
> [ MMIO registers assumed for clock control modules, but I2C
> communication may be involved in other hardware, individual for
> a (set of) clock(s) and not for an architecture or platform ]
>
> Does anybody have a good idea how we could avoid all this code
> duplication while enabling usage of the clock primitives with different
> IO accessors?
> Especially the divider and mux primitive have a lot of code that would
> be painful to maintain twice.
Hasn't past discussion already reached the point where code
duplication of the clock control logic was considered
undesirable, and "low level ops" were outlined? I.e. extending
the compile time decision for a specific clk_{read,write}l()
implementation by another potential redirection that is specific
to a clock item?
Re-submitting a series which duplicates complete clock types,
while the difference is only in how registers get accessed, is
quite saddening.
> In the next step, I encountered a divider clock whose divider is stored
> in 2 I2C registers. So now, the simple IO access replacement doesn't
> work anymore either since this clock needs 2 registers to be read and
> then shifting around the bitfields accordingly.
Are the registers adjacent and contain only bit fields for one
clock? Or do registers share parameters for several clocks, or
are not adjacent?
In the former case you may use a table from "divider value" to
"bit pattern to read/write". In the latter case, the clock
control module is rather special, and may not be easily get
mapped to the common primitives. Unless the ll_ops can implement
the required special handling.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 0/3] clk: CCF clock primitives + custom IO accessors
On Mon, Mar 03, 2014 at 09:35 -0800, Sören Brinkmann wrote: > > It would be nice if we could use the logic provided in the mux, div etc > primitives independently of how the HW is accessed and what is > necessary to shift and mask those register values around, right? I > mean, at then end we want to model a clk-(div|mux) and not a > clk-(div|mux) which has only a single, memory-mapped control register, > that does not overlap with other things, ... Did you lookup the ll_ops discussion in the thread that originated from http://article.gmane.org/gmane.linux.ports.arm.kernel/289895 and did you see the outlined logic in http://article.gmane.org/gmane.linux.ports.arm.omap/109233 and http://article.gmane.org/gmane.linux.ports.arm.omap/109381 ? Support for regmap access instead of mere MMIO was one of the things you could do with this approach. You appear to be in the situation where you need such an extension (or something similar, but you really should look into the ll_ops thing). virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] spi: core: Validate lenght of the transfers in message
On Tue, Feb 18, 2014 at 09:09 +0900, Mark Brown wrote:
>
> On Sun, Feb 16, 2014 at 03:25:12PM +0100, Gerhard Sittig wrote:
> > On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote:
>
> > - the total length of the SPI transfer cannot be empty (which I'd
> > consider an optimization, not a violation, and may need a
> > separate discussion)
>
> We probably want to allow that for people doing fun stuff with cs_change
> though I'm not convinced anything doing that is actually a good idea.
>
> > - the total length of the SPI transfer must be such that each
> > "word" must be provided within a full 1/2/4 byte entity, with
> > padding bits if the bits-per-word is "odd"
>
> > Is this a misunderstanding on my side? A terminology thing? To
> > me, the "SPI transfer" is the total payload and may have any
> > arbitrary length. What you check for is a constraint on the
> > transfer's length derived from or based on the "word length"
> > ('word' in SPI context).
>
> > So the code may be appropriate, yet the description may need an
> > update, to not have the next person ask the same questions again.
>
> It seems fairly clear to me - if we're transferring 16 bit words we need
> the transfer to me a multiple of 16 bits and so on? The requirement for
> padding is unclear I have to say.
I meant "padding" in the sense that e.g. 12bit bits-per-word
require data to be provided or consumed in 16bit quantities (2
full bytes), 20bit bits-per-word require 4 bytes per SPI word.
Why not 3 bytes? I'd guess this is due to FIFO port width.
At least this is how I read the check which this patch
implements.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] spi: Check that Quad/Dual is half duplex
On Tue, Jan 21, 2014 at 21:06 +0100, Geert Uytterhoeven wrote: > > On Tue, Jan 21, 2014 at 7:21 PM, Mark Brown wrote: > > On Tue, Jan 21, 2014 at 04:10:08PM +0100, Geert Uytterhoeven wrote: > >> From: Geert Uytterhoeven > >> Quad and Dual SPI Transfers use all available data lines > >> (incl. MOSI/MISO), hence they must be half duplex. Add a > >> check that verify that. > > > > This is surprising to me - I had expected that there would be > > extra signals that would be used for these modes, not that > > the opposite direction data line would be one of the ones > > being reused. On the other hand if this is what all the > > flash chips do then it would seem reasonable that controllers > > do the same. Can you clarify please? > > Dual SPI works by aggregating the MOSI and MISO lines for 2-bit > unidirectional transfers. > Quad SPI aggregates MOSI, MISO, and 2 additional lines for 4-bit > unidirectional transfers. Does it help to only use the MOSI/MISO names for single data line transfers, and to explicitly mention the fact that multi data line transfers change the signals' meaning to "IO0 .. IO3", and that these "IOn" lines are used in half duplex ways? > Hence Dual SPI uses the traditional 4-wire wiring, while Quad > SPI uses 6-wire. Be careful! Just because the number of wires is identical, I would not want to refer to it as "the traditional 4-wire wiring" in the dual data line case. Strictly speaking it's not that you connect MISO+MOSI with MISO+MOSI, insted you connect IO0+IO1 with IO0+IO1. It just happens that the same pins are re-used, while the signals do change their meaning. Now add unidirectional transmitters (amplifiers and/or level shifters) to the picture instead of mere wires, and you cannot run all modes across these connections any longer as you may assume at the moment. IO2 and IO3 in quad data line mode do change the meaning of the pins which they re-purpose, too (that's write protect and hold usually). Which is why the features of the SPI controller and the flash chip alone may not be sufficient to determine which modes are available, as the board may add constraints as well. > SPI FLASH chips handle it this way, and the Renesas QSPI > controller in the r8a7790/7791 SoCs, too. > > Typically the first transfer in a message is a Single Transfer > (e.g. read data command), while subsequent transfers can be > Dual or Quad Transfers (e.g. the actual data read from the > FLASH). Please note that subsequent transfers may be single data line transfers as well. :) BTW am I trying to be strict about the above implicit assumption of "dual/quad" meaning the number of data lines. To not confuse them with dual data rate transfers, which are orthogonal to the number of data lines in use, and are available in chips as well (see the S25FL256S data sheet that was referenced in the Quad-SPI discussion earlier). Let's be clear from the beginning, to not have to cleanup or guess afterwards. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/3] i2c: cadence: Document device tree bindings
On Thu, 2014-04-03 at 10:59 -0700, Soren Brinkmann wrote:
>
> Add device tree binding documentation for the Cadence I2C controller.
>
> [ ... ]
>
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
> @@ -0,0 +1,21 @@
> +Binding for the Cadence I2C controller
> +
> +Required properties:
> + compatible: Compatibility string. Must be 'cdns,i2c-r1p10'.
> + clocks: From common clock bindings. Phandle to input clock.
the usual complaint: 'clocks' items are not just phandles (your
example even suggests this); either adjust the description to
something correct, or just refer to the common clock bindings to
not duplicate their description
but I guess the I2C controller's binding should explicitly state
which clocks are required, and you might want to consider
'clock-names', too
> +
> +Optional properties:
> + clock-frequency: Desired operating frequency, in Hz, of the bus (actual may
> +be lower). Defaults to 40 if not specified.
is the value default a feature of the Linux implementation, or
consciously designed to be in the binding spec? and I agree that
the default should be the standard I2C speed instead of fast mode
> +
> +Example:
> +
> + i2c@e0004000 {
> + compatible = "cdns,i2c-r1p10";
> + clocks = <&clkc 38>;
> + interrupts = <0 25 4>;
> + reg = <0xE0004000 0x1000>;
> + clock-frequency = <40>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
use lower case hex digits, consider symbolic identifiers for
clocks and interrupt flags
the example has many more properties than the binding describes,
the additional items aren't even optional -- you might want to
refer to a few more common or general bindings
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] gpio: dwapb: use d->mask instead od BIT(bit)
On Mon, 2014-04-07 at 12:13 +0200, Sebastian Andrzej Siewior wrote:
>
> d->mask contains exact the same information as BIT(bit) so we could save
> a few cycles here.
ISTR that the benefit of saving cycles was questioned in previous
review comments. On ARM, the shift "comes for free".
I'm not saying that the patch is doing something wrong. But I
suggest to rephrase the commit message (and put the version
number into the subject prefix, should you have to resend).
Reducing the number of variables involved, or hiding details
behind common abstractions, or eliminating redundancy, all of
those benefits are as valuable. It's just that this patch does
not save any computation time.
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
>
> irq_gc_lock(igc);
> val = readl(gpio->regs + GPIO_INTEN);
> - val |= BIT(d->hwirq);
> + val |= d->mask;
these are equally costly or cheap, nothing saved here
> struct dwapb_gpio *gpio = igc->private;
> - int bit = d->hwirq;
> + u32 mask = d->mask;
> unsigned long level, polarity;
>
> if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> @@ -171,24 +171,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32
> type)
>
> switch (type) {
> case IRQ_TYPE_EDGE_BOTH:
> - level |= BIT(bit);
> - dwapb_toggle_trigger(gpio, bit);
> + level |= mask;
> + dwapb_toggle_trigger(gpio, d->hwirq);
these introduce another pointer dereference, unless 'bit' was
assigned from a pointer dereference (as is shown above), so
nothing was gained
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: cscope: issue with symlinks in tools/testing/selftests/powerpc/copyloops/
On Mon, 2014-04-07 at 06:42 -0400, Neil Horman wrote: > > On Thu, Apr 03, 2014 at 03:16:15PM +0200, Yann Droneaud wrote: > > > > [ ... ] > > > > cscope reports error when generating the cross-reference database: > > > > $ make ALLSOURCE_ARCHS=all O=./obj-cscope/ cscope > > GEN cscope > > cscope: cannot find > > file > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S > > cscope: cannot find > > file > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_64.S > > cscope: cannot find > > file > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S > > cscope: cannot find > > file > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_64.S > > > > And when calling cscope from ./obj-cscope/ directory, it reports errors > > too. > > > > Hopefully it doesn't stop it from working, so I'm still able to use > > cscope to browse kernel sources. > > > No, it won't stop it from working, it just won't search those files. I don't > recall exactly the reason, but IIRC there was a big discussion long ago about > symlinks and our ability to support them (around version 1.94 I think). We > decided to not handle symlinks, as they would either point outside our search > tree, which we didn't want to include, or would point to another file in the > search tree, which made loading them pointless (as we would cover the search > in > the pointed file). So there are valid reasons to not process those filesystem entries. Would it be useful to not emit the warnings then? Or to silent those warnings when the user knows it's perfectly legal to skip those filesytem entries? Like what you can do with the ctags(1) command and its --links option. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] gpio: dwapb: use d->mask instead od BIT(bit)
[ ignore this if you are busy :) ]
On Mon, 2014-04-07 at 20:26 +0200, Sebastian Andrzej Siewior wrote:
>
> On 04/07/2014 02:26 PM, Gerhard Sittig wrote:
> > On Mon, 2014-04-07 at 12:13 +0200, Sebastian Andrzej Siewior wrote:
> >>
> >> d->mask contains exact the same information as BIT(bit) so we could save
> >> a few cycles here.
> >
> > ISTR that the benefit of saving cycles was questioned in previous
> > review comments. On ARM, the shift "comes for free".
>
> I can't recall that some pointed this out.
http://article.gmane.org/gmane.linux.kernel.gpio/2410 raises a
concern about the cost of dereferencing pointers
That shifts might not involve additional cost appears to not have
been stated explicitly in earlier feedback, or I have missed it
in the search, doesn't matter much. Might as well have confused
this submission with another one.
> However:
> - you load one variable in both cases. Not performing the shift means
> there is at least one instruction less to be performed.
> - that gpio controller is generic IP core from Synopsys. Every can buy
> it and but into their IP core so it is not limited to ARM.
You assume that the shift is done in an individual instruction.
That does not necessarily apply to the ARM architecture, which
has a barrel shifter and can fold shifts into other instructions
"for free".
This IP core has "APB" in its name, which is a memory bus that
I've never seen in use outside of the ARM ecosphere. Whatever
that may be worth, anyway. We are not talking about the kind of
GPIO block that resides in FPGAs, we are talking about an IP
block that is "on the CPU side" of SoCs.
But see below, I do not want to block anything, feel free to
ignore my concerns as they are not strong here.
> >> --- a/drivers/gpio/gpio-dwapb.c
> >> +++ b/drivers/gpio/gpio-dwapb.c
> >> @@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
> >>
> >>irq_gc_lock(igc);
> >>val = readl(gpio->regs + GPIO_INTEN);
> >> - val |= BIT(d->hwirq);
> >> + val |= d->mask;
> >
> > these are equally costly or cheap, nothing saved here
>
> I still thing not performing an instruction is more efficient than
> performing one.
>
> >>struct dwapb_gpio *gpio = igc->private;
> >> - int bit = d->hwirq;
> >> + u32 mask = d->mask;
> >>unsigned long level, polarity;
> >>
> >>if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> >> @@ -171,24 +171,24 @@ static int dwapb_irq_set_type(struct irq_data *d,
> >> u32 type)
> >>
> >>switch (type) {
> >>case IRQ_TYPE_EDGE_BOTH:
> >> - level |= BIT(bit);
> >> - dwapb_toggle_trigger(gpio, bit);
> >> + level |= mask;
> >> + dwapb_toggle_trigger(gpio, d->hwirq);
> >
> > these introduce another pointer dereference, unless 'bit' was
> > assigned from a pointer dereference (as is shown above), so
> > nothing was gained
>
> dwapb_toggle_trigger() is a bit special and it needs both. However,
> size on ARM says
>
>textdata bss dec hex filename
>3264 96 03360 d20 drivers/gpio/gpio-dwapb.o.before
>3224 96 03320 cf8 drivers/gpio/gpio-dwapb.o.after
>
> that with the patch the code is smaller by 40 bytes. Does 40 bytes
> smaller code quality for "safe a few cycles" statement?
I would not necessarily jump to the conclusion that code of
smaller size translates into fewer instructions. It may be an
educated guess here in this specific situation (given the very
nature of the change), but does not apply in general. Actually
you can reduce code size by adding instruction overhead, and
speed up code by unrolling it. There always is the compromise
between space and time.
It's hard to tell without further analysis of the generated code
whether this specific reduction in size comes from eliminated
shifts, or from the re-use of already pre-loaded variables. I'd
rather assume the latter, and suggested such a potential cause in
my earlier reply.
Anyway, it's not that I would have strong feelings about this
change. It's just that I wanted to check whether the motivation
and the description are correct, and observed effects are not
assigned to unrelated components. Never did I question the
benefit of cleaning up redundancy, I was questioning whether the
commit message appropriately reflects what is observed.
You have presented numbers that were not available before. The
assumption that your change reduces code size is supported. The
explanation is an educa
Re: cscope: issue with symlinks in tools/testing/selftests/powerpc/copyloops/
[ removed cscope-devel from Cc:, non-subscriber mails get blocked anyway ] On Mon, 2014-04-07 at 14:42 +0200, Gerhard Sittig wrote: > > On Mon, 2014-04-07 at 06:42 -0400, Neil Horman wrote: > > > > On Thu, Apr 03, 2014 at 03:16:15PM +0200, Yann Droneaud wrote: > > > > > > [ ... ] > > > > > > cscope reports error when generating the cross-reference database: > > > > > > $ make ALLSOURCE_ARCHS=all O=./obj-cscope/ cscope > > > GEN cscope > > > cscope: cannot find > > > file > > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S > > > cscope: cannot find > > > file > > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_64.S > > > cscope: cannot find > > > file > > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S > > > cscope: cannot find > > > file > > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_64.S > > > > > > And when calling cscope from ./obj-cscope/ directory, it reports errors > > > too. > > > > > > Hopefully it doesn't stop it from working, so I'm still able to use > > > cscope to browse kernel sources. > > > > > No, it won't stop it from working, it just won't search those files. I > > don't > > recall exactly the reason, but IIRC there was a big discussion long ago > > about > > symlinks and our ability to support them (around version 1.94 I think). We > > decided to not handle symlinks, as they would either point outside our > > search > > tree, which we didn't want to include, or would point to another file in the > > search tree, which made loading them pointless (as we would cover the > > search in > > the pointed file). > > So there are valid reasons to not process those filesystem > entries. Would it be useful to not emit the warnings then? Or > to silent those warnings when the user knows it's perfectly legal > to skip those filesytem entries? Like what you can do with the > ctags(1) command and its --links option. For the record: I got a response "off list" (actually to the cscope list only which is closed for non-subscribers, while the Linux related recipients were removed despite the fact that the issue appears to be in Linux), see http://article.gmane.org/gmane.comp.programming.tools.cscope.devel/105 This reponse suggests that the issue is not in cscope(1) itself, but instead is in how the cscope(1) command got invoked. Which translates into "the Linux infrastructure does something wrong". A quick search identifies ./scripts/tags.sh:all_target_sources() as the spot where symlinks should get filtered out. Where both paths of all_target_sources() end up calling all_sources(). virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: pxa: fix bug when get gpio value
On Thu, Jan 09, 2014 at 17:25 +0800, Neil Zhang wrote:
>
> gpio_get_value should return 0 or 1.
>
> I have checked mach-mmp / mach-pxa / plat-pxa / plat-orion / mach-orion5x.
> It's OK for all of them to change this function to return 0 and 1.
>
> [ ... ]
>
> static int pxa_gpio_get(struct gpio_chip *chip, unsigned offset)
> {
> - return readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET) & (1 <<
> offset);
> + u32 gplr = readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET);
> + return !!(gplr & (1 << offset));
> }
This may be a stupid question, but isn't the "!!value" syntax
just replacing one kind of "zero or non-zero" with another kind
of "zero or non-zero"? This phrase is used to normalize
booleans, but ISTR there is no guarantee that true needs to equal
the value of 1.
Here is why I'm asking: Is there a need from GPIO get_value()
routines to return normalized values, and if so should not more
drivers receive an update? Or need get_value() routines just
return the usual integer zero/non-zero values (as is the
convention in the C programming language) and GPIO using callers
should know that they are not exactly 0 and 1?
The latter would make this change for PXA unneeded (nice to have
but not essential), and both the commit message as well as the
subject line at least need to get re-phrased, as the "bug" is in
the caller's expectation and not in the GPIO chip driver.
Please note that I'm not questioning the patches' being
applicable, but am trying to find out what else needs to be done
which previously may have gone unnoticed. A doc update maybe, to
make GPIO users aware that the return value may not be exactly 1,
and to clear up where such an assumption is wrongly made.
If the GPIO subsystem's API wants to guarantee values of 0 and 1
(which I think it doesn't), then I feel the adjustment should be
done in the gpio_get_value() routines (in all its public
variants, or a common routine which all of them pass through),
and certainly not in individual chip drivers.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: imx6q: Add missing esai_ahb clock to current clock tree
On Thu, Jan 09, 2014 at 08:55 +0100, Sascha Hauer wrote: > > [ ... ] > > We have the situation here that a single bit controls two clocks. As > Shawn mentioned just using two gates on the same bit doesn't work > properly. Do we need a new basic clock type or expand the common gate > code somehow? > This situation happens from time to time and I haven't seen a solution > for this. You may want to lookup the following message: Date: Tue, 23 Jul 2013 15:14:06 +0200 From: Gerhard Sittig To: linuxppc-...@lists.ozlabs.org, Anatolij Gustschin , Mike Turquette , linux-arm-ker...@lists.infradead.org, devicet...@vger.kernel.org Cc: [ ... ] Subject: Re: [PATCH v3 17/31] clk: mpc512x: introduce COMMON_CLK for MPC512x http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/185687.html The specific situation was for MS-CAN on PowerPC, but it inspired my outlining an approach to "shared clock gates". See an example implementation towards the end of the message with both the clk-gate.c extension, as well as rather generic example use. My approach turned out to not be needed, but it might serve as a starting point for you. You'd have to add support for static declaration though, but this should be straight forward. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: mcp23s08: Add irq functionality for i2c chips
On Fri, Jan 10, 2014 at 15:22 +0100, Lars Poeschel wrote: > > --- a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt > +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt > @@ -38,12 +38,37 @@ Required device specific properties (only for SPI chips): > removed. > - spi-max-frequency = The maximum frequency this chip is able to handle > > -Example I2C: > +Optional properties: > +- #interrupt-cells : Should be two. > + - first cell is the pin number > + - second cell is used to specify flags. > +- interrupt-controller: Marks the device node as a interrupt controller. > +NOTE: The interrupt functionality is only supported for i2c versions of the > +chips yet. Is this "IRQ feature for I2C chips only" limitation specific to the hardware or an implementation detail of the Linux driver? I could not determine this from either the binding text nor the driver source. If it's just the status of the Linux driver, then it should not be in the binding document. If it's a limitation of the hardware, then it's appropriate in the binding but I suggest to adjust the text to avoid the next person to ask the same question. :) > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -697,6 +697,7 @@ config GPIO_MCP23S08 > SPI/I2C driver for Microchip MCP23S08/MCP23S17/MCP23008/MCP23017 > I/O expanders. > This provides a GPIO interface supporting inputs and outputs. > + The I2C versions of the chips can be used as interrupt-controller. > > config GPIO_MC33880 > tristate "Freescale MC33880 high-side/low-side switch" > diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c > index 2deb0c5..1346ea6 100644 > --- a/drivers/gpio/gpio-mcp23s08.c > +++ b/drivers/gpio/gpio-mcp23s08.c > @@ -1,5 +1,10 @@ > /* > - * MCP23S08 SPI/GPIO gpio expander driver > + * MCP23S08 SPI/I2C GPIO gpio expander driver > + * > + * The inputs and outputs of the mcp23s08, mcp23s17, mcp23008 and mcp23017 > are > + * supported. > + * For the I2C versions of the chips (mcp23008 and mcp23017) generation of > + * interrupts is also supported. > */ Adjust these as well to reflect whether it's hardware or software which limits the feature. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] regmap: trivial comment fix (copy'n'paste error)
fix a trivial copy'n'paste error in the regmap kerneldoc, s/write/read/ for the regmap_read(), regmap_raw_read() and regmap_bulk_read() routines Signed-off-by: Gerhard Sittig --- drivers/base/regmap/regmap.c |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 7d689a15c500..d92648f771c4 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1573,7 +1573,7 @@ static int _regmap_read(struct regmap *map, unsigned int reg, /** * regmap_read(): Read a value from a single register * - * @map: Register map to write to + * @map: Register map to read from * @reg: Register to be read from * @val: Pointer to store read value * @@ -1600,7 +1600,7 @@ EXPORT_SYMBOL_GPL(regmap_read); /** * regmap_raw_read(): Read raw data from the device * - * @map: Register map to write to + * @map: Register map to read from * @reg: First register to be read from * @val: Pointer to store read value * @val_len: Size of data to read @@ -1679,7 +1679,7 @@ EXPORT_SYMBOL_GPL(regmap_field_read); /** * regmap_bulk_read(): Read multiple registers from the device * - * @map: Register map to write to + * @map: Register map to read from * @reg: First register to be read from * @val: Pointer to store read value, in native register size for device * @val_count: Number of registers to read -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] powerpc/qe_lib: Share the qe_lib for the others architecture
On Mon, Oct 14, 2013 at 13:09 -0700, Greg Kroah-Hartman wrote: > > On Mon, Oct 14, 2013 at 02:40:44PM -0500, Kumar Gala wrote: > > > > Greg, > > > > Wondering your thoughts on drivers/qe vs something like > > drivers/soc/fsl/qe. The QuiccEngine (qe) is a communication core on > > some of the Freescale networking SoCs that provides the ability to do > > various networking/communication functionality. "Channels" on the QE > > can be used for various different things from ethernet, ATM, UART, or > > other functions. > > What makes the code "QE" specific? Are these devices that live on the > QE "bus", or are they controlling the QE controller? You may think of the QUICC as a "programmable bitbang machine" if you like. The very same component runs arbitrary and rather different protocols depending on how you setup its parameters. There have been serial controllers capable of different protocols like UART or SPI or I2S, but all of them are "serial communication". There have been memory controllers which could bitbang different protocols (NAND, NOR/SRAM, DRAM), but all of them are "memory". The QUICC is just a little more versatile, and appears to cover cases which reside in different Linux kernel subsystems (like: it's neither serial nor network exclusively, but can be either and potentially more). IIUC the question which Kumar Gala was asking is where to put code for the component which is neither a strict subset of any subsystem. Please correct me if I'm wrong. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] pinctrl: baytrail: lock IRQs when starting them
On Tue, Dec 03, 2013 at 16:00 +0100, Linus Walleij wrote:
>
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -372,11 +372,33 @@ static void byt_irq_mask(struct irq_data *d)
> {
> }
>
> +static unsigned int byt_irq_startup(struct irq_data *d)
> +{
> + struct byt_gpio *vg = irq_data_get_irq_handler_data(d);
> +
> + if (gpio_lock_as_irq(&vg->chip, irqd_to_hwirq(d)))
> + dev_err(vg->chip.dev,
> + "unable to lock HW IRQ %lu for IRQ\n",
> + irqd_to_hwirq(d));
> + byt_irq_unmask(d);
> + return 0;
> +}
Just a thought: If failure to lock is non-fatal, should the
message be a warning then?
I do agree that failure to lock the GPIO should be non-fatal, as
there was debate and still might be concerns about how strict
locking should be.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] dmaengine: remove obsolete comment reference to dma_data_direction
On Wed, Dec 11, 2013 at 10:07 +0400, Alexander Popov wrote:
>
> enum dma_transfer_direction is currently used
> in struct dma_slave_config, so update the comment
>
> Signed-off-by: Alexander Popov
> ---
> include/linux/dmaengine.h | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 41cf0c3..bd6b882 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -305,9 +305,8 @@ enum dma_slave_buswidth {
> /**
> * struct dma_slave_config - dma slave channel runtime config
> * @direction: whether the data shall go in or out on this slave
> - * channel, right now. DMA_TO_DEVICE and DMA_FROM_DEVICE are
> - * legal values, DMA_BIDIRECTIONAL is not acceptable since we
> - * need to differentiate source and target addresses.
> + * channel, right now. DMA_MEM_TO_DEV and DMA_DEV_TO_MEM are
> + * legal values.
> * @src_addr: this is the physical address where DMA slave data
> * should be read (RX), if the source is memory this argument is
> * ignored.
While I agree with the change to the comment text, I'd put the
description of the patch differently.
You are not removing an obsolete reference, but you are fixing an
erroneous comment. It's not that dma_data_direction would have
become obsolete, instead it's that dma_slave_config was
documented wrongly. The previous comment used the wrong data
type for one of its members and thus discussed inappropriate
values out of the type's domain. Maybe "fix incorrect kerneldoc
for struct dma_slave_config" or something similar could be a
better title. Especially for those who read the log later and
neither have the patch nor its description at hand.
And you might improve the description's body. Something like
"the 'direction' member of 'struct dma_slave_config' is of data
type 'enum dma_transfer_direction', so update the 'struct
dma_slave_config' kerneldoc to refer to appropriate values" or
similar.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] Add support for flag status register on Micron chips.
On Tue, 2014-04-22 at 09:03 -0500, Graham Moore wrote:
>
> ---
> V3:
> Rebase to l2-mtd spinor branch.
> V2:
> Remove leading underscore in function names.
> Remove type cast in dev_err call and use the proper format
> specifier instead.
the patch appears to not have dev_err() references, were they
removed? see below
> [ ... ]
> + * Read the flag status register, returning its value in the location
> + * Return the status register value.
> + * Returns negative if error occurred.
> + */
> +static int read_fsr(struct spi_nor *nor)
> +{
> + int ret;
> + u8 val;
> +
> + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
> + if (ret < 0) {
> + pr_err("error %d reading FSR\n", ret);
> + return ret;
> + }
> [ ... ]
this emits a message that an error has occured, but doesn't tell
where it occured -- can you dev_err() here to make the message
even more helpful?
> @@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor *nor)
> return -ETIMEDOUT;
> }
>
> +static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
> +{
> + unsigned long deadline;
> + int sr;
> + int fsr;
> +
> + deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> +
> + do {
> + cond_resched();
> +
> + sr = read_sr(nor);
> + if (sr < 0)
> + break;
> + else if (!(sr & SR_WIP)) {
> + fsr = read_fsr(nor);
> + if (fsr < 0)
> + break;
> + if (fsr & FSR_READY)
> + return 0;
> + }
> + } while (!time_after_eq(jiffies, deadline));
> +
> + return -ETIMEDOUT;
> +}
this logic always returns "timed out" when the ready flag is not
seen, even in the case of read errors -- can you "preset" the
error code with "timed out", and update it with something more
appropriate before returning when other errors are seen?
though this is an internal helper, and callers may not tell the
situations apart in the first place, so this might be a minor nit
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net/dt: Add support for overriding phy configuration from device tree
On Wed, Jan 15, 2014 at 16:38 -0500, Matthew Garrett wrote:
>
> Some hardware may be broken in interesting and board-specific ways, such
> that various bits of functionality don't work. This patch provides a
> mechanism for overriding mii registers during init based on the contents of
> the device tree data, allowing board-specific fixups without having to
> pollute generic code.
>
> Signed-off-by: Matthew Garrett
> ---
> Documentation/devicetree/bindings/net/phy.txt | 13 +++
> drivers/net/phy/phy_device.c | 29 +-
> drivers/of/of_net.c | 124
> ++
> include/linux/of_net.h| 12 +++
> 4 files changed, 177 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/net/phy.txt
> b/Documentation/devicetree/bindings/net/phy.txt
> index 7cd18fb..552a5e0 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -23,6 +23,19 @@ Optional Properties:
>assume clause 22. The compatible list may also contain other
>elements.
>
> +The following properties may be added to either the phy node or the parent
> +ethernet device:
> +
> +- phy-mii-advertise-10half: Whether to advertise half-duplex 10MBit
> +- phy-mii-advertise-10full: Whether to advertise full-duplex 10MBit
> +- phy-mii-advertise-100half: Whether to advertise half-duplex 100MBit
> +- phy-mii-advertise-100full: Whether to advertise full-duplex 100MBit
> +- phy-mii-advertise-100base4: Whether to advertise 100base4
> +- phy-mii-advertise-1000half: Whether to advertise half-duplex 1000MBit
> +- phy-mii-advertise-1000full: Whether to advertise full-duplex 1000MBit
> +- phy-mii-as-master: Configure phy to act as master/slave
> +- phy-mii-manual-master: Enable/disable manual master/slave configuration
> +
> Example:
>
> ethernet-phy@0 {
These properties are booleans, and optional? Does it mean that
you cannot _disable_ broken features? Or does it mean that you
_must_ specify the non-broken features and thus break backwards
compatibility? Or are these properties not boolean (they are not
used in the example either, unfortunately), and the binding text
would need an update for clarity? What am I missing?
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured as gpio output
On Fri, Jan 17, 2014 at 14:11 +, Waibel Georg wrote: > > From cb384950a1153e856ec03109a5156e660a89bf6d Mon Sep 17 00:00:00 2001 > From: Georg Waibel > Date: Fri, 17 Jan 2014 14:51:38 +0100 > Subject: [PATCH 1/1] iMX gpio: Allow reading back of pin status if configured > as gpio output > > Register PSR was used to read the pin status in the mxc_gpio driver. This > register reflects the pin status if a pin is configured as gpio input. > If a pin is configured as an gpio output register PSR is not updated and > returns 0 instead of the actual pin status. Thus attempting to read back the > status of an gpio output pin via PSR returns a wrong value. > > Reading register DR instead of PSR fixes this issue: > - If pin is gpio output: DR returns the value written to DR by software > - If pin is gpio input: DR returns the value of register PSR und thus the > pin status Got curious because of your specific wording. In the output case, does the DR value reflect what you drive to the pin, or what the pin's status is? Because this need not be identical in the open collector case (or other "weak" scenarios like bus keeper, and what else HW development came up with). If DR always reflects the pin's status, then the patch would be OK but the commit message would need an update. If DR does not appropriately reflect the pin's status, then the patch would be an improvement (would fix the totem-pole case), but still would be wrong or incomplete in the open-collector case. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V7 1/1] drivers/gpio: Altera soft IP GPIO driver and devicetree binding
for (i = 0; i < mm_gc->gc.ngpio; i++) {
> + if (status & BIT(i)) {
> + generic_handle_irq(irq_find_mapping(
> + altera_gc->domain, i));
> + }
> + }
> + }
> + }
for_each_set_bit() might be more descriptive, save indentation
levels, and could use availalbe CPU instructions.
There are more whitespace issues here.
And I'm afraid that use of _relaxed() accessors makes the driver
depend on specific architectures, which should then reflect in
the Kconfig dependencies.
Given that we are not talking about a GPIO block that is
contained in SoCs which implement a specific CPU, but instead
talk about an IP block that is supposed to get implemented in
FPGAs connected to arbitrary CPUs, I'd suggest to use more
portable instructions.
> +
> + chained_irq_exit(chip, desc);
> +}
> +int altera_gpio_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + int i, id, reg, ret;
> + struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
> + sizeof(*altera_gc), GFP_KERNEL);
> + if (altera_gc == NULL) {
> + pr_err("%s: out of memory\n", node->full_name);
> + return -ENOMEM;
> + }
> + altera_gc->domain = 0;
This really needs a whitespace cleanup. I do suggest to clearly
separate declarations and instructions. This reduces diffs upon
further maintenance, and really isn't that expensive in terms of
typing characters (which should not be a criterion during
development anyway).
> +
> + spin_lock_init(&altera_gc->gpio_lock);
> +
> + id = pdev->id;
> +
> + if (of_property_read_u32(node, "altr,gpio-bank-width", ®))
> + /*By default assume full GPIO controller*/
> + altera_gc->mmchip.gc.ngpio = 32;
> + else
> + altera_gc->mmchip.gc.ngpio = reg;
> +
> + if (altera_gc->mmchip.gc.ngpio > 32) {
> + dev_warn(&pdev->dev,
> + "ngpio is greater than 32, defaulting to 32\n");
> + altera_gc->mmchip.gc.ngpio = 32;
> + }
Does this "maximum number of supported pins per bank" deserve a
symbolic name? The "32' is repeated here several times within a
few lines.
> +
> + altera_gc->mmchip.gc.direction_input= altera_gpio_direction_input;
> + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
> + altera_gc->mmchip.gc.get= altera_gpio_get;
> + altera_gc->mmchip.gc.set= altera_gpio_set;
> + altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
> + altera_gc->mmchip.gc.owner = THIS_MODULE;
> +
> + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, altera_gc);
> +
> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
> +
> + if (!altera_gc->mapped_irq)
> + goto skip_irq;
> +
> + altera_gc->domain = irq_domain_add_linear(node,
> + altera_gc->mmchip.gc.ngpio, &altera_gpio_irq_ops, altera_gc);
> +
> + if (!altera_gc->domain) {
> + ret = -ENODEV;
> + goto dispose_irq;
> + }
> +
> + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++)
> + irq_create_mapping(altera_gc->domain, i);
> +
> + if (of_property_read_u32(node, "altr,interrupt_type", ®)) {
> + ret = -EINVAL;
> + dev_err(&pdev->dev,
> + "altr,interrupt_type value not set in device tree\n");
> + goto teardown;
> + }
> + altera_gc->interrupt_trigger = reg;
> +
> + irq_set_handler_data(altera_gc->mapped_irq, altera_gc);
> + irq_set_chained_handler(altera_gc->mapped_irq, altera_gpio_irq_handler);
> +
> + return 0;
The 'skip_irq' label should be here. It's really not an
exceptional case or error path, it's just a shortcut for the
absence of an optional feature.
> +
> +teardown:
> + irq_domain_remove(altera_gc->domain);
> +dispose_irq:
> + irq_dispose_mapping(altera_gc->mapped_irq);
> + WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
> +
> + pr_err("%s: registration failed with status %d\n",
> + node->full_name, ret);
> +
> + return ret;
> +skip_irq:
> + return 0;
> +}
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V7 1/1] drivers/gpio: Altera soft IP GPIO driver and devicetree binding
On Fri, Mar 07, 2014 at 11:31 +0800, Linus Walleij wrote: > > On Mon, Mar 3, 2014 at 6:27 PM, wrote: > > > From: Tien Hock Loh > > > > Add driver support for Altera GPIO soft IP, including interrupts and I/O. > > Tested on Altera CV SoC board using dipsw and LED using LED framework. > > > > Signed-off-by: Tien Hock Loh > > Can I get some ACKs on this? Like from Gerhard, DT folks? > > Or is there still more things to do? Sorry, had flagged the patch for review, but did not get around to respond earlier. No ACK from me for v7 as it is. Am I supposed to provide ACKs at all, given that I'm a reviewer and fellow developer. I always understood that Acked-By is something more formal, where one maintainer signals to another maintainer that it's OK to go though a different tree. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: core: Validate lenght of the transfers in message
On Thu, Feb 13, 2014 at 16:46 +0200, Ivan T. Ivanov wrote: > > From: "Ivan T. Ivanov" > > SPI transfer lenght should be a power-of-two multiple > of eight bits. Are you suggesting that an SPI transfer cannot consist of e.g. three bytes? This would be surprising, and certainly would be rather wrong an assumption. Am I missing/misunderstanding something? virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] spi: core: Validate lenght of the transfers in message
On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote:
>
> From: "Ivan T. Ivanov"
>
> SPI transfer lenght should be a power-of-two multiple
> of eight bits.
Please re-check for "lenght" typos in subjects and commit logs
(code comments appear to be correct already).
The commit message still confuses me. You don't check that the
length of an SPI transfer has a power-of-two length in bytes.
Instead what the check implements is IIUC
- pad "odd" bits-per-word settings before the check to full 1/2/4
byte length specs (this is the power-of-two thing)
- the total length of the SPI transfer cannot be empty (which I'd
consider an optimization, not a violation, and may need a
separate discussion)
- the total length of the SPI transfer must be such that each
"word" must be provided within a full 1/2/4 byte entity, with
padding bits if the bits-per-word is "odd"
Is this a misunderstanding on my side? A terminology thing? To
me, the "SPI transfer" is the total payload and may have any
arbitrary length. What you check for is a constraint on the
transfer's length derived from or based on the "word length"
('word' in SPI context).
So the code may be appropriate, yet the description may need an
update, to not have the next person ask the same questions again.
> @@ -1668,6 +1669,22 @@ static int __spi_validate(struct spi_device *spi,
> struct spi_message *message)
> return -EINVAL;
> }
>
> + /*
> + * SPI transfer length should be multiple of SPI word size
> + * where SPI word size should be power-of-two multiple
> + */
> + if (xfer->bits_per_word <= 8)
> + w_size = 1;
> + else if (xfer->bits_per_word <= 16)
> + w_size = 2;
> + else
> + w_size = 4;
> +
> + n_words = xfer->len / w_size;
> + /* No partial transfers accepted */
> + if (!n_words || xfer->len % xfer->bits_per_word)
> + return -EINVAL;
> +
> if (xfer->speed_hz && master->min_speed_hz &&
> xfer->speed_hz < master->min_speed_hz)
> return -EINVAL;
[ just left the code here for comparison with the above description ]
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add support for flag status register on Micron chips
On Wed, 2014-04-09 at 12:03 +0200, Marek Vasut wrote: > > On Tuesday, April 08, 2014 at 06:12:49 PM, grmo...@altera.com wrote: > > From: Graham Moore > > > > This is a slightly different version of the patch that Insop Song > > submitted > > (http://marc.info/?i=201403012022.10111.marex%20()%20denx%20!%20de). > > > > I talked to Insop, and he agreed I should submit this patch as a follow-on > > to his. > > > > This patch uses a flag in the m25p_ids[] array to determine which chips > > need to use the FSR (Flag Status Register). > > > > Rationale for using the FSR: > > > > The Micron data sheets say we have to do this, at least for the multi-die > > 512M and 1G parts (n25q512 and n25q00). In practice, if we don't check > > the FSR for program/erase status, and we rely solely on the status > > register (SR), then we get corrupted data in the flash. > > I talked to Gerhard yesterday and he told me there is something like that on > ONFI NAND. I think I now understand why that new register is in-place. > Apparently, in the ONFI NAND case, there is a READY and TRUE-READY signal and > one of those reflects that _all_ the dies have finished their operation. This > is > in my opinion seriously misdesigned as it breaks any kind of backward > compatibility. There's a little more to it, I think. The "ready" and "true ready" are terms from Linux header files. In Micron datasheets, both of the bits 5 and 6 in the READ_STATUS response are referred to as "ready/busy", with a few footnotes to them depending on the mode of operation or the command that is being executed. The other thing is that there is the READ_STATUS command, which _might_ yield responses from different dies upon subsequent repeated reads. So you may have to determine how many dies are in the package, to repeat the STATUS query as many times, and logically combine these results in the chip's overall status. Or use the extended status query where you can address an individual die, but which is not supported by every chip. Pick your poison. The hardware R/B pins are wired-OR, such that any busy die will pull the summary pin level low. But this only tells you whether the operation is still pending, after it's complete you have to get the status and will face the situation described above. Can't tell how this NAND approach maps to the NOR subject described here. They might have a similar motivation, yet implement different approaches to the issue. > > Micron told us (Altera) that for multi-die chips based on the 65nm 256MB > > die, we need to check the SR first, then check the FSR, which is why the > > wait_for_fsr_ready function does that. Future chips based on 45 nm 512MB > > die will use the FSR only. This sounds to me similar to polling the NAND's R/B pin until the operation has completed, to then fetch the STATUS byte to determine the execution's result. Does this sound plausible? For NOR, do you poll for the "busy" condition to deassert, and check for success then? virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] ARM: dts: socfpga: add gpio pieces
On Thu, Mar 20, 2014 at 20:55 +0100, Sebastian Andrzej Siewior wrote: > > The cycloneV has three gpio controllers, each one with 29 gpios. This patch > adds the three controller with the gpio driver which is now sitting the > gpio tree. The third bank should have 27 pins, only the first two have 29. The commit message probably should not discuss which git tree a driver currently resides in. This is just a temporary detail which no longer applies to the mainline source base where your patch will end up in. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] spi: switch to devm_spi_alloc_master
On Fri, Jan 31, 2014 at 11:23 +0100, Maxime Ripard wrote:
>
> Make the existing users of devm_spi_register_master use the
> devm_spi_alloc_master function to avoid leaking memory.
>
> [ ... ]
> drivers/spi/spi-mpc512x-psc.c| 19 ---
Note that the context for the MPC512x SPI driver will change in
3.14-rc1, so you will have to rebase after the merge window.
> diff --git a/drivers/spi/spi-mpc512x-psc.c b/drivers/spi/spi-mpc512x-psc.c
> index 46d2313..f376595 100644
> --- a/drivers/spi/spi-mpc512x-psc.c
> +++ b/drivers/spi/spi-mpc512x-psc.c
> @@ -479,7 +479,7 @@ static int mpc512x_psc_spi_do_probe(struct device *dev,
> u32 regaddr,
> char clk_name[16];
> struct clk *clk;
>
> - master = spi_alloc_master(dev, sizeof *mps);
> + master = devm_spi_alloc_master(dev, sizeof *mps);
> if (master == NULL)
> return -ENOMEM;
>
> @@ -507,8 +507,7 @@ static int mpc512x_psc_spi_do_probe(struct device *dev,
> u32 regaddr,
> tempp = devm_ioremap(dev, regaddr, size);
> if (!tempp) {
> dev_err(dev, "could not ioremap I/O port range\n");
> - ret = -EFAULT;
> - goto free_master;
> + return -EFAULT;
> }
> mps->psc = tempp;
> mps->fifo =
> @@ -516,19 +515,19 @@ static int mpc512x_psc_spi_do_probe(struct device *dev,
> u32 regaddr,
> ret = devm_request_irq(dev, mps->irq, mpc512x_psc_spi_isr, IRQF_SHARED,
> "mpc512x-psc-spi", mps);
> if (ret)
> - goto free_master;
> + return ret;
> init_completion(&mps->txisrdone);
>
> psc_num = master->bus_num;
> snprintf(clk_name, sizeof(clk_name), "psc%d_mclk", psc_num);
> clk = devm_clk_get(dev, clk_name);
> - if (IS_ERR(clk)) {
> - ret = PTR_ERR(clk);
> - goto free_master;
> - }
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> ret = clk_prepare_enable(clk);
> if (ret)
> - goto free_master;
> + return ret;
> +
> mps->clk_mclk = clk;
> mps->mclk_rate = clk_get_rate(clk);
>
> @@ -544,8 +543,6 @@ static int mpc512x_psc_spi_do_probe(struct device *dev,
> u32 regaddr,
>
> free_clock:
> clk_disable_unprepare(mps->clk_mclk);
> -free_master:
> - spi_master_put(master);
>
> return ret;
> }
Reading the diff in the SPI master driver, the change appears to
be balanced, and replacing 'goto free_master' with immediate
return looks appropriate.
Can't comment on the correctness of removing the spi_master_put()
call when switching from spi_alloc_master() to
devm_spi_alloc_master(). This gets discussed in the other
subthread, dealing with the generic subsystem approach.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: add missing word in gpio dt binding doc
On Thu, Dec 19, 2013 at 15:21 +0100, Boris BREZILLON wrote: > > Signed-off-by: Boris BREZILLON > --- > Documentation/devicetree/bindings/gpio/gpio.txt |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt > b/Documentation/devicetree/bindings/gpio/gpio.txt > index e2295e3..4019ce1 100644 > --- a/Documentation/devicetree/bindings/gpio/gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt > @@ -56,7 +56,7 @@ and empty GPIO flags as accepted by the "qe_pio_e" > gpio-controller. > 2) gpio-controller nodes > > > -Every GPIO controller node must both an empty "gpio-controller" > +Every GPIO controller node must contain both an empty "gpio-controller" > property, and have #gpio-cells contain the size of the gpio-specifier. > > It might contain GPIO hog definitions. GPIO hogging is a mechanism providing > -- > 1.7.9.5 The context of the patch appears to contain stuff that gets introduced with the RFC patch you sent out after this one. If this suspicion(sp?) is true, you may have to re-create the patch on top of something official. :) And you may want to mark this patch as "trivial". virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 04/12] clk: Add regmap core helpers for enable/disable/is_enabled
On Wed, Dec 18, 2013 at 20:50 -0800, Mike Turquette wrote: > > Adding all of this stuff to struct clk_hw makes me a sad panda. You are > essentially sharing a common set of ops for clocks that use regmap as > their io operation back-end, and that is a good thing. > > However, why not just do this as drivers/clk/clk-regmap.c, or > drivers/clk/clk-gate-regmap.c? Putting the clk_ops callback functions in > drivers/clk/clk.c is a little weird and putting those struct members > into struct clk_hw is definitely strange. Wasn't the idea to extend the set of register accessor routines in such that memory mapped I/O as well as regmap style becomes possible? This is what I understood from past iterations of discussing this approach. I agree that duplicating the clock gate's implementation just to access the register in a different way feels strange and somehow unfortunate. There still may be the issue of expensive operations only being allowed within prepare and unprepare, while enable and disable are supposed to be "cheap and straight forward", and should not block and thus may not use external communication. But that appears to be orthogonal to the API which wraps register access. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] clk: remove CONFIG_COMMON_CLK_DEBUG
On Wed, Dec 18, 2013 at 21:43 -0800, Mike Turquette wrote:
>
> Populate ${DEBUGS_MOUNT_POINT}/clk if CONFIG_DEBUG_FS is set. This
> eliminates the extra (annoying) step of enabling the config option
> manually.
>
> Signed-off-by: Mike Turquette
Yes, please! The debugfs information is just too useful to hide
behind another option that requires manual enabling. :)
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 03/15] clk: Add regmap core helpers for enable/disable/is_enabled
On Thu, Dec 26, 2013 at 11:31 -0800, Stephen Boyd wrote:
>
> On 12/24, Gerhard Sittig wrote:
> > On Mon, Dec 23, 2013 at 17:12 -0800, Stephen Boyd wrote:
> > >
> > > The clock framework already has support for simple gate clocks
> > > but if drivers want to use the gate clock functionality they need
> > > to wrap the gate clock in another struct and chain the ops by
> > > calling the gate ops from their own custom ops. Plus the gate
> > > clock implementation only supports MMIO accessors so other bus
> > > type clocks don't benefit from the potential code reuse. Add some
> > > simple regmap helpers for enable/disable/is_enabled that drivers
> > > can use as drop in replacements for their clock ops or as simple
> > > functions they call from their own custom ops. This is based on
> > > similar helps in the regulator framework.
> >
> > The same comment applies as to the previous version. Is it
> > useful to introduce copies of the gate handling while the
> > difference in only in how the hardware registers get accessed?
> >
>
> I don't plan to use the clk-gate.c implementation because I need
> more than just a bit toggling clock. We can easily make
> clk-gate.c use these helpers if you're worried about the very
> small amount of code duplication between the two. I'd be glad to
> do that, I just didn't include it here because I don't have a use
> for it.
OK, then I simply misunderstood. From past threads I got the
impression that your clock item was "a gate with regmap instead
of mmio for hardware access, everything else being the same".
The concerns were not so much about the size of duplicated code,
but the "quality step" in starting duplication at all. But since
I was wrong, nevermind.
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -177,11 +177,21 @@ struct clk_init_data {
> > > [ ... ]
> > > @@ -447,6 +457,9 @@ struct clk *__clk_lookup(const char *name);
> > > long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
> > > unsigned long *best_parent_rate,
> > > struct clk **best_parent_p);
> > > +int clk_is_enabled_regmap(struct clk_hw *hw);
> > > +int clk_enable_regmap(struct clk_hw *hw);
> > > +void clk_disable_regmap(struct clk_hw *hw);
> >
> > Looking at the patch: Do you expect callers to remember whether
> > a clock gate is backed by mmio or by regmap access, to call a
> > different set of routines?
>
> There are only regmap functions. I'm not sure where the choice
> is, but I expect the callers to know what they're doing. If you
> look at the rest of this series you'll see that I assign these
> functions directly to the clk_ops, or I call them from the
> enable/disable functions that need to do some status bit polling
> after the clock is enabled or disabled.
>
> > Should this not be hidden behind the
> > API and be transparent after clock registration?
>
> I don't really understand what you mean by hiding it behind the
> API? What API? If we're talking about clk_register_gate() I think
> we would need to add a clk_register_regmap_gate() function
> because the reg argument is an __iomem pointer. It doesn't look
> like it can be transparent unless that pointer is reused as an
> offset. I don't attempt to do anything about that here though
> because I don't use the clk-gate.c code.
See above, it's simple. I misunderstood, asked (in a previous
thread), got no response, saw another submission, asked again.
Now that you told me, it's clear I got something wrong. Thank
you for telling me what I missed before.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ARM] Fix kernel compile error: drivers/crypto/ixp4xx_crypto.c.
On Tue, Dec 31, 2013 at 11:51 +0100, Krzysztof Hałasa wrote:
>
> drivers/crypto/ixp4xx_crypto.c: In function 'ixp_module_init':
> drivers/crypto/ixp4xx_crypto.c:1419:2: error: 'dev' undeclared (first use in
> this function)
>
> Now builds. Not tested on real hw.
>
> Signed-off-by: Krzysztof Hałasa
I understood that a 'Fixes: ' line is suggested when
you can clearly identify what broke what you fix. Is it 27c1789ca6a6
"DMA-API: crypto: remove last references to 'static struct device *dev'"?
Then v3.13-rc1 would have been affected.
> --- a/drivers/crypto/ixp4xx_crypto.c
> +++ b/drivers/crypto/ixp4xx_crypto.c
> @@ -1410,14 +1410,12 @@ static const struct platform_device_info ixp_dev_info
> __initdata = {
> static int __init ixp_module_init(void)
> {
> int num = ARRAY_SIZE(ixp4xx_algos);
> - int i, err ;
> + int i, err;
>
> pdev = platform_device_register_full(&ixp_dev_info);
> if (IS_ERR(pdev))
> return PTR_ERR(pdev);
>
> - dev = &pdev->dev;
> -
> spin_lock_init(&desc_lock);
> spin_lock_init(&emerg_lock);
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ARM] Fix kernel compile error: drivers/crypto/ixp4xx_crypto.c.
On Tue, Dec 31, 2013 at 18:14 +, Russell King - ARM Linux wrote: > > Please get rid of your Mail-Followup-To: header: > > Mail-Followup-To: Krzysztof Hałasa , > > lkml , > > linux-arm-ker...@lists.infradead.org, > > Russell King , > > Christian Hohnstaedt , > > Herbert Xu > > User-Agent: Mutt/1.5.21 (2010-09-15) > > > It causes all recipients following the thread to be moved into the To: > header when someone replies to one of your messages, which is deemed to > be anti-social. You can kill this header by adding: > > set followup_to=no > > to your .muttrc file. Thank you for telling me, I was not aware. Had no MFT related setting in my config, learned from the manual that $followup_to defaults to yes, have turned it off now. Other mutt users may want to check as well. Happy new year! :) virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ARM] Fix kernel compile error: drivers/crypto/ixp4xx_crypto.c.
On Tue, Dec 31, 2013 at 10:01 -0800, Randy Dunlap wrote: > > On 12/31/13 05:48, Gerhard Sittig wrote: > > On Tue, Dec 31, 2013 at 11:51 +0100, Krzysztof Hałasa wrote: > >> > >> drivers/crypto/ixp4xx_crypto.c: In function 'ixp_module_init': > >> drivers/crypto/ixp4xx_crypto.c:1419:2: error: 'dev' undeclared (first use > >> in this function) > >> > >> Now builds. Not tested on real hw. > >> > >> Signed-off-by: Krzysztof Hałasa > > > > I understood that a 'Fixes: ' line is suggested when > > you can clearly identify what broke what you fix. Is it 27c1789ca6a6 > > "DMA-API: crypto: remove last references to 'static struct device *dev'"? > > Then v3.13-rc1 would have been affected. > > is 'Fixes: ' documented somewhere? Last time I checked (few days ago, 'git grep -w Fixes:') it wasn't, at least not within the kernel source tree and its Documentation hierarchy. Today's master still does not have it. But a quick search in recent LAKML messages reveals that it's been in use by e.g. Olof Johansson, Jason Cooper, Rusty Russell, Dan Carpenter, Russell King, Mark Brown, Stephen Warren, Paul Walmsley, and I'm not making this up but am just referring to what repeatedly was requested in the past. The form used there was "Fixes: ()" though. A doc update may be due to have a canonical format. I do see the benefit of this tag, as it does not hide this information in the commit message's prose, makes submitters think about that property in more explicit ways and makes them provide the information such that others need not do the research, and greatly helps those involved in tests and release management to determine affected versions and required actions that need to be taken. The tag is very useful. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 04/12] clk: Add regmap core helpers for enable/disable/is_enabled
On Thu, Dec 19, 2013 at 21:24 +0100, Gerhard Sittig wrote: > > Wasn't the idea to extend the set of register accessor routines > in such that memory mapped I/O as well as > regmap style becomes possible? This is what I understood from > past iterations of discussing this approach. Just FYI and to have the pointer in some archive: Had a look at recent threads because it appears that several pending changes have similar needs to adjust the kind of hardware access, while sharing common code to handle the actual clock item logic would be desirable. Please check "[PATCHv12 00/49] ARM: TI SoC clock DT conversion" from Tero Kristo as of 2013-12-20, which "in bypassing" introduces low level ops for register access (patches 06 and following). I suggested to separate this hardware access aspect out of the "TI SoC clock" series (almost missed the fact that it changes behaviour in common code), and to add regmap support or at least allow to do regmap on top of the ll_ops infrastructure. With the proposed generic approach all of us should be happy. The vast amount of ARM code should not experience any change, the queued PowerPC support can keep working, new TI SoC clocks can get implemented, Steven you can get regmap access, and more platforms may use the common code even if readl()/writel() won't fit them. Let's followup in Tero's v12 feedback thread to make that happen. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 03/15] clk: Add regmap core helpers for enable/disable/is_enabled
On Mon, Dec 23, 2013 at 17:12 -0800, Stephen Boyd wrote:
>
> The clock framework already has support for simple gate clocks
> but if drivers want to use the gate clock functionality they need
> to wrap the gate clock in another struct and chain the ops by
> calling the gate ops from their own custom ops. Plus the gate
> clock implementation only supports MMIO accessors so other bus
> type clocks don't benefit from the potential code reuse. Add some
> simple regmap helpers for enable/disable/is_enabled that drivers
> can use as drop in replacements for their clock ops or as simple
> functions they call from their own custom ops. This is based on
> similar helps in the regulator framework.
The same comment applies as to the previous version. Is it
useful to introduce copies of the gate handling while the
difference in only in how the hardware registers get accessed?
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -177,11 +177,21 @@ struct clk_init_data {
> [ ... ]
> @@ -447,6 +457,9 @@ struct clk *__clk_lookup(const char *name);
> long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
> unsigned long *best_parent_rate,
> struct clk **best_parent_p);
> +int clk_is_enabled_regmap(struct clk_hw *hw);
> +int clk_enable_regmap(struct clk_hw *hw);
> +void clk_disable_regmap(struct clk_hw *hw);
Looking at the patch: Do you expect callers to remember whether
a clock gate is backed by mmio or by regmap access, to call a
different set of routines? Should this not be hidden behind the
API and be transparent after clock registration?
I'd suggest to fold regmap support into Tero Kristo's ll_ops
approach, and to discuss this in his v12 thread.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] clk: support hardware-specific debugfs entries
On Thu, Dec 26, 2013 at 08:26 -0600, Alex Elder wrote:
>
> @@ -140,6 +147,9 @@ struct clk_ops {
> int (*set_rate)(struct clk_hw *hw, unsigned long,
> unsigned long);
> void(*init)(struct clk_hw *hw);
> +#ifdef CONFIG_COMMON_CLK_DEBUG
> + int (*debug_init)(struct clk_hw *hw, struct dentry *dentry);
> +#endif
> };
Please note that this CONFIG_COMMON_CLK_DEBUG switch may go away
(or get replaced) soon. Mike plans to effectively enable common
clock debugging as soon as debugfs is in effect (which I welcome
very much).
So you should monitor current development and adjust your patch,
or be prepared for breakage and fixing afterwards. I'd suggest
to just declare the .debug_init member unconditionally now.
Since Mike's patch very likely gets accepted, and nothing will
break when yours gets added later (regardless of the number of
review iterations it will see).
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
