Re: [PATCH] usb: gadget: udc-xilinx: compress return logic into one line
On 10.7.2017 05:04, Gustavo A. R. Silva wrote:
> Simplify return logic to avoid unnecessary variable assignment.
>
> This issue was detected using Coccinelle and the following
> semantic patch:
>
> @@
> local idexpression ret;
> expression e;
> @@
>
> -ret =
> +return
> e;
> -return ret;
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/usb/gadget/udc/udc-xilinx.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/udc-xilinx.c
> b/drivers/usb/gadget/udc/udc-xilinx.c
> index de207a9..552389d 100644
> --- a/drivers/usb/gadget/udc/udc-xilinx.c
> +++ b/drivers/usb/gadget/udc/udc-xilinx.c
> @@ -1217,14 +1217,13 @@ static const struct usb_ep_ops xusb_ep_ops = {
> static int xudc_get_frame(struct usb_gadget *gadget)
> {
> struct xusb_udc *udc;
> - int frame;
>
> if (!gadget)
> return -ENODEV;
>
> udc = to_udc(gadget);
> - frame = udc->read_fn(udc->addr + XUSB_FRAMENUM_OFFSET);
> - return frame;
> +
> + return udc->read_fn(udc->addr + XUSB_FRAMENUM_OFFSET);
> }
>
> /**
>
Acked-by: Michal Simek
Thanks,
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: xudc: fix return value check in xudc_probe()
On 04/16/2015 02:17 PM, weiyj...@163.com wrote:
> From: Wei Yongjun
>
> In case of error, the function devm_ioremap_resource() returns
> ERR_PTR() and never returns NULL. The NULL test in the return
> value check should be replaced with IS_ERR().
>
> Signed-off-by: Wei Yongjun
> ---
> drivers/usb/gadget/udc/udc-xilinx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/udc-xilinx.c
> b/drivers/usb/gadget/udc/udc-xilinx.c
> index dd3e9fd..cfe0349 100644
> --- a/drivers/usb/gadget/udc/udc-xilinx.c
> +++ b/drivers/usb/gadget/udc/udc-xilinx.c
> @@ -2071,8 +2071,8 @@ static int xudc_probe(struct platform_device *pdev)
> /* Map the registers */
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> udc->addr = devm_ioremap_resource(&pdev->dev, res);
> - if (!udc->addr)
> - return -ENOMEM;
> + if (IS_ERR(udc->addr))
> + return PTR_ERR(udc->addr);
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
>
Reviewed-by: Michal Simek
Thanks,
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers/usb/host/ehci-xilinx-of.c: Include "linux/of_irq.h" to avoid compiling error
Hi Alan and Greg, On 09/20/2014 06:19 AM, Chen Gang wrote: > Hello Maintainers: > > Please help check this patch, when you have time. > > Thanks. > > On 09/08/2014 01:20 PM, Michal Simek wrote: >> On 09/03/2014 05:50 PM, Chen Gang wrote: >>> Need include it for irq_of_parse_and_map(), the related error with >>> allmodconfig under microblaze: >>> >>> drivers/usb/host/ehci-xilinx-of.c: In function ‘ehci_hcd_xilinx_of_probe’: >>> drivers/usb/host/ehci-xilinx-of.c:156:2: error: implicit declaration of >>> function ‘irq_of_parse_and_map’ [-Werror=implicit-function-declaration] >>> irq = irq_of_parse_and_map(dn, 0); >>> ^ >>> >>> Signed-off-by: Chen Gang >>> --- >>> drivers/usb/host/ehci-xilinx-of.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/usb/host/ehci-xilinx-of.c >>> b/drivers/usb/host/ehci-xilinx-of.c >>> index fe57710..a232836 100644 >>> --- a/drivers/usb/host/ehci-xilinx-of.c >>> +++ b/drivers/usb/host/ehci-xilinx-of.c >>> @@ -31,6 +31,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> /** >>> * ehci_xilinx_port_handed_over - hand the port out if failed to enable it >>> >> >> Acked-by: Michal Simek Alan: Can you please add this patch to your queue? Greg: If Alan is not maintaining this part of kernel, is this patch in your queue? I have also not a problem to add this patch through my microblaze tree but I think it will be much better to use any USB tree. Thanks, Michal -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers/usb/host/ehci-xilinx-of.c: Include "linux/of_irq.h" to avoid compiling error
On 09/03/2014 05:50 PM, Chen Gang wrote: > Need include it for irq_of_parse_and_map(), the related error with > allmodconfig under microblaze: > > drivers/usb/host/ehci-xilinx-of.c: In function ‘ehci_hcd_xilinx_of_probe’: > drivers/usb/host/ehci-xilinx-of.c:156:2: error: implicit declaration of > function ‘irq_of_parse_and_map’ [-Werror=implicit-function-declaration] > irq = irq_of_parse_and_map(dn, 0); > ^ > > Signed-off-by: Chen Gang > --- > drivers/usb/host/ehci-xilinx-of.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/host/ehci-xilinx-of.c > b/drivers/usb/host/ehci-xilinx-of.c > index fe57710..a232836 100644 > --- a/drivers/usb/host/ehci-xilinx-of.c > +++ b/drivers/usb/host/ehci-xilinx-of.c > @@ -31,6 +31,7 @@ > #include > #include > #include > +#include > > /** > * ehci_xilinx_port_handed_over - hand the port out if failed to enable it > Acked-by: Michal Simek Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 1/2] usb: doc: udc-xilinx: Add devicetree bindings
On 04/03/2014 04:59 PM, Felipe Balbi wrote:
> On Thu, Apr 03, 2014 at 01:05:18PM +0530, Subbaraya Sundeep Bhatta wrote:
>> Add devicetree bindings for Xilinx axi udc driver.
>>
>> Signed-off-by: Subbaraya Sundeep Bhatta
>> ---
>> Changes for v2:
>> - replaced xlnx,include-dma with xlnx,has-builtin-dma
>>
>> .../devicetree/bindings/usb/udc-xilinx.txt | 20
>>
>> 1 files changed, 20 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/usb/udc-xilinx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/udc-xilinx.txt
>> b/Documentation/devicetree/bindings/usb/udc-xilinx.txt
>> new file mode 100644
>> index 000..7c24fac
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/udc-xilinx.txt
>> @@ -0,0 +1,20 @@
>> +Xilinx AXI USB2 device controller
>> +
>> +Required properties:
>> +- compatible: Should be "xlnx,axi-usb2-device-4.00.a"
>> +- reg : Physical base address and size of the Axi USB2
>> + device registers map.
>> +- interrupts: Property with a value describing the interrupt
>> + number.
>> +- interrupt-parent : Must be core interrupt controller
>> +- xlnx,has-builtin-dma : if DMA is included
>
> isn't there a configuration register to tell you this ?
I have checked this with Sundeep and there is nothing like that in the HW.
>
>> +
>> +Example:
>> +axi-usb2-device@42e0 {
>> +compatible = "xlnx,axi-usb2-device-4.00.a";
>> +interrupt-parent = <0x1>;
>
> why isn't interrupt-parent a phandle ?
Just for the record: Using number here should be also fine because DTC
is converting it to numbers with linux,phandle and phandle.
[linux-next]$ dtc -O dts -I dtb /tftpboot/devicetree.dtb | less
...
ps7-scugic@f8f01000 {
#address-cells = <0x2>;
#interrupt-cells = <0x3>;
#size-cells = <0x1>;
compatible = "arm,cortex-a9-gic", "arm,gic";
interrupt-controller;
num_cpus = <0x2>;
num_interrupts = <0x60>;
reg = <0xf8f01000 0x1000 0xf8f00100 0x100>;
linux,phandle = <0x3>;
phandle = <0x3>;
};
ps7-scutimer@f8f00600 {
clocks = <0x2 0x4>;
compatible = "arm,cortex-a9-twd-timer";
interrupt-parent = <0x3>;
interrupts = <0x1 0xd 0x301>;
reg = <0xf8f00600 0x20>;
};
...
but anyway Sundeep with change it to any sensible value <&intc>;
Thanks for pointing to it,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
>> +struct xusb_udc {
>> +struct usb_gadget gadget;
>> +struct xusb_ep ep[8];
>> +struct usb_gadget_driver *driver;
>> +struct cmdbuf ch9cmd;
>> +u32 usb_state;
>> +u32 remote_wkp;
>> +unsigned int (*read_fn)(void __iomem *);
>> +void (*write_fn)(void __iomem *, u32, u32);
>
> why do you need these to be function pointers ? Because of endianness ?
> generic readl()/writel() already take care of that.
readl from asm-generic/io.h is converting value from little endian IO
to cpu endianness.
This IP exists also in big endian version.
It means we have to support all possible combinations.
IP little and reading it on little or big endian CPU
IP big and reading it on little or big endian CPU.
Look below.
>> +spin_lock_init(&udc->lock);
>> +
>> +/* Check for IP endianness */
>> +udc->write_fn = xudc_write32_be;
>> +udc->read_fn = xudc_read32_be;
>> +udc->write_fn(udc->base_address, XUSB_TESTMODE_OFFSET, TEST_J);
>> +if ((udc->read_fn(udc->base_address + XUSB_TESTMODE_OFFSET))
>> +!= TEST_J) {
>> +udc->write_fn = xudc_write32;
>> +udc->read_fn = xudc_read32;
>> +}
>
> hmm... isn't there a configuration register to check this out ?
Sundeep can tell us if there is any configuration register but
I don't think so in connection to my experience with Xilinx soft IPs.
Endian detection directly on IP itself came from my discussion
on drivers/spi/spi-xilinx.c that this is only one way how to do it.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
signature.asc
Description: OpenPGP digital signature
[PATCH] usb: phy: Add ulpi IDs for SMSC USB3320 and TI TUSB1210
Add new ulpi IDs which are available on Xilinx Zynq boards.
Signed-off-by: Michal Simek
---
http://www.ti.com/lit/ds/symlink/tusb1210.pdf page 29
I don't know why but value is not written in smsc manual here.
http://ww1.microchip.com/downloads/en/DeviceDoc/3320.pdf
page 62
---
drivers/usb/phy/phy-ulpi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/phy/phy-ulpi.c b/drivers/usb/phy/phy-ulpi.c
index 217339d..17ea3f2 100644
--- a/drivers/usb/phy/phy-ulpi.c
+++ b/drivers/usb/phy/phy-ulpi.c
@@ -47,6 +47,8 @@ struct ulpi_info {
static struct ulpi_info ulpi_ids[] = {
ULPI_INFO(ULPI_ID(0x04cc, 0x1504), "NXP ISP1504"),
ULPI_INFO(ULPI_ID(0x0424, 0x0006), "SMSC USB331x"),
+ ULPI_INFO(ULPI_ID(0x0424, 0x0007), "SMSC USB3320"),
+ ULPI_INFO(ULPI_ID(0x0451, 0x1507), "TI TUSB1210"),
};
static int ulpi_set_otg_flags(struct usb_phy *phy)
--
1.8.2.3
pgpYQSjLdiNVP.pgp
Description: PGP signature
Re: SPDX-License-Identifier
On 02/24/2014 02:41 PM, Theodore Ts'o wrote: > On Mon, Feb 24, 2014 at 11:12:53AM +0100, Michal Simek wrote: >>> But of course, I'm not a lawyer, and if your company has is paying for >>> the development of the driver, the Golden Rule applies (he who has the >>> Gold, makes the Rules), and each of our respective corporate lawyers >>> may have different opinions about what might happen if the question >>> was ever to be adjudicated in court. >> >> Aren't all these points already answered by SPDX project? >> I believe that they should know how this should be handled properly. > > The SPDX can not give legal advice; not to you, and not to your > company. One lawyer might believe that > > /* > * SPDX-License-Identifier: GPL-2.0 > */ > > Might be sufficient. Others might believe you need to do: > > /* > * Copyright Ty Coon, 2012. > * > * SPDX-License-Identifier: GPL-2.0 > */ > > Still others might believe you need at the very least: > > /* > * Copyright Ty Coon, 2012. > * > * All Rights Reserved. > * > * SPDX-License-Identifier: GPL-2.0 > */ Aren't these differences already present in the header? > > As far as I know, there is no case law on point about whether or not > SPDX-License-Identifier has legal significance, or whether the court > would consider that to be a valid copyright permission statement. So > any "answers" made by any lawyer would be guesses. Of course, an > guess by a lawyer which is retained by *you* or your company and fully > informed with the unique parameters of your situation would constitute > legal advice. Anything else, including anything any of us could say > on this mailing list, would be biovating. :-) I think make sense to wait for Wolfgang about his experience because I believe he has considered it before u-boot change. BTW: Isn't this a good topic for kernel-summit? :-) Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature
Re: SPDX-License-Identifier
On 02/21/2014 08:01 PM, Theodore Ts'o wrote: > On Fri, Feb 21, 2014 at 09:57:20AM -0800, Greg Kroah-Hartman wrote: >>> But shouldn't we at least write somewhere >>> that it has connection to spdx.org where you can find out that licenses. >> >> Why? Are these licenses so unknown that no one knows what they are? >> And, as part of the kernel-as-a-whole-work, they all resolve to GPLv2 >> anyway, and we have that license in the source tree, so nothing else >> should be needed. > > Note that not all lawyers are in agreement about this, so if this is a > driver being developed by a company, you may want to ask your > corporate counsel if they have an opinion about this. I've received > advice of the form that it's not obvious that regardless of whether or > not us *engineers* understand what all of the licensing terms mean, > what's important is whether someone who is accused of "borrowing" > GPL'ed code and dropping it in a driver for some other OS can convince > a judge whether or not it's considered "obvious" from a legal > perspective what an SPDX header means, and what is implied by an SPDX > license identifer. > > Also note that with the advent of web sites that allow people to do > web searches and turn up a singleton file via some gitweb interface, > the fact that the full license text is distributed alongside the > tarball might or might have as much legal significance as it once had. > > But of course, I'm not a lawyer, and if your company has is paying for > the development of the driver, the Golden Rule applies (he who has the > Gold, makes the Rules), and each of our respective corporate lawyers > may have different opinions about what might happen if the question > was ever to be adjudicated in court. Thanks Ted. Aren't all these points already answered by SPDX project? I believe that they should know how this should be handled properly. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature
Re: SPDX-License-Identifier
On 02/21/2014 05:56 PM, Greg Kroah-Hartman wrote: > On Fri, Feb 21, 2014 at 10:20:45AM -0600, Felipe Balbi wrote: >> Hi, >> >> On Fri, Feb 21, 2014 at 05:18:39PM +0100, Michal Simek wrote: >>> On 02/21/2014 05:12 PM, Felipe Balbi wrote: >>>> On Fri, Feb 21, 2014 at 05:04:26PM +0100, Michal Simek wrote: >>>>> On 02/21/2014 05:04 PM, Greg Kroah-Hartman wrote: >>>>>> On Fri, Feb 21, 2014 at 07:38:16AM +0100, Michal Simek wrote: >>>>>>> BTW: u-boot started to use SPDX-License-Identifier >>>>>>> which will be nice to start to use. >>>>>> >>>>>> I agree, feel free to start sending patches to use this type of >>>>>> identifier for drivers. >>>>> >>>>> But we probably need to add that Licenses to one location. >>>>> Documentation/Licenses? >>>> >>>> Just add to the drivers themselves, just like u-boot is doing. A simple: >>>> >>>>$ git grep -e SPDX-License-Identifier >>>> >>>> will tell you filename and license. Or did I misunderstand your question ? >>> >>> But for doing this it is probably necessary to have location where >>> you will place that licenses and you will explain what it is how >>> it is done by Wolfgang in this commit. >>> >>> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=eca3aeb352c964bdb28b8e191d6326370245e03f >>> >>> Then yes, adding one line is enough. >> >> spdx.org has all licenses, why don't we just rely on that instead of >> adding every other license to the kernel source ? > > Yes, all that will be acceptable is a one-line identifier in the file. > No need to have all the different licenses in the kernel source, that's > crazy and not needed at all. > > I've told the SPDX people this in the past, and they keep promising that > they will do the comment work, but it's been months and I have yet to > see a single patch... But shouldn't we at least write somewhere that it has connection to spdx.org where you can find out that licenses. I have no problem to use this one-line identifier at all. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature
Re: SPDX-License-Identifier
On 02/21/2014 05:12 PM, Felipe Balbi wrote: > On Fri, Feb 21, 2014 at 05:04:26PM +0100, Michal Simek wrote: >> On 02/21/2014 05:04 PM, Greg Kroah-Hartman wrote: >>> On Fri, Feb 21, 2014 at 07:38:16AM +0100, Michal Simek wrote: >>>> BTW: u-boot started to use SPDX-License-Identifier >>>> which will be nice to start to use. >>> >>> I agree, feel free to start sending patches to use this type of >>> identifier for drivers. >> >> But we probably need to add that Licenses to one location. >> Documentation/Licenses? > > Just add to the drivers themselves, just like u-boot is doing. A simple: > > $ git grep -e SPDX-License-Identifier > > will tell you filename and license. Or did I misunderstand your question ? But for doing this it is probably necessary to have location where you will place that licenses and you will explain what it is how it is done by Wolfgang in this commit. http://git.denx.de/?p=u-boot.git;a=commitdiff;h=eca3aeb352c964bdb28b8e191d6326370245e03f Then yes, adding one line is enough. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature
Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
On 02/21/2014 05:04 PM, Greg Kroah-Hartman wrote: > On Fri, Feb 21, 2014 at 07:38:16AM +0100, Michal Simek wrote: >> BTW: u-boot started to use SPDX-License-Identifier >> which will be nice to start to use. > > I agree, feel free to start sending patches to use this type of > identifier for drivers. But we probably need to add that Licenses to one location. Documentation/Licenses? Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature
Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
On 02/21/2014 04:42 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 21, 2014 at 02:41:03PM +0100, Michal Simek wrote:
>>>>> + /* Map the registers */
>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> + udc->base_address = devm_ioremap_nocache(&pdev->dev, res->start,
>>>>> + resource_size(res));
>>>>
>>>> use devm_ioremap_resource() instead.
>>>
>>> Also, res might be NULL. You should check that before dereferencing it.
>>
>> yes it is necessary for both cases with devm_ioremap_nocache
>> or with devm_ioremap_resource.
>
> read the source Luke:
>
> | void __iomem *devm_ioremap_resource(struct device *dev, struct resource
> *res)
> | {
> | resource_size_t size;
> | const char *name;
> | void __iomem *dest_ptr;
> |
> | BUG_ON(!dev);
> |
> | if (!res || resource_type(res) != IORESOURCE_MEM) {
>
> already done for you
>
> | dev_err(dev, "invalid resource\n");
> | return ERR_PTR(-EINVAL);
> | }
> |
> | size = resource_size(res);
> | name = res->name ?: dev_name(dev);
> |
> | if (!devm_request_mem_region(dev, res->start, size, name)) {
> | dev_err(dev, "can't request region for resource %pR\n", res);
> | return ERR_PTR(-EBUSY);
> | }
> |
> | if (res->flags & IORESOURCE_CACHEABLE)
> | dest_ptr = devm_ioremap(dev, res->start, size);
> | else
> | dest_ptr = devm_ioremap_nocache(dev, res->start, size);
I have read it just not sure if IORESOURCE_CACHEABLE is setup by default
or not.
If yes, then you have to setup res->flags in your driver and have to
check it.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
signature.asc
Description: OpenPGP digital signature
Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
>> If we can point to standard interrupt description then please point me to >> exact description you would like to see here and we can change it. > > Unfortunately I'm not aware of a generic interrupts document. I just > don't see the point in each document listing interrupt-parent as a > requiredp roeprty when it's not. That said this is a trivial detail and > not really a blocker. I agree with you that copying this part again and again is just problematic. Time to time I see that IRQs doesn't need to be described too. I am also not sure if kernel can work without interrupt-parent at all. I expect that it won't work and because we have interrupt parent in every node (which is generated) it is probably required in our setup. As you said it is just trivial detail for me too. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature
Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
Hi Mark,
On 02/21/2014 01:04 PM, Mark Rutland wrote:
>
> On Thu, Feb 20, 2014 at 06:23:13PM +, Felipe Balbi wrote:
>> Hi,
>>
>> On Wed, Feb 19, 2014 at 03:10:45PM +0530, Subbaraya Sundeep Bhatta wrote:
>>> This patch adds xilinx axi usb2 device driver support
>>>
>>> Signed-off-by: Subbaraya Sundeep Bhatta
>>> ---
>>> .../devicetree/bindings/usb/xilinx_usb.txt | 21 +
>>> drivers/usb/gadget/Kconfig | 14 +
>>> drivers/usb/gadget/Makefile|1 +
>>> drivers/usb/gadget/xilinx_udc.c| 2045
>>>
>>> 4 files changed, 2081 insertions(+), 0 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/usb/xilinx_usb.txt
>>> create mode 100644 drivers/usb/gadget/xilinx_udc.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/xilinx_usb.txt
>>> b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
>>> new file mode 100644
>>> index 000..acf03ab
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
>>> @@ -0,0 +1,21 @@
>>> +Xilinx AXI USB2 device controller
>>> +
>>> +Required properties:
>>> +- compatible : Should be "xlnx,axi-usb2-device-4.00.a"
>
> Is "axi-usb2-device" the official device name?
It is the name of IP which Xilinx have and we are using names in this format.
>>> +- reg : Physical base address and size of the Axi USB2
>>> + device registers map.
>>> +- interrupts : Property with a value describing the interrupt
>>> + number.
>
> Does the device only have a single interrupt?
I believe so.
>>> +- interrupt-parent : Must be core interrupt controller
>
> Is this strictly necessary?
I am not sure what do you mean by that. If you mean that interrupt-parent
can be written to the root of DTS file then we can setup system with more
interrupt controllers that's why it is required.
If we can point to standard interrupt description then please point me to
exact description you would like to see here and we can change it.
>>> +- xlnx,include-dma : Must be 1 or 0 based on whether DMA is included
>>> + in IP or not.
>
> Perhaps xlnx,has-builtin-dma would better describe this?
No opinion.
>>> +
>>> +Example:
>>> + axi-usb2-device@42e0 {
>>> +compatible = "xlnx,axi-usb2-device-4.00.a";
>>> +interrupt-parent = <0x1>;
>>> +interrupts = <0x0 0x39 0x1>;
>>> +reg = <0x42e0 0x1>;
>>> +xlnx,include-dma = <0x1>;
>>> +};
>>> +
>>
>> you need to Cc devicet...@vger.kernel.org for this.
>
> Cheers Filipe; thanks for adding us to Cc :)
>
Sundeep with CC devicetree list in next patch version.
>>> + /* Map the registers */
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + udc->base_address = devm_ioremap_nocache(&pdev->dev, res->start,
>>> +resource_size(res));
>>
>> use devm_ioremap_resource() instead.
>
> Also, res might be NULL. You should check that before dereferencing it.
yes it is necessary for both cases with devm_ioremap_nocache
or with devm_ioremap_resource.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
signature.asc
Description: OpenPGP digital signature
Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
Hi, >> + * Copyright (C) 2010 - 2014 Xilinx, Inc. >> + * >> + * Some parts of this driver code is based on the driver for at91-series >> + * USB peripheral controller (at91_udc.c). >> + * >> + * This program is free software; you can redistribute it >> + * and/or modify it under the terms of the GNU General Public >> + * License as published by the Free Software Foundation; >> + * either version 2 of the License, or (at your option) any >> + * later version. > > are you sure you want to allow people ot use GPL v3 on this driver ? The license is the same as is in at91_udc.c driver. From my understanding if this driver is based on that one we have to follow license from it. And this is problem in general with all licenses in the kernel which use this fragment. I don't think we can change it to be just GPLv2. Please correct me if I am wrong. BTW: u-boot started to use SPDX-License-Identifier which will be nice to start to use. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform signature.asc Description: OpenPGP digital signature
