Message-ID: <20240315010142.qmgx7dugwwb2ort2@synopsys.com>
Date: Fri, 15 Mar 2024 01:01:48 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Michal Simek <michal.simek@....com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "Pandey, Radhey Shyam" <radhey.shyam.pandey@....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

On Thu, Mar 07, 2024, Michal Simek wrote:
> 
> 
> On 3/7/24 02:44, Thinh Nguyen wrote:
> > 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?
> 
> Not sure I fully understand what you mean by more predictable.
> Are you asking for simple read/write interface to dwc3 core IP?
> Do you want there any limitation for accesses to be able to control it?
> 
> I don't think we have any issue with your suggestion but it is unclear how
> exactly it should look like.
> Can you please sketch it?
> 

Hi,

Sorry, my suggestion would require a handler for the glue driver and the
core. But the various implementations in different glue drivers are
handled in such a way that won't be simple to pass this handle around.
This can get hairy quickly...

Let's scratch what I suggested.

Instead, perhaps we can do it as following:
 * Keep the setting of the controller registers in the core
 * Create a software_node to pass a software property to the core

These software properties will not be documented in the devicetree
binding. Just document them in the driver core header. They are simply
driver properties that get passed through software node.

You can add the software node using device_add_software_node(). This can
be done before calling of_platform_populate() in dwc3-xilinx (can be
done in pltfm_init())

Let me know if this works for you.

Thanks,
Thinh

Powered by blists - more mailing lists