RE: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-09-22 Thread Pawel Laszczak
>
>
>On 20-09-22 13:06:26, Pawel Laszczak wrote:
>> Hi,
>>
>> >
>> >On Mon, Jun 29, 2020 at 03:41:49AM +, Peter Chen wrote:
>> >> On 20-06-26 07:19:56, Pawel Laszczak wrote:
>> >> > Hi Felipe,
>> >> >
>> >> > >
>> >> > >Hi,
>> >> > >
>> >> > >Pawel Laszczak  writes:
>> >> > >> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>> >> > >>
>> >> > >> The Cadence USBSS DRD Controller is a highly configurable IP Core 
>> >> > >> which
>> >> > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
>> >> > >> Host Only (XHCI)configurations.
>> >> > >>
>> >> > >> The current driver has been validated with FPGA burned. We have 
>> >> > >> support
>> >> > >> for PCIe bus, which is used on FPGA prototyping.
>> >> > >>
>> >> > >> The host side of USBSS-DRD controller is compliance with XHCI
>> >> > >> specification, so it works with standard XHCI Linux driver.
>> >> > >>
>> >> > >> The host side of USBSS DRD controller is compliant with XHCI.
>> >> > >> The architecture for device side is almost the same as for host side,
>> >> > >> and most of the XHCI specification can be used to understand how
>> >> > >> this controller operates.
>> >> > >>
>> >> > >> This controller and driver support Full Speed, Hight Speed, Supper 
>> >> > >> Speed
>> >> > >> and Supper Speed Plus USB protocol.
>> >> > >>
>> >> > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
>> >> > >> The last letter of this acronym means PLUS. The formal name of 
>> >> > >> controller
>> >> > >> is USBSSP but it's to generic so I've decided to use CDNSP.
>> >> > >>
>> >> > >> The patch 1: adds DT binding.
>> >> > >> The patch 2: adds PCI to platform wrapper used on Cadnece testing
>> >> > >>  platform. It is FPGA based on platform.
>> >> > >> The patches 3-5: add the main part of driver and has been 
>> >> > >> intentionally
>> >> > >>  split into 3 part. In my opinion such division should 
>> >> > >> not
>> >> > >>  affect understanding and reviewing the driver, and 
>> >> > >> cause that
>> >> > >>  main patch (4/5) is little smaller. Patch 3 introduces 
>> >> > >> main
>> >> > >>  header file for driver, 4 is the main part that 
>> >> > >> implements all
>> >> > >>  functionality of driver and 5 introduces tracepoints.
>> >> > >
>> >> > >I'm more interested in how is this different from CDNS3. Aren't they 
>> >> > >SW compatible?
>> >> >
>> >> > In general, the controller can be split into 2 part- DRD part and the 
>> >> > rest UDC.
>> >> >
>> >> > The second part UDC which consist gadget.c, ring.c and mem.c file is 
>> >> > completely different.
>> >> >
>> >> > The DRD part contains drd.c and core.c.
>> >> > cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP 
>> >> > has similar, but has different register space.
>> >> > Some register was moved, some was removed and some was added.
>> >> >
>> >> > core.c is very similar and eventually could be common for both drivers. 
>> >> >  I thought about this but
>> >> > I wanted to avoid interfering with cdns3 driver at this point CDNSP is 
>> >> > still under testing and
>> >> > CDNS3 is used by some products on the market.
>> >>
>> >> Pawel, I suggest adding CDNSP at driver/staging first since it is still
>> >> under testing. When you are thinking the driver (as well as hardware) are
>> >> mature, you could try to add gadget part (eg, gadget-v2) and make
>> >> necessary changes for core.c.
>> >
>> >I only take code for drivers/staging/ that for some reason is not
>> >meeting the normal coding style/rules/whatever.  For stuff that is an
>> >obvious duplicate of existing code like this, and needs to be
>> >rearchitected.  It is much more work to try to convert code once it is
>> >in the tree than to just do it out of the tree on your own and resubmit
>> >it, as you don't have to follow the in-kernel rules of "one patch does
>> >one thing" that you would if it was in staging.
>> >
>> >So don't think that staging is the right place for this, just spend a
>> >few weeks to get it right and then resubmit it.
>> >
>>
>> I had idea to reuse indirect the core.c and drd.c in cdnsp driver. Of 
>> course, I've made
>> the necessary changes to make possible reuse this code.
>> My approach was to add this file in Makefile in cdnsp but this concept 
>> failed.
>> It even worked until I started testing cdns3 and cdnsp as build in kernel :)
>>
>> With this approach I have issue with " multiple definition of .. "
>>
>> How should it look like such reusable code ?
>>
>> After my experience with above concept I think that only way is to move 
>> common code
>> to separate module,  similar as it is in drivers/usb/common directory or 
>> libcomposite.ko module.
>>
>
>Could you use compatible string or IP revision number to dynamic judge
>which part of code you should use? That is to say there is only one
>Cadence 3 USB driver folder -- cdns3, you only add one gadget file for
>cdnsp re

