| Message ID | 20171207142715.32578-4-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Dec 07, 2017 at 03:27:10PM +0100, Jerome Brunet wrote: > Add read and write helpers to manipulate banked registers on this PHY > This helps clarify the settings applied to these registers in the init > function and upcoming changes. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/net/phy/meson-gxl.c | 103 ++++++++++++++++++++++++++++---------------- > 1 file changed, 67 insertions(+), 36 deletions(-) > > diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c > index d82aa8cea401..05054770aefb 100644 > --- a/drivers/net/phy/meson-gxl.c > +++ b/drivers/net/phy/meson-gxl.c > @@ -45,11 +45,13 @@ > #define FR_PLL_DIV0 0x1c > #define FR_PLL_DIV1 0x1d > > -static int meson_gxl_config_init(struct phy_device *phydev) > +static int meson_gxl_open_banks(struct phy_device *phydev) Hi Jerome Does the word bank come from the datasheet? Most of the phy drives use page instead. Also, we have discovered a race condition which affects drivers using pages, which can lead to corruption of registers. At some point, i expect we will be adding helpers to access paged registers, which do the right thing with respect to locks. So it would be nice if you used the work page, not bank. Thanks Andrew
On 07/12/2017 16:46, Andrew Lunn wrote: > On Thu, Dec 07, 2017 at 03:27:10PM +0100, Jerome Brunet wrote: >> Add read and write helpers to manipulate banked registers on this PHY >> This helps clarify the settings applied to these registers in the init >> function and upcoming changes. >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/net/phy/meson-gxl.c | 103 ++++++++++++++++++++++++++++---------------- >> 1 file changed, 67 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c >> index d82aa8cea401..05054770aefb 100644 >> --- a/drivers/net/phy/meson-gxl.c >> +++ b/drivers/net/phy/meson-gxl.c >> @@ -45,11 +45,13 @@ >> #define FR_PLL_DIV0 0x1c >> #define FR_PLL_DIV1 0x1d >> >> -static int meson_gxl_config_init(struct phy_device *phydev) >> +static int meson_gxl_open_banks(struct phy_device *phydev) > > Hi Jerome > > Does the word bank come from the datasheet? Most of the phy drives use > page instead. Yes, it's explicitly described as banks in the datasheet. Neil > > Also, we have discovered a race condition which affects drivers using > pages, which can lead to corruption of registers. At some point, i > expect we will be adding helpers to access paged registers, which do > the right thing with respect to locks. > > So it would be nice if you used the work page, not bank. > > Thanks > > Andrew > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, 2017-12-07 at 16:46 +0100, Andrew Lunn wrote: > On Thu, Dec 07, 2017 at 03:27:10PM +0100, Jerome Brunet wrote: > > Add read and write helpers to manipulate banked registers on this PHY > > This helps clarify the settings applied to these registers in the init > > function and upcoming changes. > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > --- > > drivers/net/phy/meson-gxl.c | 103 ++++++++++++++++++++++++++++------------- > > --- > > 1 file changed, 67 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c > > index d82aa8cea401..05054770aefb 100644 > > --- a/drivers/net/phy/meson-gxl.c > > +++ b/drivers/net/phy/meson-gxl.c > > @@ -45,11 +45,13 @@ > > #define FR_PLL_DIV0 0x1c > > #define FR_PLL_DIV1 0x1d > > > > -static int meson_gxl_config_init(struct phy_device *phydev) > > +static int meson_gxl_open_banks(struct phy_device *phydev) > > Hi Jerome > > Does the word bank come from the datasheet? Most of the phy drives use > page instead. > > Also, we have discovered a race condition which affects drivers using > pages, which can lead to corruption of registers. At some point, i > expect we will be adding helpers to access paged registers, which do > the right thing with respect to locks. > > So it would be nice if you used the work page, not bank. Banks actually comes from the datasheet, Yes. I don't mind renaming it but I would be making things up. As you wish ? Does the usual pages comes with this weird toggle thing to open the access ? Would we able to use these generic helpers with our this kind of quirks ? > > Thanks > > Andrew
> Banks actually comes from the datasheet, Yes. > I don't mind renaming it but I would be making things up. As you wish ? Keep it as is for the moment. > Does the usual pages comes with this weird toggle thing to open the access ? > Would we able to use these generic helpers with our this kind of quirks ? I don't think the API has been defined yet. But what has been discussed is adding functions to struct phy_driver. The driver can then implement whatever is needed to select a given page. There will then be helpers which take the lock, select the page, do the read/write, select page 0, and unlock. Supporting this funny toggle should not be a problem. Andrew
On Thu, Dec 07, 2017 at 05:02:35PM +0100, Andrew Lunn wrote: > > Banks actually comes from the datasheet, Yes. > > I don't mind renaming it but I would be making things up. As you wish ? > > Keep it as is for the moment. > > > Does the usual pages comes with this weird toggle thing to open the access ? > > Would we able to use these generic helpers with our this kind of quirks ? > > I don't think the API has been defined yet. But what has been > discussed is adding functions to struct phy_driver. The driver can > then implement whatever is needed to select a given page. There will > then be helpers which take the lock, select the page, do the > read/write, select page 0, and unlock. I'm not sure adding generic helpers really works, because the indirection through phy_driver just makes the code more complex. For Marvell, I ended up with: http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=phy&id=9ca46084228bb1b8851e7d24276d85c4ec6e13ae and the preceding three commits. The "generic" versions I came up with were basically lifting marvell_read_paged(), marvell_write_paged() and marvell_modify_paged() to phylib, along with the lower leve marvell_save_page(), marvell_select_page() and marvell_restore_page(). The result is a fair amount of out-of-line code and an assumption that it's a single register. As soon as a PHY has other requirements, these generic implementations don't work. Also, unfortunately, we can't get help from sparse for statically checking the locking - the __acquire()/__release() annotations don't work for mutexes.
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c index d82aa8cea401..05054770aefb 100644 --- a/drivers/net/phy/meson-gxl.c +++ b/drivers/net/phy/meson-gxl.c @@ -45,11 +45,13 @@ #define FR_PLL_DIV0 0x1c #define FR_PLL_DIV1 0x1d -static int meson_gxl_config_init(struct phy_device *phydev) +static int meson_gxl_open_banks(struct phy_device *phydev) { int ret; - /* Enable Analog and DSP register Bank access by */ + /* Enable Analog and DSP register Bank access by + * toggling TSTCNTL_TEST_MODE bit in the TSTCNTL register + */ ret = phy_write(phydev, TSTCNTL, 0); if (ret) return ret; @@ -59,55 +61,84 @@ static int meson_gxl_config_init(struct phy_device *phydev) ret = phy_write(phydev, TSTCNTL, 0); if (ret) return ret; - ret = phy_write(phydev, TSTCNTL, TSTCNTL_TEST_MODE); - if (ret) - return ret; + return phy_write(phydev, TSTCNTL, TSTCNTL_TEST_MODE); +} - /* Write CONFIG_A6*/ - ret = phy_write(phydev, TSTWRITE, 0x8e0d) +static void meson_gxl_close_banks(struct phy_device *phydev) +{ + phy_write(phydev, TSTCNTL, 0); +} + +static int meson_gxl_read_reg(struct phy_device *phydev, + unsigned int bank, unsigned int reg) +{ + int ret; + + ret = meson_gxl_open_banks(phydev); if (ret) - return ret; - ret = phy_write(phydev, TSTCNTL, - TSTCNTL_WRITE - | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_ANALOG_DSP) - | TSTCNTL_TEST_MODE - | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, A6_CONFIG_REG)); + goto out; + + ret = phy_write(phydev, TSTCNTL, TSTCNTL_READ | + FIELD_PREP(TSTCNTL_REG_BANK_SEL, bank) | + TSTCNTL_TEST_MODE | + FIELD_PREP(TSTCNTL_READ_ADDRESS, reg)); if (ret) - return ret; + goto out; - /* Enable fractional PLL */ - ret = phy_write(phydev, TSTWRITE, 0x0005); + ret = phy_read(phydev, TSTREAD1); +out: + /* Close the bank access on our way out */ + meson_gxl_close_banks(phydev); + return ret; +} + +static int meson_gxl_write_reg(struct phy_device *phydev, + unsigned int bank, unsigned int reg, + uint16_t value) +{ + int ret; + + ret = meson_gxl_open_banks(phydev); if (ret) - return ret; - ret = phy_write(phydev, TSTCNTL, - TSTCNTL_WRITE - | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST) - | TSTCNTL_TEST_MODE - | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_CONTROL)); + goto out; + + ret = phy_write(phydev, TSTWRITE, value); if (ret) - return ret; + goto out; - /* Program fraction FR_PLL_DIV1 */ - ret = phy_write(phydev, TSTWRITE, 0x029a); + ret = phy_write(phydev, TSTCNTL, TSTCNTL_WRITE | + FIELD_PREP(TSTCNTL_REG_BANK_SEL, bank) | + TSTCNTL_TEST_MODE | + FIELD_PREP(TSTCNTL_WRITE_ADDRESS, reg)); + +out: + /* Close the bank access on our way out */ + meson_gxl_close_banks(phydev); + return ret; +} + +static int meson_gxl_config_init(struct phy_device *phydev) +{ + int ret; + + /* Write CONFIG_A6*/ + ret = meson_gxl_write_reg(phydev, BANK_ANALOG_DSP, A6_CONFIG_REG, + 0x8e0d); if (ret) return ret; - ret = phy_write(phydev, TSTCNTL, - TSTCNTL_WRITE - | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST) - | TSTCNTL_TEST_MODE - | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_DIV1)); + + /* Enable fractional PLL */ + ret = meson_gxl_write_reg(phydev, BANK_BIST, FR_PLL_CONTROL, 0x5); if (ret) return ret; /* Program fraction FR_PLL_DIV1 */ - ret = phy_write(phydev, TSTWRITE, 0xaaaa); + ret = meson_gxl_write_reg(phydev, BANK_BIST, FR_PLL_DIV1, 0x029a); if (ret) return ret; - ret = phy_write(phydev, TSTCNTL, - TSTCNTL_WRITE - | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST) - | TSTCNTL_TEST_MODE - | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_DIV0)); + + /* Program fraction FR_PLL_DIV1 */ + ret = meson_gxl_write_reg(phydev, BANK_BIST, FR_PLL_DIV0, 0xaaaa); if (ret) return ret;
Add read and write helpers to manipulate banked registers on this PHY This helps clarify the settings applied to these registers in the init function and upcoming changes. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/net/phy/meson-gxl.c | 103 ++++++++++++++++++++++++++++---------------- 1 file changed, 67 insertions(+), 36 deletions(-)