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

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

[Radhey Shyam Pandey]

From: Piyush Mehta <piyush.mehta@amd.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@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.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")

v1 link:
https://lore.kernel.org/all/20231013053448.11056-1-piyush.mehta@amd.com
---
 drivers/usb/dwc3/core.c | 26 ++++++++++++++++++++++++++
 drivers/usb/dwc3/core.h |  5 +++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 3e55838c0001..3acd4ab3fcca 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_graph.h>
 #include <linux/acpi.h>
 #include <linux/pinctrl/consumer.h>
@@ -559,6 +560,29 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
 		parms->hwparams9 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS9);
 }
 
+static void dwc3_config_soc_bus(struct dwc3 *dwc)
+{
+	if (dwc->dev->of_node) {
+		struct device_node *parent = of_get_parent(dwc->dev->of_node);
+
+		if (of_device_is_compatible(parent, "xlnx,zynqmp-dwc3") ||
+		    of_device_is_compatible(parent, "xlnx,versal-dwc3")) {
+			if (of_dma_is_coherent(dwc->dev->of_node)) {
+				u32 reg;
+
+				reg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
+				reg |= DWC3_GSBUSCFG0_DATRDREQINFO_MASK |
+					DWC3_GSBUSCFG0_DESRDREQINFO_MASK |
+					DWC3_GSBUSCFG0_DATWRREQINFO_MASK |
+					DWC3_GSBUSCFG0_DESWRREQINFO_MASK;
+				dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, reg);
+			}
+		}
+
+		of_node_put(parent);
+	}
+}
+
 static int dwc3_core_ulpi_init(struct dwc3 *dwc)
 {
 	int intf;
@@ -1256,6 +1280,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
 
 	dwc3_set_incr_burst_type(dwc);
 
+	dwc3_config_soc_bus(dwc);
+
 	ret = dwc3_phy_power_on(dwc);
 	if (ret)
 		goto err_exit_phy;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 25dc0599345e..bf19a20e240f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -175,6 +175,11 @@
 #define DWC3_LLUCTL		0xd024
 
 /* Bit fields */
+/* Global SoC Bus Configuration Register: AHB-prot/AXI-cache/OCP-ReqInfo */
+#define DWC3_GSBUSCFG0_DATRDREQINFO_MASK	GENMASK(31, 28)
+#define DWC3_GSBUSCFG0_DESRDREQINFO_MASK	GENMASK(27, 24)
+#define DWC3_GSBUSCFG0_DATWRREQINFO_MASK	GENMASK(23, 20)
+#define DWC3_GSBUSCFG0_DESWRREQINFO_MASK	GENMASK(19, 16)
 
 /* Global SoC Bus Configuration INCRx Register 0 */
 #define DWC3_GSBUSCFG0_INCR256BRSTENA	(1 << 7) /* INCR256 burst */
-- 
2.34.1

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

[Thinh Nguyen]

On Sat, Feb 24, 2024, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <piyush.mehta@amd.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@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.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")
> 
> v1 link:
> https://urldefense.com/v3/__https://lore.kernel.org/all/20231013053448.11056-1-piyush.mehta@amd.com__;!!A4F2R9G_pg!ffADlNJmRyCb1C2B0z21-8AbJE4YDyiXNxKBKBqmhmBOSSZBokHS_qyetGEqb7Cwawe0Wvbz2aqSZz_bTfvS0cXdwXLVWw$ 

Please check the comment from the previous thread:
https://lore.kernel.org/linux-usb/20240223224744.vptvfkqzgqv24ptz@synopsys.com/T/#t

Thanks,
Thinh

> ---
>  drivers/usb/dwc3/core.c | 26 ++++++++++++++++++++++++++
>  drivers/usb/dwc3/core.h |  5 +++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 3e55838c0001..3acd4ab3fcca 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_graph.h>
>  #include <linux/acpi.h>
>  #include <linux/pinctrl/consumer.h>
> @@ -559,6 +560,29 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
>  		parms->hwparams9 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS9);
>  }
>  
> +static void dwc3_config_soc_bus(struct dwc3 *dwc)
> +{
> +	if (dwc->dev->of_node) {
> +		struct device_node *parent = of_get_parent(dwc->dev->of_node);
> +
> +		if (of_device_is_compatible(parent, "xlnx,zynqmp-dwc3") ||
> +		    of_device_is_compatible(parent, "xlnx,versal-dwc3")) {
> +			if (of_dma_is_coherent(dwc->dev->of_node)) {
> +				u32 reg;
> +
> +				reg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> +				reg |= DWC3_GSBUSCFG0_DATRDREQINFO_MASK |
> +					DWC3_GSBUSCFG0_DESRDREQINFO_MASK |
> +					DWC3_GSBUSCFG0_DATWRREQINFO_MASK |
> +					DWC3_GSBUSCFG0_DESWRREQINFO_MASK;
> +				dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, reg);
> +			}
> +		}
> +
> +		of_node_put(parent);
> +	}
> +}
> +
>  static int dwc3_core_ulpi_init(struct dwc3 *dwc)
>  {
>  	int intf;
> @@ -1256,6 +1280,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  
>  	dwc3_set_incr_burst_type(dwc);
>  
> +	dwc3_config_soc_bus(dwc);
> +
>  	ret = dwc3_phy_power_on(dwc);
>  	if (ret)
>  		goto err_exit_phy;
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 25dc0599345e..bf19a20e240f 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -175,6 +175,11 @@
>  #define DWC3_LLUCTL		0xd024
>  
>  /* Bit fields */
> +/* Global SoC Bus Configuration Register: AHB-prot/AXI-cache/OCP-ReqInfo */
> +#define DWC3_GSBUSCFG0_DATRDREQINFO_MASK	GENMASK(31, 28)
> +#define DWC3_GSBUSCFG0_DESRDREQINFO_MASK	GENMASK(27, 24)
> +#define DWC3_GSBUSCFG0_DATWRREQINFO_MASK	GENMASK(23, 20)
> +#define DWC3_GSBUSCFG0_DESWRREQINFO_MASK	GENMASK(19, 16)
>  
>  /* Global SoC Bus Configuration INCRx Register 0 */
>  #define DWC3_GSBUSCFG0_INCR256BRSTENA	(1 << 7) /* INCR256 burst */
> -- 
> 2.34.1
> 

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

[Thinh Nguyen]

On Fri, Feb 23, 2024, Thinh Nguyen wrote:
> On Sat, Feb 24, 2024, Radhey Shyam Pandey wrote:
> > From: Piyush Mehta <piyush.mehta@amd.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@amd.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.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

> > 
> > v1 link:
> > https://urldefense.com/v3/__https://lore.kernel.org/all/20231013053448.11056-1-piyush.mehta@amd.com__;!!A4F2R9G_pg!ffADlNJmRyCb1C2B0z21-8AbJE4YDyiXNxKBKBqmhmBOSSZBokHS_qyetGEqb7Cwawe0Wvbz2aqSZz_bTfvS0cXdwXLVWw$ 
> 
> Please check the comment from the previous thread:
> https://lore.kernel.org/linux-usb/20240223224744.vptvfkqzgqv24ptz@synopsys.com/T/#t
> 

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

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Sent: Saturday, February 24, 2024 4:38 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.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@amd.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@amd.com>
> > > Signed-off-by: Radhey Shyam Pandey
> <radhey.shyam.pandey@amd.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;
	dwc3_writel(dwc->regs , DWC3_GSBUSCFG0, reg);
}