Re: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-09-22 Thread Peter Chen
On 20-09-22 13:06:26, Pawel Laszczak wrote:
> Hi,
> 
> >
> >On Mon, Jun 29, 2020 at 03:41:49AM +, Peter Chen wrote:
> >> On 20-06-26 07:19:56, Pawel Laszczak wrote:
> >> > Hi Felipe,
> >> >
> >> > >
> >> > >Hi,
> >> > >
> >> > >Pawel Laszczak  writes:
> >> > >> This patch introduce new Cadence USBSS DRD driver to linux kernel.
> >> > >>
> >> > >> The Cadence USBSS DRD Controller is a highly configurable IP Core 
> >> > >> which
> >> > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
> >> > >> Host Only (XHCI)configurations.
> >> > >>
> >> > >> The current driver has been validated with FPGA burned. We have 
> >> > >> support
> >> > >> for PCIe bus, which is used on FPGA prototyping.
> >> > >>
> >> > >> The host side of USBSS-DRD controller is compliance with XHCI
> >> > >> specification, so it works with standard XHCI Linux driver.
> >> > >>
> >> > >> The host side of USBSS DRD controller is compliant with XHCI.
> >> > >> The architecture for device side is almost the same as for host side,
> >> > >> and most of the XHCI specification can be used to understand how
> >> > >> this controller operates.
> >> > >>
> >> > >> This controller and driver support Full Speed, Hight Speed, Supper 
> >> > >> Speed
> >> > >> and Supper Speed Plus USB protocol.
> >> > >>
> >> > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> >> > >> The last letter of this acronym means PLUS. The formal name of 
> >> > >> controller
> >> > >> is USBSSP but it's to generic so I've decided to use CDNSP.
> >> > >>
> >> > >> The patch 1: adds DT binding.
> >> > >> The patch 2: adds PCI to platform wrapper used on Cadnece testing
> >> > >>  platform. It is FPGA based on platform.
> >> > >> The patches 3-5: add the main part of driver and has been 
> >> > >> intentionally
> >> > >>  split into 3 part. In my opinion such division should not
> >> > >>  affect understanding and reviewing the driver, and cause 
> >> > >> that
> >> > >>  main patch (4/5) is little smaller. Patch 3 introduces 
> >> > >> main
> >> > >>  header file for driver, 4 is the main part that 
> >> > >> implements all
> >> > >>  functionality of driver and 5 introduces tracepoints.
> >> > >
> >> > >I'm more interested in how is this different from CDNS3. Aren't they SW 
> >> > >compatible?
> >> >
> >> > In general, the controller can be split into 2 part- DRD part and the 
> >> > rest UDC.
> >> >
> >> > The second part UDC which consist gadget.c, ring.c and mem.c file is 
> >> > completely different.
> >> >
> >> > The DRD part contains drd.c and core.c.
> >> > cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP 
> >> > has similar, but has different register space.
> >> > Some register was moved, some was removed and some was added.
> >> >
> >> > core.c is very similar and eventually could be common for both drivers.  
> >> > I thought about this but
> >> > I wanted to avoid interfering with cdns3 driver at this point CDNSP is 
> >> > still under testing and
> >> > CDNS3 is used by some products on the market.
> >>
> >> Pawel, I suggest adding CDNSP at driver/staging first since it is still
> >> under testing. When you are thinking the driver (as well as hardware) are
> >> mature, you could try to add gadget part (eg, gadget-v2) and make
> >> necessary changes for core.c.
> >
> >I only take code for drivers/staging/ that for some reason is not
> >meeting the normal coding style/rules/whatever.  For stuff that is an
> >obvious duplicate of existing code like this, and needs to be
> >rearchitected.  It is much more work to try to convert code once it is
> >in the tree than to just do it out of the tree on your own and resubmit
> >it, as you don't have to follow the in-kernel rules of "one patch does
> >one thing" that you would if it was in staging.
> >
> >So don't think that staging is the right place for this, just spend a
> >few weeks to get it right and then resubmit it.
> >
> 
> I had idea to reuse indirect the core.c and drd.c in cdnsp driver. Of course, 
> I've made
> the necessary changes to make possible reuse this code.
> My approach was to add this file in Makefile in cdnsp but this concept 
> failed. 
> It even worked until I started testing cdns3 and cdnsp as build in kernel :)
> 
> With this approach I have issue with " multiple definition of .. "
> 
> How should it look like such reusable code ?
> 
> After my experience with above concept I think that only way is to move 
> common code
> to separate module,  similar as it is in drivers/usb/common directory or 
> libcomposite.ko module.
> 

