Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port
On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> Depending of chip revision reset lines are inverted. It is also
> necessary to read PCIE_FTS_NUM register before enabling the phy.
> Hence update the code to achieve this.
>
> Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out
> 'mt7621_pcie_enable_port'
> function"
> Reported-by: NeilBrown
>
> Signed-off-by: Sergio Paracuellos
> ---
> drivers/staging/mt7621-pci/pci-mt7621.c | 38 +
> 1 file changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
> b/drivers/staging/mt7621-pci/pci-mt7621.c
> index ba81b34dc1b7..1b63706e129b 100644
> --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port
> *port)
> set_phy_for_ssc(port);
> }
>
> +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
> +{
> + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> +
> + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
> + reset_control_assert(port->pcie_rst);
> + else
> + reset_control_deassert(port->pcie_rst);
> +}
> +
> +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> +{
> + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> +
> + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
> + reset_control_deassert(port->pcie_rst);
> + else
> + reset_control_assert(port->pcie_rst);
> +}
The commit message is very good that on some chips assert and deassert
mean the opposite but I feel like this should be commented in the code
as well or people reading this code will be very confused.
Also it would be better if we could change this from a white list to a
black list. In other words, if they were to come out with new revs
of the hardware, we should assume that assert means assert and deassert
means deassert.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port
On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter wrote:
>
> On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> > Depending of chip revision reset lines are inverted. It is also
> > necessary to read PCIE_FTS_NUM register before enabling the phy.
> > Hence update the code to achieve this.
> >
> > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out
> > 'mt7621_pcie_enable_port'
> > function"
> > Reported-by: NeilBrown
> >
> > Signed-off-by: Sergio Paracuellos
> > ---
> > drivers/staging/mt7621-pci/pci-mt7621.c | 38 +
> > 1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
> > b/drivers/staging/mt7621-pci/pci-mt7621.c
> > index ba81b34dc1b7..1b63706e129b 100644
> > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port
> > *port)
> > set_phy_for_ssc(port);
> > }
> >
> > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
> > +{
> > + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > +
> > + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
> > + reset_control_assert(port->pcie_rst);
> > + else
> > + reset_control_deassert(port->pcie_rst);
> > +}
> > +
> > +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> > +{
> > + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > +
> > + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
> > + reset_control_deassert(port->pcie_rst);
> > + else
> > + reset_control_assert(port->pcie_rst);
> > +}
>
> The commit message is very good that on some chips assert and deassert
> mean the opposite but I feel like this should be commented in the code
> as well or people reading this code will be very confused.
>
Ok, Dan. Agreed. I will add some comment in next series.
> Also it would be better if we could change this from a white list to a
> black list. In other words, if they were to come out with new revs
> of the hardware, we should assume that assert means assert and deassert
> means deassert.
I understand what you are saying but I don't know how to handle those
white and black lists... Is there some kind of example where I can
take a look to handle this in a proper way?
>
> regards,
> dan carpenter
Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port
On Mon, Nov 26, 2018 at 08:57:09PM +0100, Sergio Paracuellos wrote:
> On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter
> wrote:
> >
> > On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> > > Depending of chip revision reset lines are inverted. It is also
> > > necessary to read PCIE_FTS_NUM register before enabling the phy.
> > > Hence update the code to achieve this.
> > >
> > > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out
> > > 'mt7621_pcie_enable_port'
> > > function"
> > > Reported-by: NeilBrown
> > >
> > > Signed-off-by: Sergio Paracuellos
> > > ---
> > > drivers/staging/mt7621-pci/pci-mt7621.c | 38 +
> > > 1 file changed, 32 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
> > > b/drivers/staging/mt7621-pci/pci-mt7621.c
> > > index ba81b34dc1b7..1b63706e129b 100644
> > > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> > > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> > > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct
> > > mt7621_pcie_port *port)
> > > set_phy_for_ssc(port);
> > > }
> > >
> > > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
> > > +{
> > > + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > > +
> > > + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
> > > + reset_control_assert(port->pcie_rst);
> > > + else
> > > + reset_control_deassert(port->pcie_rst);
> > > +}
> > > +
> > > +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> > > +{
> > > + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > > +
> > > + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
> > > + reset_control_deassert(port->pcie_rst);
> > > + else
> > > + reset_control_assert(port->pcie_rst);
> > > +}
> >
> > The commit message is very good that on some chips assert and deassert
> > mean the opposite but I feel like this should be commented in the code
> > as well or people reading this code will be very confused.
> >
>
> Ok, Dan. Agreed. I will add some comment in next series.
>
> > Also it would be better if we could change this from a white list to a
> > black list. In other words, if they were to come out with new revs
> > of the hardware, we should assume that assert means assert and deassert
> > means deassert.
>
> I understand what you are saying but I don't know how to handle those
> white and black lists... Is there some kind of example where I can
> take a look to handle this in a proper way?
>
What I mean is flip it around so that it becomes:
static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
{
u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
/* Some revisions have assert and deassert reversed. */
if (chip_in_list_of_reversed_revs) {
reset_control_assert(port->pcie_rst);
return;
}
reset_control_deassert(port->pcie_rst);
}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port
On Tue, Nov 27 2018, Dan Carpenter wrote:
> On Mon, Nov 26, 2018 at 08:57:09PM +0100, Sergio Paracuellos wrote:
>> On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter
>> wrote:
>> >
>> > On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
>> > > Depending of chip revision reset lines are inverted. It is also
>> > > necessary to read PCIE_FTS_NUM register before enabling the phy.
>> > > Hence update the code to achieve this.
>> > >
>> > > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out
>> > > 'mt7621_pcie_enable_port'
>> > > function"
>> > > Reported-by: NeilBrown
>> > >
>> > > Signed-off-by: Sergio Paracuellos
>> > > ---
>> > > drivers/staging/mt7621-pci/pci-mt7621.c | 38 +
>> > > 1 file changed, 32 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > b/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > index ba81b34dc1b7..1b63706e129b 100644
>> > > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct
>> > > mt7621_pcie_port *port)
>> > > set_phy_for_ssc(port);
>> > > }
>> > >
>> > > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
>> > > +{
>> > > + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>> > > +
>> > > + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
>> > > + reset_control_assert(port->pcie_rst);
>> > > + else
>> > > + reset_control_deassert(port->pcie_rst);
>> > > +}
>> > > +
>> > > +static inline void mt7621_control_deassert(struct mt7621_pcie_port
>> > > *port)
>> > > +{
>> > > + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>> > > +
>> > > + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
>> > > + reset_control_deassert(port->pcie_rst);
>> > > + else
>> > > + reset_control_assert(port->pcie_rst);
>> > > +}
>> >
>> > The commit message is very good that on some chips assert and deassert
>> > mean the opposite but I feel like this should be commented in the code
>> > as well or people reading this code will be very confused.
>> >
>>
>> Ok, Dan. Agreed. I will add some comment in next series.
>>
>> > Also it would be better if we could change this from a white list to a
>> > black list. In other words, if they were to come out with new revs
>> > of the hardware, we should assume that assert means assert and deassert
>> > means deassert.
>>
>> I understand what you are saying but I don't know how to handle those
>> white and black lists... Is there some kind of example where I can
>> take a look to handle this in a proper way?
>>
>
> What I mean is flip it around so that it becomes:
>
> static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> {
> u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>
> /* Some revisions have assert and deassert reversed. */
> if (chip_in_list_of_reversed_revs) {
Unfortunately we don't have that list.
All we have comes from:
https://github.com/mqmaker/linux/blob/master/arch/mips/ralink/pci.c#L97
which suggests that any MT7621 RAlink pci controller that doesn't have 0x0101 at
the end of the chip_id has inverted PCI resets.
We don't have a list of all MT7621 CHIP_IDs to produce a blacklist from.
The only other info we have is that 0x030103 is one particular MT7621
CHIP_ID that is inverted (the one that I own).
So while I agree that a black-list would be best, that facts are that we
don't have one.
Thanks,
NeilBrown
> reset_control_assert(port->pcie_rst);
> return;
> }
>
> reset_control_deassert(port->pcie_rst);
> }
>
> regards,
> dan carpenter
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port
On Tue, Nov 27, 2018 at 01:12:42PM +1100, NeilBrown wrote:
> On Tue, Nov 27 2018, Dan Carpenter wrote:
>
> > On Mon, Nov 26, 2018 at 08:57:09PM +0100, Sergio Paracuellos wrote:
> >> On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter
> >> wrote:
> >> >
> >> > On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> >> > > Depending of chip revision reset lines are inverted. It is also
> >> > > necessary to read PCIE_FTS_NUM register before enabling the phy.
> >> > > Hence update the code to achieve this.
> >> > >
> >> > > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out
> >> > > 'mt7621_pcie_enable_port'
> >> > > function"
> >> > > Reported-by: NeilBrown
> >> > >
> >> > > Signed-off-by: Sergio Paracuellos
> >> > > ---
> >> > > drivers/staging/mt7621-pci/pci-mt7621.c | 38 +
> >> > > 1 file changed, 32 insertions(+), 6 deletions(-)
> >> > >
> >> > > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
> >> > > b/drivers/staging/mt7621-pci/pci-mt7621.c
> >> > > index ba81b34dc1b7..1b63706e129b 100644
> >> > > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> >> > > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> >> > > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct
> >> > > mt7621_pcie_port *port)
> >> > > set_phy_for_ssc(port);
> >> > > }
> >> > >
> >> > > +static inline void mt7621_control_assert(struct mt7621_pcie_port
> >> > > *port)
> >> > > +{
> >> > > + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> >> > > +
> >> > > + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
> >> > > + reset_control_assert(port->pcie_rst);
> >> > > + else
> >> > > + reset_control_deassert(port->pcie_rst);
> >> > > +}
> >> > > +
> >> > > +static inline void mt7621_control_deassert(struct mt7621_pcie_port
> >> > > *port)
> >> > > +{
> >> > > + u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> >> > > +
> >> > > + if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
> >> > > + reset_control_deassert(port->pcie_rst);
> >> > > + else
> >> > > + reset_control_assert(port->pcie_rst);
> >> > > +}
> >> >
> >> > The commit message is very good that on some chips assert and deassert
> >> > mean the opposite but I feel like this should be commented in the code
> >> > as well or people reading this code will be very confused.
> >> >
> >>
> >> Ok, Dan. Agreed. I will add some comment in next series.
> >>
> >> > Also it would be better if we could change this from a white list to a
> >> > black list. In other words, if they were to come out with new revs
> >> > of the hardware, we should assume that assert means assert and deassert
> >> > means deassert.
> >>
> >> I understand what you are saying but I don't know how to handle those
> >> white and black lists... Is there some kind of example where I can
> >> take a look to handle this in a proper way?
> >>
> >
> > What I mean is flip it around so that it becomes:
> >
> > static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> > {
> > u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> >
> > /* Some revisions have assert and deassert reversed. */
> > if (chip_in_list_of_reversed_revs) {
>
> Unfortunately we don't have that list.
> All we have comes from:
> https://github.com/mqmaker/linux/blob/master/arch/mips/ralink/pci.c#L97
> which suggests that any MT7621 RAlink pci controller that doesn't have 0x0101
> at
> the end of the chip_id has inverted PCI resets.
> We don't have a list of all MT7621 CHIP_IDs to produce a blacklist from.
>
> The only other info we have is that 0x030103 is one particular MT7621
> CHIP_ID that is inverted (the one that I own).
>
> So while I agree that a black-list would be best, that facts are that we
> don't have one.
Ah... :(
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