Thanks,
Radhey

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

[Thinh Nguyen]

Hi,

On Tue, Feb 27, 2024, Pandey, Radhey Shyam wrote:
> > -----Original Message-----
> > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > Sent: Saturday, February 24, 2024 4:38 AM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
> > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.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@amd.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@amd.com>
> > > > Signed-off-by: Radhey Shyam Pandey
> > <radhey.shyam.pandey@amd.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

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

[Michal Simek]

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@synopsys.com>
>>> Sent: Saturday, February 24, 2024 4:38 AM
>>> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
>>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.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@amd.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@amd.com>
>>>>> Signed-off-by: Radhey Shyam Pandey
>>> <radhey.shyam.pandey@amd.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?

Thanks,
Michal

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

[Thinh Nguyen]

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@synopsys.com>
> > > > Sent: Saturday, February 24, 2024 4:38 AM
> > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.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@amd.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@amd.com>
> > > > > > Signed-off-by: Radhey Shyam Pandey
> > > > <radhey.shyam.pandey@amd.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

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

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Sent: Friday, March 15, 2024 6:32 AM
> To: Simek, Michal <michal.simek@amd.com>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Pandey, Radhey Shyam
> <radhey.shyam.pandey@amd.com>; gregkh@linuxfoundation.org; linux-
> usb@vger.kernel.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.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@synopsys.com>
> > > > > Sent: Saturday, February 24, 2024 4:38 AM
> > > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
> > > > > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.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@amd.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@amd.com>
> > > > > > > Signed-off-by: Radhey Shyam Pandey
> > > > > <radhey.shyam.pandey@amd.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
Thanks. By software property you mean flags or caps that can be passed
glue drivers to dwc3 core driver ?