Could you use compatible string or IP revision number to dynamic judge
which part of code you should use? That is to say there is only one
Cadence 3 USB driver folder -- cdns3, you only add one gadget file for
cdnsp revision?

-- 

Thanks,
Peter Chen

RE: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-09-22 Thread Pawel Laszczak
Hi,

>
>On Mon, Jun 29, 2020 at 03:41:49AM +, Peter Chen wrote:
>> On 20-06-26 07:19:56, Pawel Laszczak wrote:
>> > Hi Felipe,
>> >
>> > >
>> > >Hi,
>> > >
>> > >Pawel Laszczak  writes:
>> > >> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>> > >>
>> > >> The Cadence USBSS DRD Controller is a highly configurable IP Core which
>> > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
>> > >> Host Only (XHCI)configurations.
>> > >>
>> > >> The current driver has been validated with FPGA burned. We have support
>> > >> for PCIe bus, which is used on FPGA prototyping.
>> > >>
>> > >> The host side of USBSS-DRD controller is compliance with XHCI
>> > >> specification, so it works with standard XHCI Linux driver.
>> > >>
>> > >> The host side of USBSS DRD controller is compliant with XHCI.
>> > >> The architecture for device side is almost the same as for host side,
>> > >> and most of the XHCI specification can be used to understand how
>> > >> this controller operates.
>> > >>
>> > >> This controller and driver support Full Speed, Hight Speed, Supper Speed
>> > >> and Supper Speed Plus USB protocol.
>> > >>
>> > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
>> > >> The last letter of this acronym means PLUS. The formal name of 
>> > >> controller
>> > >> is USBSSP but it's to generic so I've decided to use CDNSP.
>> > >>
>> > >> The patch 1: adds DT binding.
>> > >> The patch 2: adds PCI to platform wrapper used on Cadnece testing
>> > >>  platform. It is FPGA based on platform.
>> > >> The patches 3-5: add the main part of driver and has been intentionally
>> > >>  split into 3 part. In my opinion such division should not
>> > >>  affect understanding and reviewing the driver, and cause 
>> > >> that
>> > >>  main patch (4/5) is little smaller. Patch 3 introduces main
>> > >>  header file for driver, 4 is the main part that implements 
>> > >> all
>> > >>  functionality of driver and 5 introduces tracepoints.
>> > >
>> > >I'm more interested in how is this different from CDNS3. Aren't they SW 
>> > >compatible?
>> >
>> > In general, the controller can be split into 2 part- DRD part and the rest 
>> > UDC.
>> >
>> > The second part UDC which consist gadget.c, ring.c and mem.c file is 
>> > completely different.
>> >
>> > The DRD part contains drd.c and core.c.
>> > cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP has 
>> > similar, but has different register space.
>> > Some register was moved, some was removed and some was added.
>> >
>> > core.c is very similar and eventually could be common for both drivers.  I 
>> > thought about this but
>> > I wanted to avoid interfering with cdns3 driver at this point CDNSP is 
>> > still under testing and
>> > CDNS3 is used by some products on the market.
>>
>> Pawel, I suggest adding CDNSP at driver/staging first since it is still
>> under testing. When you are thinking the driver (as well as hardware) are
>> mature, you could try to add gadget part (eg, gadget-v2) and make
>> necessary changes for core.c.
>
>I only take code for drivers/staging/ that for some reason is not
>meeting the normal coding style/rules/whatever.  For stuff that is an
>obvious duplicate of existing code like this, and needs to be
>rearchitected.  It is much more work to try to convert code once it is
>in the tree than to just do it out of the tree on your own and resubmit
>it, as you don't have to follow the in-kernel rules of "one patch does
>one thing" that you would if it was in staging.
>
>So don't think that staging is the right place for this, just spend a
>few weeks to get it right and then resubmit it.
>

