| Message ID | 20230524144539.62618-3-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | microchip_t1s: Update on Microchip 10BASE-T1S PHY driver | expand |
On Wed, May 24, 2023 at 08:15:35PM +0530, Parthiban Veerasooran wrote: > Replace read-modify-write code in the lan867x_config_init function to > avoid handling data type mismatch and to simplify the code. > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, May 24, 2023 at 08:15:35PM +0530, Parthiban Veerasooran wrote: > Replace read-modify-write code in the lan867x_config_init function to > avoid handling data type mismatch and to simplify the code. > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> > --- > drivers/net/phy/microchip_t1s.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c > index a42a6bb6e3bd..b5b5a95fa6e7 100644 > --- a/drivers/net/phy/microchip_t1s.c > +++ b/drivers/net/phy/microchip_t1s.c > @@ -31,19 +31,19 @@ > * W 0x1F 0x0099 0x7F80 ------ > */ > > -static const int lan867x_fixup_registers[12] = { > +static const u32 lan867x_fixup_registers[12] = { > 0x00D0, 0x00D1, 0x0084, 0x0085, > 0x008A, 0x0087, 0x0088, 0x008B, > 0x0080, 0x00F1, 0x0096, 0x0099, > }; > > -static const int lan867x_fixup_values[12] = { > +static const u16 lan867x_fixup_values[12] = { > 0x0002, 0x0000, 0x3380, 0x0006, > 0xC000, 0x801C, 0x033F, 0x0404, > 0x0600, 0x2400, 0x2000, 0x7F80, > }; > > -static const int lan867x_fixup_masks[12] = { > +static const u16 lan867x_fixup_masks[12] = { > 0x0E03, 0x0300, 0xFFC0, 0x000F, > 0xF800, 0x801C, 0x1FFF, 0xFFFF, > 0x0600, 0x7F00, 0x2000, 0xFFFF, > @@ -63,23 +63,22 @@ static int lan867x_config_init(struct phy_device *phydev) > * used, which might then write the same value back as read + modified. > */ > > - int reg_value; > int err; > - int reg; > > /* Read-Modified Write Pseudocode (from AN1699) > * current_val = read_register(mmd, addr) // Read current register value > * new_val = current_val AND (NOT mask) // Clear bit fields to be written > * new_val = new_val OR value // Set bits > - * write_register(mmd, addr, new_val) // Write back updated register value > + * write_register(mmd, addr, new_val) // Write back updated register value. > + * Although AN1699 says Read, Modify, Write, the write is not required if > + * the register already has the required value. > */ Nitpick, I think this block comment can be reduced to: /* The following block deviates from AN1699 which states that a values * should be written back, even if unmodified. * Which is not necessary, so it's safe to use phy_modify_mmd here.*/ The comment I added was intended to describe why I was doing weird things, but now I think it's more interesting to describe why we're deviating from the AN. Or the block comment could be dropped all togheter, I'm guessing no one is going to consult the AN if things 'just work' > for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) { > - reg = lan867x_fixup_registers[i]; > - reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg); > - reg_value &= ~lan867x_fixup_masks[i]; > - reg_value |= lan867x_fixup_values[i]; > - err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value); > - if (err != 0) > + err = phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + lan867x_fixup_registers[i], > + lan867x_fixup_masks[i], > + lan867x_fixup_values[i]); > + if (err) > return err; > } > > -- > 2.34.1 >
On Wed, May 24, 2023 at 08:15:35PM +0530, Parthiban Veerasooran wrote: > Replace read-modify-write code in the lan867x_config_init function to > avoid handling data type mismatch and to simplify the code. > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> > --- > drivers/net/phy/microchip_t1s.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c > index a42a6bb6e3bd..b5b5a95fa6e7 100644 > --- a/drivers/net/phy/microchip_t1s.c > +++ b/drivers/net/phy/microchip_t1s.c > @@ -31,19 +31,19 @@ > * W 0x1F 0x0099 0x7F80 ------ > */ > > -static const int lan867x_fixup_registers[12] = { > +static const u32 lan867x_fixup_registers[12] = { > 0x00D0, 0x00D1, 0x0084, 0x0085, > 0x008A, 0x0087, 0x0088, 0x008B, > 0x0080, 0x00F1, 0x0096, 0x0099, > }; > > -static const int lan867x_fixup_values[12] = { > +static const u16 lan867x_fixup_values[12] = { > 0x0002, 0x0000, 0x3380, 0x0006, > 0xC000, 0x801C, 0x033F, 0x0404, > 0x0600, 0x2400, 0x2000, 0x7F80, > }; > > -static const int lan867x_fixup_masks[12] = { > +static const u16 lan867x_fixup_masks[12] = { > 0x0E03, 0x0300, 0xFFC0, 0x000F, > 0xF800, 0x801C, 0x1FFF, 0xFFFF, > 0x0600, 0x7F00, 0x2000, 0xFFFF, > @@ -63,23 +63,22 @@ static int lan867x_config_init(struct phy_device *phydev) > * used, which might then write the same value back as read + modified. > */ > > - int reg_value; > int err; > - int reg; > > /* Read-Modified Write Pseudocode (from AN1699) > * current_val = read_register(mmd, addr) // Read current register value > * new_val = current_val AND (NOT mask) // Clear bit fields to be written > * new_val = new_val OR value // Set bits > - * write_register(mmd, addr, new_val) // Write back updated register value > + * write_register(mmd, addr, new_val) // Write back updated register value. > + * Although AN1699 says Read, Modify, Write, the write is not required if > + * the register already has the required value. > */ > for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) { > - reg = lan867x_fixup_registers[i]; > - reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg); > - reg_value &= ~lan867x_fixup_masks[i]; > - reg_value |= lan867x_fixup_values[i]; > - err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value); > - if (err != 0) > + err = phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + lan867x_fixup_registers[i], > + lan867x_fixup_masks[i], > + lan867x_fixup_values[i]); This change answers an uncertainty in the block comment in the top of this func, pasted here for your convenience /* HW quirk: Microchip states in the application note (AN1699) for the phy * that a set of read-modify-write (rmw) operations has to be performed * on a set of seemingly magic registers. * The result of these operations is just described as 'optimal performance' * Microchip gives no explanation as to what these mmd regs do, * in fact they are marked as reserved in the datasheet. * It is unclear if phy_modify_mmd would be safe to use or if a write * really has to happen to each register. * In order to exactly conform to what is stated in the AN phy_write_mmd is * used, which might then write the same value back as read + modified. */ This change also invalidates most of the comment. I think this should be reduced to something along the lines of: /* HW quirk: Microchip states in the application note (AN1699) for the phy * that a set of read-modify-write (rmw) operations has to be performed * on a set of seemingly magic registers. * The result of these operations is just described as 'optimal performance' * Microchip gives no explanation as to what these mmd regs do, * in fact they are marked as reserved in the datasheet.*/ Additionally I don't mind it if you change the tone of the comment. This was brought up in the sitdown we had, where it was explained from Microchip that documenting what the reg operations actually does would expose to much of the internal workings of the chip. Possibly this comment should move down to where the fixup reg operations are performed, and replace the comment about the 'read write modify' stuff all togheter. In my opinion I kind of loose context about what the func does when it first explains the fixup stuff, then actually does something with the STS2 regs, and finally actually does the fixup. > + if (err) > return err; > } > > -- > 2.34.1 >
> This change also invalidates most of the comment. I think this should be > reduced to something along the lines of: > /* HW quirk: Microchip states in the application note (AN1699) for the phy > * that a set of read-modify-write (rmw) operations has to be performed > * on a set of seemingly magic registers. > * The result of these operations is just described as 'optimal performance' > * Microchip gives no explanation as to what these mmd regs do, > * in fact they are marked as reserved in the datasheet.*/ I agree the comments should be reviewed in light of these changes. > > Additionally I don't mind it if you change the tone of the comment. This was brought > up in the sitdown we had, where it was explained from Microchip that > documenting what the reg operations actually does would expose to much > of the internal workings of the chip. They cannot care too much, or the firmware in the PHY would do this where it is all hidden away. Andrew
Hi Ramon, On 25/05/23 8:38 pm, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, May 24, 2023 at 08:15:35PM +0530, Parthiban Veerasooran wrote: >> Replace read-modify-write code in the lan867x_config_init function to >> avoid handling data type mismatch and to simplify the code. >> >> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> >> --- >> drivers/net/phy/microchip_t1s.c | 23 +++++++++++------------ >> 1 file changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c >> index a42a6bb6e3bd..b5b5a95fa6e7 100644 >> --- a/drivers/net/phy/microchip_t1s.c >> +++ b/drivers/net/phy/microchip_t1s.c >> @@ -31,19 +31,19 @@ >> * W 0x1F 0x0099 0x7F80 ------ >> */ >> >> -static const int lan867x_fixup_registers[12] = { >> +static const u32 lan867x_fixup_registers[12] = { >> 0x00D0, 0x00D1, 0x0084, 0x0085, >> 0x008A, 0x0087, 0x0088, 0x008B, >> 0x0080, 0x00F1, 0x0096, 0x0099, >> }; >> >> -static const int lan867x_fixup_values[12] = { >> +static const u16 lan867x_fixup_values[12] = { >> 0x0002, 0x0000, 0x3380, 0x0006, >> 0xC000, 0x801C, 0x033F, 0x0404, >> 0x0600, 0x2400, 0x2000, 0x7F80, >> }; >> >> -static const int lan867x_fixup_masks[12] = { >> +static const u16 lan867x_fixup_masks[12] = { >> 0x0E03, 0x0300, 0xFFC0, 0x000F, >> 0xF800, 0x801C, 0x1FFF, 0xFFFF, >> 0x0600, 0x7F00, 0x2000, 0xFFFF, >> @@ -63,23 +63,22 @@ static int lan867x_config_init(struct phy_device *phydev) >> * used, which might then write the same value back as read + modified. >> */ >> >> - int reg_value; >> int err; >> - int reg; >> >> /* Read-Modified Write Pseudocode (from AN1699) >> * current_val = read_register(mmd, addr) // Read current register value >> * new_val = current_val AND (NOT mask) // Clear bit fields to be written >> * new_val = new_val OR value // Set bits >> - * write_register(mmd, addr, new_val) // Write back updated register value >> + * write_register(mmd, addr, new_val) // Write back updated register value. >> + * Although AN1699 says Read, Modify, Write, the write is not required if >> + * the register already has the required value. >> */ > > Nitpick, I think this block comment can be reduced to: > /* The following block deviates from AN1699 which states that a values > * should be written back, even if unmodified. > * Which is not necessary, so it's safe to use phy_modify_mmd here.*/ > > The comment I added was intended to describe why I was doing weird > things, but now I think it's more interesting to describe why we're > deviating from the AN. > > Or the block comment could be dropped all togheter, I'm guessing no one > is going to consult the AN if things 'just work' > By consolidating all your comments in the other emails as well on this 2nd patch, do you agree for my below proposal? We will remove all block comments and simply put AN1699 reference as we did for lan865x_revb0_config_init with a small addition on top of phy_modify_mmd for loop? so the comment will look like below, /* Reference to AN1699 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8670-1-2-config-60001699.pdf * AN1699 says Read, Modify, Write, but the Write is not required if the register already has the required value. So it is safe to use phy_modify_mmd here. */ So in future, if someone wants to know about this configuration they can simply refer the AN1699. What do you think? Best Regards, Parthiban V >> for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) { >> - reg = lan867x_fixup_registers[i]; >> - reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg); >> - reg_value &= ~lan867x_fixup_masks[i]; >> - reg_value |= lan867x_fixup_values[i]; >> - err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value); >> - if (err != 0) >> + err = phy_modify_mmd(phydev, MDIO_MMD_VEND2, >> + lan867x_fixup_registers[i], >> + lan867x_fixup_masks[i], >> + lan867x_fixup_values[i]); >> + if (err) >> return err; >> } >> >> -- >> 2.34.1 >>
On Fri, May 26, 2023 at 05:48:25AM +0000, Parthiban.Veerasooran@microchip.com wrote: > Hi Ramon, > > Nitpick, I think this block comment can be reduced to: > > /* The following block deviates from AN1699 which states that a values > > * should be written back, even if unmodified. > > * Which is not necessary, so it's safe to use phy_modify_mmd here.*/ > > > > The comment I added was intended to describe why I was doing weird > > things, but now I think it's more interesting to describe why we're > > deviating from the AN. > > > > Or the block comment could be dropped all togheter, I'm guessing no one > > is going to consult the AN if things 'just work' > > > By consolidating all your comments in the other emails as well on this > 2nd patch, do you agree for my below proposal? > > We will remove all block comments and simply put AN1699 reference as we > did for lan865x_revb0_config_init with a small addition on top of > phy_modify_mmd for loop? so the comment will look like below, > > /* Reference to AN1699 > * > https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8670-1-2-config-60001699.pdf > * AN1699 says Read, Modify, Write, but the Write is not required if > the register already has the required value. So it is safe to use > phy_modify_mmd here. > */ > > So in future, if someone wants to know about this configuration they can > simply refer the AN1699. > > What do you think? > I'm not sure about the link, resources have a tendency to move. Otherwise LGTM > Best Regards, > Parthiban V
On 26/05/23 12:40 pm, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, May 26, 2023 at 05:48:25AM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi Ramon, >>> Nitpick, I think this block comment can be reduced to: >>> /* The following block deviates from AN1699 which states that a values >>> * should be written back, even if unmodified. >>> * Which is not necessary, so it's safe to use phy_modify_mmd here.*/ >>> >>> The comment I added was intended to describe why I was doing weird >>> things, but now I think it's more interesting to describe why we're >>> deviating from the AN. >>> >>> Or the block comment could be dropped all togheter, I'm guessing no one >>> is going to consult the AN if things 'just work' >>> >> By consolidating all your comments in the other emails as well on this >> 2nd patch, do you agree for my below proposal? >> >> We will remove all block comments and simply put AN1699 reference as we >> did for lan865x_revb0_config_init with a small addition on top of >> phy_modify_mmd for loop? so the comment will look like below, >> >> /* Reference to AN1699 >> * >> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8670-1-2-config-60001699.pdf >> * AN1699 says Read, Modify, Write, but the Write is not required if >> the register already has the required value. So it is safe to use >> phy_modify_mmd here. >> */ >> >> So in future, if someone wants to know about this configuration they can >> simply refer the AN1699. >> >> What do you think? >> > > I'm not sure about the link, resources have a tendency to move. Yes, I agree with you but somehow there is no way for giving the reference to this document. May be we will keep this link for the reference, later if someone is not able to access the link then they can request Microchip to get the document. What do you think about this proposal? If you agree then I will proceed for preparing the next version with your comments. > Otherwise LGTM > >> Best Regards, >> Parthiban V
> >> /* Reference to AN1699 > >> * > >> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8670-1-2-config-60001699.pdf > >> * AN1699 says Read, Modify, Write, but the Write is not required if > >> the register already has the required value. So it is safe to use > >> phy_modify_mmd here. > >> */ > >> > >> So in future, if someone wants to know about this configuration they can > >> simply refer the AN1699. > >> > >> What do you think? > >> > > > > I'm not sure about the link, resources have a tendency to move. > Yes, I agree with you but somehow there is no way for giving the > reference to this document. May be we will keep this link for the > reference, later if someone is not able to access the link then they can > request Microchip to get the document. > > What do you think about this proposal? If you agree then I will proceed > for preparing the next version with your comments. Thumbs up from me R > > Otherwise LGTM > > > >> Best Regards, > >> Parthiban V >
Hi Ramon, On 26/05/23 7:32 pm, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>>> /* Reference to AN1699 >>>> * >>>> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8670-1-2-config-60001699.pdf >>>> * AN1699 says Read, Modify, Write, but the Write is not required if >>>> the register already has the required value. So it is safe to use >>>> phy_modify_mmd here. >>>> */ >>>> >>>> So in future, if someone wants to know about this configuration they can >>>> simply refer the AN1699. >>>> >>>> What do you think? >>>> >>> >>> I'm not sure about the link, resources have a tendency to move. >> Yes, I agree with you but somehow there is no way for giving the >> reference to this document. May be we will keep this link for the >> reference, later if someone is not able to access the link then they can >> request Microchip to get the document. >> >> What do you think about this proposal? If you agree then I will proceed >> for preparing the next version with your comments. > > Thumbs up from me Thanks. Best Regards, Parthiban V > R > >>> Otherwise LGTM >>> >>>> Best Regards, >>>> Parthiban V >>
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index a42a6bb6e3bd..b5b5a95fa6e7 100644 --- a/drivers/net/phy/microchip_t1s.c +++ b/drivers/net/phy/microchip_t1s.c @@ -31,19 +31,19 @@ * W 0x1F 0x0099 0x7F80 ------ */ -static const int lan867x_fixup_registers[12] = { +static const u32 lan867x_fixup_registers[12] = { 0x00D0, 0x00D1, 0x0084, 0x0085, 0x008A, 0x0087, 0x0088, 0x008B, 0x0080, 0x00F1, 0x0096, 0x0099, }; -static const int lan867x_fixup_values[12] = { +static const u16 lan867x_fixup_values[12] = { 0x0002, 0x0000, 0x3380, 0x0006, 0xC000, 0x801C, 0x033F, 0x0404, 0x0600, 0x2400, 0x2000, 0x7F80, }; -static const int lan867x_fixup_masks[12] = { +static const u16 lan867x_fixup_masks[12] = { 0x0E03, 0x0300, 0xFFC0, 0x000F, 0xF800, 0x801C, 0x1FFF, 0xFFFF, 0x0600, 0x7F00, 0x2000, 0xFFFF, @@ -63,23 +63,22 @@ static int lan867x_config_init(struct phy_device *phydev) * used, which might then write the same value back as read + modified. */ - int reg_value; int err; - int reg; /* Read-Modified Write Pseudocode (from AN1699) * current_val = read_register(mmd, addr) // Read current register value * new_val = current_val AND (NOT mask) // Clear bit fields to be written * new_val = new_val OR value // Set bits - * write_register(mmd, addr, new_val) // Write back updated register value + * write_register(mmd, addr, new_val) // Write back updated register value. + * Although AN1699 says Read, Modify, Write, the write is not required if + * the register already has the required value. */ for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) { - reg = lan867x_fixup_registers[i]; - reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg); - reg_value &= ~lan867x_fixup_masks[i]; - reg_value |= lan867x_fixup_values[i]; - err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value); - if (err != 0) + err = phy_modify_mmd(phydev, MDIO_MMD_VEND2, + lan867x_fixup_registers[i], + lan867x_fixup_masks[i], + lan867x_fixup_values[i]); + if (err) return err; }
Replace read-modify-write code in the lan867x_config_init function to avoid handling data type mismatch and to simplify the code. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/phy/microchip_t1s.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)