dwc3_set_quirks(struct dwc3 *dwc, u64 flags);

Defines quirks in core.h

DWC3_FLAGS_COMMON
DWC3_XLNX_CCI
DWC3_XLNX_IPD
DWC3_REALTEK_RES_FIX

Then based on these quirks/flags program it in core.c.
Is this approach fine and aligned with your thoughts?

Thanks,
Radhey

> 
> 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

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

[Thinh Nguyen]

+devicetree maintainers

On Mon, Mar 18, 2024, Pandey, Radhey Shyam wrote:
> > 
> > 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
> Thanks. By software property you mean flags or caps that can be passed
> glue drivers to dwc3 core driver ?
> 
> dwc3_set_quirks(struct dwc3 *dwc, u64 flags);
> 
> Defines quirks in core.h
> 
> DWC3_FLAGS_COMMON
> DWC3_XLNX_CCI
> DWC3_XLNX_IPD
> DWC3_REALTEK_RES_FIX
> 
> Then based on these quirks/flags program it in core.c.
> Is this approach fine and aligned with your thoughts?
> 

Not as a flag. Create 4 properties for GSBUSCFG0.DATRDREQINFO,
DESRDREQINFO, DATWRREQINFO, and DESWRREQINFO in your glue driver. Pass
them through your software node as PROPERTY_ENTRY_U16. The core will
override the default coreConsultant value of GSBUSCFG0 based on these
properties in dwc3_get_properties().

Check drivers/usb/dwc3/host.c for reference.

> 
> > 
> > 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.
> > 

Hi Rob/Krzysztof,

Just want to check in with you for your opinion. To summarize my
suggestion to Pandey, here are the key notes:
* Platform specific settings are set in glue drivers (match through
  compatible string)
* These settings are set by controller registers that should only
  be accessible in the dwc3 core
* So, the suggestion is to pass these settings as properties using
  software_node created from the glue driver to the dwc3 core
* These properties will not be documented in the devicetree binding, but
  only in the driver

We're already doing that to some properties such as
"linux,sysdev_is_parent"

If this suggestion makes sense, would the prefix "linux," for linux
specific binding or "snps," is a better fit?

Thanks,
Thinh

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

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Sent: Wednesday, March 20, 2024 6:18 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Krzysztof
> Kozlowski <krzysztof.kozlowski@linaro.org>; Rob Herring
> <robh+dt@kernel.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Simek, Michal
> <michal.simek@amd.com>; devicetree@vger.kernel.org;
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v2] usb: dwc3: core: enable CCI support for AMD-xilinx
> DWC3 controller
> 
> +devicetree maintainers
> 
> On Mon, Mar 18, 2024, Pandey, Radhey Shyam wrote:
> > >
> > > 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
> > Thanks. By software property you mean flags or caps that can be passed
> > glue drivers to dwc3 core driver ?
> >
> > dwc3_set_quirks(struct dwc3 *dwc, u64 flags);
> >
> > Defines quirks in core.h
> >
> > DWC3_FLAGS_COMMON
> > DWC3_XLNX_CCI
> > DWC3_XLNX_IPD
> > DWC3_REALTEK_RES_FIX
> >
> > Then based on these quirks/flags program it in core.c.
> > Is this approach fine and aligned with your thoughts?
> >
> 
> Not as a flag. Create 4 properties for GSBUSCFG0.DATRDREQINFO,
> DESRDREQINFO, DATWRREQINFO, and DESWRREQINFO in your glue driver.
> Pass them through your software node as PROPERTY_ENTRY_U16. The core
> will override the default coreConsultant value of GSBUSCFG0 based on these
> properties in dwc3_get_properties().

Thanks . It is clear to me now.
> 
> Check drivers/usb/dwc3/host.c for reference.
> 
> >
> > >
> > > 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.
> > >
> 
> Hi Rob/Krzysztof,
> 
> Just want to check in with you for your opinion. To summarize my suggestion
> to Pandey, here are the key notes:
> * Platform specific settings are set in glue drivers (match through
>   compatible string)
> * These settings are set by controller registers that should only
>   be accessible in the dwc3 core
> * So, the suggestion is to pass these settings as properties using
>   software_node created from the glue driver to the dwc3 core
> * These properties will not be documented in the devicetree binding, but
>   only in the driver
> 
> We're already doing that to some properties such as
> "linux,sysdev_is_parent"
> 
> If this suggestion makes sense, would the prefix "linux," for linux specific
> binding or "snps," is a better fit?

Rob/Krzysztof: Please let me know your thoughts on Thinh's suggestion?
Based on it I can work and submit the next version.

Thanks,
Radhey