I had idea to reuse indirect the core.c and drd.c in cdnsp driver. Of course, 
I've made
the necessary changes to make possible reuse this code.
My approach was to add this file in Makefile in cdnsp but this concept failed. 
It even worked until I started testing cdns3 and cdnsp as build in kernel :)

With this approach I have issue with " multiple definition of .. "

How should it look like such reusable code ?

After my experience with above concept I think that only way is to move common 
code
to separate module,  similar as it is in drivers/usb/common directory or 
libcomposite.ko module.

Am I right that it's the only correct way ?

Regards,
Pawel


Re: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-06-29 Thread gre...@linuxfoundation.org
On Mon, Jun 29, 2020 at 03:41:49AM +, Peter Chen wrote:
> On 20-06-26 07:19:56, Pawel Laszczak wrote:
> > Hi Felipe,
> > 
> > >
> > >Hi,
> > >
> > >Pawel Laszczak  writes:
> > >> This patch introduce new Cadence USBSS DRD driver to linux kernel.
> > >>
> > >> The Cadence USBSS DRD Controller is a highly configurable IP Core which
> > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
> > >> Host Only (XHCI)configurations.
> > >>
> > >> The current driver has been validated with FPGA burned. We have support
> > >> for PCIe bus, which is used on FPGA prototyping.
> > >>
> > >> The host side of USBSS-DRD controller is compliance with XHCI
> > >> specification, so it works with standard XHCI Linux driver.
> > >>
> > >> The host side of USBSS DRD controller is compliant with XHCI.
> > >> The architecture for device side is almost the same as for host side,
> > >> and most of the XHCI specification can be used to understand how
> > >> this controller operates.
> > >>
> > >> This controller and driver support Full Speed, Hight Speed, Supper Speed
> > >> and Supper Speed Plus USB protocol.
> > >>
> > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> > >> The last letter of this acronym means PLUS. The formal name of controller
> > >> is USBSSP but it's to generic so I've decided to use CDNSP.
> > >>
> > >> The patch 1: adds DT binding.
> > >> The patch 2: adds PCI to platform wrapper used on Cadnece testing
> > >>  platform. It is FPGA based on platform.
> > >> The patches 3-5: add the main part of driver and has been intentionally
> > >>  split into 3 part. In my opinion such division should not
> > >>  affect understanding and reviewing the driver, and cause 
> > >> that
> > >>  main patch (4/5) is little smaller. Patch 3 introduces main
> > >>  header file for driver, 4 is the main part that implements 
> > >> all
> > >>  functionality of driver and 5 introduces tracepoints.
> > >
> > >I'm more interested in how is this different from CDNS3. Aren't they SW 
> > >compatible?
> > 
> > In general, the controller can be split into 2 part- DRD part and the rest 
> > UDC. 
> > 
> > The second part UDC which consist gadget.c, ring.c and mem.c file is 
> > completely different. 
> > 
> > The DRD part contains drd.c and core.c. 
> > cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP has 
> > similar, but has different register space.
> > Some register was moved, some was removed and some was added.  
> > 
> > core.c is very similar and eventually could be common for both drivers.  I 
> > thought about this but
> > I wanted to avoid interfering with cdns3 driver at this point CDNSP is 
> > still under testing and 
> > CDNS3 is used by some products on the market. 
> 
> Pawel, I suggest adding CDNSP at driver/staging first since it is still
> under testing. When you are thinking the driver (as well as hardware) are
> mature, you could try to add gadget part (eg, gadget-v2) and make
> necessary changes for core.c.

I only take code for drivers/staging/ that for some reason is not
meeting the normal coding style/rules/whatever.  For stuff that is an
obvious duplicate of existing code like this, and needs to be
rearchitected.  It is much more work to try to convert code once it is
in the tree than to just do it out of the tree on your own and resubmit
it, as you don't have to follow the in-kernel rules of "one patch does
one thing" that you would if it was in staging.

So don't think that staging is the right place for this, just spend a
few weeks to get it right and then resubmit it.

thanks,

greg k-h


RE: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-06-29 Thread Pawel Laszczak


