Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On 2018/12/4 20:00, Lorenzo Pieralisi wrote:
> On Tue, Dec 04, 2018 at 06:40:55PM +0800, Hanjie Lin wrote:
>>
>>
>> On 2018/12/4 6:57, Bjorn Helgaas wrote:
>>> On Mon, Dec 03, 2018 at 04:41:50PM +, Lorenzo Pieralisi wrote:
On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
[...]
> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
> size,
> + u32 *val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> + /*
> + * there is a bug of MESON AXG pcie controller that software can not
> + * programe PCI_CLASS_DEVICE register, so we must return a fake right
> + * value to ensure driver could probe successfully.
> + */
> + if (where == PCI_CLASS_REVISION) {
> + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> + /* keep revision id */
> + *val &= PCI_CLASS_REVISION_MASK;
> + *val |= PCI_CLASS_BRIDGE_PCI << 16;
> + return PCIBIOS_SUCCESSFUL;
> + }
As I said before, this looks broken. If this code (or other drivers with
the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
it according to your comment above.
I would like to pick Bjorn's brain on this to see what we can really do
to fix this (and other) drivers.
>>>
>>> - Check to see whether you're reading anything in the 32-bit dword at
>>> offset 0x08.
>>>
>>> - Do the 32-bit readl().
>>>
>>> - Insert the correct Sub-Class and Base Class code (you also throw
>>> away the Programming Interface; not sure why that is)
>>>
>>> - If you're reading something smaller than 32 bits, mask & shift as
>>> needed. pci_bridge_emul_conf_read() does something similar that
>>> you might be able to copy.
>>>
>>> Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI? There
>>> are several places in the kernel that currently depend on it, but I
>>> think several of them *should* be checking dev->hdr_type to identify a
>>> type 1 header instead.
>>>
>>> Bjorn
>>>
>>> .
>>>
>>
>> Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from
>> 0xa/PCI_CLASS_DEVICE)
>> that I didn't considered.
>>
>> As your suggestion, I consider some code below may help this issue:
>> 1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
>>request all other *size* bytes will return error and dw_pcie_read()
>>will also check register alignment.
>>
>> 2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or
>> 0xa/PCI_CLASS_REVISION,
>>we may need to correct class code.
>>As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes
>> from 0x8/PCI_CLASS_DEVICE
>>or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class
>> code.
>>
>> ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.
>
> You can fix this too.
>
>> static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>> u32 *val)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> int ret;
>>
>> ret = dw_pcie_read(pci->dbi_base + where, size, val);
>> if (ret != PCIBIOS_SUCCESSFUL)
>> return ret;
>>
>> /*
>> * there is a bug of MESON AXG pcie controller that software can not
>> * programe PCI_CLASS_DEVICE register, so we must return a fake right
>
> "There is a bug in the MESON AXG pcie controller whereby software cannot
> programme the PCI_CLASS_DEVICE register, so we must fabricate the return
> value in the config accessors."
>
Ok, it's more clear :)
>> * value to ensure driver could probe successfully.
>> */
>> if (where == PCI_CLASS_REVISION && size == 4)
>> *val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0x);
>> else if (where == PCI_CLASS_DEVICE && size == 2)
>> *val = PCI_CLASS_BRIDGE_PCI;
>
> You can further filter it with (where & 0x1) == PCI_CLASS_DEVICE
> and handle the size accordingly, so that even a byte access would
> work, for completeness.
>
> Lorenzo
>
Of course, I will add process to handle one-byte access from 0xa/0xb register
in next-version.
thanks.
hanjie
>> return PCIBIOS_SUCCESSFUL;
>> }
>>
>> 3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
>>or pci_setup_device() will get failed:
>>
>>...
>>class = pci_class(dev);
>>dev->revision = class & 0xff;
>>dev->class = class >> 8; /* upper 3 bytes */
>>
>>switch (dev->hdr_type) { /* header type */
>>...
>>case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
>> if (class != PCI_CLASS_BRIDGE_PCI) /* class must be
>> PCI_CLASS_BRIDGE_PCI */
>> goto bad;
>>
>>
>> thanks.
>>
>> hanjie
>
> .
>
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On 2018/12/4 20:00, Lorenzo Pieralisi wrote:
> On Tue, Dec 04, 2018 at 06:40:55PM +0800, Hanjie Lin wrote:
>>
>>
>> On 2018/12/4 6:57, Bjorn Helgaas wrote:
>>> On Mon, Dec 03, 2018 at 04:41:50PM +, Lorenzo Pieralisi wrote:
On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
[...]
> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
> size,
> + u32 *val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> + /*
> + * there is a bug of MESON AXG pcie controller that software can not
> + * programe PCI_CLASS_DEVICE register, so we must return a fake right
> + * value to ensure driver could probe successfully.
> + */
> + if (where == PCI_CLASS_REVISION) {
> + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> + /* keep revision id */
> + *val &= PCI_CLASS_REVISION_MASK;
> + *val |= PCI_CLASS_BRIDGE_PCI << 16;
> + return PCIBIOS_SUCCESSFUL;
> + }
As I said before, this looks broken. If this code (or other drivers with
the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
it according to your comment above.
I would like to pick Bjorn's brain on this to see what we can really do
to fix this (and other) drivers.
>>>
>>> - Check to see whether you're reading anything in the 32-bit dword at
>>> offset 0x08.
>>>
>>> - Do the 32-bit readl().
>>>
>>> - Insert the correct Sub-Class and Base Class code (you also throw
>>> away the Programming Interface; not sure why that is)
>>>
>>> - If you're reading something smaller than 32 bits, mask & shift as
>>> needed. pci_bridge_emul_conf_read() does something similar that
>>> you might be able to copy.
>>>
>>> Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI? There
>>> are several places in the kernel that currently depend on it, but I
>>> think several of them *should* be checking dev->hdr_type to identify a
>>> type 1 header instead.
>>>
>>> Bjorn
>>>
>>> .
>>>
>>
>> Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from
>> 0xa/PCI_CLASS_DEVICE)
>> that I didn't considered.
>>
>> As your suggestion, I consider some code below may help this issue:
>> 1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
>>request all other *size* bytes will return error and dw_pcie_read()
>>will also check register alignment.
>>
>> 2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or
>> 0xa/PCI_CLASS_REVISION,
>>we may need to correct class code.
>>As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes
>> from 0x8/PCI_CLASS_DEVICE
>>or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class
>> code.
>>
>> ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.
>
> You can fix this too.
>
>> static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>> u32 *val)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> int ret;
>>
>> ret = dw_pcie_read(pci->dbi_base + where, size, val);
>> if (ret != PCIBIOS_SUCCESSFUL)
>> return ret;
>>
>> /*
>> * there is a bug of MESON AXG pcie controller that software can not
>> * programe PCI_CLASS_DEVICE register, so we must return a fake right
>
> "There is a bug in the MESON AXG pcie controller whereby software cannot
> programme the PCI_CLASS_DEVICE register, so we must fabricate the return
> value in the config accessors."
>
Ok, it's more clear :)
>> * value to ensure driver could probe successfully.
>> */
>> if (where == PCI_CLASS_REVISION && size == 4)
>> *val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0x);
>> else if (where == PCI_CLASS_DEVICE && size == 2)
>> *val = PCI_CLASS_BRIDGE_PCI;
>
> You can further filter it with (where & 0x1) == PCI_CLASS_DEVICE
> and handle the size accordingly, so that even a byte access would
> work, for completeness.
>
> Lorenzo
>
Of course, I will add process to handle one-byte access from 0xa/0xb register
in next-version.
thanks.
hanjie
>> return PCIBIOS_SUCCESSFUL;
>> }
>>
>> 3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
>>or pci_setup_device() will get failed:
>>
>>...
>>class = pci_class(dev);
>>dev->revision = class & 0xff;
>>dev->class = class >> 8; /* upper 3 bytes */
>>
>>switch (dev->hdr_type) { /* header type */
>>...
>>case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
>> if (class != PCI_CLASS_BRIDGE_PCI) /* class must be
>> PCI_CLASS_BRIDGE_PCI */
>> goto bad;
>>
>>
>> thanks.
>>
>> hanjie
>
> .
>
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Tue, Dec 04, 2018 at 06:40:55PM +0800, Hanjie Lin wrote:
>
>
> On 2018/12/4 6:57, Bjorn Helgaas wrote:
> > On Mon, Dec 03, 2018 at 04:41:50PM +, Lorenzo Pieralisi wrote:
> >> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
> >>
> >> [...]
> >>
> >>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
> >>> size,
> >>> + u32 *val)
> >>> +{
> >>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>> +
> >>> + /*
> >>> + * there is a bug of MESON AXG pcie controller that software can not
> >>> + * programe PCI_CLASS_DEVICE register, so we must return a fake right
> >>> + * value to ensure driver could probe successfully.
> >>> + */
> >>> + if (where == PCI_CLASS_REVISION) {
> >>> + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> >>> + /* keep revision id */
> >>> + *val &= PCI_CLASS_REVISION_MASK;
> >>> + *val |= PCI_CLASS_BRIDGE_PCI << 16;
> >>> + return PCIBIOS_SUCCESSFUL;
> >>> + }
> >>
> >> As I said before, this looks broken. If this code (or other drivers with
> >> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> >> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> >> it according to your comment above.
> >>
> >> I would like to pick Bjorn's brain on this to see what we can really do
> >> to fix this (and other) drivers.
> >
> > - Check to see whether you're reading anything in the 32-bit dword at
> > offset 0x08.
> >
> > - Do the 32-bit readl().
> >
> > - Insert the correct Sub-Class and Base Class code (you also throw
> > away the Programming Interface; not sure why that is)
> >
> > - If you're reading something smaller than 32 bits, mask & shift as
> > needed. pci_bridge_emul_conf_read() does something similar that
> > you might be able to copy.
> >
> > Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI? There
> > are several places in the kernel that currently depend on it, but I
> > think several of them *should* be checking dev->hdr_type to identify a
> > type 1 header instead.
> >
> > Bjorn
> >
> > .
> >
>
> Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from
> 0xa/PCI_CLASS_DEVICE)
> that I didn't considered.
>
> As your suggestion, I consider some code below may help this issue:
> 1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
>request all other *size* bytes will return error and dw_pcie_read()
>will also check register alignment.
>
> 2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or
> 0xa/PCI_CLASS_REVISION,
>we may need to correct class code.
>As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes
> from 0x8/PCI_CLASS_DEVICE
>or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class
> code.
>
> ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.
You can fix this too.
> static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> u32 *val)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> int ret;
>
> ret = dw_pcie_read(pci->dbi_base + where, size, val);
> if (ret != PCIBIOS_SUCCESSFUL)
> return ret;
>
> /*
> * there is a bug of MESON AXG pcie controller that software can not
> * programe PCI_CLASS_DEVICE register, so we must return a fake right
"There is a bug in the MESON AXG pcie controller whereby software cannot
programme the PCI_CLASS_DEVICE register, so we must fabricate the return
value in the config accessors."
> * value to ensure driver could probe successfully.
> */
> if (where == PCI_CLASS_REVISION && size == 4)
> *val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0x);
> else if (where == PCI_CLASS_DEVICE && size == 2)
> *val = PCI_CLASS_BRIDGE_PCI;
You can further filter it with (where & 0x1) == PCI_CLASS_DEVICE
and handle the size accordingly, so that even a byte access would
work, for completeness.
Lorenzo
> return PCIBIOS_SUCCESSFUL;
> }
>
> 3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
>or pci_setup_device() will get failed:
>
>...
>class = pci_class(dev);
>dev->revision = class & 0xff;
>dev->class = class >> 8; /* upper 3 bytes */
>
>switch (dev->hdr_type) { /* header type */
>...
>case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
> if (class != PCI_CLASS_BRIDGE_PCI) /* class must be
> PCI_CLASS_BRIDGE_PCI */
> goto bad;
>
>
> thanks.
>
> hanjie
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Tue, Dec 04, 2018 at 06:40:55PM +0800, Hanjie Lin wrote:
>
>
> On 2018/12/4 6:57, Bjorn Helgaas wrote:
> > On Mon, Dec 03, 2018 at 04:41:50PM +, Lorenzo Pieralisi wrote:
> >> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
> >>
> >> [...]
> >>
> >>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
> >>> size,
> >>> + u32 *val)
> >>> +{
> >>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>> +
> >>> + /*
> >>> + * there is a bug of MESON AXG pcie controller that software can not
> >>> + * programe PCI_CLASS_DEVICE register, so we must return a fake right
> >>> + * value to ensure driver could probe successfully.
> >>> + */
> >>> + if (where == PCI_CLASS_REVISION) {
> >>> + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> >>> + /* keep revision id */
> >>> + *val &= PCI_CLASS_REVISION_MASK;
> >>> + *val |= PCI_CLASS_BRIDGE_PCI << 16;
> >>> + return PCIBIOS_SUCCESSFUL;
> >>> + }
> >>
> >> As I said before, this looks broken. If this code (or other drivers with
> >> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> >> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> >> it according to your comment above.
> >>
> >> I would like to pick Bjorn's brain on this to see what we can really do
> >> to fix this (and other) drivers.
> >
> > - Check to see whether you're reading anything in the 32-bit dword at
> > offset 0x08.
> >
> > - Do the 32-bit readl().
> >
> > - Insert the correct Sub-Class and Base Class code (you also throw
> > away the Programming Interface; not sure why that is)
> >
> > - If you're reading something smaller than 32 bits, mask & shift as
> > needed. pci_bridge_emul_conf_read() does something similar that
> > you might be able to copy.
> >
> > Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI? There
> > are several places in the kernel that currently depend on it, but I
> > think several of them *should* be checking dev->hdr_type to identify a
> > type 1 header instead.
> >
> > Bjorn
> >
> > .
> >
>
> Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from
> 0xa/PCI_CLASS_DEVICE)
> that I didn't considered.
>
> As your suggestion, I consider some code below may help this issue:
> 1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
>request all other *size* bytes will return error and dw_pcie_read()
>will also check register alignment.
>
> 2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or
> 0xa/PCI_CLASS_REVISION,
>we may need to correct class code.
>As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes
> from 0x8/PCI_CLASS_DEVICE
>or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class
> code.
>
> ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.
You can fix this too.
> static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> u32 *val)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> int ret;
>
> ret = dw_pcie_read(pci->dbi_base + where, size, val);
> if (ret != PCIBIOS_SUCCESSFUL)
> return ret;
>
> /*
> * there is a bug of MESON AXG pcie controller that software can not
> * programe PCI_CLASS_DEVICE register, so we must return a fake right
"There is a bug in the MESON AXG pcie controller whereby software cannot
programme the PCI_CLASS_DEVICE register, so we must fabricate the return
value in the config accessors."
> * value to ensure driver could probe successfully.
> */
> if (where == PCI_CLASS_REVISION && size == 4)
> *val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0x);
> else if (where == PCI_CLASS_DEVICE && size == 2)
> *val = PCI_CLASS_BRIDGE_PCI;
You can further filter it with (where & 0x1) == PCI_CLASS_DEVICE
and handle the size accordingly, so that even a byte access would
work, for completeness.
Lorenzo
> return PCIBIOS_SUCCESSFUL;
> }
>
> 3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
>or pci_setup_device() will get failed:
>
>...
>class = pci_class(dev);
>dev->revision = class & 0xff;
>dev->class = class >> 8; /* upper 3 bytes */
>
>switch (dev->hdr_type) { /* header type */
>...
>case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
> if (class != PCI_CLASS_BRIDGE_PCI) /* class must be
> PCI_CLASS_BRIDGE_PCI */
> goto bad;
>
>
> thanks.
>
> hanjie
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On 2018/12/4 6:57, Bjorn Helgaas wrote:
> On Mon, Dec 03, 2018 at 04:41:50PM +, Lorenzo Pieralisi wrote:
>> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
>>
>> [...]
>>
>>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
>>> size,
>>> + u32 *val)
>>> +{
>>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +
>>> + /*
>>> +* there is a bug of MESON AXG pcie controller that software can not
>>> +* programe PCI_CLASS_DEVICE register, so we must return a fake right
>>> +* value to ensure driver could probe successfully.
>>> +*/
>>> + if (where == PCI_CLASS_REVISION) {
>>> + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
>>> + /* keep revision id */
>>> + *val &= PCI_CLASS_REVISION_MASK;
>>> + *val |= PCI_CLASS_BRIDGE_PCI << 16;
>>> + return PCIBIOS_SUCCESSFUL;
>>> + }
>>
>> As I said before, this looks broken. If this code (or other drivers with
>> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
>> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
>> it according to your comment above.
>>
>> I would like to pick Bjorn's brain on this to see what we can really do
>> to fix this (and other) drivers.
>
> - Check to see whether you're reading anything in the 32-bit dword at
> offset 0x08.
>
> - Do the 32-bit readl().
>
> - Insert the correct Sub-Class and Base Class code (you also throw
> away the Programming Interface; not sure why that is)
>
> - If you're reading something smaller than 32 bits, mask & shift as
> needed. pci_bridge_emul_conf_read() does something similar that
> you might be able to copy.
>
> Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI? There
> are several places in the kernel that currently depend on it, but I
> think several of them *should* be checking dev->hdr_type to identify a
> type 1 header instead.
>
> Bjorn
>
> .
>
Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from
0xa/PCI_CLASS_DEVICE)
that I didn't considered.
As your suggestion, I consider some code below may help this issue:
1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
request all other *size* bytes will return error and dw_pcie_read()
will also check register alignment.
2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or
0xa/PCI_CLASS_REVISION,
we may need to correct class code.
As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes from
0x8/PCI_CLASS_DEVICE
or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class code.
ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.
static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
u32 *val)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
int ret;
ret = dw_pcie_read(pci->dbi_base + where, size, val);
if (ret != PCIBIOS_SUCCESSFUL)
return ret;
/*
* there is a bug of MESON AXG pcie controller that software can not
* programe PCI_CLASS_DEVICE register, so we must return a fake right
* value to ensure driver could probe successfully.
*/
if (where == PCI_CLASS_REVISION && size == 4)
*val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0x);
else if (where == PCI_CLASS_DEVICE && size == 2)
*val = PCI_CLASS_BRIDGE_PCI;
return PCIBIOS_SUCCESSFUL;
}
3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
or pci_setup_device() will get failed:
...
class = pci_class(dev);
dev->revision = class & 0xff;
dev->class = class >> 8; /* upper 3 bytes */
switch (dev->hdr_type) { /* header type */
...
case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
if (class != PCI_CLASS_BRIDGE_PCI) /* class must be PCI_CLASS_BRIDGE_PCI
*/
goto bad;
thanks.
hanjie
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On 2018/12/4 6:57, Bjorn Helgaas wrote:
> On Mon, Dec 03, 2018 at 04:41:50PM +, Lorenzo Pieralisi wrote:
>> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
>>
>> [...]
>>
>>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
>>> size,
>>> + u32 *val)
>>> +{
>>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +
>>> + /*
>>> +* there is a bug of MESON AXG pcie controller that software can not
>>> +* programe PCI_CLASS_DEVICE register, so we must return a fake right
>>> +* value to ensure driver could probe successfully.
>>> +*/
>>> + if (where == PCI_CLASS_REVISION) {
>>> + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
>>> + /* keep revision id */
>>> + *val &= PCI_CLASS_REVISION_MASK;
>>> + *val |= PCI_CLASS_BRIDGE_PCI << 16;
>>> + return PCIBIOS_SUCCESSFUL;
>>> + }
>>
>> As I said before, this looks broken. If this code (or other drivers with
>> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
>> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
>> it according to your comment above.
>>
>> I would like to pick Bjorn's brain on this to see what we can really do
>> to fix this (and other) drivers.
>
> - Check to see whether you're reading anything in the 32-bit dword at
> offset 0x08.
>
> - Do the 32-bit readl().
>
> - Insert the correct Sub-Class and Base Class code (you also throw
> away the Programming Interface; not sure why that is)
>
> - If you're reading something smaller than 32 bits, mask & shift as
> needed. pci_bridge_emul_conf_read() does something similar that
> you might be able to copy.
>
> Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI? There
> are several places in the kernel that currently depend on it, but I
> think several of them *should* be checking dev->hdr_type to identify a
> type 1 header instead.
>
> Bjorn
>
> .
>
Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from
0xa/PCI_CLASS_DEVICE)
that I didn't considered.
As your suggestion, I consider some code below may help this issue:
1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
request all other *size* bytes will return error and dw_pcie_read()
will also check register alignment.
2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or
0xa/PCI_CLASS_REVISION,
we may need to correct class code.
As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes from
0x8/PCI_CLASS_DEVICE
or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class code.
ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.
static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
u32 *val)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
int ret;
ret = dw_pcie_read(pci->dbi_base + where, size, val);
if (ret != PCIBIOS_SUCCESSFUL)
return ret;
/*
* there is a bug of MESON AXG pcie controller that software can not
* programe PCI_CLASS_DEVICE register, so we must return a fake right
* value to ensure driver could probe successfully.
*/
if (where == PCI_CLASS_REVISION && size == 4)
*val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0x);
else if (where == PCI_CLASS_DEVICE && size == 2)
*val = PCI_CLASS_BRIDGE_PCI;
return PCIBIOS_SUCCESSFUL;
}
3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
or pci_setup_device() will get failed:
...
class = pci_class(dev);
dev->revision = class & 0xff;
dev->class = class >> 8; /* upper 3 bytes */
switch (dev->hdr_type) { /* header type */
...
case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
if (class != PCI_CLASS_BRIDGE_PCI) /* class must be PCI_CLASS_BRIDGE_PCI
*/
goto bad;
thanks.
hanjie
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Mon, Dec 03, 2018 at 04:41:50PM +, Lorenzo Pieralisi wrote:
> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
>
> [...]
>
> > +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
> > size,
> > + u32 *val)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > + /*
> > +* there is a bug of MESON AXG pcie controller that software can not
> > +* programe PCI_CLASS_DEVICE register, so we must return a fake right
> > +* value to ensure driver could probe successfully.
> > +*/
> > + if (where == PCI_CLASS_REVISION) {
> > + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> > + /* keep revision id */
> > + *val &= PCI_CLASS_REVISION_MASK;
> > + *val |= PCI_CLASS_BRIDGE_PCI << 16;
> > + return PCIBIOS_SUCCESSFUL;
> > + }
>
> As I said before, this looks broken. If this code (or other drivers with
> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> it according to your comment above.
>
> I would like to pick Bjorn's brain on this to see what we can really do
> to fix this (and other) drivers.
- Check to see whether you're reading anything in the 32-bit dword at
offset 0x08.
- Do the 32-bit readl().
- Insert the correct Sub-Class and Base Class code (you also throw
away the Programming Interface; not sure why that is)
- If you're reading something smaller than 32 bits, mask & shift as
needed. pci_bridge_emul_conf_read() does something similar that
you might be able to copy.
Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI? There
are several places in the kernel that currently depend on it, but I
think several of them *should* be checking dev->hdr_type to identify a
type 1 header instead.
Bjorn
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Mon, Dec 03, 2018 at 04:41:50PM +, Lorenzo Pieralisi wrote:
> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
>
> [...]
>
> > +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
> > size,
> > + u32 *val)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > + /*
> > +* there is a bug of MESON AXG pcie controller that software can not
> > +* programe PCI_CLASS_DEVICE register, so we must return a fake right
> > +* value to ensure driver could probe successfully.
> > +*/
> > + if (where == PCI_CLASS_REVISION) {
> > + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> > + /* keep revision id */
> > + *val &= PCI_CLASS_REVISION_MASK;
> > + *val |= PCI_CLASS_BRIDGE_PCI << 16;
> > + return PCIBIOS_SUCCESSFUL;
> > + }
>
> As I said before, this looks broken. If this code (or other drivers with
> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> it according to your comment above.
>
> I would like to pick Bjorn's brain on this to see what we can really do
> to fix this (and other) drivers.
- Check to see whether you're reading anything in the 32-bit dword at
offset 0x08.
- Do the 32-bit readl().
- Insert the correct Sub-Class and Base Class code (you also throw
away the Programming Interface; not sure why that is)
- If you're reading something smaller than 32 bits, mask & shift as
needed. pci_bridge_emul_conf_read() does something similar that
you might be able to copy.
Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI? There
are several places in the kernel that currently depend on it, but I
think several of them *should* be checking dev->hdr_type to identify a
type 1 header instead.
Bjorn
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
[...]
> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> + u32 *val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> + /*
> + * there is a bug of MESON AXG pcie controller that software can not
> + * programe PCI_CLASS_DEVICE register, so we must return a fake right
> + * value to ensure driver could probe successfully.
> + */
> + if (where == PCI_CLASS_REVISION) {
> + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> + /* keep revision id */
> + *val &= PCI_CLASS_REVISION_MASK;
> + *val |= PCI_CLASS_BRIDGE_PCI << 16;
> + return PCIBIOS_SUCCESSFUL;
> + }
As I said before, this looks broken. If this code (or other drivers with
the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
it according to your comment above.
I would like to pick Bjorn's brain on this to see what we can really do
to fix this (and other) drivers.
Thanks,
Lorenzo
> + return dw_pcie_read(pci->dbi_base + where, size, val);
> +}
> +
> +static int meson_pcie_wr_own_conf(struct pcie_port *pp, int where,
> + int size, u32 val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> + return dw_pcie_write(pci->dbi_base + where, size, val);
> +}
> +
> +static int meson_pcie_link_up(struct dw_pcie *pci)
> +{
> + struct meson_pcie *mp = to_meson_pcie(pci);
> + struct device *dev = pci->dev;
> + u32 smlh_up = 0;
> + u32 ltssm_up = 0;
> + u32 rdlh_up = 0;
> + u32 speed_okay = 0;
> + u32 cnt = 0;
> + u32 state12, state17;
> +
> + while (smlh_up == 0 || rdlh_up == 0 || ltssm_up == 0 ||
> +speed_okay == 0) {
> + udelay(20);
> +
> + state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
> + state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
> + smlh_up = IS_SMLH_LINK_UP(state12);
> + rdlh_up = IS_RDLH_LINK_UP(state12);
> + ltssm_up = IS_LTSSM_UP(state12);
> +
> + if (PM_CURRENT_STATE(state17) < PCIE_GEN3)
> + speed_okay = 1;
> +
> + if (smlh_up)
> + dev_dbg(dev, "smlh_link_up is on\n");
> + if (rdlh_up)
> + dev_dbg(dev, "rdlh_link_up is on\n");
> + if (ltssm_up)
> + dev_dbg(dev, "ltssm_up is on\n");
> + if (speed_okay)
> + dev_dbg(dev, "speed_okay\n");
> +
> + cnt++;
> +
> + if (cnt >= WAIT_LINKUP_TIMEOUT) {
> + dev_err(dev, "Error: Wait linkup timeout.\n");
> + return 0;
> + }
> + }
> +
> + return 1;
> +}
> +
> +static int meson_pcie_host_init(struct pcie_port *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct meson_pcie *mp = to_meson_pcie(pci);
> + int ret;
> +
> + ret = meson_pcie_establish_link(mp);
> + if (ret)
> + return ret;
> +
> + meson_pcie_enable_interrupts(mp);
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_host_ops meson_pcie_host_ops = {
> + .rd_own_conf = meson_pcie_rd_own_conf,
> + .wr_own_conf = meson_pcie_wr_own_conf,
> + .host_init = meson_pcie_host_init,
> +};
> +
> +static int meson_add_pcie_port(struct meson_pcie *mp,
> +struct platform_device *pdev)
> +{
> + struct dw_pcie *pci = >pci;
> + struct pcie_port *pp = >pp;
> + struct device *dev = >dev;
> + int ret;
> +
> + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> + pp->msi_irq = platform_get_irq(pdev, 0);
> + if (pp->msi_irq < 0) {
> + dev_err(dev, "failed to get msi irq\n");
> + return pp->msi_irq;
> + }
> + }
> +
> + pp->ops = _pcie_host_ops;
> + pci->dbi_base = mp->mem_res.elbi_base;
> +
> + ret = dw_pcie_host_init(pp);
> + if (ret) {
> + dev_err(dev, "failed to initialize host\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> + .link_up = meson_pcie_link_up,
> +};
> +
> +static int meson_pcie_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct dw_pcie *pci;
> + struct meson_pcie *mp;
> + int ret;
> +
> + mp = devm_kzalloc(dev, sizeof(*mp), GFP_KERNEL);
> + if (!mp)
> + return -ENOMEM;
> +
> + pci = >pci;
> + pci->dev = dev;
> + pci->ops = _pcie_ops;
> +
> + mp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(mp->reset_gpio)) {
> + dev_err(dev, "Get reset gpio failed\n");
> + return PTR_ERR(mp->reset_gpio);
> + }
> +
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
[...]
> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> + u32 *val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> + /*
> + * there is a bug of MESON AXG pcie controller that software can not
> + * programe PCI_CLASS_DEVICE register, so we must return a fake right
> + * value to ensure driver could probe successfully.
> + */
> + if (where == PCI_CLASS_REVISION) {
> + *val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> + /* keep revision id */
> + *val &= PCI_CLASS_REVISION_MASK;
> + *val |= PCI_CLASS_BRIDGE_PCI << 16;
> + return PCIBIOS_SUCCESSFUL;
> + }
As I said before, this looks broken. If this code (or other drivers with
the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
it according to your comment above.
I would like to pick Bjorn's brain on this to see what we can really do
to fix this (and other) drivers.
Thanks,
Lorenzo
> + return dw_pcie_read(pci->dbi_base + where, size, val);
> +}
> +
> +static int meson_pcie_wr_own_conf(struct pcie_port *pp, int where,
> + int size, u32 val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> + return dw_pcie_write(pci->dbi_base + where, size, val);
> +}
> +
> +static int meson_pcie_link_up(struct dw_pcie *pci)
> +{
> + struct meson_pcie *mp = to_meson_pcie(pci);
> + struct device *dev = pci->dev;
> + u32 smlh_up = 0;
> + u32 ltssm_up = 0;
> + u32 rdlh_up = 0;
> + u32 speed_okay = 0;
> + u32 cnt = 0;
> + u32 state12, state17;
> +
> + while (smlh_up == 0 || rdlh_up == 0 || ltssm_up == 0 ||
> +speed_okay == 0) {
> + udelay(20);
> +
> + state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
> + state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
> + smlh_up = IS_SMLH_LINK_UP(state12);
> + rdlh_up = IS_RDLH_LINK_UP(state12);
> + ltssm_up = IS_LTSSM_UP(state12);
> +
> + if (PM_CURRENT_STATE(state17) < PCIE_GEN3)
> + speed_okay = 1;
> +
> + if (smlh_up)
> + dev_dbg(dev, "smlh_link_up is on\n");
> + if (rdlh_up)
> + dev_dbg(dev, "rdlh_link_up is on\n");
> + if (ltssm_up)
> + dev_dbg(dev, "ltssm_up is on\n");
> + if (speed_okay)
> + dev_dbg(dev, "speed_okay\n");
> +
> + cnt++;
> +
> + if (cnt >= WAIT_LINKUP_TIMEOUT) {
> + dev_err(dev, "Error: Wait linkup timeout.\n");
> + return 0;
> + }
> + }
> +
> + return 1;
> +}
> +
> +static int meson_pcie_host_init(struct pcie_port *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct meson_pcie *mp = to_meson_pcie(pci);
> + int ret;
> +
> + ret = meson_pcie_establish_link(mp);
> + if (ret)
> + return ret;
> +
> + meson_pcie_enable_interrupts(mp);
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_host_ops meson_pcie_host_ops = {
> + .rd_own_conf = meson_pcie_rd_own_conf,
> + .wr_own_conf = meson_pcie_wr_own_conf,
> + .host_init = meson_pcie_host_init,
> +};
> +
> +static int meson_add_pcie_port(struct meson_pcie *mp,
> +struct platform_device *pdev)
> +{
> + struct dw_pcie *pci = >pci;
> + struct pcie_port *pp = >pp;
> + struct device *dev = >dev;
> + int ret;
> +
> + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> + pp->msi_irq = platform_get_irq(pdev, 0);
> + if (pp->msi_irq < 0) {
> + dev_err(dev, "failed to get msi irq\n");
> + return pp->msi_irq;
> + }
> + }
> +
> + pp->ops = _pcie_host_ops;
> + pci->dbi_base = mp->mem_res.elbi_base;
> +
> + ret = dw_pcie_host_init(pp);
> + if (ret) {
> + dev_err(dev, "failed to initialize host\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> + .link_up = meson_pcie_link_up,
> +};
> +
> +static int meson_pcie_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct dw_pcie *pci;
> + struct meson_pcie *mp;
> + int ret;
> +
> + mp = devm_kzalloc(dev, sizeof(*mp), GFP_KERNEL);
> + if (!mp)
> + return -ENOMEM;
> +
> + pci = >pci;
> + pci->dev = dev;
> + pci->ops = _pcie_ops;
> +
> + mp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(mp->reset_gpio)) {
> + dev_err(dev, "Get reset gpio failed\n");
> + return PTR_ERR(mp->reset_gpio);
> + }
> +
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Thu, Nov 29, 2018 at 08:08:57PM +0800, Hanjie Lin wrote: > Yes, it's really a silly mistake. No one gets annoyed about typos. We're all human and we make mistakes. (This was going to an uplifting email, but then I had to mention that what does make people annoyed is OS abstraction layers. People try to use the same driver on both linux and windows. Absolute madness!) regards, dan carpenter
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Thu, Nov 29, 2018 at 08:08:57PM +0800, Hanjie Lin wrote: > Yes, it's really a silly mistake. No one gets annoyed about typos. We're all human and we make mistakes. (This was going to an uplifting email, but then I had to mention that what does make people annoyed is OS abstraction layers. People try to use the same driver on both linux and windows. Absolute madness!) regards, dan carpenter
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On 2018/11/29 17:03, Dan Carpenter wrote:
> Hi Yue,
>
> url:
> https://github.com/0day-ci/linux/commits/Hanjie-Lin/dt-bindings-PCI-meson-add-DT-bindings-for-Amlogic-Meson-PCIe-controller/20181122-225955
> base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
>
> smatch warnings:
> drivers/pci/controller/dwc/pci-meson.c:171 meson_pcie_get_mem_shared() error:
> passing non negative 6 to ERR_PTR
>
> #
> https://github.com/0day-ci/linux/commit/c882cdc75e49b6de65cd3d95ebf688272af6b5f9
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout c882cdc75e49b6de65cd3d95ebf688272af6b5f9
> vim +171 drivers/pci/controller/dwc/pci-meson.c
>
> c882cdc75 Yue Wang 2018-11-22 160
> c882cdc75 Yue Wang 2018-11-22 161 static void __iomem
> *meson_pcie_get_mem_shared(struct platform_device *pdev,
> c882cdc75 Yue Wang 2018-11-22 162
> struct meson_pcie *mp,
> c882cdc75 Yue Wang 2018-11-22 163
> const char *id)
> c882cdc75 Yue Wang 2018-11-22 164 {
> c882cdc75 Yue Wang 2018-11-22 165struct device *dev = mp->pci.dev;
> c882cdc75 Yue Wang 2018-11-22 166struct resource *res;
> c882cdc75 Yue Wang 2018-11-22 167
> c882cdc75 Yue Wang 2018-11-22 168res =
> platform_get_resource_byname(pdev, IORESOURCE_MEM, id);
> c882cdc75 Yue Wang 2018-11-22 169if (!res) {
> c882cdc75 Yue Wang 2018-11-22 170dev_err(dev, "No REG resource
> %s\n", id);
> c882cdc75 Yue Wang 2018-11-22 @171return ERR_PTR(ENXIO);
>^
>-ENXIO
>
> c882cdc75 Yue Wang 2018-11-22 172}
> c882cdc75 Yue Wang 2018-11-22 173
> c882cdc75 Yue Wang 2018-11-22 174return devm_ioremap(dev, res->start,
> resource_size(res));
> c882cdc75 Yue Wang 2018-11-22 175 }
> c882cdc75 Yue Wang 2018-11-22 176
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
> .
>
Yes, it's really a silly mistake.
I'll fix it.
thanks.
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On 2018/11/29 17:03, Dan Carpenter wrote:
> Hi Yue,
>
> url:
> https://github.com/0day-ci/linux/commits/Hanjie-Lin/dt-bindings-PCI-meson-add-DT-bindings-for-Amlogic-Meson-PCIe-controller/20181122-225955
> base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
>
> smatch warnings:
> drivers/pci/controller/dwc/pci-meson.c:171 meson_pcie_get_mem_shared() error:
> passing non negative 6 to ERR_PTR
>
> #
> https://github.com/0day-ci/linux/commit/c882cdc75e49b6de65cd3d95ebf688272af6b5f9
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout c882cdc75e49b6de65cd3d95ebf688272af6b5f9
> vim +171 drivers/pci/controller/dwc/pci-meson.c
>
> c882cdc75 Yue Wang 2018-11-22 160
> c882cdc75 Yue Wang 2018-11-22 161 static void __iomem
> *meson_pcie_get_mem_shared(struct platform_device *pdev,
> c882cdc75 Yue Wang 2018-11-22 162
> struct meson_pcie *mp,
> c882cdc75 Yue Wang 2018-11-22 163
> const char *id)
> c882cdc75 Yue Wang 2018-11-22 164 {
> c882cdc75 Yue Wang 2018-11-22 165struct device *dev = mp->pci.dev;
> c882cdc75 Yue Wang 2018-11-22 166struct resource *res;
> c882cdc75 Yue Wang 2018-11-22 167
> c882cdc75 Yue Wang 2018-11-22 168res =
> platform_get_resource_byname(pdev, IORESOURCE_MEM, id);
> c882cdc75 Yue Wang 2018-11-22 169if (!res) {
> c882cdc75 Yue Wang 2018-11-22 170dev_err(dev, "No REG resource
> %s\n", id);
> c882cdc75 Yue Wang 2018-11-22 @171return ERR_PTR(ENXIO);
>^
>-ENXIO
>
> c882cdc75 Yue Wang 2018-11-22 172}
> c882cdc75 Yue Wang 2018-11-22 173
> c882cdc75 Yue Wang 2018-11-22 174return devm_ioremap(dev, res->start,
> resource_size(res));
> c882cdc75 Yue Wang 2018-11-22 175 }
> c882cdc75 Yue Wang 2018-11-22 176
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
> .
>
Yes, it's really a silly mistake.
I'll fix it.
thanks.
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
Hi Yue,
url:
https://github.com/0day-ci/linux/commits/Hanjie-Lin/dt-bindings-PCI-meson-add-DT-bindings-for-Amlogic-Meson-PCIe-controller/20181122-225955
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
smatch warnings:
drivers/pci/controller/dwc/pci-meson.c:171 meson_pcie_get_mem_shared() error:
passing non negative 6 to ERR_PTR
#
https://github.com/0day-ci/linux/commit/c882cdc75e49b6de65cd3d95ebf688272af6b5f9
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c882cdc75e49b6de65cd3d95ebf688272af6b5f9
vim +171 drivers/pci/controller/dwc/pci-meson.c
c882cdc75 Yue Wang 2018-11-22 160
c882cdc75 Yue Wang 2018-11-22 161 static void __iomem
*meson_pcie_get_mem_shared(struct platform_device *pdev,
c882cdc75 Yue Wang 2018-11-22 162
struct meson_pcie *mp,
c882cdc75 Yue Wang 2018-11-22 163
const char *id)
c882cdc75 Yue Wang 2018-11-22 164 {
c882cdc75 Yue Wang 2018-11-22 165 struct device *dev = mp->pci.dev;
c882cdc75 Yue Wang 2018-11-22 166 struct resource *res;
c882cdc75 Yue Wang 2018-11-22 167
c882cdc75 Yue Wang 2018-11-22 168 res =
platform_get_resource_byname(pdev, IORESOURCE_MEM, id);
c882cdc75 Yue Wang 2018-11-22 169 if (!res) {
c882cdc75 Yue Wang 2018-11-22 170 dev_err(dev, "No REG resource
%s\n", id);
c882cdc75 Yue Wang 2018-11-22 @171 return ERR_PTR(ENXIO);
^
-ENXIO
c882cdc75 Yue Wang 2018-11-22 172 }
c882cdc75 Yue Wang 2018-11-22 173
c882cdc75 Yue Wang 2018-11-22 174 return devm_ioremap(dev, res->start,
resource_size(res));
c882cdc75 Yue Wang 2018-11-22 175 }
c882cdc75 Yue Wang 2018-11-22 176
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
Hi Yue,
url:
https://github.com/0day-ci/linux/commits/Hanjie-Lin/dt-bindings-PCI-meson-add-DT-bindings-for-Amlogic-Meson-PCIe-controller/20181122-225955
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
smatch warnings:
drivers/pci/controller/dwc/pci-meson.c:171 meson_pcie_get_mem_shared() error:
passing non negative 6 to ERR_PTR
#
https://github.com/0day-ci/linux/commit/c882cdc75e49b6de65cd3d95ebf688272af6b5f9
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c882cdc75e49b6de65cd3d95ebf688272af6b5f9
vim +171 drivers/pci/controller/dwc/pci-meson.c
c882cdc75 Yue Wang 2018-11-22 160
c882cdc75 Yue Wang 2018-11-22 161 static void __iomem
*meson_pcie_get_mem_shared(struct platform_device *pdev,
c882cdc75 Yue Wang 2018-11-22 162
struct meson_pcie *mp,
c882cdc75 Yue Wang 2018-11-22 163
const char *id)
c882cdc75 Yue Wang 2018-11-22 164 {
c882cdc75 Yue Wang 2018-11-22 165 struct device *dev = mp->pci.dev;
c882cdc75 Yue Wang 2018-11-22 166 struct resource *res;
c882cdc75 Yue Wang 2018-11-22 167
c882cdc75 Yue Wang 2018-11-22 168 res =
platform_get_resource_byname(pdev, IORESOURCE_MEM, id);
c882cdc75 Yue Wang 2018-11-22 169 if (!res) {
c882cdc75 Yue Wang 2018-11-22 170 dev_err(dev, "No REG resource
%s\n", id);
c882cdc75 Yue Wang 2018-11-22 @171 return ERR_PTR(ENXIO);
^
-ENXIO
c882cdc75 Yue Wang 2018-11-22 172 }
c882cdc75 Yue Wang 2018-11-22 173
c882cdc75 Yue Wang 2018-11-22 174 return devm_ioremap(dev, res->start,
resource_size(res));
c882cdc75 Yue Wang 2018-11-22 175 }
c882cdc75 Yue Wang 2018-11-22 176
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
From: Yue Wang The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core. This patch adds the driver support for Meson PCIe controller. Signed-off-by: Yue Wang Signed-off-by: Hanjie Lin --- MAINTAINERS| 7 + drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile| 1 + drivers/pci/controller/dwc/pci-meson.c | 597 + 4 files changed, 615 insertions(+) create mode 100644 drivers/pci/controller/dwc/pci-meson.c diff --git a/MAINTAINERS b/MAINTAINERS index 77b1174..4fb9098 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11530,6 +11530,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/ S: Supported F: drivers/pci/controller/ +PCIE DRIVER FOR AMLOGIC MESON +M: Yue Wang +L: linux-...@vger.kernel.org +L: linux-amlo...@lists.infradead.org +S: Maintained +F: drivers/pci/controller/dwc/pci-meson.c + PCIE DRIVER FOR AXIS ARTPEC M: Jesper Nilsson L: linux-arm-ker...@axis.com diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 91b0194..7800322 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -193,4 +193,14 @@ config PCIE_HISI_STB help Say Y here if you want PCIe controller support on HiSilicon STB SoCs +config PCI_MESON + bool "MESON PCIe controller" + depends on PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + help + Say Y here if you want to enable PCI controller support on Amlogic + SoCs. The PCI controller on Amlogic is based on DesignWare hardware + and therefore the driver re-uses the DesignWare core functions to + implement the driver. + endmenu diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index fcf91ea..e05a015 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o +obj-$(CONFIG_PCI_MESON) += pci-meson.o # The following drivers are for devices that use the generic ACPI # pci_root.c driver but don't support standard ECAM config access. diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c new file mode 100644 index 000..ef30a5a --- /dev/null +++ b/drivers/pci/controller/dwc/pci-meson.c @@ -0,0 +1,597 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe host controller driver for Amlogic MESON SoCs + * + * Copyright (c) 2018 Amlogic, inc. + * Author: Yue Wang + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "pcie-designware.h" + +#define to_meson_pcie(x) dev_get_drvdata((x)->dev) + +/* External local bus interface registers */ +#define PLR_OFFSET 0x700 +#define PCIE_PORT_LINK_CTRL_OFF(PLR_OFFSET + 0x10) +#define FAST_LINK_MODE BIT(7) +#define LINK_CAPABLE_MASK GENMASK(21, 16) +#define LINK_CAPABLE_X1BIT(16) + +#define PCIE_GEN2_CTRL_OFF (PLR_OFFSET + 0x10c) +#define NUM_OF_LANES_MASK GENMASK(12, 8) +#define NUM_OF_LANES_X1BIT(8) +#define DIRECT_SPEED_CHANGEBIT(17) + +#define TYPE1_HDR_OFFSET 0x0 +#define PCIE_STATUS_COMMAND(TYPE1_HDR_OFFSET + 0x04) +#define PCI_IO_EN BIT(0) +#define PCI_MEM_SPACE_EN BIT(1) +#define PCI_BUS_MASTER_EN BIT(2) + +#define PCIE_BASE_ADDR0(TYPE1_HDR_OFFSET + 0x10) +#define PCIE_BASE_ADDR1(TYPE1_HDR_OFFSET + 0x14) + +#define PCIE_CAP_OFFSET0x70 +#define PCIE_DEV_CTRL_DEV_STUS (PCIE_CAP_OFFSET + 0x08) +#define PCIE_CAP_MAX_PAYLOAD_MASK GENMASK(7, 5) +#define PCIE_CAP_MAX_PAYLOAD_SIZE(x) ((x) << 5) +#define PCIE_CAP_MAX_READ_REQ_MASK GENMASK(14, 12) +#define PCIE_CAP_MAX_READ_REQ_SIZE(x) ((x) << 12) + +#define PCI_CLASS_REVISION_MASKGENMASK(7, 0) + +/* PCIe specific config registers */ +#define PCIE_CFG0 0x0 +#define APP_LTSSM_ENABLE BIT(7) + +#define PCIE_CFG_STATUS12 0x30 +#define IS_SMLH_LINK_UP(x) ((x) & (1 << 6)) +#define IS_RDLH_LINK_UP(x) ((x) & (1 << 16)) +#define IS_LTSSM_UP(x) x) >> 10) & 0x1f) == 0x11) + +#define PCIE_CFG_STATUS17 0x44 +#define PM_CURRENT_STATE(x)(((x) >> 7) & 0x1) + +#define WAIT_LINKUP_TIMEOUT2000 +#define PORT_CLK_RATE 1UL +#define MAX_PAYLOAD_SIZE 256 +#define MAX_READ_REQ_SIZE 256 +#define MESON_PCIE_PHY_POWERUP
[PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
From: Yue Wang The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core. This patch adds the driver support for Meson PCIe controller. Signed-off-by: Yue Wang Signed-off-by: Hanjie Lin --- MAINTAINERS| 7 + drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile| 1 + drivers/pci/controller/dwc/pci-meson.c | 597 + 4 files changed, 615 insertions(+) create mode 100644 drivers/pci/controller/dwc/pci-meson.c diff --git a/MAINTAINERS b/MAINTAINERS index 77b1174..4fb9098 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11530,6 +11530,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/ S: Supported F: drivers/pci/controller/ +PCIE DRIVER FOR AMLOGIC MESON +M: Yue Wang +L: linux-...@vger.kernel.org +L: linux-amlo...@lists.infradead.org +S: Maintained +F: drivers/pci/controller/dwc/pci-meson.c + PCIE DRIVER FOR AXIS ARTPEC M: Jesper Nilsson L: linux-arm-ker...@axis.com diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 91b0194..7800322 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -193,4 +193,14 @@ config PCIE_HISI_STB help Say Y here if you want PCIe controller support on HiSilicon STB SoCs +config PCI_MESON + bool "MESON PCIe controller" + depends on PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + help + Say Y here if you want to enable PCI controller support on Amlogic + SoCs. The PCI controller on Amlogic is based on DesignWare hardware + and therefore the driver re-uses the DesignWare core functions to + implement the driver. + endmenu diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index fcf91ea..e05a015 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o +obj-$(CONFIG_PCI_MESON) += pci-meson.o # The following drivers are for devices that use the generic ACPI # pci_root.c driver but don't support standard ECAM config access. diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c new file mode 100644 index 000..ef30a5a --- /dev/null +++ b/drivers/pci/controller/dwc/pci-meson.c @@ -0,0 +1,597 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe host controller driver for Amlogic MESON SoCs + * + * Copyright (c) 2018 Amlogic, inc. + * Author: Yue Wang + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "pcie-designware.h" + +#define to_meson_pcie(x) dev_get_drvdata((x)->dev) + +/* External local bus interface registers */ +#define PLR_OFFSET 0x700 +#define PCIE_PORT_LINK_CTRL_OFF(PLR_OFFSET + 0x10) +#define FAST_LINK_MODE BIT(7) +#define LINK_CAPABLE_MASK GENMASK(21, 16) +#define LINK_CAPABLE_X1BIT(16) + +#define PCIE_GEN2_CTRL_OFF (PLR_OFFSET + 0x10c) +#define NUM_OF_LANES_MASK GENMASK(12, 8) +#define NUM_OF_LANES_X1BIT(8) +#define DIRECT_SPEED_CHANGEBIT(17) + +#define TYPE1_HDR_OFFSET 0x0 +#define PCIE_STATUS_COMMAND(TYPE1_HDR_OFFSET + 0x04) +#define PCI_IO_EN BIT(0) +#define PCI_MEM_SPACE_EN BIT(1) +#define PCI_BUS_MASTER_EN BIT(2) + +#define PCIE_BASE_ADDR0(TYPE1_HDR_OFFSET + 0x10) +#define PCIE_BASE_ADDR1(TYPE1_HDR_OFFSET + 0x14) + +#define PCIE_CAP_OFFSET0x70 +#define PCIE_DEV_CTRL_DEV_STUS (PCIE_CAP_OFFSET + 0x08) +#define PCIE_CAP_MAX_PAYLOAD_MASK GENMASK(7, 5) +#define PCIE_CAP_MAX_PAYLOAD_SIZE(x) ((x) << 5) +#define PCIE_CAP_MAX_READ_REQ_MASK GENMASK(14, 12) +#define PCIE_CAP_MAX_READ_REQ_SIZE(x) ((x) << 12) + +#define PCI_CLASS_REVISION_MASKGENMASK(7, 0) + +/* PCIe specific config registers */ +#define PCIE_CFG0 0x0 +#define APP_LTSSM_ENABLE BIT(7) + +#define PCIE_CFG_STATUS12 0x30 +#define IS_SMLH_LINK_UP(x) ((x) & (1 << 6)) +#define IS_RDLH_LINK_UP(x) ((x) & (1 << 16)) +#define IS_LTSSM_UP(x) x) >> 10) & 0x1f) == 0x11) + +#define PCIE_CFG_STATUS17 0x44 +#define PM_CURRENT_STATE(x)(((x) >> 7) & 0x1) + +#define WAIT_LINKUP_TIMEOUT2000 +#define PORT_CLK_RATE 1UL +#define MAX_PAYLOAD_SIZE 256 +#define MAX_READ_REQ_SIZE 256 +#define MESON_PCIE_PHY_POWERUP
