Re: [net PATCH 4/9] octeontx2-af: Remove TOS field from MKEX TX
Hi Jakub,
On Tue, Mar 16, 2021 at 10:53 PM Jakub Kicinski wrote:
>
> On Tue, 16 Mar 2021 14:57:08 +0530 Hariprasad Kelam wrote:
> > From: Subbaraya Sundeep
> >
> > TOS overlaps with DMAC field in mcam search key and hence installing
> > rules for TX side are failing. Hence remove TOS field from TX profile.
>
> Could you clarify what "installing rules is failing" means?
> Return error or does not behave correctly?
Returns error. The MKEX profile can be in a way where higher layer packet fields
can overwrite lower layer packet fields in output MCAM Key. The commit
42006910 ("octeontx2-af: cleanup KPU config data") introduced TX TOS field and
it overwrites DMAC. AF driver return error when TX rule is installed
with DMAC as
match criteria since DMAC gets overwritten and cannot be supported. Layers from
lower to higher in our case:
LA - Ethernet
LB - VLAN
LC - IP
LD - TCP/UDP
and so on.
We make sure there are no overlaps between layers but TOS got added by mistake.
We will elaborate the commit description and send the next version.
Thanks,
Sundeep
Re: [net PATCH 3/9] octeontx2-af: Do not allocate memory for devlink private
On Wed, Mar 17, 2021 at 1:57 AM Jakub Kicinski wrote:
>
> On Tue, 16 Mar 2021 23:33:40 +0530 sundeep subbaraya wrote:
> > On Tue, Mar 16, 2021 at 10:53 PM Jakub Kicinski wrote:
> > >
> > > On Tue, 16 Mar 2021 14:57:07 +0530 Hariprasad Kelam wrote:
> > > > From: Subbaraya Sundeep
> > > >
> > > > Memory for driver private structure rvu_devlink is
> > > > also allocated during devlink_alloc. Hence use
> > > > the allocated memory by devlink_alloc and access it
> > > > by devlink_priv call.
> > > >
> > > > Fixes: fae06da4("octeontx2-af: Add devlink suppoort to af driver")
> > > > Signed-off-by: Subbaraya Sundeep
> > > > Signed-off-by: Hariprasad Kelam
> > > > Signed-off-by: Sunil Kovvuri Goutham
> > >
> > > Does it fix any bug? Looks like a coding improvement.
> >
> > Without this we cannot fetch our private struct 'rvu_devlink' from any
> > of the functions in devlink_ops which may get added in future.
>
> "which may get added in future" does not sound like it's fixing
> an existing problem to me :(
>
> If you have particular case where the existing setup is problematic
> please describe it in more detail, or mention what other fix depends
> on this patch. Otherwise sending this one patch for net-next would
> be better IMHO.
Sure will send this one patch to net-next.
Thanks,
Sundeep
Re: [net PATCH 3/9] octeontx2-af: Do not allocate memory for devlink private
Hi Jakub,
On Tue, Mar 16, 2021 at 10:53 PM Jakub Kicinski wrote:
>
> On Tue, 16 Mar 2021 14:57:07 +0530 Hariprasad Kelam wrote:
> > From: Subbaraya Sundeep
> >
> > Memory for driver private structure rvu_devlink is
> > also allocated during devlink_alloc. Hence use
> > the allocated memory by devlink_alloc and access it
> > by devlink_priv call.
> >
> > Fixes: fae06da4("octeontx2-af: Add devlink suppoort to af driver")
> > Signed-off-by: Subbaraya Sundeep
> > Signed-off-by: Hariprasad Kelam
> > Signed-off-by: Sunil Kovvuri Goutham
>
> Does it fix any bug? Looks like a coding improvement.
Without this we cannot fetch our private struct 'rvu_devlink' from any
of the functions in devlink_ops which may get added in future.
Thanks,
Sundeep
Re: [PATCH] PCI: Do not use bus number zero from EA capability
Hi Andrew,
On Mon, Sep 23, 2019 at 6:05 PM Andrew Murray wrote:
>
> On Mon, Sep 02, 2019 at 09:00:03PM +0530, sundeep.l...@gmail.com wrote:
> > From: Subbaraya Sundeep
> >
> > As per the spec, "Enhanced Allocation (EA) for Memory
> > and I/O Resources" ECN, approved 23 October 2014,
> > sec 6.9.1.2, fixed bus numbers of a bridge can be zero
>
> s/can/must/
>
> The spec uses the term *must*. "Can" implies that this is optional.
>
Yes will change to must.
> > when no function that uses EA is located behind it.
> > Hence assign bus numbers sequentially when fixed bus
> > numbers are zero.
>
> Perhaps s/sequentially/as per normal/ or similar. As we're not doing
> anything different here.
>
Ok will change
> >
> > Fixes: 2dbce590117981196fe355efc0569bc6f949ae9b
>
> Is it worth describing what actually goes wrong without this patch - and
> when this occurs? I guess it's possible for a bridge to have an EA
> capability, but no devices using EA behind it - and thus in this suitation
> the downstream devices have unnecessary bus number constraints?
>
EA is for functions which are permanently connected to host bridge.
In our case all the on chip devices use EA and bridges which are there for
connecting off chip devices use EA with fixed bus numbers as zero.
Bus numbers for those bridges need to be configured as per
normal enumeration, failing to do so makes those bridges not functional
because secondary and subordinate bus numbers are 0.
> >
> > Signed-off-by: Subbaraya Sundeep
>
> Does this need to be CC'd to stable?
>
Ok will CC stable from v2
> > ---
> > drivers/pci/probe.c | 25 +
> > 1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index a3c7338..c06ca4c 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1095,27 +1095,28 @@ static unsigned int
> > pci_scan_child_bus_extend(struct pci_bus *bus,
> > * @sub: updated with subordinate bus number from EA
> > *
> > * If @dev is a bridge with EA capability, update @sec and @sub with
> > - * fixed bus numbers from the capability and return true. Otherwise,
> > - * return false.
> > + * fixed bus numbers from the capability. Otherwise @sec and @sub
> > + * will be zeroed.
> > */
> > -static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub)
> > +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub)
> > {
> > int ea, offset;
> > u32 dw;
> >
> > + *sec = *sub = 0;
> > +
> > if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
> > - return false;
> > + return;
> >
> > /* find PCI EA capability in list */
> > ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> > if (!ea)
> > - return false;
> > + return;
> >
> > offset = ea + PCI_EA_FIRST_ENT;
> > pci_read_config_dword(dev, offset, &dw);
> > *sec = dw & PCI_EA_SEC_BUS_MASK;
> > *sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
>
> Is there any value in doing any sanity checking here? E.g. sub !=0, sub > sec?
>
I don't think it is needed since we read hardwired values from HW.
> > - return true;
> > }
> >
> > /*
> > @@ -1151,7 +1152,6 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > u16 bctl;
> > u8 primary, secondary, subordinate;
> > int broken = 0;
> > - bool fixed_buses;
> > u8 fixed_sec, fixed_sub;
> > int next_busnr;
> >
> > @@ -1254,11 +1254,12 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > pci_write_config_word(dev, PCI_STATUS, 0x);
> >
> > /* Read bus numbers from EA Capability (if present) */
> > - fixed_buses = pci_ea_fixed_busnrs(dev, &fixed_sec,
> > &fixed_sub);
> > - if (fixed_buses)
> > + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
> > +
> > + next_busnr = max + 1;
> > + /* Use secondary bus number in EA */
> > + if (fixed_sec)
> > next_busnr = fixed_sec;
> > - else
> > - next_busnr = max + 1;
>
> There is a subtle style change here (assigning and then potentially
> reassigning
> with a new value vs assigning once using both if/else). No idea if this
> matters
> but I thought I'd point it out in case it wasn't intentional.
>
This is intentional just to avoid else case.
Thanks for review. I will send v2.
Sundeep
> Thanks,
>
> Andrew Murray
>
> >
> > /*
> >* Prevent assigning a bus number that already exists.
> > @@ -1336,7 +1337,7 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> >* If fixed subordinate bus number exists from EA
> >* capability then use it.
> >*/
> > - if (fixed_buses)
> > + if (fixed_sub)
> >
Re: [PATCH] PCI: Do not use bus number zero from EA capability
Hi Bjorn,
Please let me know if you have any comments on the patch.
Thanks,
Sundeep
On Mon, Sep 2, 2019 at 9:00 PM wrote:
>
> From: Subbaraya Sundeep
>
> As per the spec, "Enhanced Allocation (EA) for Memory
> and I/O Resources" ECN, approved 23 October 2014,
> sec 6.9.1.2, fixed bus numbers of a bridge can be zero
> when no function that uses EA is located behind it.
> Hence assign bus numbers sequentially when fixed bus
> numbers are zero.
>
> Fixes: 2dbce590117981196fe355efc0569bc6f949ae9b
>
> Signed-off-by: Subbaraya Sundeep
> ---
> drivers/pci/probe.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index a3c7338..c06ca4c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1095,27 +1095,28 @@ static unsigned int pci_scan_child_bus_extend(struct
> pci_bus *bus,
> * @sub: updated with subordinate bus number from EA
> *
> * If @dev is a bridge with EA capability, update @sec and @sub with
> - * fixed bus numbers from the capability and return true. Otherwise,
> - * return false.
> + * fixed bus numbers from the capability. Otherwise @sec and @sub
> + * will be zeroed.
> */
> -static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub)
> +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub)
> {
> int ea, offset;
> u32 dw;
>
> + *sec = *sub = 0;
> +
> if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
> - return false;
> + return;
>
> /* find PCI EA capability in list */
> ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> if (!ea)
> - return false;
> + return;
>
> offset = ea + PCI_EA_FIRST_ENT;
> pci_read_config_dword(dev, offset, &dw);
> *sec = dw & PCI_EA_SEC_BUS_MASK;
> *sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
> - return true;
> }
>
> /*
> @@ -1151,7 +1152,6 @@ static int pci_scan_bridge_extend(struct pci_bus *bus,
> struct pci_dev *dev,
> u16 bctl;
> u8 primary, secondary, subordinate;
> int broken = 0;
> - bool fixed_buses;
> u8 fixed_sec, fixed_sub;
> int next_busnr;
>
> @@ -1254,11 +1254,12 @@ static int pci_scan_bridge_extend(struct pci_bus
> *bus, struct pci_dev *dev,
> pci_write_config_word(dev, PCI_STATUS, 0x);
>
> /* Read bus numbers from EA Capability (if present) */
> - fixed_buses = pci_ea_fixed_busnrs(dev, &fixed_sec,
> &fixed_sub);
> - if (fixed_buses)
> + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
> +
> + next_busnr = max + 1;
> + /* Use secondary bus number in EA */
> + if (fixed_sec)
> next_busnr = fixed_sec;
> - else
> - next_busnr = max + 1;
>
> /*
> * Prevent assigning a bus number that already exists.
> @@ -1336,7 +1337,7 @@ static int pci_scan_bridge_extend(struct pci_bus *bus,
> struct pci_dev *dev,
> * If fixed subordinate bus number exists from EA
> * capability then use it.
> */
> - if (fixed_buses)
> + if (fixed_sub)
> max = fixed_sub;
> pci_bus_update_busn_res_end(child, max);
> pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max);
> --
> 2.7.4
>
Re: [PATCH v2] PCI: assign bus numbers present in EA capability for bridges
Hi Bjorn,
On Thu, Apr 18, 2019 at 1:46 AM Bjorn Helgaas wrote:
>
> On Mon, Nov 19, 2018 at 06:44:32PM +0530, sundeep.l...@gmail.com wrote:
> > From: Subbaraya Sundeep
> >
> > As per the spec, bridges with EA capability work
> > with fixed secondary and subordinate bus numbers.
> > Hence assign bus numbers to bridges from EA if the
> > capability exists.
> >
> > Signed-off-by: Subbaraya Sundeep
>
> I applied this with minor revisions to pci/enumeration for v5.2,
> thanks!
>
> > ---
> > Changes for v2:
> > No changes just added Sean Stalley who did EA support for BARs.
> >
> > drivers/pci/probe.c | 58
> > ---
> > include/uapi/linux/pci_regs.h | 6 +
> > 2 files changed, 60 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b1c05b5..f41d2e6 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1030,6 +1030,40 @@ static void pci_enable_crs(struct pci_dev *pdev)
> >
> > static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> > unsigned int available_buses);
> > +/*
> > + * pci_ea_fixed_busnrs() - Read fixed Secondary and Subordinate bus
> > + * numbers from EA capability.
> > + * @dev: Bridge with EA
> > + * @secondary: updated with secondary bus number in EA
> > + * @subordinate: updated with subordinate bus number in EA
> > + *
> > + * If it is a bridge with EA capability then fixed bus numbers are
> > + * read from EA capability list and secondary, subordinate reference
> > + * variables will be updated. Otherwise secondary and subordinate reference
> > + * variables will be zeroed.
> > + */
> > +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *secondary,
> > + u8 *subordinate)
> > +{
> > + int ea;
> > + int offset;
> > + u32 dw;
> > +
> > + *secondary = *subordinate = 0;
> > +
> > + if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
> > + return;
> > +
> > + /* find PCI EA capability in list */
> > + ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> > + if (!ea)
> > + return;
> > +
> > + offset = ea + PCI_EA_FIRST_ENT;
> > + pci_read_config_dword(dev, offset, &dw);
> > + *secondary = dw & PCI_EA_SEC_BUS_MASK;
> > + *subordinate = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
> > +}
> >
> > /*
> > * pci_scan_bridge_extend() - Scan buses behind a bridge
> > @@ -1064,6 +1098,8 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > u16 bctl;
> > u8 primary, secondary, subordinate;
> > int broken = 0;
> > + u8 fixed_sec, fixed_sub;
> > + int next_busnr;
> >
> > /*
> >* Make sure the bridge is powered on to be able to access config
> > @@ -1163,17 +1199,25 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > /* Clear errors */
> > pci_write_config_word(dev, PCI_STATUS, 0x);
> >
> > + /* read bus numbers from EA */
> > + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
> > +
> > + next_busnr = max + 1;
> > + /* Use secondary bus number in EA */
> > + if (fixed_sec)
> > + next_busnr = fixed_sec;
>
> I initially thought this was not quite safe because EA could supply a
> secondary bus number of 0, in which case we would erroneously ignore
> it. But it's actually not possible for a PCI-to-PCI bridge to have a
When EA supplies secondary bus number as 0 then we
should proceed with normal sequence where we scan with max + 1
as bus number for next round. Otherwise subordinate bus number
will always be returning zero.
As per the spec, regd. Fixed Secondary Bus Number and
Fixed Subordinate Bus Number:
"If at least one Function that uses EA is located behind this function,
then this field must be set to indicate the bus number of the PCI bus segment
to which the secondary interface of this Function is connected.
If no Function that uses EA is located behind this function,
then this field must be set to all 0’s."
which implies that normal enumeration should take place behind this
last EA bridge.
Shall I revert your changes which returns bool ?
Or simply add a check if (fixed_buses && fixed_sec) ?
Sorry for the trouble.
Thanks,
Sundeep
> secondary bus number of 0, so it *is* safe.
>
> But it's still a little subtle and since I've already done the work to
> add a bool return value from pci_ea_fixed_busnrs(), maybe it will be
> OK to keep that. The patch as applied is below, let me know if you
> have any comments.
>
> > +
> > /*
> >* Prevent assigning a bus number that already exists.
> >* This can happen when a bridge is hot-plugged, so in this
> >* case we only re-scan this bus.
> >*/
> > - child = pci_fi
Re: [PATCH v2] PCI: assign bus numbers present in EA capability for bridges
Hi Bjorn,
On Thu, Apr 18, 2019 at 1:46 AM Bjorn Helgaas wrote:
>
> On Mon, Nov 19, 2018 at 06:44:32PM +0530, sundeep.l...@gmail.com wrote:
> > From: Subbaraya Sundeep
> >
> > As per the spec, bridges with EA capability work
> > with fixed secondary and subordinate bus numbers.
> > Hence assign bus numbers to bridges from EA if the
> > capability exists.
> >
> > Signed-off-by: Subbaraya Sundeep
>
> I applied this with minor revisions to pci/enumeration for v5.2,
> thanks!
>
> > ---
> > Changes for v2:
> > No changes just added Sean Stalley who did EA support for BARs.
> >
> > drivers/pci/probe.c | 58
> > ---
> > include/uapi/linux/pci_regs.h | 6 +
> > 2 files changed, 60 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b1c05b5..f41d2e6 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1030,6 +1030,40 @@ static void pci_enable_crs(struct pci_dev *pdev)
> >
> > static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> > unsigned int available_buses);
> > +/*
> > + * pci_ea_fixed_busnrs() - Read fixed Secondary and Subordinate bus
> > + * numbers from EA capability.
> > + * @dev: Bridge with EA
> > + * @secondary: updated with secondary bus number in EA
> > + * @subordinate: updated with subordinate bus number in EA
> > + *
> > + * If it is a bridge with EA capability then fixed bus numbers are
> > + * read from EA capability list and secondary, subordinate reference
> > + * variables will be updated. Otherwise secondary and subordinate reference
> > + * variables will be zeroed.
> > + */
> > +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *secondary,
> > + u8 *subordinate)
> > +{
> > + int ea;
> > + int offset;
> > + u32 dw;
> > +
> > + *secondary = *subordinate = 0;
> > +
> > + if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
> > + return;
> > +
> > + /* find PCI EA capability in list */
> > + ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> > + if (!ea)
> > + return;
> > +
> > + offset = ea + PCI_EA_FIRST_ENT;
> > + pci_read_config_dword(dev, offset, &dw);
> > + *secondary = dw & PCI_EA_SEC_BUS_MASK;
> > + *subordinate = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
> > +}
> >
> > /*
> > * pci_scan_bridge_extend() - Scan buses behind a bridge
> > @@ -1064,6 +1098,8 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > u16 bctl;
> > u8 primary, secondary, subordinate;
> > int broken = 0;
> > + u8 fixed_sec, fixed_sub;
> > + int next_busnr;
> >
> > /*
> >* Make sure the bridge is powered on to be able to access config
> > @@ -1163,17 +1199,25 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > /* Clear errors */
> > pci_write_config_word(dev, PCI_STATUS, 0x);
> >
> > + /* read bus numbers from EA */
> > + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
> > +
> > + next_busnr = max + 1;
> > + /* Use secondary bus number in EA */
> > + if (fixed_sec)
> > + next_busnr = fixed_sec;
>
> I initially thought this was not quite safe because EA could supply a
> secondary bus number of 0, in which case we would erroneously ignore
> it. But it's actually not possible for a PCI-to-PCI bridge to have a
> secondary bus number of 0, so it *is* safe.
>
> But it's still a little subtle and since I've already done the work to
> add a bool return value from pci_ea_fixed_busnrs(), maybe it will be
> OK to keep that. The patch as applied is below, let me know if you
> have any comments.
>
Your changes looks good. Thanks for applying the patch.
Sundeep
> > +
> > /*
> >* Prevent assigning a bus number that already exists.
> >* This can happen when a bridge is hot-plugged, so in this
> >* case we only re-scan this bus.
> >*/
> > - child = pci_find_bus(pci_domain_nr(bus), max+1);
> > + child = pci_find_bus(pci_domain_nr(bus), next_busnr);
> > if (!child) {
> > - child = pci_add_new_bus(bus, dev, max+1);
> > + child = pci_add_new_bus(bus, dev, next_busnr);
> > if (!child)
> > goto out;
> > - pci_bus_insert_busn_res(child, max+1,
> > + pci_bus_insert_busn_res(child, next_busnr,
> > bus->busn_res.end);
> > }
> > max++;
> > @@ -1234,7 +1278,13 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > max += i;
> > }
> >
>
Re: [v3 PATCH 1/2] PCI: read fixed bus numbers in EA for type 1 functions
Ping.
Thanks,
Sundeep
On Tue, Feb 5, 2019 at 9:30 AM sundeep subbaraya wrote:
>
> Hi Bjorn,
>
> Any comments?
>
> Thanks,
> Sundeep
>
> On Wed, Jan 23, 2019 at 6:48 PM wrote:
> >
> > From: Subbaraya Sundeep
> >
> > As per the spec - ECN_Enhanced_Allocation_23_Oct_2014_Final
> > and section 6.9.1.2, EA capability contains fixed secondary
> > and subordinate bus numbers for type 1 functions.
> > This patch adds support to read the fixed bus numbers
> > from EA capability for bridge.
> >
> > Signed-off-by: Subbaraya Sundeep
> > ---
> > v3:
> > As per Bjorn's suggestion placed EA stuff in pci_ea_init and
> > captured bus numbers in pci_dev
> > v2:
> > None just added Sean
> >
> > drivers/pci/pci.c | 10 --
> > include/linux/pci.h | 4
> > include/uapi/linux/pci_regs.h | 4
> > 3 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index c25acac..484b63e 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2909,6 +2909,7 @@ void pci_ea_init(struct pci_dev *dev)
> > u8 num_ent;
> > int offset;
> > int i;
> > + u32 dw;
> >
> > /* find PCI EA capability in list */
> > ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> > @@ -2922,9 +2923,14 @@ void pci_ea_init(struct pci_dev *dev)
> >
> > offset = ea + PCI_EA_FIRST_ENT;
> >
> > - /* Skip DWORD 2 for type 1 functions */
> > - if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> > + /* Note fixed bus numbers for type 1 functions */
> > + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > + pci_read_config_dword(dev, offset, &dw);
> > + dev->fixed_sec_busnr = dw & PCI_EA_FIXED_SEC_BUS;
> > + dev->fixed_sub_busnr = (dw & PCI_EA_FIXED_SUB_BUS) >>
> > + PCI_EA_FIXED_SUB_SHIFT;
> > offset += 4;
> > + }
> >
> > /* parse each EA entry */
> > for (i = 0; i < num_ent; ++i)
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 65f1d8c..3e9a3ae 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -459,6 +459,10 @@ struct pci_dev {
> > char*driver_override; /* Driver name to force a match */
> >
> > unsigned long priv_flags; /* Private flags for the PCI driver
> > */
> > +
> > + /* bus numbers from EA capability if this device is a bridge */
> > + u8 fixed_sec_busnr; /* Fixed Secondary Bus number */
> > + u8 fixed_sub_busnr; /* Fixed Subordinate Bus number */
> > };
> >
> > static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index e1e9888..51e9ac0 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -372,6 +372,10 @@
> > #define PCI_EA_FIRST_ENT_BRIDGE8 /* First EA Entry for
> > Bridges */
> > #define PCI_EA_ES 0x0007 /* Entry Size */
> > #define PCI_EA_BEI0x00f0 /* BAR Equivalent Indicator */
> > +/* Fixed Secondary and Subordinate bus numbers in EA for Bridge */
> > +#define PCI_EA_FIXED_SEC_BUS 0xff
> > +#define PCI_EA_FIXED_SUB_BUS 0xff00
> > +#define PCI_EA_FIXED_SUB_SHIFT 8
> > /* 0-5 map to BARs 0-5 respectively */
> > #define PCI_EA_BEI_BAR0 0
> > #define PCI_EA_BEI_BAR5 5
> > --
> > 1.8.3.1
> >
Re: [v3 PATCH 1/2] PCI: read fixed bus numbers in EA for type 1 functions
Hi Bjorn,
Any comments?
Thanks,
Sundeep
On Wed, Jan 23, 2019 at 6:48 PM wrote:
>
> From: Subbaraya Sundeep
>
> As per the spec - ECN_Enhanced_Allocation_23_Oct_2014_Final
> and section 6.9.1.2, EA capability contains fixed secondary
> and subordinate bus numbers for type 1 functions.
> This patch adds support to read the fixed bus numbers
> from EA capability for bridge.
>
> Signed-off-by: Subbaraya Sundeep
> ---
> v3:
> As per Bjorn's suggestion placed EA stuff in pci_ea_init and
> captured bus numbers in pci_dev
> v2:
> None just added Sean
>
> drivers/pci/pci.c | 10 --
> include/linux/pci.h | 4
> include/uapi/linux/pci_regs.h | 4
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c25acac..484b63e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2909,6 +2909,7 @@ void pci_ea_init(struct pci_dev *dev)
> u8 num_ent;
> int offset;
> int i;
> + u32 dw;
>
> /* find PCI EA capability in list */
> ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> @@ -2922,9 +2923,14 @@ void pci_ea_init(struct pci_dev *dev)
>
> offset = ea + PCI_EA_FIRST_ENT;
>
> - /* Skip DWORD 2 for type 1 functions */
> - if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> + /* Note fixed bus numbers for type 1 functions */
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> + pci_read_config_dword(dev, offset, &dw);
> + dev->fixed_sec_busnr = dw & PCI_EA_FIXED_SEC_BUS;
> + dev->fixed_sub_busnr = (dw & PCI_EA_FIXED_SUB_BUS) >>
> + PCI_EA_FIXED_SUB_SHIFT;
> offset += 4;
> + }
>
> /* parse each EA entry */
> for (i = 0; i < num_ent; ++i)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 65f1d8c..3e9a3ae 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -459,6 +459,10 @@ struct pci_dev {
> char*driver_override; /* Driver name to force a match */
>
> unsigned long priv_flags; /* Private flags for the PCI driver */
> +
> + /* bus numbers from EA capability if this device is a bridge */
> + u8 fixed_sec_busnr; /* Fixed Secondary Bus number */
> + u8 fixed_sub_busnr; /* Fixed Subordinate Bus number */
> };
>
> static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e1e9888..51e9ac0 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -372,6 +372,10 @@
> #define PCI_EA_FIRST_ENT_BRIDGE8 /* First EA Entry for Bridges
> */
> #define PCI_EA_ES 0x0007 /* Entry Size */
> #define PCI_EA_BEI0x00f0 /* BAR Equivalent Indicator */
> +/* Fixed Secondary and Subordinate bus numbers in EA for Bridge */
> +#define PCI_EA_FIXED_SEC_BUS 0xff
> +#define PCI_EA_FIXED_SUB_BUS 0xff00
> +#define PCI_EA_FIXED_SUB_SHIFT 8
> /* 0-5 map to BARs 0-5 respectively */
> #define PCI_EA_BEI_BAR0 0
> #define PCI_EA_BEI_BAR5 5
> --
> 1.8.3.1
>
Re: [PATCH v2] PCI: assign bus numbers present in EA capability for bridges
Hi Bjorn,
On Thu, Nov 29, 2018 at 3:25 AM Bjorn Helgaas wrote:
>
> On Mon, Nov 19, 2018 at 06:44:32PM +0530, sundeep.l...@gmail.com wrote:
> > From: Subbaraya Sundeep
> >
> > As per the spec, bridges with EA capability work
> > with fixed secondary and subordinate bus numbers.
> > Hence assign bus numbers to bridges from EA if the
> > capability exists.
>
> A reference to the spec section would be good, i.e., PCIe r4.0, sec xxx.
>
Ok. I referred ECN 2014 section 6.9.1.2.
> > Signed-off-by: Subbaraya Sundeep
> > ---
> > Changes for v2:
> > No changes just added Sean Stalley who did EA support for BARs.
> >
> > drivers/pci/probe.c | 58
> > ---
> > include/uapi/linux/pci_regs.h | 6 +
> > 2 files changed, 60 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b1c05b5..f41d2e6 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1030,6 +1030,40 @@ static void pci_enable_crs(struct pci_dev *pdev)
> >
> > static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> > unsigned int available_buses);
> > +/*
> > + * pci_ea_fixed_busnrs() - Read fixed Secondary and Subordinate bus
> > + * numbers from EA capability.
> > + * @dev: Bridge with EA
> > + * @secondary: updated with secondary bus number in EA
> > + * @subordinate: updated with subordinate bus number in EA
> > + *
> > + * If it is a bridge with EA capability then fixed bus numbers are
> > + * read from EA capability list and secondary, subordinate reference
> > + * variables will be updated. Otherwise secondary and subordinate reference
> > + * variables will be zeroed.
> > + */
> > +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *secondary,
> > + u8 *subordinate)
> > +{
> > + int ea;
> > + int offset;
> > + u32 dw;
> > +
> > + *secondary = *subordinate = 0;
> > +
> > + if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
> > + return;
> > +
> > + /* find PCI EA capability in list */
> > + ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> > + if (!ea)
> > + return;
> > +
> > + offset = ea + PCI_EA_FIRST_ENT;
> > + pci_read_config_dword(dev, offset, &dw);
>
> "Num Entries" in the first DW of the capability is allowed to be zero,
> in which case this word (the second DW) is invalid. [See comments
> below; this code would be valid based on the 2014 ECN, but not per the
> 2017 PCIe r4.0 spec]
>
Yes but Entries follow after first DW of EA capability for devices and
after second
DW for bridges.
2014 ECN says for bridges DW2 of EA must be present:
"For Type 1 functions only, there is a second DW in the capability,
preceding the first entry.
This second DW must be included in the Enhanced Allocation Capability
whenever this
capability is implemented in a Type 1 Function"
So for normal device EA DW1, Entries(if any)
for bridges EA DW1,EA DW2, Entries(if any)
> It would be much better if this function could be somehow incorporated
> into pci_ea_init(), which already knows how to parse much of the EA
> capability.
>
I initially thought of this but didn't do it to avoid new members in pci_dev.
> > + *secondary = dw & PCI_EA_SEC_BUS_MASK;
> > + *subordinate = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
> > +}
> >
> > /*
> > * pci_scan_bridge_extend() - Scan buses behind a bridge
> > @@ -1064,6 +1098,8 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > u16 bctl;
> > u8 primary, secondary, subordinate;
> > int broken = 0;
> > + u8 fixed_sec, fixed_sub;
> > + int next_busnr;
> >
> > /*
> >* Make sure the bridge is powered on to be able to access config
> > @@ -1163,17 +1199,25 @@ static int pci_scan_bridge_extend(struct pci_bus
> > *bus, struct pci_dev *dev,
> > /* Clear errors */
> > pci_write_config_word(dev, PCI_STATUS, 0x);
> >
> > + /* read bus numbers from EA */
> > + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
> > +
> > + next_busnr = max + 1;
> > + /* Use secondary bus number in EA */
> > + if (fixed_sec)
> > + next_busnr = fixed_sec;
> > +
> > /*
> >* Prevent assigning a bus number that already exists.
> >* This can happen when a bridge is hot-plugged, so in this
> >* case we only re-scan this bus.
> >*/
> > - child = pci_find_bus(pci_domain_nr(bus), max+1);
> > + child = pci_find_bus(pci_domain_nr(bus), next_busnr);
> > if (!child) {
> > - child = pci_add_new_bus(bus, dev, max+1);
> > + child = pci_add_new_bus(bus, dev, next_busnr);
> > if (!child)
> > goto out;
Re: [PATCH v2] PCI: assign bus numbers present in EA capability for bridges
Hi Bjorn / Sean,
Any comments?
Thanks,
Sundeep
On Mon, Nov 19, 2018 at 6:44 PM wrote:
>
> From: Subbaraya Sundeep
>
> As per the spec, bridges with EA capability work
> with fixed secondary and subordinate bus numbers.
> Hence assign bus numbers to bridges from EA if the
> capability exists.
>
> Signed-off-by: Subbaraya Sundeep
> ---
> Changes for v2:
> No changes just added Sean Stalley who did EA support for BARs.
>
> drivers/pci/probe.c | 58
> ---
> include/uapi/linux/pci_regs.h | 6 +
> 2 files changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b1c05b5..f41d2e6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1030,6 +1030,40 @@ static void pci_enable_crs(struct pci_dev *pdev)
>
> static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> unsigned int available_buses);
> +/*
> + * pci_ea_fixed_busnrs() - Read fixed Secondary and Subordinate bus
> + * numbers from EA capability.
> + * @dev: Bridge with EA
> + * @secondary: updated with secondary bus number in EA
> + * @subordinate: updated with subordinate bus number in EA
> + *
> + * If it is a bridge with EA capability then fixed bus numbers are
> + * read from EA capability list and secondary, subordinate reference
> + * variables will be updated. Otherwise secondary and subordinate reference
> + * variables will be zeroed.
> + */
> +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *secondary,
> + u8 *subordinate)
> +{
> + int ea;
> + int offset;
> + u32 dw;
> +
> + *secondary = *subordinate = 0;
> +
> + if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
> + return;
> +
> + /* find PCI EA capability in list */
> + ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> + if (!ea)
> + return;
> +
> + offset = ea + PCI_EA_FIRST_ENT;
> + pci_read_config_dword(dev, offset, &dw);
> + *secondary = dw & PCI_EA_SEC_BUS_MASK;
> + *subordinate = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
> +}
>
> /*
> * pci_scan_bridge_extend() - Scan buses behind a bridge
> @@ -1064,6 +1098,8 @@ static int pci_scan_bridge_extend(struct pci_bus *bus,
> struct pci_dev *dev,
> u16 bctl;
> u8 primary, secondary, subordinate;
> int broken = 0;
> + u8 fixed_sec, fixed_sub;
> + int next_busnr;
>
> /*
> * Make sure the bridge is powered on to be able to access config
> @@ -1163,17 +1199,25 @@ static int pci_scan_bridge_extend(struct pci_bus
> *bus, struct pci_dev *dev,
> /* Clear errors */
> pci_write_config_word(dev, PCI_STATUS, 0x);
>
> + /* read bus numbers from EA */
> + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
> +
> + next_busnr = max + 1;
> + /* Use secondary bus number in EA */
> + if (fixed_sec)
> + next_busnr = fixed_sec;
> +
> /*
> * Prevent assigning a bus number that already exists.
> * This can happen when a bridge is hot-plugged, so in this
> * case we only re-scan this bus.
> */
> - child = pci_find_bus(pci_domain_nr(bus), max+1);
> + child = pci_find_bus(pci_domain_nr(bus), next_busnr);
> if (!child) {
> - child = pci_add_new_bus(bus, dev, max+1);
> + child = pci_add_new_bus(bus, dev, next_busnr);
> if (!child)
> goto out;
> - pci_bus_insert_busn_res(child, max+1,
> + pci_bus_insert_busn_res(child, next_busnr,
> bus->busn_res.end);
> }
> max++;
> @@ -1234,7 +1278,13 @@ static int pci_scan_bridge_extend(struct pci_bus *bus,
> struct pci_dev *dev,
> max += i;
> }
>
> - /* Set subordinate bus number to its real value */
> + /*
> +* Set subordinate bus number to its real value.
> +* If fixed subordinate bus number exists from EA
> +* capability then use it.
> +*/
> + if (fixed_sub)
> + max = fixed_sub;
> pci_bus_update_busn_res_end(child, max);
> pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max);
> }
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e1e9888..c3d0904 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -372,6 +372,12 @@
> #define PCI_EA_FIRST_ENT_BRIDGE8 /* First EA Entry for Bridges
> */
> #de
Re: [PATCH] PCI: assign bus numbers present in EA capability for bridges
Hi,
Any comments?
Thanks,
Sundeep
On Thu, Nov 1, 2018 at 3:01 PM wrote:
>
> From: Subbaraya Sundeep
>
> As per the spec, bridges with EA capability work
> with fixed secondary and subordinate bus numbers.
> Hence assign bus numbers to bridges from EA if the
> capability exists.
>
> Signed-off-by: Subbaraya Sundeep
> ---
> drivers/pci/probe.c | 58
> ---
> include/uapi/linux/pci_regs.h | 6 +
> 2 files changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b1c05b5..f41d2e6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1030,6 +1030,40 @@ static void pci_enable_crs(struct pci_dev *pdev)
>
> static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> unsigned int available_buses);
> +/*
> + * pci_ea_fixed_busnrs() - Read fixed Secondary and Subordinate bus
> + * numbers from EA capability.
> + * @dev: Bridge with EA
> + * @secondary: updated with secondary bus number in EA
> + * @subordinate: updated with subordinate bus number in EA
> + *
> + * If it is a bridge with EA capability then fixed bus numbers are
> + * read from EA capability list and secondary, subordinate reference
> + * variables will be updated. Otherwise secondary and subordinate reference
> + * variables will be zeroed.
> + */
> +static void pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *secondary,
> + u8 *subordinate)
> +{
> + int ea;
> + int offset;
> + u32 dw;
> +
> + *secondary = *subordinate = 0;
> +
> + if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE)
> + return;
> +
> + /* find PCI EA capability in list */
> + ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> + if (!ea)
> + return;
> +
> + offset = ea + PCI_EA_FIRST_ENT;
> + pci_read_config_dword(dev, offset, &dw);
> + *secondary = dw & PCI_EA_SEC_BUS_MASK;
> + *subordinate = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
> +}
>
> /*
> * pci_scan_bridge_extend() - Scan buses behind a bridge
> @@ -1064,6 +1098,8 @@ static int pci_scan_bridge_extend(struct pci_bus *bus,
> struct pci_dev *dev,
> u16 bctl;
> u8 primary, secondary, subordinate;
> int broken = 0;
> + u8 fixed_sec, fixed_sub;
> + int next_busnr;
>
> /*
> * Make sure the bridge is powered on to be able to access config
> @@ -1163,17 +1199,25 @@ static int pci_scan_bridge_extend(struct pci_bus
> *bus, struct pci_dev *dev,
> /* Clear errors */
> pci_write_config_word(dev, PCI_STATUS, 0x);
>
> + /* read bus numbers from EA */
> + pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
> +
> + next_busnr = max + 1;
> + /* Use secondary bus number in EA */
> + if (fixed_sec)
> + next_busnr = fixed_sec;
> +
> /*
> * Prevent assigning a bus number that already exists.
> * This can happen when a bridge is hot-plugged, so in this
> * case we only re-scan this bus.
> */
> - child = pci_find_bus(pci_domain_nr(bus), max+1);
> + child = pci_find_bus(pci_domain_nr(bus), next_busnr);
> if (!child) {
> - child = pci_add_new_bus(bus, dev, max+1);
> + child = pci_add_new_bus(bus, dev, next_busnr);
> if (!child)
> goto out;
> - pci_bus_insert_busn_res(child, max+1,
> + pci_bus_insert_busn_res(child, next_busnr,
> bus->busn_res.end);
> }
> max++;
> @@ -1234,7 +1278,13 @@ static int pci_scan_bridge_extend(struct pci_bus *bus,
> struct pci_dev *dev,
> max += i;
> }
>
> - /* Set subordinate bus number to its real value */
> + /*
> +* Set subordinate bus number to its real value.
> +* If fixed subordinate bus number exists from EA
> +* capability then use it.
> +*/
> + if (fixed_sub)
> + max = fixed_sub;
> pci_bus_update_busn_res_end(child, max);
> pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max);
> }
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e1e9888..c3d0904 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -372,6 +372,12 @@
> #define PCI_EA_FIRST_ENT_BRIDGE8 /* First EA Entry for Bridges
> */
> #define PCI_EA_ES 0x0007 /* Entry Size */
> #define PCI_EA_BEI0x00f0 /* BAR E
Re: [PATCH v3 1/2] phy: zynqmp: Add phy driver for xilinx zynqmp phy core
Hi Anurag, On Wed, Sep 5, 2018 at 10:14 PM Anurag Kumar Vulisha wrote: > > ZynqMP SoC has a Gigabit Transceiver with four lanes. All the high speed > peripherals such as USB, SATA, PCIE, Display Port and Ethernet SGMII can > rely on any of the four GT lanes for PHY layer. This patch adds driver > for that ZynqMP GT core. > > Signed-off-by: Anurag Kumar Vulisha > --- > Changes in v3: > 1. Corrected the Documentation as suggested by Vivek Gautam > > Changes in v2: > 1. Fixed the compilation error when compiled phy-zynqmp.c as a module > 2. Added CONFIG_PM macro in phy-zynqmp.c driver > --- > drivers/phy/Kconfig|8 + > drivers/phy/Makefile |1 + > drivers/phy/phy-zynqmp.c | 1581 > > include/dt-bindings/phy/phy.h |2 + > include/linux/phy/phy-zynqmp.h | 52 ++ > 5 files changed, 1644 insertions(+) > create mode 100644 drivers/phy/phy-zynqmp.c > create mode 100644 include/linux/phy/phy-zynqmp.h > > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig > index 5c8d452..14cf3330 100644 > --- a/drivers/phy/Kconfig > +++ b/drivers/phy/Kconfig > @@ -40,6 +40,14 @@ config PHY_XGENE > help > This option enables support for APM X-Gene SoC multi-purpose PHY. > > +config PHY_XILINX_ZYNQMP > + tristate "Xilinx ZynqMP PHY driver" > + depends on ARCH_ZYNQMP > + select GENERIC_PHY > + help > + Enable this to support ZynqMP High Speed Gigabit Transceiver > + that is part of ZynqMP SoC. > + > source "drivers/phy/allwinner/Kconfig" > source "drivers/phy/amlogic/Kconfig" > source "drivers/phy/broadcom/Kconfig" > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile > index 84e3bd9..f2a8d27 100644 > --- a/drivers/phy/Makefile > +++ b/drivers/phy/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_GENERIC_PHY) += phy-core.o > obj-$(CONFIG_PHY_LPC18XX_USB_OTG) += phy-lpc18xx-usb-otg.o > obj-$(CONFIG_PHY_XGENE)+= phy-xgene.o > obj-$(CONFIG_PHY_PISTACHIO_USB)+= phy-pistachio-usb.o > +obj-$(CONFIG_PHY_XILINX_ZYNQMP)+= phy-zynqmp.o > obj-$(CONFIG_ARCH_SUNXI) += allwinner/ > obj-$(CONFIG_ARCH_MESON) += amlogic/ > obj-$(CONFIG_LANTIQ) += lantiq/ > diff --git a/drivers/phy/phy-zynqmp.c b/drivers/phy/phy-zynqmp.c > new file mode 100644 > index 000..84efc3d > --- /dev/null > +++ b/drivers/phy/phy-zynqmp.c > @@ -0,0 +1,1581 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * phy-zynqmp.c - PHY driver for Xilinx ZynqMP GT. > + * > + * Copyright (C) 2018 Xilinx Inc. > + * > + * Author: Anurag Kumar Vulisha This driver was initially written by me and sent for review till v2: https://lore.kernel.org/patchwork/patch/635317/ I remember there was no support for reset that time and could not test DP. I guess you should add my name too. Subbaraya Sundeep . Thanks, Sundeep > + * > + * This driver is tested for USB, SATA and Display Port currently. > + * Other controllers PCIe and SGMII should also work but that is > + * experimental as of now. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* Inter Connect Matrix parameters */ > +#define ICM_CFG0 0x10010 > +#define ICM_CFG1 0x10014 > +#define ICM_CFG0_L0_MASK 0x07 > +#define ICM_CFG0_L1_MASK 0x70 > +#define ICM_CFG1_L2_MASK 0x07 > +#define ICM_CFG2_L3_MASK 0x70 > +#define ICM_CFG_SHIFT 4 > + > +/* Inter Connect Matrix allowed protocols */ > +#define ICM_PROTOCOL_PD0x0 > +#define ICM_PROTOCOL_PCIE 0x1 > +#define ICM_PROTOCOL_SATA 0x2 > +#define ICM_PROTOCOL_USB 0x3 > +#define ICM_PROTOCOL_DP0x4 > +#define ICM_PROTOCOL_SGMII 0x5 > + > +/* Test Mode common reset control parameters */ > +#define TM_CMN_RST 0x10018 > +#define TM_CMN_RST_EN 0x1 > +#define TM_CMN_RST_SET 0x2 > +#define TM_CMN_RST_MASK0x3 > + > +/* Refclk selection parameters */ > +#define PLL_REF_SEL0 0x1 > +#define PLL_REF_OFFSET 0x4 > +#define PLL_FREQ_MASK 0x1F > +#define PLL_STATUS_READ_OFFSET 0x4000 > +#define PLL_STATUS_LOCKED 0x10 > + > +/* PLL SSC step size offsets */ > +#define L0_L0_REF_CLK_SEL 0x2860 > +#define L0_PLL_SS_STEPS_0_LSB 0x2368 > +#define L0_PLL_SS_STEPS_1_MSB 0x236C > +#define L0_PLL_SS_STEP_SIZE_0_LSB 0x2370 > +#define L0_PLL_SS_STEP_SIZE_1 0x2374 > +#define L0_PLL_SS_STEP_SIZE_2 0x2378 > +#define L0_PLL_SS_STEP_SIZE_3_MSB
Re: [PATCH 1/2] usb: chipidea: add xilinx zynq platform data
Hi,
On Wed, Aug 26, 2015 at 8:57 PM, Nathan Sullivan wrote:
> The Xilinx Zynq udc does not need the CI_HDRC_DISABLE_STREAMING flag,
> unlike the default platform data. Add platform data specific to the
> Zynq udc.
>
> Based on a patch by the same name from the Xilinx vendor tree.
I am that Xilinx guy who sent this patch :). It is in Xilinx tree as
temporary fix and
I did not debug further why UDC works only when streaming is enabled.
Probably this is right time to post my question here.
I was expecting like:
Streaming disabled - both low bandwidth and high bandwidth systems
should work fine
Streaming enabled - only for high bandwidth systems
but this is not the case, Zynq UDC works only when Streaming is enabled.
Please correct me if I am wrong.
Thanks,
Sundeep
>
> Signed-off-by: Nathan Sullivan
> ---
> drivers/usb/chipidea/ci_hdrc_usb2.c | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c
> b/drivers/usb/chipidea/ci_hdrc_usb2.c
> index 9eae1a1..4456d2c 100644
> --- a/drivers/usb/chipidea/ci_hdrc_usb2.c
> +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -30,18 +31,36 @@ static const struct ci_hdrc_platform_data
> ci_default_pdata = {
> .flags = CI_HDRC_DISABLE_STREAMING,
> };
>
> +static struct ci_hdrc_platform_data ci_zynq_pdata = {
> + .capoffset = DEF_CAPOFFSET,
> +};
> +
> +static const struct of_device_id ci_hdrc_usb2_of_match[] = {
> + { .compatible = "chipidea,usb2"},
> + { .compatible = "xlnx,zynq-usb-2.20a", .data = &ci_zynq_pdata},
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ci_hdrc_usb2_of_match);
> +
> static int ci_hdrc_usb2_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct ci_hdrc_usb2_priv *priv;
> struct ci_hdrc_platform_data *ci_pdata = dev_get_platdata(dev);
> int ret;
> + const struct of_device_id *match;
>
> if (!ci_pdata) {
> ci_pdata = devm_kmalloc(dev, sizeof(*ci_pdata), GFP_KERNEL);
> *ci_pdata = ci_default_pdata; /* struct copy */
> }
>
> + match = of_match_device(ci_hdrc_usb2_of_match, &pdev->dev);
> + if (match && match->data) {
> + /* struct copy */
> + *ci_pdata = *(struct ci_hdrc_platform_data *)match->data;
> + }
> +
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
> @@ -96,12 +115,6 @@ static int ci_hdrc_usb2_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> -static const struct of_device_id ci_hdrc_usb2_of_match[] = {
> - { .compatible = "chipidea,usb2" },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, ci_hdrc_usb2_of_match);
> -
> static struct platform_driver ci_hdrc_usb2_driver = {
> .probe = ci_hdrc_usb2_probe,
> .remove = ci_hdrc_usb2_remove,
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: DWC3 linux driver query
Hi, On Fri, May 29, 2015 at 8:35 PM, Felipe Balbi wrote: > On Fri, May 29, 2015 at 07:01:16PM +0530, sundeep subbaraya wrote: >> Hi Felipe, >> >> On Thu, May 28, 2015 at 7:41 PM, Felipe Balbi wrote: >> > Hi, >> > >> > On Thu, May 28, 2015 at 04:53:09PM +0530, sundeep subbaraya wrote: >> >> Hi Felipe and Paul, >> > >> > btw, Paul has left Synopys :-) >> >> ahh I see.. >> > >> >> I am seeing an issue while testing iperf for USB ethernet gadget with >> >> dwc3 controller in 2.0 mode. After debugging I figured out that: >> >> >> >> 1. Network gadget queues say 3 requests. (for IN endpoint) >> >> 2. It turns out with req.no_interrupt flag, >> >> DWC3 driver issues START_TRANSFER with req0:IOC, req1, req2:LST >> >> 3. As per driver state machine, we get XFERNOTREADY then prepare these >> >> TRBs and issue start transfer. Make Endpoint state as Busy. >> >> 4. Endpoint state is set to free in XFERINPROGRESS or XFERCOMPLETE event. >> >> 5. The issue I see here is there are NAKs going to host (seen in >> >> analyzer) in between req0 and req2 hence XFERNOTREADY(Transfer Active) >> >> events in between XFERINPROGRESS and XFERCOMPLETE events. >> >> 6. As a result, EP is set as free in XFERINPROGRESS, since EP is free >> >> start transfer command is issued in XFERNOTREADY handler.The command >> >> fails since controller did not release the xfer resource yet. >> >> >> >> I feel controller behaviour is fine since it sends NAK and writes that >> >> event. Driver may have to be modified to make EP as free only in >> >> XFERCOMPLETE event handler (ofcourse not for Isoc). >> > >> > this sounds like the correct solution. >> > >> >> Note I am testing on a platform which is very slow (the interface >> >> between DDR and core runs at 4Mhz). >> > >> > sweet :-) >> >> No it is not :) :). I struggled a lot while debugging :( >> >> > >> >> Where as on Zynq (DWC3 in PL), a faster system compared to above one I >> > >> > hey, when will we see a glue layer for Zynq ? :-) >> >> we don't have hardware with 2.0 and 3.0 PHY together currently. I will >> write and test >> once the hardware is ready :) >> >> > >> >> do not see any NAKs in between Start transfer command and XFERCOMPLETE >> >> event. >> >> >> >> What do you guys say? Do you agree linux driver has to be modified or >> >> Core should never issue NAKs in between Start transfer and >> >> XFERCOMPLETE? >> > >> > well, if we queued enough transfers, it shouldn't NAK. OTOH, we >> > shouldn't allow for a new StartTransfer command until all pending >> > requests have been transferred. >> >> Yes correct but the hardware design is very slow here so hitting this case >> > >> >> A patch correcting DEPCMD status macros has been applied. Thank you >> >> Felipe for trace points in driver otherwise it would have taken very >> >> long time to figure out the root cause :) . >> > >> > yeah, those are really helpful :-) >> > >> >> Below is the trace log:(enabled only for IN bulk endpoint) >> >> >> >> irq/97-dwc3-1308 [001] d... 553.713513: dwc3_msg: ep1in-bulk >> >> XFERNOTREADY.Transfer Not Active >> >> irq/97-dwc3-1308 [001] d... 553.713768: dwc3_msg: ep1in-bulk: >> >> req ffc039a68580 dma 011c60a2 length 1558 IOC >> >> >> >> irq/97-dwc3-1308 [001] d... 553.714266: dwc3_msg: ep1in-bulk: >> >> req ffc039a687c0 dma 011c10a2 length 1558 >> >> >> >> irq/97-dwc3-1308 [001] d... 553.714753: dwc3_msg: ep1in-bulk: >> >> req ffc039a68700 dma 011c18a2 length 1558 last IOC >> >> >> >> irq/97-dwc3-1308 [001] d... 553.717768: dwc3_msg: ep1in-bulk >> >> XFERNOTREADY.Transfer Active >> >> irq/97-dwc3-1308 [001] d... 553.718203: dwc3_msg: ep1in-bulk EP >> >> BUSY >> >> irq/97-dwc3-1308 [001] d... 553.718412: dwc3_msg: ep1in-bulk >> >> XFERNOTREADY.Transfer Active >> >> irq/97-dwc3-1308 [001] d... 553.718638: dwc3_msg: ep1in-bulk EP >> >> BUSY >> >> irq/97-dwc3-1308 [001] d... 553.718837: dwc3_msg: ep1in-bulk >> >> XFERNOTREADY.Transfer Active >> &g
Re: DWC3 linux driver query
Hi Felipe, On Thu, May 28, 2015 at 7:41 PM, Felipe Balbi wrote: > Hi, > > On Thu, May 28, 2015 at 04:53:09PM +0530, sundeep subbaraya wrote: >> Hi Felipe and Paul, > > btw, Paul has left Synopys :-) ahh I see.. > >> I am seeing an issue while testing iperf for USB ethernet gadget with >> dwc3 controller in 2.0 mode. After debugging I figured out that: >> >> 1. Network gadget queues say 3 requests. (for IN endpoint) >> 2. It turns out with req.no_interrupt flag, >> DWC3 driver issues START_TRANSFER with req0:IOC, req1, req2:LST >> 3. As per driver state machine, we get XFERNOTREADY then prepare these >> TRBs and issue start transfer. Make Endpoint state as Busy. >> 4. Endpoint state is set to free in XFERINPROGRESS or XFERCOMPLETE event. >> 5. The issue I see here is there are NAKs going to host (seen in >> analyzer) in between req0 and req2 hence XFERNOTREADY(Transfer Active) >> events in between XFERINPROGRESS and XFERCOMPLETE events. >> 6. As a result, EP is set as free in XFERINPROGRESS, since EP is free >> start transfer command is issued in XFERNOTREADY handler.The command >> fails since controller did not release the xfer resource yet. >> >> I feel controller behaviour is fine since it sends NAK and writes that >> event. Driver may have to be modified to make EP as free only in >> XFERCOMPLETE event handler (ofcourse not for Isoc). > > this sounds like the correct solution. > >> Note I am testing on a platform which is very slow (the interface >> between DDR and core runs at 4Mhz). > > sweet :-) No it is not :) :). I struggled a lot while debugging :( > >> Where as on Zynq (DWC3 in PL), a faster system compared to above one I > > hey, when will we see a glue layer for Zynq ? :-) we don't have hardware with 2.0 and 3.0 PHY together currently. I will write and test once the hardware is ready :) > >> do not see any NAKs in between Start transfer command and XFERCOMPLETE >> event. >> >> What do you guys say? Do you agree linux driver has to be modified or >> Core should never issue NAKs in between Start transfer and >> XFERCOMPLETE? > > well, if we queued enough transfers, it shouldn't NAK. OTOH, we > shouldn't allow for a new StartTransfer command until all pending > requests have been transferred. Yes correct but the hardware design is very slow here so hitting this case > >> A patch correcting DEPCMD status macros has been applied. Thank you >> Felipe for trace points in driver otherwise it would have taken very >> long time to figure out the root cause :) . > > yeah, those are really helpful :-) > >> Below is the trace log:(enabled only for IN bulk endpoint) >> >> irq/97-dwc3-1308 [001] d... 553.713513: dwc3_msg: ep1in-bulk >> XFERNOTREADY.Transfer Not Active >> irq/97-dwc3-1308 [001] d... 553.713768: dwc3_msg: ep1in-bulk: >> req ffc039a68580 dma 011c60a2 length 1558 IOC >> >> irq/97-dwc3-1308 [001] d... 553.714266: dwc3_msg: ep1in-bulk: >> req ffc039a687c0 dma 011c10a2 length 1558 >> >> irq/97-dwc3-1308 [001] d... 553.714753: dwc3_msg: ep1in-bulk: >> req ffc039a68700 dma 011c18a2 length 1558 last IOC >> >> irq/97-dwc3-1308 [001] d... 553.717768: dwc3_msg: ep1in-bulk >> XFERNOTREADY.Transfer Active >> irq/97-dwc3-1308 [001] d... 553.718203: dwc3_msg: ep1in-bulk EP BUSY >> irq/97-dwc3-1308 [001] d... 553.718412: dwc3_msg: ep1in-bulk >> XFERNOTREADY.Transfer Active >> irq/97-dwc3-1308 [001] d... 553.718638: dwc3_msg: ep1in-bulk EP BUSY >> irq/97-dwc3-1308 [001] d... 553.718837: dwc3_msg: ep1in-bulk >> XFERNOTREADY.Transfer Active >> irq/97-dwc3-1308 [001] d... 553.719049: dwc3_msg: ep1in-bulk EP BUSY >> irq/97-dwc3-1308 [001] d... 553.719248: dwc3_msg: ep1in-bulk >> XFERINPROGRESS >> irq/97-dwc3-1308 [001] d... 553.719520: dwc3_msg: request >> ffc039a68580 from ep1in-bulk completed 1558/1558 ===> 0 >> >> irq/97-dwc3-1308 [001] d... 553.720225: dwc3_msg: ep1in-bulk >> XFERNOTREADY.Transfer Active >> irq/97-dwc3-1308 [001] d... 553.720612: dwc3_msg: ep1in-bulk >> XFERNOTREADY.Transfer Active >> irq/97-dwc3-1308 [001] d... 553.720826: dwc3_msg: ep1in-bulk EP BUSY >> irq/97-dwc3-1308 [001] d... 553.721026: dwc3_msg: ep1in-bulk >> XFERNOTREADY.Transfer Active >> irq/97-dwc3-1308 [001] d... 553.721243: dwc3_msg: ep1in-bulk EP BUSY >> irq/97-dwc3-1308 [001] d... 553.721446: dwc3_msg: ep1in-bulk >> XFERCOMPLETE >> irq/97-dwc3-
DWC3 linux driver query
Hi Felipe and Paul, I am seeing an issue while testing iperf for USB ethernet gadget with dwc3 controller in 2.0 mode. After debugging I figured out that: 1. Network gadget queues say 3 requests. (for IN endpoint) 2. It turns out with req.no_interrupt flag, DWC3 driver issues START_TRANSFER with req0:IOC, req1, req2:LST 3. As per driver state machine, we get XFERNOTREADY then prepare these TRBs and issue start transfer. Make Endpoint state as Busy. 4. Endpoint state is set to free in XFERINPROGRESS or XFERCOMPLETE event. 5. The issue I see here is there are NAKs going to host (seen in analyzer) in between req0 and req2 hence XFERNOTREADY(Transfer Active) events in between XFERINPROGRESS and XFERCOMPLETE events. 6. As a result, EP is set as free in XFERINPROGRESS, since EP is free start transfer command is issued in XFERNOTREADY handler.The command fails since controller did not release the xfer resource yet. I feel controller behaviour is fine since it sends NAK and writes that event. Driver may have to be modified to make EP as free only in XFERCOMPLETE event handler (ofcourse not for Isoc). Note I am testing on a platform which is very slow (the interface between DDR and core runs at 4Mhz). Where as on Zynq (DWC3 in PL), a faster system compared to above one I do not see any NAKs in between Start transfer command and XFERCOMPLETE event. What do you guys say? Do you agree linux driver has to be modified or Core should never issue NAKs in between Start transfer and XFERCOMPLETE? A patch correcting DEPCMD status macros has been applied. Thank you Felipe for trace points in driver otherwise it would have taken very long time to figure out the root cause :) . Below is the trace log:(enabled only for IN bulk endpoint) irq/97-dwc3-1308 [001] d... 553.713513: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Not Active irq/97-dwc3-1308 [001] d... 553.713768: dwc3_msg: ep1in-bulk: req ffc039a68580 dma 011c60a2 length 1558 IOC irq/97-dwc3-1308 [001] d... 553.714266: dwc3_msg: ep1in-bulk: req ffc039a687c0 dma 011c10a2 length 1558 irq/97-dwc3-1308 [001] d... 553.714753: dwc3_msg: ep1in-bulk: req ffc039a68700 dma 011c18a2 length 1558 last IOC irq/97-dwc3-1308 [001] d... 553.717768: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.718203: dwc3_msg: ep1in-bulk EP BUSY irq/97-dwc3-1308 [001] d... 553.718412: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.718638: dwc3_msg: ep1in-bulk EP BUSY irq/97-dwc3-1308 [001] d... 553.718837: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.719049: dwc3_msg: ep1in-bulk EP BUSY irq/97-dwc3-1308 [001] d... 553.719248: dwc3_msg: ep1in-bulk XFERINPROGRESS irq/97-dwc3-1308 [001] d... 553.719520: dwc3_msg: request ffc039a68580 from ep1in-bulk completed 1558/1558 ===> 0 irq/97-dwc3-1308 [001] d... 553.720225: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.720612: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.720826: dwc3_msg: ep1in-bulk EP BUSY irq/97-dwc3-1308 [001] d... 553.721026: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.721243: dwc3_msg: ep1in-bulk EP BUSY irq/97-dwc3-1308 [001] d... 553.721446: dwc3_msg: ep1in-bulk XFERCOMPLETE irq/97-dwc3-1308 [001] d... 553.721711: dwc3_msg: request ffc039a687c0 from ep1in-bulk completed 1558/1558 ===> 0 irq/97-dwc3-1308 [001] d... 553.722411: dwc3_msg: request ffc039a68700 from ep1in-bulk completed 1558/1558 ===> 0 irq/97-dwc3-1308 [001] d... 553.722910: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Not Active irq/97-dwc3-1308 [001] d... 553.723159: dwc3_msg: ep1in-bulk: req ffc039a68ac0 dma 398b18a2 length 1558 irq/97-dwc3-1308 [001] d... 553.723649: dwc3_msg: ep1in-bulk: req ffc039a68c40 dma 3a1ce8a2 length 1558 irq/97-dwc3-1308 [001] d... 553.724136: dwc3_msg: ep1in-bulk: req ffc039a68580 dma 3cc258a2 length 1558 last irq/97-dwc3-1308 [001] d... 553.724722: dwc3_msg: CMD Error:1 for ep 3 irq/97-dwc3-1308 [001] d... 553.727245: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.727767: dwc3_msg: CMD Error:1 for ep 3 irq/97-dwc3-1308 [001] d... 553.728049: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.728394: dwc3_msg: CMD Error:1 for ep 3 irq/97-dwc3-1308 [001] d... 553.731226: dwc3_msg: ep1in-bulk XFERNOTREADY.Transfer Active irq/97-dwc3-1308 [001] d... 553.731658: dwc3_msg: CMD Error:1 for ep 3 Thanks, Sundeep -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vg
Re: query on DWC3
Hi Felipe, On Thu, Jan 8, 2015 at 10:27 PM, Felipe Balbi wrote: > Hi, > > On Tue, Jan 06, 2015 at 12:39:55PM +0530, sundeep subbaraya wrote: >> >> On Sun, Dec 14, 2014 at 08:39:18AM +0530, sundeep subbaraya wrote: >> >> > Hi Paul, >> >> > >> >> > As per my understanding, for BULK OUT we do queue a request with 512 >> >> > bytes length since we do not >> >> >> >> sometimes we _do_ know the size. In case of Mass Storage, we _know_ that >> >> the first bulk out transfer will be 31-bytes (CBW), if we were to start >> >> a 31-byte transfer, we would't receive anything. >> >> >> >> > know the length of the transfer Host is going to send. For Control OUT >> >> > we know the length in wLength of >> >> > Setup packet, hence I assumed there is no difference in programming >> >> > model of Control IN and OUT. >> >> >> >> there is _no_ difference. It's just that it was agreed that for anything >> >> other than control ep, the function drivers would take care of it. See >> >> the uses of quirk_ep_out_aligned_size. >> >> got it..:) >> >> > >> > btw, why are you reimplementing the driver when there's a perfectly good >> > driver to use in mainline kernel ? >> >> I am writing a bare metal driver and it didn't work without alignment >> check mentioned above. > > just make sure you don't copy GPL code into your bare metal driver, > otherwise your bare metal driver will be GPL by definition. Also, since I didn't copy linux code. I have gone through dwc3 driver while writing udc-xilinx.c and now it is helping me to understand dwc3 controller quickly.:) > you're not using Linux at all, you should be asking support from > Synopsys instead, not the Linux USB mailing list. Sure will ask support from Synopsis. Thanks, Sundeep > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: query on DWC3
Hi, On Mon, Dec 22, 2014 at 9:42 PM, Felipe Balbi wrote: > Hi again, > > On Mon, Dec 22, 2014 at 10:11:23AM -0600, Felipe Balbi wrote: >> (please don't top-post) Sure. >> >> On Sun, Dec 14, 2014 at 08:39:18AM +0530, sundeep subbaraya wrote: >> > Hi Paul, >> > >> > As per my understanding, for BULK OUT we do queue a request with 512 >> > bytes length since we do not >> >> sometimes we _do_ know the size. In case of Mass Storage, we _know_ that >> the first bulk out transfer will be 31-bytes (CBW), if we were to start >> a 31-byte transfer, we would't receive anything. >> >> > know the length of the transfer Host is going to send. For Control OUT >> > we know the length in wLength of >> > Setup packet, hence I assumed there is no difference in programming >> > model of Control IN and OUT. >> >> there is _no_ difference. It's just that it was agreed that for anything >> other than control ep, the function drivers would take care of it. See >> the uses of quirk_ep_out_aligned_size. got it..:) > > btw, why are you reimplementing the driver when there's a perfectly good > driver to use in mainline kernel ? I am writing a bare metal driver and it didn't work without alignment check mentioned above. > > -- > balbi Thanks, Sundeep.B.S. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: query on DWC3
Hi Paul,
As per my understanding, for BULK OUT we do queue a request with 512
bytes length since we do not
know the length of the transfer Host is going to send. For Control OUT
we know the length in wLength of
Setup packet, hence I assumed there is no difference in programming
model of Control IN and OUT.
Now it is clear for me. Thanks for the clarification :)
Sundeep
On Sun, Dec 14, 2014 at 5:21 AM, Paul Zimmerman
wrote:
>> From: linux-usb-ow...@vger.kernel.org
>> [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of sundeep subbaraya
>> Sent: Friday, December 12, 2014 9:13 PM
>>
>> Hi Felipe,
>>
>> In DWC3 driver, for three stage Control OUT transfer there is a check:
>>
>>else if (!IS_ALIGNED(req->request.length, dep->endpoint.maxpacket)
>>
>> && (dep->number == 0)) {.
>> }
>>
>> I understand that we check for alignment of sizes and if not we
>> prepare trb with maxpacket and enable interrupt on short packet. In
>> databook I could not find anything related to this, it talks only
>> about addresses should be aligned. In Control transfer programming
>> model there is no difference between Control OUT or IN transfer, if
>> three stage transfer - prepare trb with length wLength. Initially I
>> followed this and not able to receive data on EP0 OUT.:( .After adding
>> the above check it is working. Please help me to understand why we do
>> this?
>
> Hi Sundeep,
>
> Please see section 8.2.3.3 "Buffer Size Rules and Zero-Length Packets"
> in the databook:
>
> For OUT endpoints, the following rules apply:
>
> - The total size of a Buffer Descriptor must be a multiple of
> MaxPacketSize
>
> The wording may be a little confusing, it actually means that the size
> of the data buffer for OUT endpoints must be a multiple of MaxPacketSize.
> Section 8.2.5 states it more clearly:
>
> - An OUT transfer’s transfer size (Total TRB buffer allocation)
> must be a multiple of MaxPacketSize even if software is expecting
> a fixed non-multiple of MaxPacketSize transfer from the Host.
>
> This rule applies to all OUT endpoint types, including Control endpoints.
>
> --
> Paul
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
query on DWC3
Hi Felipe,
In DWC3 driver, for three stage Control OUT transfer there is a check:
else if (!IS_ALIGNED(req->request.length, dep->endpoint.maxpacket)
&& (dep->number == 0)) {.
}
I understand that we check for alignment of sizes and if not we
prepare trb with maxpacket and enable interrupt on short packet. In
databook I could not find anything related to this, it talks only
about addresses should be aligned. In Control transfer programming
model there is no difference between Control OUT or IN transfer, if
three stage transfer - prepare trb with length wLength. Initially I
followed this and not able to receive data on EP0 OUT.:( .After adding
the above check it is working. Please help me to understand why we do
this?
Thank in advance,
Sundeep.B.S.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: axienet: remove unnecessary ether_setup after alloc_etherdev
Thanks David, Sundeep. On Sat, Sep 13, 2014 at 3:46 AM, David Miller wrote: > From: Subbaraya Sundeep Bhatta > Date: Thu, 11 Sep 2014 14:53:33 +0530 > >> calling ether_setup is redundant since alloc_etherdev calls >> it. >> >> Signed-off-by: Subbaraya Sundeep Bhatta > > Applied. > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iio: adc: xilinx-xadc: assign auxiliary channels address correctly
Thanks Lars and Jonathan,
Sundeep.
On Sun, Sep 14, 2014 at 1:34 AM, Jonathan Cameron wrote:
> On 12/09/14 18:25, Lars-Peter Clausen wrote:
>> On 09/11/2014 10:55 AM, Subbaraya Sundeep Bhatta wrote:
>>> This patch fixes incorrect logic for assigning address
>>> to auxiliary channels of xilinx xadc.
>>>
>>> Signed-off-by: Subbaraya Sundeep Bhatta
>>
>> Acked-by: Lars-Peter Clausen
> Applied to the fixes-togreg branch of iio.git and flagged
> for stable.
>
> Thanks,
>
> Jonathan
>>
>>> ---
>>> drivers/iio/adc/xilinx-xadc-core.c |2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/xilinx-xadc-core.c
>>> b/drivers/iio/adc/xilinx-xadc-core.c
>>> index fd2745c..626b397 100644
>>> --- a/drivers/iio/adc/xilinx-xadc-core.c
>>> +++ b/drivers/iio/adc/xilinx-xadc-core.c
>>> @@ -1126,7 +1126,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev,
>>> struct device_node *np,
>>> chan->address = XADC_REG_VPVN;
>>> } else {
>>> chan->scan_index = 15 + reg;
>>> -chan->scan_index = XADC_REG_VAUX(reg - 1);
>>> +chan->address = XADC_REG_VAUX(reg - 1);
>>> }
>>> num_channels++;
>>> chan++;
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
Hi Felipe, On Thu, Aug 21, 2014 at 7:30 PM, Felipe Balbi wrote: > On Thu, Aug 21, 2014 at 12:19:03PM +0530, sundeep subbaraya wrote: >> Hi Daniel, >> >> On Tue, Aug 19, 2014 at 3:26 PM, Daniel Mack wrote: >> > Hi, >> > >> > On 07/22/2014 11:08 AM, Subbaraya Sundeep Bhatta wrote: >> >> This patch adds xilinx usb2 device driver support >> > >> > Add some more information here, please. Copying the text from the >> > Kconfig option is already a good start. >> > >> >> drivers/usb/gadget/Kconfig | 14 + >> >> drivers/usb/gadget/Makefile |1 + >> >> drivers/usb/gadget/udc-xilinx.c | 2261 >> >> +++ >> >> 3 files changed, 2276 insertions(+), 0 deletions(-) >> >> create mode 100644 drivers/usb/gadget/udc-xilinx.c >> > >> > As your driver has a DT binding, you have to describe it in >> > Documentation/devicetree/bindings. >> > >> >> --- a/drivers/usb/gadget/Kconfig >> >> +++ b/drivers/usb/gadget/Kconfig >> >> @@ -459,6 +459,20 @@ config USB_EG20T >> >> ML7213/ML7831 is companion chip for Intel Atom E6xx series. >> >> ML7213/ML7831 is completely compatible for Intel EG20T PCH. >> >> >> >> +config USB_GADGET_XILINX >> >> + tristate "Xilinx USB Driver" >> >> + depends on COMPILE_TEST >> > >> > Why would you depend on that? >> >> Felipe asked to make this since this is USB soft IP driver and its >> dependencies have tendency to grow. > > that's not exactly what I asked :-) Usually you only add COMPILE_TEST > when you have an ARCH dependency. So something like: > > depends on ARCH_ARM || COMPILE_TEST > > would make it clear that this driver is only available on ARM, but when > doing my build tests, I'd still be able to compile it on x86. Ok got it :) > >> > Also, your code uses device tree functions unconditionally, which is >> > fine, but it must hence depend on 'OF'. >> >> Ok will add OF along with COMPILE_TEST > > so this would be: > > depends on OF || COMPILE_TEST Ok Thanks, Sundeep > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
Hi Daniel,
On Tue, Aug 19, 2014 at 3:26 PM, Daniel Mack wrote:
> Hi,
>
> On 07/22/2014 11:08 AM, Subbaraya Sundeep Bhatta wrote:
>> This patch adds xilinx usb2 device driver support
>
> Add some more information here, please. Copying the text from the
> Kconfig option is already a good start.
>
>> drivers/usb/gadget/Kconfig | 14 +
>> drivers/usb/gadget/Makefile |1 +
>> drivers/usb/gadget/udc-xilinx.c | 2261
>> +++
>> 3 files changed, 2276 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/usb/gadget/udc-xilinx.c
>
> As your driver has a DT binding, you have to describe it in
> Documentation/devicetree/bindings.
>
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -459,6 +459,20 @@ config USB_EG20T
>> ML7213/ML7831 is companion chip for Intel Atom E6xx series.
>> ML7213/ML7831 is completely compatible for Intel EG20T PCH.
>>
>> +config USB_GADGET_XILINX
>> + tristate "Xilinx USB Driver"
>> + depends on COMPILE_TEST
>
> Why would you depend on that?
Felipe asked to make this since this is USB soft IP driver and its
dependencies have tendency to grow.
Currently tested on arm and microblaze architectures.
>
> Also, your code uses device tree functions unconditionally, which is
> fine, but it must hence depend on 'OF'.
Ok will add OF along with COMPILE_TEST
>
>> +struct xusb_ep {
>> + struct usb_ep ep_usb;
>> + struct list_head queue;
>> + struct xusb_udc *udc;
>> + const struct usb_endpoint_descriptor *desc;
>> + u32 rambase;
>> + u32 offset;
>> + char name[4];
>> + u16 epnumber;
>> + u16 maxpacket;
>> + u16 buffer0count;
>> + u16 buffer1count;
>> + u8 buffer0ready;
>> + u8 buffer1ready;
>> + u8 eptype;
>> + u8 curbufnum;
>> + u8 is_in;
>> + u8 is_iso;
>
> Some of those could probably become booleans.
Ok
>
>> +struct xusb_udc {
>> + struct usb_gadget gadget;
>> + struct xusb_ep ep[8];
>> + struct usb_gadget_driver *driver;
>> + struct usb_ctrlrequest setup;
>> + struct xusb_req *req;
>> + struct device *dev;
>> + u32 usb_state;
>> + u32 remote_wkp;
>> + u32 setupseqtx;
>> + u32 setupseqrx;
>> + void __iomem *base_address;
>> + spinlock_t lock;
>> + bool dma_enabled;
>> +
>> + unsigned int (*read_fn)(void __iomem *);
>> + void (*write_fn)(void __iomem *, u32, u32);
>> +};
>> +
>> +/* Endpoint buffer start addresses in the core */
>> +static u32 rambase[8] = { 0x22, 0x1000, 0x1100, 0x1200, 0x1300, 0x1400,
>> 0x1500,
>> + 0x1600 };
>
> As stated by Peter, indenting such lines to match the position of the
> line before makes such code much prettier and more readable. It's also
> done in the majority of drivers.
>
> This counts for many locations in your code.
Ok
>
>> +/* Control endpoint configuration.*/
>> +static const struct usb_endpoint_descriptor config_bulk_out_desc = {
>> + .bLength= USB_DT_ENDPOINT_SIZE,
>> + .bDescriptorType= USB_DT_ENDPOINT,
>> + .bEndpointAddress = USB_DIR_OUT,
>> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
>> + .wMaxPacketSize = cpu_to_le16(64),
>
> Why not use EP0_MAX_PACKET here?
Ok
>
>> +static void xudc_wrstatus(struct xusb_udc *udc)
>> +{
>> + struct xusb_ep *ep0 = &udc->ep[XUSB_EP_NUMBER_ZERO];
>> + u32 epcfgreg;
>> +
>> + epcfgreg = udc->read_fn(udc->base_address + ep0->offset)|
>> + XUSB_EP_CFG_DATA_TOGGLE_MASK;
>> + udc->write_fn(udc->base_address, ep0->offset, epcfgreg);
>> + udc->write_fn(udc->base_address, ep0->offset +
>> XUSB_EP_BUF0COUNT_OFFSET,
>> + 0);
>
> Just a nit, but renaming 'base_address' to something like 'base' or
> 'addr' would help you get around or maximum line constraint here and in
> some other places.
Ok i will do this
>
>> +static int xudc_dma_send(struct xusb_ep *ep, struct xusb_req *req,
>> + u8 *buffer, u32 length)
>> +{
>> + u32 *eprambase;
>> + dma_addr_t src;
>> + dma_addr_t dst;
>> + int ret;
>> + struct xusb_udc *udc = ep->udc;
>> +
>> + src = req->usb_req.dma + req->usb_req.actual;
>> + if (req->usb_req.length)
>> + dma_sync_single_for_device(udc->dev, src,
>> + length, DMA_TO_DEVICE);
>> + if (!ep->curbufnum && !ep->buffer0ready) {
>> + /* Get the Buffer address and copy the transmit data.*/
>> + eprambase = (u32 __force *)(udc->base_address +
>> + ep->rambase);
>> + dst = virt_to_phys(eprambase);
>> + udc->write_fn(udc->base_address, ep->offset +
>> + XUSB_EP_BUF0COUNT_OFFSET, length);
>> + udc->write_fn(udc->base_address, XUSB_DMA_CONTROL_OFFSET,
>> + XUSB_DMA_BRR_CTRL | (1 << ep->epnumber));
>> +
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
Hi,
On Tue, Jul 22, 2014 at 3:32 PM, Varka Bhadram wrote:
> On 07/22/2014 02:38 PM, Subbaraya Sundeep Bhatta wrote:
>>
>> +#include
>> +#include
>> +#include
>> +#include "gadget_chips.h"
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>
>
> Normally we will put the includes in alphabetical because it looks good
> and readable.
>
> But we have to include the local headers separately after all the main
> includes...
> #include
> #include
>
> #include
>
> #include "gadget_chips.h"
>
> (...)
>
Ok
>
>>
>> + switch (udc->setupseqtx) {
>> + case STATUS_PHASE:
>> + switch (udc->setup.bRequest) {
>> + case USB_REQ_SET_ADDRESS:
>> + /* Set the address of the device.*/
>> + udc->write_fn(udc->base_address,
>> + XUSB_ADDRESS_OFFSET,
>> + udc->setup.wValue);
>
>
> Should match open parenthesis
not necessary, as per coding style spaces should not be used --
Outside of comments, documentation and except in Kconfig, spaces are never
used for indentation.
>
> udc->write_fn(udc->base_address,
> XUSB_ADDRESS_OFFSET,
>
> udc->setup.wValue);
>
>
>> + break;
>> + case USB_REQ_SET_FEATURE:
>> + if (udc->setup.bRequestType ==
>> + USB_RECIP_DEVICE) {
>> + if (udc->setup.wValue ==
>> + USB_DEVICE_TEST_MODE)
>> + udc->write_fn(udc->base_address,
>> +
>> XUSB_TESTMODE_OFFSET,
>> + test_mode);
>
>
> Dto
not necessary
>
>
>> + }
>> + break;
>> + }
>> + req->usb_req.actual = req->usb_req.length;
>> + xudc_done(ep0, req, 0);
>> + break;
>> + case DATA_PHASE:
>> + if (!bytes_to_tx) {
>> + /*
>> +* We're done with data transfer, next
>> +* will be zero length OUT with data toggle of
>> +* 1. Setup data_toggle.
>> +*/
>> + epcfgreg = udc->read_fn(udc->base_address +
>> + ep0->offset);
>> + epcfgreg |= XUSB_EP_CFG_DATA_TOGGLE_MASK;
>> + udc->write_fn(udc->base_address, ep0->offset,
>> epcfgreg);
>> + udc->setupseqtx = STATUS_PHASE;
>> + } else {
>> + length = count = min_t(u32, bytes_to_tx,
>> + EP0_MAX_PACKET);
>> + /* Copy the data to be transmitted into the DPRAM.
>> */
>> + ep0rambase = (u8 __force *) (udc->base_address +
>> + (ep0->rambase << 2));
>> +
>> + buffer = req->usb_req.buf + req->usb_req.actual;
>> + req->usb_req.actual = req->usb_req.actual +
>> length;
>> + memcpy((void *)ep0rambase, buffer, length);
>> + }
>> + udc->write_fn(udc->base_address, XUSB_EP_BUF0COUNT_OFFSET,
>> + count);
>> + udc->write_fn(udc->base_address, XUSB_BUFFREADY_OFFSET,
>> 1);
>> + break;
>> + default:
>> + break;
>> + }
>> +}
>> +
>> +/**
>> + * xudc_ctrl_ep_handler - Endpoint 0 interrupt handler.
>> + * @udc: pointer to the udc structure.
>> + * @intrstatus:It's the mask value for the interrupt sources on
>> endpoint 0.
>> + *
>> + * Processes the commands received during enumeration phase.
>> + */
>> +static void xudc_ctrl_ep_handler(struct xusb_udc *udc, u32 intrstatus)
>> +{
>> +
>> + if (intrstatus & XUSB_STATUS_SETUP_PACKET_MASK) {
>> + xudc_handle_setup(udc);
>> + } else {
>> + if (intrstatus & XUSB_STATUS_FIFO_BUFF_RDY_MASK)
>> + xudc_ep0_out(udc);
>> + else if (intrstatus & XUSB_STATUS_FIFO_BUFF_FREE_MASK)
>> + xudc_ep0_in(udc);
>> + }
>> +}
>> +
>> +/**
>> + * xudc_nonctrl_ep_handler - Non control endpoint interrupt handler.
>> + * @udc: pointer to the udc structure.
>> + * @epnum: End point number for which the interrupt is to be processed
>> + * @intrstatus:mask value for interrupt sources of endpoints
>> other
>> + * than endpoint 0.
>> + *
>> + * Processes the buffer completion interrupts.
>> + */
>> +static void xudc_nonctrl_ep_handler(struct xusb_udc *udc, u8 epnum,
>> + u32 intrstatus)
>
>
> Should match open parenthesis:
not necessary
>
>
> sta
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe, On Wed, Jul 2, 2014 at 10:16 PM, Felipe Balbi wrote: > Hi, > > On Sun, May 25, 2014 at 11:10:30PM +0530, sundeep subbaraya wrote: >> Hi Felipe, >> >> Please take a look at below about how this IP works: >> >> IN: >> req.buf ---> DMA (transfers from ddr to IP buffer, raise DMA >> done interrupt and set Buffer ready to transfer data to Host)>Host >> PC >> buffer sent interrupt >> >> OUT: >> Host PC--->buffer ready interrupt--->DMA (transfer from IP buffer >> to DDR,DMA done interrupt, set Buffer ready to receive next data from >> Host)-->req.buf >> >> I written logic to call completion in DMA done handler because it >> works for both IN and OUT eps. But I see significant performance >> degradation (by copying a file to mass storage gadget). DMA can handle >> unaligned address too but it has ep max limit (say 512 for bulk). >> Hence it is SW job to split packets and to drive DMA till req.length >> completes. I feel polling for a while is faster than switching between >> interrupt handler back and forth rapidly. Moreover, DMA may or may not >> be present in IP based on user configuration at design time. >> >> I fixed all other comments expect this one if you do not agree for >> polling then I may have to create threads and do this stuff. What do >> you suggest? > > Can you resend your driver so we can review again everything you have > fixed ? I guess you have no other way... polling it is. Sure. I will resend the driver. Thanks, Sundeep.B.S. > > cheers > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] usb: doc: udc-xilinx: Add devicetree bindings
Hi,
On Fri, Jul 4, 2014 at 10:10 PM, Mark Rutland wrote:
> On Tue, Jun 24, 2014 at 07:44:10AM +0100, sundeep subbaraya wrote:
>> Ping
>>
>> Thanks,
>> Sundeep.B.S.
>>
>> On Tue, Jun 10, 2014 at 5:34 PM,
>> wrote:
>> > From: Subbaraya Sundeep Bhatta
>> >
>> > Add devicetree bindings for Xilinx axi udc driver.
>> >
>> > Signed-off-by: Subbaraya Sundeep Bhatta
>> > ---
>> > Changes for v3:
>> > - None
>> > 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"
>
> That's an interesting name. What's the product name? Is the a manual?
Here is the manual for the IP :
http://www.xilinx.com/support/documentation/ip_documentation/axi_usb2_device/v5_0/pg137-axi-usb2-device.pdf
This driver works with PLB IP version that's why maybe make more sense
to use skip axi from compatible string and use just
xlnx,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.
>
> Describe what this logically is. Everyone knows that this is a property
> with a value, and everyone knows that an interrupts property describes
> interrupts.
>
> The ket point is _which_ interrupts this describes.
Ok
>
>> > +- interrupt-parent : Must be core interrupt controller
>
> This isn't strictly necessary anyway.
>
>> > +- xlnx,include-dma : if DMA is included
>
> You appear to have forgotten to fix this name, judging by the changelog
> and example.
>
>> > +
>> > +Example:
>> > + axi-usb2-device@42e0 {
>> > +compatible = "xlnx,axi-usb2-device-4.00.a";
>> > +interrupt-parent = <0x1>;
>
> Huh? Who writes a raw phandle value?
>
>> > +interrupts = <0x0 0x39 0x1>;
>> > +reg = <0x42e0 0x1>;
>> > +xlnx,has-builtin-dma;
>
> As mentioned above, this conflicts with the binding documentation.
> Please ensure the example is aligned with the documentation.
Thank you for pointing this i sent an old one, will fix this.
Sundeep.B.S.
>
> Thanks,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] usb: doc: udc-xilinx: Add devicetree bindings
Ping
Thanks,
Sundeep.B.S.
On Tue, Jun 10, 2014 at 5:34 PM, wrote:
> From: Subbaraya Sundeep Bhatta
>
> Add devicetree bindings for Xilinx axi udc driver.
>
> Signed-off-by: Subbaraya Sundeep Bhatta
> ---
> Changes for v3:
> - None
> 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,include-dma : if DMA is included
> +
> +Example:
> + axi-usb2-device@42e0 {
> +compatible = "xlnx,axi-usb2-device-4.00.a";
> +interrupt-parent = <0x1>;
> +interrupts = <0x0 0x39 0x1>;
> +reg = <0x42e0 0x1>;
> +xlnx,has-builtin-dma;
> +};
> +
> --
> 1.7.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe, Please take a look at below about how this IP works: IN: req.buf ---> DMA (transfers from ddr to IP buffer, raise DMA done interrupt and set Buffer ready to transfer data to Host)>Host PC buffer sent interrupt OUT: Host PC--->buffer ready interrupt--->DMA (transfer from IP buffer to DDR,DMA done interrupt, set Buffer ready to receive next data from Host)-->req.buf I written logic to call completion in DMA done handler because it works for both IN and OUT eps. But I see significant performance degradation (by copying a file to mass storage gadget). DMA can handle unaligned address too but it has ep max limit (say 512 for bulk). Hence it is SW job to split packets and to drive DMA till req.length completes. I feel polling for a while is faster than switching between interrupt handler back and forth rapidly. Moreover, DMA may or may not be present in IP based on user configuration at design time. I fixed all other comments expect this one if you do not agree for polling then I may have to create threads and do this stuff. What do you suggest? Thanks, Sundeep On Wed, Apr 30, 2014 at 10:24 PM, Felipe Balbi wrote: > Hi, > > On Wed, Apr 23, 2014 at 09:57:52AM +0530, sundeep subbaraya wrote: >> >> > I get the impression that the two of you are arguing past each other. >> >> > It appears that Sundeep is talking about transferring data from the >> >> > gadget driver's buffer to an internal buffer in the UDC hardware, but >> >> > Felipe is talking about transferring data from the UDC to the host. >> >> > >> >> > As I understand it, Sundeep said that when the gadget driver queues a >> >> > data-IN request, the UDC driver copies as much of the data buffer as >> >> > possible into a hardware FIFO. If it succeeds in copying all the data >> >> > into the FIFO then the request's completion routine gets called >> >> > immediately, even though the data doesn't get sent from the FIFO to the >> >> > host until the host asks for it. >> >> > >> >> > If only part of the data can be copied into the FIFO then the request >> >> > is added to the ep's request queue before the usb_ep_queue() call >> >> > returns. When space becomes available in the FIFO, the data will be >> >> > copied and eventually sent to the host. When all the data has been >> >> > copied to the FIFO, the request's completion routine will be called. >> > >> > there seems to be a slight problem with this approach: how will the IP >> > know that even though you copied X bytes into the FIFO, it should wait >> > for another Y bytes before shifting data to the wire ? How will it know >> > that it shouldn't generate CRC yet because there's still data to be >> > added ? >> >> No. IP does/need not know that it has to wait for Y bytes.We just >> write X bytes into >> HW buffer and count as X in buffer count register. IP generates CRC >> for bytes based >> on Count register and sends data to Host. Let us consider this >> scenario of bulk IN transfer: >> req.length = 5120 and wMaxPacketSize = 512, ep_queue is called once >> and is returned with >> status 0. In ep_queue this code snippet, >>if (xudc_write_fifo(ep, req) == 1) >> req = NULL; >>if(req != NULL) >> list_add_tail(&req->queue, &ep->queue); >> >> xudc_write_fifo does the following if HW buffers not busy: >> copies 512 bytes to HW buffer >> set count and ready registers so that IP can start data transfer to host >> changes req.actual to 512 and returns 0(if req.length > >> wMaxPacketSize) and 1(if req.length < wMaxPacketSize). > > you should return a proper error code, not 1. > >> Since return is zero this request is added to queue. When data >> transfer to host is completed IP generates >> an interrupt. In the interrupt handler we again call write_fifo if >> request list is not empty. >>if (list_empty(&ep->queue)) >> req = NULL; >> else >> req = list_entry(ep->queue.next, struct xusb_req, queue); >> if (!req) >> return; > > okay, this can be improved a bit though: > > if (list_empty(&ep->queue)) > return; > > req = list_first_entry(&ep->queue, struct xusb_req, queue); > >> if (ep->is_in) >> xudc_write_fifo(ep, req); >>
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi, On Tue, Apr 22, 2014 at 8:19 PM, Felipe Balbi wrote: > Hi, > > On Tue, Apr 22, 2014 at 12:58:41PM +0530, sundeep subbaraya wrote: >> Hi, >> >> On Mon, Apr 21, 2014 at 10:09 PM, Alan Stern >> wrote: >> > On Mon, 21 Apr 2014, Felipe Balbi wrote: >> > >> >> Hi, >> >> >> >> On Fri, Apr 18, 2014 at 07:34:08PM +0530, sundeep subbaraya wrote: >> >> >> >> >> >> >> >> > >> in ep_queue driver starts dma transfer from/to IP buffer to/from >> >> > >> req->buf. >> >> > >> If transfer is completed then request is not added to ep request >> >> > >> queue >> >> > >> and returns from ep_queue. >> >> > >> If transfer is not completed (actual < length) then request is added >> >> > >> to queue and returns from ep_queue. >> >> > > >> >> > > This is wrong. Why wouldn't you give gadget driver the chance to >> >> > > decide >> >> > > if it needs to queue the request again or not ? >> >> > >> >> > When does gadget driver decides to queue the same request again? >> >> > if -EBUSY is returned from ep_queue or req.status != 0 in completion >> >> > routine? >> >> >> >> whenever it so decides. Different gadget drivers might have different >> >> requirements. The code is open and sits under drivers/usb/gadget/ why >> >> don't you have a read ? >> > >> > I get the impression that the two of you are arguing past each other. >> > It appears that Sundeep is talking about transferring data from the >> > gadget driver's buffer to an internal buffer in the UDC hardware, but >> > Felipe is talking about transferring data from the UDC to the host. >> > >> > As I understand it, Sundeep said that when the gadget driver queues a >> > data-IN request, the UDC driver copies as much of the data buffer as >> > possible into a hardware FIFO. If it succeeds in copying all the data >> > into the FIFO then the request's completion routine gets called >> > immediately, even though the data doesn't get sent from the FIFO to the >> > host until the host asks for it. >> > >> > If only part of the data can be copied into the FIFO then the request >> > is added to the ep's request queue before the usb_ep_queue() call >> > returns. When space becomes available in the FIFO, the data will be >> > copied and eventually sent to the host. When all the data has been >> > copied to the FIFO, the request's completion routine will be called. > > there seems to be a slight problem with this approach: how will the IP > know that even though you copied X bytes into the FIFO, it should wait > for another Y bytes before shifting data to the wire ? How will it know > that it shouldn't generate CRC yet because there's still data to be > added ? No. IP does/need not know that it has to wait for Y bytes.We just write X bytes into HW buffer and count as X in buffer count register. IP generates CRC for bytes based on Count register and sends data to Host. Let us consider this scenario of bulk IN transfer: req.length = 5120 and wMaxPacketSize = 512, ep_queue is called once and is returned with status 0. In ep_queue this code snippet, if (xudc_write_fifo(ep, req) == 1) req = NULL; if(req != NULL) list_add_tail(&req->queue, &ep->queue); xudc_write_fifo does the following if HW buffers not busy: copies 512 bytes to HW buffer set count and ready registers so that IP can start data transfer to host changes req.actual to 512 and returns 0(if req.length > wMaxPacketSize) and 1(if req.length < wMaxPacketSize). Since return is zero this request is added to queue. When data transfer to host is completed IP generates an interrupt. In the interrupt handler we again call write_fifo if request list is not empty. if (list_empty(&ep->queue)) req = NULL; else req = list_entry(ep->queue.next, struct xusb_req, queue); if (!req) return; if (ep->is_in) xudc_write_fifo(ep, req); else xudc_read_fifo(ep, req); This happens 10 times(since length 5120) and completion is called. > If there's no space in the FIFO yet, why copy data at all ? If HW buffers are busy(IP is still transferring previous data to Host from buffer) then xudc_write_fifo returns 0 without changi
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi, On Mon, Apr 21, 2014 at 10:09 PM, Alan Stern wrote: > On Mon, 21 Apr 2014, Felipe Balbi wrote: > >> Hi, >> >> On Fri, Apr 18, 2014 at 07:34:08PM +0530, sundeep subbaraya wrote: >> >> >> >> > >> in ep_queue driver starts dma transfer from/to IP buffer to/from >> > >> req->buf. >> > >> If transfer is completed then request is not added to ep request queue >> > >> and returns from ep_queue. >> > >> If transfer is not completed (actual < length) then request is added >> > >> to queue and returns from ep_queue. >> > > >> > > This is wrong. Why wouldn't you give gadget driver the chance to decide >> > > if it needs to queue the request again or not ? >> > >> > When does gadget driver decides to queue the same request again? >> > if -EBUSY is returned from ep_queue or req.status != 0 in completion >> > routine? >> >> whenever it so decides. Different gadget drivers might have different >> requirements. The code is open and sits under drivers/usb/gadget/ why >> don't you have a read ? > > I get the impression that the two of you are arguing past each other. > It appears that Sundeep is talking about transferring data from the > gadget driver's buffer to an internal buffer in the UDC hardware, but > Felipe is talking about transferring data from the UDC to the host. > > As I understand it, Sundeep said that when the gadget driver queues a > data-IN request, the UDC driver copies as much of the data buffer as > possible into a hardware FIFO. If it succeeds in copying all the data > into the FIFO then the request's completion routine gets called > immediately, even though the data doesn't get sent from the FIFO to the > host until the host asks for it. > > If only part of the data can be copied into the FIFO then the request > is added to the ep's request queue before the usb_ep_queue() call > returns. When space becomes available in the FIFO, the data will be > copied and eventually sent to the host. When all the data has been > copied to the FIFO, the request's completion routine will be called. > > Thus there never is any need for the gadget driver to queue the request > again. An incomplete transfer means the FIFO didn't have enough room > when the request was submitted; it doesn't mean that the data didn't > eventually get sent to the host. Exactly Alan,this is what I was trying to say. Probably I was not clear in explaining. I didnt see any harm this way and even this implementation is same like at91_udc.c. I have been reading mas_storage to understand when does gadget driver tries to enqueue a request again. Since different gadget drivers might have different requirements (agree with Felipe), wanted to know criteria for queuing a same request again. I will change this implementation as per Felipe comments and test with some of the gadgets. Thanks Alan and Felipe, Sundeep > > HTH, > > Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe,
On Thu, Apr 17, 2014 at 8:31 PM, Felipe Balbi wrote:
> On Thu, Apr 17, 2014 at 03:01:37PM +0530, sundeep subbaraya wrote:
>> Hi Felipe,
>>
>> On Wed, Apr 16, 2014 at 11:32 PM, Felipe Balbi wrote:
>> > Hi,
>> >
>> > On Wed, Apr 16, 2014 at 04:09:28PM +0530, sundeep subbaraya wrote:
>> >> Hi Felipe,
>> >>
>> >> On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi wrote:
>> >>
>> >> >> +static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
>> >> >
>> >> > please prepend this with xudc_, it makes tracing a lot easier.
>> >> >
>> >> >> +{
>> >> >> + struct xusb_udc *udc;
>> >> >> + int rc = 0;
>> >> >> + unsigned long timeout;
>> >> >> +
>> >> >> + udc = ep->udc;
>> >> >> + /*
>> >> >> + * Set the addresses in the DMA source and
>> >> >> + * destination registers and then set the length
>> >> >> + * into the DMA length register.
>> >> >> + */
>> >> >> + udc->write_fn(udc->base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
>> >> >> + udc->write_fn(udc->base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
>> >> >> + udc->write_fn(udc->base_address, XUSB_DMA_LENGTH_OFFSET, length);
>> >> >> +
>> >> >> + /*
>> >> >> + * Wait till DMA transaction is complete and
>> >> >> + * check whether the DMA transaction was
>> >> >> + * successful.
>> >> >> + */
>> >> >> + while ((udc->read_fn(ep->udc->base_address +
>> >> >> XUSB_DMA_STATUS_OFFSET) &
>> >> >> + XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
>> >> >> + timeout = jiffies + 1;
>> >> >> +
>> >> >> + if (time_after(jiffies, timeout)) {
>> >> >> + rc = -ETIMEDOUT;
>> >> >> + goto clean;
>> >> >> + }
>> >> >> + }
>> >> >
>> >> > don't you get an IRQ for DMA completion ? If you do, you could be using
>> >> > wait_for_completion()
>> >>
>> >> This function is called in interrupt context when buffer is ready or
>> >> free. It initiates DMA to transfer data from IP buffer to memory.
>> >> Hence it waits in busy loop till DMA completes
>> >
>> > wait, so you start_dma() before your gadget driver asks you to ?
>>
>> in ep_queue driver starts dma transfer from/to IP buffer to/from req->buf.
>> If transfer is completed then request is not added to ep request queue
>> and returns from ep_queue.
>> If transfer is not completed (actual < length) then request is added
>> to queue and returns from ep_queue.
>
> This is wrong. Why wouldn't you give gadget driver the chance to decide
> if it needs to queue the request again or not ?
When does gadget driver decides to queue the same request again?
if -EBUSY is returned from ep_queue or req.status != 0 in completion
routine?
there are cases where ping-pong buffers both are busy. currently this driver
adds request to queue only when buffers are busy. In normal cases request is
processed without adding to queue.
do i need to have another local queue for requests waiting since
buffers are busy?
Or dequeue the request ?
>
>> when buffer processed interrupt occurs, handler starts dma if there is
>> request in queue and calls complete call back (when actual == length)
>> Hence answer is Yes for some transfers (start dma called from
>> interrupt handler not from ep_queue).
>
> this also seems wrong(-ish).
>
> 1) as Paul pointed out, you can't rely on jiffies if you're calling this
> with IRQs disabled.
>
> 2) you don't really need to wait until DMA finishes its transaction
> before returning from start_dma(), just use the DMA completion IRQ,
>
> 3) I don't see anywhere any sanitizing of arguments, can your DMA really
> handle any alignment/unaligned addresses or should you make sure you're
> getting good arguments?
>
> You need to work on this a little bit, I guess.
Yes am working on this and will be using seperate ep_ops for ep0 like
drivers/usb/dwc3/gadget.c since ep0 has no dma and ping-pong buffers
Thanks,
Sundeep.B.S.
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi,
On Thu, Apr 17, 2014 at 1:38 AM, Paul Zimmerman
wrote:
>> From: linux-usb-ow...@vger.kernel.org
>> [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of sundeep subbaraya
>> Sent: Wednesday, April 16, 2014 3:39 AM
>>
>> Hi Felipe,
>>
>> On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi wrote:
>>
>> >> +static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
>> >
>> > please prepend this with xudc_, it makes tracing a lot easier.
>> >
>> >> +{
>> >> + struct xusb_udc *udc;
>> >> + int rc = 0;
>> >> + unsigned long timeout;
>> >> +
>> >> + udc = ep->udc;
>> >> + /*
>> >> + * Set the addresses in the DMA source and
>> >> + * destination registers and then set the length
>> >> + * into the DMA length register.
>> >> + */
>> >> + udc->write_fn(udc->base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
>> >> + udc->write_fn(udc->base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
>> >> + udc->write_fn(udc->base_address, XUSB_DMA_LENGTH_OFFSET, length);
>> >> +
>> >> + /*
>> >> + * Wait till DMA transaction is complete and
>> >> + * check whether the DMA transaction was
>> >> + * successful.
>> >> + */
>> >> + while ((udc->read_fn(ep->udc->base_address +
>> >> XUSB_DMA_STATUS_OFFSET) &
>> >> + XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
>> >> + timeout = jiffies + 1;
>> >> +
>> >> + if (time_after(jiffies, timeout)) {
>> >> + rc = -ETIMEDOUT;
>> >> + goto clean;
>> >> + }
>> >> + }
>> >
>> > don't you get an IRQ for DMA completion ? If you do, you could be using
>> > wait_for_completion()
>>
>> This function is called in interrupt context when buffer is ready or
>> free. It initiates
>> DMA to transfer data from IP buffer to memory. Hence it waits in busy
>> loop till DMA
>> completes
>
> If this function is called in interrupt context, then you can't use
> jiffies for your timeout, since jiffies may not get updated while in
> interrupt context.
Yes. It is obvious that this logic is buggy, will change this.
Thanks,
Sundeep.B.S.
>
> --
> Paul
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe,
On Wed, Apr 16, 2014 at 11:32 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Apr 16, 2014 at 04:09:28PM +0530, sundeep subbaraya wrote:
>> Hi Felipe,
>>
>> On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi wrote:
>>
>> >> +static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
>> >
>> > please prepend this with xudc_, it makes tracing a lot easier.
>> >
>> >> +{
>> >> + struct xusb_udc *udc;
>> >> + int rc = 0;
>> >> + unsigned long timeout;
>> >> +
>> >> + udc = ep->udc;
>> >> + /*
>> >> + * Set the addresses in the DMA source and
>> >> + * destination registers and then set the length
>> >> + * into the DMA length register.
>> >> + */
>> >> + udc->write_fn(udc->base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
>> >> + udc->write_fn(udc->base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
>> >> + udc->write_fn(udc->base_address, XUSB_DMA_LENGTH_OFFSET, length);
>> >> +
>> >> + /*
>> >> + * Wait till DMA transaction is complete and
>> >> + * check whether the DMA transaction was
>> >> + * successful.
>> >> + */
>> >> + while ((udc->read_fn(ep->udc->base_address +
>> >> XUSB_DMA_STATUS_OFFSET) &
>> >> + XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
>> >> + timeout = jiffies + 1;
>> >> +
>> >> + if (time_after(jiffies, timeout)) {
>> >> + rc = -ETIMEDOUT;
>> >> + goto clean;
>> >> + }
>> >> + }
>> >
>> > don't you get an IRQ for DMA completion ? If you do, you could be using
>> > wait_for_completion()
>>
>> This function is called in interrupt context when buffer is ready or
>> free. It initiates DMA to transfer data from IP buffer to memory.
>> Hence it waits in busy loop till DMA completes
>
> wait, so you start_dma() before your gadget driver asks you to ?
in ep_queue driver starts dma transfer from/to IP buffer to/from req->buf.
If transfer is completed then request is not added to ep request queue
and returns from ep_queue.
If transfer is not completed (actual < length) then request is added
to queue and returns from ep_queue.
when buffer processed interrupt occurs, handler starts dma if there is
request in queue and calls complete call back (when actual == length)
Hence answer is Yes for some transfers (start dma called from
interrupt handler not from ep_queue).
Thanks,
Sundeep.B.S.
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe,
On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi wrote:
>> +static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
>
> please prepend this with xudc_, it makes tracing a lot easier.
>
>> +{
>> + struct xusb_udc *udc;
>> + int rc = 0;
>> + unsigned long timeout;
>> +
>> + udc = ep->udc;
>> + /*
>> + * Set the addresses in the DMA source and
>> + * destination registers and then set the length
>> + * into the DMA length register.
>> + */
>> + udc->write_fn(udc->base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
>> + udc->write_fn(udc->base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
>> + udc->write_fn(udc->base_address, XUSB_DMA_LENGTH_OFFSET, length);
>> +
>> + /*
>> + * Wait till DMA transaction is complete and
>> + * check whether the DMA transaction was
>> + * successful.
>> + */
>> + while ((udc->read_fn(ep->udc->base_address + XUSB_DMA_STATUS_OFFSET) &
>> + XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
>> + timeout = jiffies + 1;
>> +
>> + if (time_after(jiffies, timeout)) {
>> + rc = -ETIMEDOUT;
>> + goto clean;
>> + }
>> + }
>
> don't you get an IRQ for DMA completion ? If you do, you could be using
> wait_for_completion()
This function is called in interrupt context when buffer is ready or
free. It initiates
DMA to transfer data from IP buffer to memory. Hence it waits in busy
loop till DMA
completes
Thanks,
Sundeep.B.S.
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Fwd: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
On Mon, Apr 7, 2014 at 10:05 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Apr 07, 2014 at 02:36:13PM +0530, sundeep subbaraya wrote:
>> >> +/**
>> >> + * xudc_wrstatus - Sets up the usb device status stages.
>> >> + * @udc: pointer to the usb device controller structure.
>> >> + */
>> >> +static void xudc_wrstatus(struct xusb_udc *udc)
>> >> +{
>> >> + u32 epcfgreg;
>> >> +
>> >> + epcfgreg = udc->read_fn(udc->base_address +
>> >> + udc->ep[XUSB_EP_NUMBER_ZERO].offset)|
>> >> + XUSB_EP_CFG_DATA_TOGGLE_MASK;
>> >
>> > are you really trying to mask here ? If you're trying to mask you should
>> > be using a bitwise and.
>>
>> This is used for making DATA1 packet for status stage during control
>> transfers.
>> IP has internally a weak check for alternating between DATA0 and DATA1
>> packets using
>> this bit. Firmware can set this bit to send a DATA1 packet. As we
>> always need to send DATA1
>> for status stage, we force IP to send DATA1 packet whatever maybe in this
>> bit because of alternating behavior. Is this is confusing for the name
>> *_TOGGLE_MASK ?
>
> yeah, I guess it was the suffix _MASK, nevermind then ;-)
OK :)
>
>> >> +static int xudc_get_frame(struct usb_gadget *gadget)
>> >> +{
>> >> +
>> >> + struct xusb_udc *udc = to_udc(gadget);
>> >> + unsigned long flags;
>> >> + int retval;
>> >> +
>> >> + if (!gadget)
>> >> + return -ENODEV;
>> >
>> > oh boy... so you first deref gadget, then you check for it ?
>>
>> Yes i too had this doubt after looking at some of the functions (like
>> ep_queue) of other controller drivers.
>
> if there are other gadget drivers doing this, they're wrong and need to
> be fixed.
>
>> I tested sending a NULL to container_of macro I see no NULL exception.
>
> yeah, container_of() will *always* return a valid pointer, even if it's
> argument is NULL.
>
>> Hence using container_of
>> macro first and then checking for NULL input is fine. If you insist
>
> no, it's not. You'd waste cpu cycles with pointer arithmetic for no
> reason.
>
>> this i need to change code at other
>> places too.
>
> yes.
ok will modify
>
>> >> +static int xudc_wakeup(struct usb_gadget *gadget)
>> >> +{
>> >> + struct xusb_udc *udc = to_udc(gadget);
>> >> + u32 crtlreg;
>> >> + int status = -EINVAL;
>> >> + unsigned long flags;
>> >> +
>> >> + spin_lock_irqsave(&udc->lock, flags);
>> >> +
>> >> + /* Remote wake up not enabled by host */
>> >> + if (!udc->remote_wkp)
>> >> + goto done;
>> >> +
>> >> + crtlreg = udc->read_fn(udc->base_address + XUSB_CONTROL_OFFSET);
>> >> + /* set remote wake up bit */
>> >> + udc->write_fn(udc->base_address, XUSB_CONTROL_OFFSET, crtlreg |
>> >> + XUSB_CONTROL_USB_RMTWAKE_MASK);
>> >> + /* wait for a while and reset remote wake up bit */
>> >> + mdelay(2);
>> >
>> > why 2 ms ? why not 5 ? why not 1 ? shouldn't you be polling a bit in a
>> > register or something ?
>>
>> IP datasheet says writing Remote wake bit to this register will send
>> remote wake up
>> signalling to host immediately and this bit will not be cleared by
>> hardware, firmware has
>> to clear it. There is no bit for polling.
>
> then you need a better comment stating this detail.
sure
>
>> >> +static const struct usb_gadget_ops xusb_udc_ops = {
>> >> + .get_frame = xudc_get_frame,
>> >> + .wakeup = xudc_wakeup,
>> >> + .udc_start = xudc_start,
>> >> + .udc_stop = xudc_stop,
>> >
>> > no pullup ??? What gives ? This HW doesn't support it ? really ?
>>
>> Yes, there is no pull up. writing to control register to start udc is
>> sufficient for this IP.
>
> right, but is there a bit inside control register which actually starts
> the IP ? You might want to move that to ->pullup(), see how we did on
> drivers/usb/dwc3/gadget.c::dwc3_gadget_pullup(); we're basically using
> that to control RUN/STOP bit in DCTL register. That bit has two
> functions: a) actua
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Michal,
On Thu, Apr 3, 2014 at 8:53 PM, Michal Simek wrote:
>>> +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.
>
Yes, there is no configuration register for endianess.
Thanks,
Sundeep.
> 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support
On Fri, Feb 21, 2014 at 9:09 PM, Felipe Balbi wrote: > Hi, > > On Fri, Feb 21, 2014 at 11:27:07AM +, Subbaraya Sundeep Bhatta wrote: >> > From the looks of it, I doubt this was actually tested, you need a lot >> > of work on this driver. >> Tested on both ARM and Microblaze architectures with Mass storage gadget. >> Will send a v2 after addressing all your comments. > > clearly you didn't try to remove and reinsert the module or you would > see a whole bunch of errors. > Yes you are correct. My console hung up as soon as i rmmod my driver. Could you please point me where am wrong. >> This email and any attachments are intended for the sole use of the >> named recipient(s) and contain(s) confidential information that may be >> proprietary, privileged or copyrighted under applicable law. If you >> are not the intended recipient, do not read, copy, or forward this >> email message or any attachments. Delete this email message and any >> attachments immediately. > > remove this footer message, btw, when sending emails to public mailing > lists. > Sure > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] Request for suggestions
Hi All, AXI bus Monitor(AM) has set of counters which are used to collect metrics related to AXI transactions happening between Masters and Slaves in an AXI system. Example metrics which can be calculated using AM are as below: Read Latency Write Latency Idle Read/Write Cycle counts Number of Transactions Number of Bytes transferred Number of Read/Write Last signals issued. etc., AM raises interrupts during counter overflows and there is Sample Interval down counter which raises interrupt and takes snapshot of all counters and stores in set of registers. Use case for this IP will be as follows: User confiures his required metrics to be collected by Counters. User sets Sample Interval down counter depending on the metrics monitoring sampling rate. Start counters. Collect metrics data when AM issues Sample Interval down Counter interrupt. What would be the appropriate framework/interface of Linux Driver for AM? I have been thinking of interfaces- UIO Char sysfs The main problem I see is signalling interrupts to Userspace. Please suggest me the best interface. Thanks, Sundeep -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