>> > Hi Felipe,
>> >
>> > >
>> > >Hi,
>> > >
>> > >Pawel Laszczak  writes:
>> > >> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>> > >>
>> > >> The Cadence USBSS DRD Controller is a highly configurable IP Core which
>> > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
>> > >> Host Only (XHCI)configurations.
>> > >>
>> > >> The current driver has been validated with FPGA burned. We have support
>> > >> for PCIe bus, which is used on FPGA prototyping.
>> > >>
>> > >> The host side of USBSS-DRD controller is compliance with XHCI
>> > >> specification, so it works with standard XHCI Linux driver.
>> > >>
>> > >> The host side of USBSS DRD controller is compliant with XHCI.
>> > >> The architecture for device side is almost the same as for host side,
>> > >> and most of the XHCI specification can be used to understand how
>> > >> this controller operates.
>> > >>
>> > >> This controller and driver support Full Speed, Hight Speed, Supper Speed
>> > >> and Supper Speed Plus USB protocol.
>> > >>
>> > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
>> > >> The last letter of this acronym means PLUS. The formal name of 
>> > >> controller
>> > >> is USBSSP but it's to generic so I've decided to use CDNSP.
>> > >>
>> > >> The patch 1: adds DT binding.
>> > >> The patch 2: adds PCI to platform wrapper used on Cadnece testing
>> > >>  platform. It is FPGA based on platform.
>> > >> The patches 3-5: add the main part of driver and has been intentionally
>> > >>  split into 3 part. In my opinion such division should not
>> > >>  affect understanding and reviewing the driver, and cause 
>> > >> that
>> > >>  main patch (4/5) is little smaller. Patch 3 introduces main
>> > >>  header file for driver, 4 is the main part that implements 
>> > >> all
>> > >>  functionality of driver and 5 introduces tracepoints.
>> > >
>> > >I'm more interested in how is this different from CDNS3. Aren't they SW 
>> > >compatible?
>> >
>> > In general, the controller can be split into 2 part- DRD part and the rest 
>> > UDC.
>> >
>> > The second part UDC which consist gadget.c, ring.c and mem.c file is 
>> > completely different.
>> >
>> > The DRD part contains drd.c and core.c.
>> > cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP has 
>> > similar, but has different register space.
>> > Some register was moved, some was removed and some was added.
>> >
>> > core.c is very similar and eventually could be common for both drivers.  I 
>> > thought about this but
>> > I wanted to avoid interfering with cdns3 driver at this point CDNSP is 
>> > still under testing and
>> > CDNS3 is used by some products on the market.
>>
>> Pawel, I suggest adding CDNSP at driver/staging first since it is still
>> under testing. When you are thinking the driver (as well as hardware) are
>> mature, you could try to add gadget part (eg, gadget-v2) and make
>> necessary changes for core.c.
>
>I only take code for drivers/staging/ that for some reason is not
>meeting the normal coding style/rules/whatever.  For stuff that is an
>obvious duplicate of existing code like this, and needs to be
>rearchitected.  It is much more work to try to convert code once it is
>in the tree than to just do it out of the tree on your own and resubmit
>it, as you don't have to follow the in-kernel rules of "one patch does
>one thing" that you would if it was in staging.
>
>So don't think that staging is the right place for this, just spend a
>few weeks to get it right and then resubmit it.
>

Ok, 
I try to reuse the code from cdns3. Where  such common code should be
placed ? Should I move it to e.g. drivers/usb/common/cdns or it should remain 
in 
cdns3 directory.

thanks,

pawel


