linux-kernel - Re: [PATCH v2] usb: dwc3: core: enable CCI support for AMD-xilinx DWC3 controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240307014402.omtcsltp4pxpraf7@synopsys.com>
Date: Thu, 7 Mar 2024 01:44:09 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: "Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "git (AMD-Xilinx)" <git@....com>
Subject: Re: [PATCH v2] usb: dwc3: core: enable CCI support for AMD-xilinx
 DWC3 controller

Hi,

On Tue, Feb 27, 2024, Pandey, Radhey Shyam wrote:
> > -----Original Message-----
> > From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
> > Sent: Saturday, February 24, 2024 4:38 AM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@....com>
> > Cc: gregkh@...uxfoundation.org; linux-usb@...r.kernel.org; linux-
> > kernel@...r.kernel.org; git (AMD-Xilinx) <git@....com>
> > Subject: Re: [PATCH v2] usb: dwc3: core: enable CCI support for AMD-xilinx
> > DWC3 controller
> > 
> > On Fri, Feb 23, 2024, Thinh Nguyen wrote:
> > > On Sat, Feb 24, 2024, Radhey Shyam Pandey wrote:
> > > > From: Piyush Mehta <piyush.mehta@....com>
> > > >
> > > > The GSBUSCFG0 register bits [31:16] are used to configure the cache type
> > > > settings of the descriptor and data write/read transfers (Cacheable,
> > > > Bufferable/ Posted). When CCI is enabled in the design, DWC3 core
> > GSBUSCFG0
> > > > cache bits must be updated to support CCI enabled transfers in USB.
> > > >
> > > > Signed-off-by: Piyush Mehta <piyush.mehta@....com>
> > > > Signed-off-by: Radhey Shyam Pandey
> > <radhey.shyam.pandey@....com>
> > > > ----
> > > > changes for v2:
> > > > Make GSBUSCFG0 configuration specific to AMD-xilinx platform.
> > > > Taken reference from existing commit ec5eb43813a4 ("usb: dwc3: core:
> > > > add support for realtek SoCs custom's global register start address")
> > 
> > Regarding that change from Realtek, it's a special case. I want to avoid
> > doing platform specific checks in the core.c if possible. Eventually, I
> > want to move that logic from Realtek to its glue driver.
> > 
> > BR,
> > Thinh
> Thanks. As you suggested I tried "temporarily memory map and update this 
> register in your Xilinx glue driver. Its value should retain after soft reset".
> 
> Did ioremap for core register space once again in glue driver but it resulted
> in below error:
> dwc3 fe200000.usb: can't request region for resource [mem 0xfe200000-0xfe23ffff]
> dwc3-xilinx ff9d0000.usb: error -EBUSY: failed to map DWC3 registers
> 
> So to avoid remapping, now get the struct dwc3 platform data handle in glue
> driver and pass it to dwc3_readl/writel() like the below sequence. Is that fine? 
> If yes I will respin v3 with these changes and also do some more 
> sanity tests.
> 
> drivers/usb/dwc3/dwc3-xilinx.c
> #include "io.h"
> 
> <snip>
> ret = of_platform_populate(np, NULL, NULL, dev);
> if (ret) {
> 	dev_err(dev, "failed to register dwc3 core - %d\n", ret);
>     goto err_clk_put;
> }
> 
> dwc3_np = of_get_compatible_child(np, "snps,dwc3");
> priv_data->dwc3 = of_find_device_by_node(dwc3_np);
> dwc = platform_get_drvdata(priv_data->dwc3);
> if (of_dma_is_coherent(dev->of_node)) {
> 	reg = dwc3_readl(dwc->regs , DWC3_GSBUSCFG0);
> 	reg |= DWC3_GSBUSCFG0_DATRDREQINFO_MASK |
>               DWC3_GSBUSCFG0_DESRDREQINFO_MASK |
>               DWC3_GSBUSCFG0_DATWRREQINFO_MASK |
>               DWC3_GSBUSCFG0_DESWRREQINFO_MASK;

It's a bit odd to use "mask" as value instead of some defined
macros/values.

> 	dwc3_writel(dwc->regs , DWC3_GSBUSCFG0, reg);
> }
> 

Perhaps it may be better to add a new interface for the core to interact
with the glue drivers. The core can use these callbacks to properly set
platform specific configuration at specified sequence of the controller
initialization. It will be better defined and more predictable than what
we have here. What do you think?

Thanks,
Thinh