Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1242123 > unrolled thread

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-10-08 11:50 +0200
    Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver Ley Foon Tan <lftan@altera.com> - 2015-10-08 12:10 +0200
      Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver Bjorn Helgaas <helgaas@kernel.org> - 2015-10-10 01:20 +0200
        Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver Arnd Bergmann <arnd@arndb.de> - 2015-10-12 14:10 +0200
          Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver Ley Foon Tan <lftan@altera.com> - 2015-10-13 09:50 +0200

#1242123 — Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> +				 int where, int size, u32 value)
> +{
> +	struct altera_pcie *pcie = bus->sysdata;
> +	u32 data32;
> +	u32 shift = 8 * (where & 3);
> +	int ret;
> +
> +	if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	/* write partial */
> +	if (size != sizeof(u32)) {
> +		ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> +					 where & ~DWORD_MASK, &data32);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (size) {
> +	case 1:
> +		data32 = (data32 & ~(0xff << shift)) |
> +				((value & 0xff) << shift);
> +		break;
> +	case 2:
> +		data32 = (data32 & ~(0xffff << shift)) |
> +				((value & 0xffff) << shift);
> +		break;
> +	default:
> +		data32 = value;

Can you generate proper 1, 2 and 4 byte configuration accesses?  That
is much preferred over the above read-modify-write, as there are
registers in PCI and PCIe that are read/write-1-to-clear.  The above
has the effect of inadvertently clearing those RW1C bits.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1242143

On Thu, Oct 8, 2015 at 5:47 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>
> On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> > +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> > +                              int where, int size, u32 value)
> > +{
> > +     struct altera_pcie *pcie = bus->sysdata;
> > +     u32 data32;
> > +     u32 shift = 8 * (where & 3);
> > +     int ret;
> > +
> > +     if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> > +             return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > +     /* write partial */
> > +     if (size != sizeof(u32)) {
> > +             ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> > +                                      where & ~DWORD_MASK, &data32);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     switch (size) {
> > +     case 1:
> > +             data32 = (data32 & ~(0xff << shift)) |
> > +                             ((value & 0xff) << shift);
> > +             break;
> > +     case 2:
> > +             data32 = (data32 & ~(0xffff << shift)) |
> > +                             ((value & 0xffff) << shift);
> > +             break;
> > +     default:
> > +             data32 = value;
>
> Can you generate proper 1, 2 and 4 byte configuration accesses?  That
> is much preferred over the above read-modify-write, as there are
> registers in PCI and PCIe that are read/write-1-to-clear.  The above
> has the effect of inadvertently clearing those RW1C bits.
No, hardware can only access 4-byte aligned address.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243734

On Thu, Oct 08, 2015 at 06:03:24PM +0800, Ley Foon Tan wrote:
> On Thu, Oct 8, 2015 at 5:47 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >
> > On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> > > +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> > > +                              int where, int size, u32 value)
> > > +{
> > > +     struct altera_pcie *pcie = bus->sysdata;
> > > +     u32 data32;
> > > +     u32 shift = 8 * (where & 3);
> > > +     int ret;
> > > +
> > > +     if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> > > +             return PCIBIOS_DEVICE_NOT_FOUND;
> > > +
> > > +     /* write partial */
> > > +     if (size != sizeof(u32)) {
> > > +             ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> > > +                                      where & ~DWORD_MASK, &data32);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     switch (size) {
> > > +     case 1:
> > > +             data32 = (data32 & ~(0xff << shift)) |
> > > +                             ((value & 0xff) << shift);
> > > +             break;
> > > +     case 2:
> > > +             data32 = (data32 & ~(0xffff << shift)) |
> > > +                             ((value & 0xffff) << shift);
> > > +             break;
> > > +     default:
> > > +             data32 = value;
> >
> > Can you generate proper 1, 2 and 4 byte configuration accesses?  That
> > is much preferred over the above read-modify-write, as there are
> > registers in PCI and PCIe that are read/write-1-to-clear.  The above
> > has the effect of inadvertently clearing those RW1C bits.
> No, hardware can only access 4-byte aligned address.

This is non-spec compliant, and we really should have some way of
flagging that because it may break things in ways that would be very
difficult to debug, e.g., we can lose RW1C status bits when writing an
adjacent register, so they would just silently disappear.

I don't know if this should be a kernel taint, a simple warning in
dmesg, or what.  I guess the tainting mechanism is probably too
general-purpose for this, and add_taint() doesn't give any dmesg
indication.  We wouldn't see the taint unless the problem actually
caused an oops or panic.  In this case, I think I want a clue in dmesg
so we have a chance of seeing it even if there is no oops.  So
probably something like a dev_warn("non-compliant config accesses")
would work.

You really should double-check with the hardware guys, because it's
pretty obvious that the PCI spec requires 1- and 2-byte config
accesses to work correctly.  For example, if you read/modify/write to
update PCI_COMMAND, you will inadvertently clear the RW1C bits in
PCI_STATUS.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244626

On Friday 09 October 2015 18:15:40 Bjorn Helgaas wrote:
> 
> I don't know if this should be a kernel taint, a simple warning in
> dmesg, or what.  I guess the tainting mechanism is probably too
> general-purpose for this, and add_taint() doesn't give any dmesg
> indication.  We wouldn't see the taint unless the problem actually
> caused an oops or panic.  In this case, I think I want a clue in dmesg
> so we have a chance of seeing it even if there is no oops.  So
> probably something like a dev_warn("non-compliant config accesses")
> would work.
> 
> You really should double-check with the hardware guys, because it's
> pretty obvious that the PCI spec requires 1- and 2-byte config
> accesses to work correctly.  For example, if you read/modify/write to
> update PCI_COMMAND, you will inadvertently clear the RW1C bits in
> PCI_STATUS.

Would it help to require a DT property here that flags the device
as having a broken config space?

Then we could implement both in the driver, and only use the
RMW based implementation if the firmware describes the device
as "altera,broken-pci-config-space".

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245414

On Mon, Oct 12, 2015 at 8:03 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Friday 09 October 2015 18:15:40 Bjorn Helgaas wrote:
> >
> > I don't know if this should be a kernel taint, a simple warning in
> > dmesg, or what.  I guess the tainting mechanism is probably too
> > general-purpose for this, and add_taint() doesn't give any dmesg
> > indication.  We wouldn't see the taint unless the problem actually
> > caused an oops or panic.  In this case, I think I want a clue in dmesg
> > so we have a chance of seeing it even if there is no oops.  So
> > probably something like a dev_warn("non-compliant config accesses")
> > would work.
> >
> > You really should double-check with the hardware guys, because it's
> > pretty obvious that the PCI spec requires 1- and 2-byte config
> > accesses to work correctly.  For example, if you read/modify/write to
> > update PCI_COMMAND, you will inadvertently clear the RW1C bits in
> > PCI_STATUS.
>
> Would it help to require a DT property here that flags the device
> as having a broken config space?
>
> Then we could implement both in the driver, and only use the
> RMW based implementation if the firmware describes the device
> as "altera,broken-pci-config-space".
>

I have checked the PCI/TLP specification, the address needs to be
4-byte aligned. But, we can use "byte enable" field to update specific
bytes.
For example, if byte enable is 0x3 (0011b), that mean it only update
lower 2 bytes. By doing this, we can resolve the RW1C issue here.
I will update the driver with this in next revision.

Thanks for reviewing.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web