Re: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-06-29 Thread gre...@linuxfoundation.org
On Mon, Jun 29, 2020 at 11:20:00AM +, Pawel Laszczak wrote:
> 
> >> > Hi Felipe,
> >> >
> >> > >
> >> > >Hi,
> >> > >
> >> > >Pawel Laszczak  writes:
> >> > >> This patch introduce new Cadence USBSS DRD driver to linux kernel.
> >> > >>
> >> > >> The Cadence USBSS DRD Controller is a highly configurable IP Core 
> >> > >> which
> >> > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
> >> > >> Host Only (XHCI)configurations.
> >> > >>
> >> > >> The current driver has been validated with FPGA burned. We have 
> >> > >> support
> >> > >> for PCIe bus, which is used on FPGA prototyping.
> >> > >>
> >> > >> The host side of USBSS-DRD controller is compliance with XHCI
> >> > >> specification, so it works with standard XHCI Linux driver.
> >> > >>
> >> > >> The host side of USBSS DRD controller is compliant with XHCI.
> >> > >> The architecture for device side is almost the same as for host side,
> >> > >> and most of the XHCI specification can be used to understand how
> >> > >> this controller operates.
> >> > >>
> >> > >> This controller and driver support Full Speed, Hight Speed, Supper 
> >> > >> Speed
> >> > >> and Supper Speed Plus USB protocol.
> >> > >>
> >> > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> >> > >> The last letter of this acronym means PLUS. The formal name of 
> >> > >> controller
> >> > >> is USBSSP but it's to generic so I've decided to use CDNSP.
> >> > >>
> >> > >> The patch 1: adds DT binding.
> >> > >> The patch 2: adds PCI to platform wrapper used on Cadnece testing
> >> > >>  platform. It is FPGA based on platform.
> >> > >> The patches 3-5: add the main part of driver and has been 
> >> > >> intentionally
> >> > >>  split into 3 part. In my opinion such division should not
> >> > >>  affect understanding and reviewing the driver, and cause 
> >> > >> that
> >> > >>  main patch (4/5) is little smaller. Patch 3 introduces 
> >> > >> main
> >> > >>  header file for driver, 4 is the main part that 
> >> > >> implements all
> >> > >>  functionality of driver and 5 introduces tracepoints.
> >> > >
> >> > >I'm more interested in how is this different from CDNS3. Aren't they SW 
> >> > >compatible?
> >> >
> >> > In general, the controller can be split into 2 part- DRD part and the 
> >> > rest UDC.
> >> >
> >> > The second part UDC which consist gadget.c, ring.c and mem.c file is 
> >> > completely different.
> >> >
> >> > The DRD part contains drd.c and core.c.
> >> > cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP 
> >> > has similar, but has different register space.
> >> > Some register was moved, some was removed and some was added.
> >> >
> >> > core.c is very similar and eventually could be common for both drivers.  
> >> > I thought about this but
> >> > I wanted to avoid interfering with cdns3 driver at this point CDNSP is 
> >> > still under testing and
> >> > CDNS3 is used by some products on the market.
> >>
> >> Pawel, I suggest adding CDNSP at driver/staging first since it is still
> >> under testing. When you are thinking the driver (as well as hardware) are
> >> mature, you could try to add gadget part (eg, gadget-v2) and make
> >> necessary changes for core.c.
> >
> >I only take code for drivers/staging/ that for some reason is not
> >meeting the normal coding style/rules/whatever.  For stuff that is an
> >obvious duplicate of existing code like this, and needs to be
> >rearchitected.  It is much more work to try to convert code once it is
> >in the tree than to just do it out of the tree on your own and resubmit
> >it, as you don't have to follow the in-kernel rules of "one patch does
> >one thing" that you would if it was in staging.
> >
> >So don't think that staging is the right place for this, just spend a
> >few weeks to get it right and then resubmit it.
> >
> 
> Ok, 
> I try to reuse the code from cdns3. Where  such common code should be
> placed ? Should I move it to e.g. drivers/usb/common/cdns or it should remain 
> in 
> cdns3 directory.

In the cdns3 directory makes the most sense.

thanks,

greg k-h


Re: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-06-28 Thread Peter Chen
On 20-06-26 07:19:56, Pawel Laszczak wrote:
> Hi Felipe,
> 
> >
> >Hi,
> >
> >Pawel Laszczak  writes:
> >> This patch introduce new Cadence USBSS DRD driver to linux kernel.
> >>
> >> The Cadence USBSS DRD Controller is a highly configurable IP Core which
> >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
> >> Host Only (XHCI)configurations.
> >>
> >> The current driver has been validated with FPGA burned. We have support
> >> for PCIe bus, which is used on FPGA prototyping.
> >>
> >> The host side of USBSS-DRD controller is compliance with XHCI
> >> specification, so it works with standard XHCI Linux driver.
> >>
> >> The host side of USBSS DRD controller is compliant with XHCI.
> >> The architecture for device side is almost the same as for host side,
> >> and most of the XHCI specification can be used to understand how
> >> this controller operates.
> >>
> >> This controller and driver support Full Speed, Hight Speed, Supper Speed
> >> and Supper Speed Plus USB protocol.
> >>
> >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> >> The last letter of this acronym means PLUS. The formal name of controller
> >> is USBSSP but it's to generic so I've decided to use CDNSP.
> >>
> >> The patch 1: adds DT binding.
> >> The patch 2: adds PCI to platform wrapper used on Cadnece testing
> >>  platform. It is FPGA based on platform.
> >> The patches 3-5: add the main part of driver and has been intentionally
> >>  split into 3 part. In my opinion such division should not
> >>  affect understanding and reviewing the driver, and cause that
> >>  main patch (4/5) is little smaller. Patch 3 introduces main
> >>  header file for driver, 4 is the main part that implements all
> >>  functionality of driver and 5 introduces tracepoints.
> >
> >I'm more interested in how is this different from CDNS3. Aren't they SW 
> >compatible?
> 
> In general, the controller can be split into 2 part- DRD part and the rest 
> UDC. 
> 
> The second part UDC which consist gadget.c, ring.c and mem.c file is 
> completely different. 
> 
> The DRD part contains drd.c and core.c. 
> cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP has 
> similar, but has different register space.
> Some register was moved, some was removed and some was added.  
> 
> core.c is very similar and eventually could be common for both drivers.  I 
> thought about this but
> I wanted to avoid interfering with cdns3 driver at this point CDNSP is still 
> under testing and 
> CDNS3 is used by some products on the market. 

Pawel, I suggest adding CDNSP at driver/staging first since it is still
under testing. When you are thinking the driver (as well as hardware) are
mature, you could try to add gadget part (eg, gadget-v2) and make
necessary changes for core.c.

-- 

Thanks,
Peter Chen

RE: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-06-26 Thread Pawel Laszczak
Hi Felipe,

>
>Hi,
>
>Pawel Laszczak  writes:
>> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>>
>> The Cadence USBSS DRD Controller is a highly configurable IP Core which
>> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
>> Host Only (XHCI)configurations.
>>
>> The current driver has been validated with FPGA burned. We have support
>> for PCIe bus, which is used on FPGA prototyping.
>>
>> The host side of USBSS-DRD controller is compliance with XHCI
>> specification, so it works with standard XHCI Linux driver.
>>
>> The host side of USBSS DRD controller is compliant with XHCI.
>> The architecture for device side is almost the same as for host side,
>> and most of the XHCI specification can be used to understand how
>> this controller operates.
>>
>> This controller and driver support Full Speed, Hight Speed, Supper Speed
>> and Supper Speed Plus USB protocol.
>>
>> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
>> The last letter of this acronym means PLUS. The formal name of controller
>> is USBSSP but it's to generic so I've decided to use CDNSP.
>>
>> The patch 1: adds DT binding.
>> The patch 2: adds PCI to platform wrapper used on Cadnece testing
>>  platform. It is FPGA based on platform.
>> The patches 3-5: add the main part of driver and has been intentionally
>>  split into 3 part. In my opinion such division should not
>>  affect understanding and reviewing the driver, and cause that
>>  main patch (4/5) is little smaller. Patch 3 introduces main
>>  header file for driver, 4 is the main part that implements all
>>  functionality of driver and 5 introduces tracepoints.
>
>I'm more interested in how is this different from CDNS3. Aren't they SW 
>compatible?

In general, the controller can be split into 2 part- DRD part and the rest UDC. 

The second part UDC which consist gadget.c, ring.c and mem.c file is completely 
different. 

The DRD part contains drd.c and core.c. 
cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP has 
similar, but has different register space.
Some register was moved, some was removed and some was added.  

core.c is very similar and eventually could be common for both drivers.  I 
thought about this but
I wanted to avoid interfering with cdns3 driver at this point CDNSP is still 
under testing and 
CDNS3 is used by some products on the market. 

I thought about merging cdnsp core.c  with cdns3 core.c but after upstreaming 
over, but 
I'm not sure if it's a good idea.

>
>--
>balbi

pawel


Re: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-06-25 Thread Felipe Balbi

Hi,

Pawel Laszczak  writes:
> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>
> The Cadence USBSS DRD Controller is a highly configurable IP Core which
> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
> Host Only (XHCI)configurations.
>
> The current driver has been validated with FPGA burned. We have support
> for PCIe bus, which is used on FPGA prototyping.
>
> The host side of USBSS-DRD controller is compliance with XHCI
> specification, so it works with standard XHCI Linux driver.
>
> The host side of USBSS DRD controller is compliant with XHCI.
> The architecture for device side is almost the same as for host side,
> and most of the XHCI specification can be used to understand how
> this controller operates.
>
> This controller and driver support Full Speed, Hight Speed, Supper Speed
> and Supper Speed Plus USB protocol.
>
> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> The last letter of this acronym means PLUS. The formal name of controller
> is USBSSP but it's to generic so I've decided to use CDNSP.
>
> The patch 1: adds DT binding.
> The patch 2: adds PCI to platform wrapper used on Cadnece testing
>  platform. It is FPGA based on platform.
> The patches 3-5: add the main part of driver and has been intentionally
>  split into 3 part. In my opinion such division should not
>  affect understanding and reviewing the driver, and cause that
>  main patch (4/5) is little smaller. Patch 3 introduces main
>  header file for driver, 4 is the main part that implements all
>  functionality of driver and 5 introduces tracepoints.

I'm more interested in how is this different from CDNS3. Aren't they SW 
compatible?

-- 
balbi


signature.asc
Description: PGP signature


[PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.

2020-06-25 Thread Pawel Laszczak
This patch introduce new Cadence USBSS DRD driver to linux kernel.

The Cadence USBSS DRD Controller is a highly configurable IP Core which
can be instantiated as Dual-Role Device (DRD), Peripheral Only and
Host Only (XHCI)configurations.

The current driver has been validated with FPGA burned. We have support
for PCIe bus, which is used on FPGA prototyping.

The host side of USBSS-DRD controller is compliance with XHCI
specification, so it works with standard XHCI Linux driver.

The host side of USBSS DRD controller is compliant with XHCI.
The architecture for device side is almost the same as for host side,
and most of the XHCI specification can be used to understand how
this controller operates.

This controller and driver support Full Speed, Hight Speed, Supper Speed
and Supper Speed Plus USB protocol.

The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
The last letter of this acronym means PLUS. The formal name of controller
is USBSSP but it's to generic so I've decided to use CDNSP.

The patch 1: adds DT binding.
The patch 2: adds PCI to platform wrapper used on Cadnece testing
 platform. It is FPGA based on platform.
The patches 3-5: add the main part of driver and has been intentionally
 split into 3 part. In my opinion such division should not
 affect understanding and reviewing the driver, and cause that
 main patch (4/5) is little smaller. Patch 3 introduces main
 header file for driver, 4 is the main part that implements all
 functionality of driver and 5 introduces tracepoints.

---

Pawel Laszczak (5):
  dt-bindings: add binding for CDNSP-DRD controller
  usb:cdns3: Add pci to platform driver wrapper
  usb: cdnsp: Device side header file for CDNSP driver
  usb: cdnsp: usb:cdns3 Add main part of Cadence USBSSP DRD Driver
  usb: cdnsp: Add tracepoints for CDNSP driver

 .../devicetree/bindings/usb/cdns-cdnsp.yaml   |  104 +
 drivers/usb/Kconfig   |1 +
 drivers/usb/Makefile  |1 +
 drivers/usb/cdnsp/Kconfig |   49 +
 drivers/usb/cdnsp/Makefile|   15 +
 drivers/usb/cdnsp/cdnsp-pci.c |  214 ++
 drivers/usb/cdnsp/core.c  |  632 +
 drivers/usb/cdnsp/core.h  |   90 +
 drivers/usb/cdnsp/debug.h |  583 
 drivers/usb/cdnsp/drd.c   |  372 +++
 drivers/usb/cdnsp/drd.h   |  127 +
 drivers/usb/cdnsp/ep0.c   |  482 
 drivers/usb/cdnsp/export.h|   36 +
 drivers/usb/cdnsp/gadget.c| 1990 ++
 drivers/usb/cdnsp/gadget.h| 1586 +++
 drivers/usb/cdnsp/host.c  |   74 +
 drivers/usb/cdnsp/mem.c   | 1340 +
 drivers/usb/cdnsp/ring.c  | 2443 +
 drivers/usb/cdnsp/trace.c |   12 +
 drivers/usb/cdnsp/trace.h |  839 ++
 20 files changed, 10990 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/cdns-cdnsp.yaml
 create mode 100644 drivers/usb/cdnsp/Kconfig
 create mode 100644 drivers/usb/cdnsp/Makefile
 create mode 100644 drivers/usb/cdnsp/cdnsp-pci.c
 create mode 100644 drivers/usb/cdnsp/core.c
 create mode 100644 drivers/usb/cdnsp/core.h
 create mode 100644 drivers/usb/cdnsp/debug.h
 create mode 100644 drivers/usb/cdnsp/drd.c
 create mode 100644 drivers/usb/cdnsp/drd.h
 create mode 100644 drivers/usb/cdnsp/ep0.c
 create mode 100644 drivers/usb/cdnsp/export.h
 create mode 100644 drivers/usb/cdnsp/gadget.c
 create mode 100644 drivers/usb/cdnsp/gadget.h
 create mode 100644 drivers/usb/cdnsp/host.c
 create mode 100644 drivers/usb/cdnsp/mem.c
 create mode 100644 drivers/usb/cdnsp/ring.c
 create mode 100644 drivers/usb/cdnsp/trace.c
 create mode 100644 drivers/usb/cdnsp/trace.h

-- 
2.17.1