| Message ID | 20240306085017.21731-4-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface | expand |
> enum oa_tc6_register_op { > + OA_TC6_CTRL_REG_READ = 0, > OA_TC6_CTRL_REG_WRITE = 1, > }; I thought it looked a little odd when the enum was added in the previous patch with the first value of 1, and only one value. Now it makes more sense. The actual value appears to not matter? It is always > + if (reg_op == OA_TC6_CTRL_REG_WRITE) So i would drop the numbering, and leave it to the compiler. The patches will then look less odd. > +static int oa_tc6_check_ctrl_read_reply(struct oa_tc6 *tc6, u8 size) > +{ > + u32 *tx_buf = tc6->spi_ctrl_tx_buf; > + u32 *rx_buf = tc6->spi_ctrl_rx_buf + OA_TC6_CTRL_IGNORED_SIZE; > + > + /* The echoed control read header must match with the one that was > + * transmitted. > + */ > + if (*tx_buf != *rx_buf) > + return -ENODEV; Another case where -EPROTO might be better? Andrew
Hi Andrew, On 07/03/24 5:49 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> enum oa_tc6_register_op { >> + OA_TC6_CTRL_REG_READ = 0, >> OA_TC6_CTRL_REG_WRITE = 1, >> }; > > I thought it looked a little odd when the enum was added in the > previous patch with the first value of 1, and only one value. Now it > makes more sense. Ok. > > The actual value appears to not matter? It is always > >> + if (reg_op == OA_TC6_CTRL_REG_WRITE) > > So i would drop the numbering, and leave it to the compiler. The > patches will then look less odd. "drop the numbering", do you refer to this patch alone or previous patch also? If it is for this patch alone then it makes sense as they are going to be 0 and 1 anyway. But if we drop the numbering in the previous patch it will become 0 which will create an issue in the below line as it needs 1, FIELD_PREP(OA_TC6_CTRL_HEADER_WRITE, reg_op) Could you clarify? or do I have any misunderstanding here? > >> +static int oa_tc6_check_ctrl_read_reply(struct oa_tc6 *tc6, u8 size) >> +{ >> + u32 *tx_buf = tc6->spi_ctrl_tx_buf; >> + u32 *rx_buf = tc6->spi_ctrl_rx_buf + OA_TC6_CTRL_IGNORED_SIZE; >> + >> + /* The echoed control read header must match with the one that was >> + * transmitted. >> + */ >> + if (*tx_buf != *rx_buf) >> + return -ENODEV; > > Another case where -EPROTO might be better? Yes, I will use -EPROTO in the next version as it is likely "Protocol error" Best regards, Parthiban V > > Andrew >
On Thu, Mar 07, 2024 at 07:04:20AM +0000, Parthiban.Veerasooran@microchip.com wrote: > Hi Andrew, > > On 07/03/24 5:49 am, Andrew Lunn wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > >> enum oa_tc6_register_op { > >> + OA_TC6_CTRL_REG_READ = 0, > >> OA_TC6_CTRL_REG_WRITE = 1, > >> }; > > > > I thought it looked a little odd when the enum was added in the > > previous patch with the first value of 1, and only one value. Now it > > makes more sense. > Ok. > > > > The actual value appears to not matter? It is always > > > >> + if (reg_op == OA_TC6_CTRL_REG_WRITE) > > > > So i would drop the numbering, and leave it to the compiler. The > > patches will then look less odd. > "drop the numbering", do you refer to this patch alone or previous patch > also? If it is for this patch alone then it makes sense as they are > going to be 0 and 1 anyway. But if we drop the numbering in the previous > patch it will become 0 which will create an issue in the below line as > it needs 1, > > FIELD_PREP(OA_TC6_CTRL_HEADER_WRITE, reg_op) That is why i asked: > The actual value appears to not matter? It is always > > + if (reg_op == OA_TC6_CTRL_REG_WRITE) So the actual value does matter, so keep it in the previous patch. Does the value of OA_TC6_CTRL_REG_READ matter? Is it also used in FIELD_PREP etc? If not, taking away the = 0 will emphasise that OA_TC6_CTRL_REG_WRITE has to be 1. Andrew
Hi Andrew, On 07/03/24 6:52 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Thu, Mar 07, 2024 at 07:04:20AM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi Andrew, >> >> On 07/03/24 5:49 am, Andrew Lunn wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>>> enum oa_tc6_register_op { >>>> + OA_TC6_CTRL_REG_READ = 0, >>>> OA_TC6_CTRL_REG_WRITE = 1, >>>> }; >>> >>> I thought it looked a little odd when the enum was added in the >>> previous patch with the first value of 1, and only one value. Now it >>> makes more sense. >> Ok. >>> >>> The actual value appears to not matter? It is always >>> >>>> + if (reg_op == OA_TC6_CTRL_REG_WRITE) >>> >>> So i would drop the numbering, and leave it to the compiler. The >>> patches will then look less odd. >> "drop the numbering", do you refer to this patch alone or previous patch >> also? If it is for this patch alone then it makes sense as they are >> going to be 0 and 1 anyway. But if we drop the numbering in the previous >> patch it will become 0 which will create an issue in the below line as >> it needs 1, >> >> FIELD_PREP(OA_TC6_CTRL_HEADER_WRITE, reg_op) > > That is why i asked: > >> The actual value appears to not matter? It is always >> >> + if (reg_op == OA_TC6_CTRL_REG_WRITE) > > So the actual value does matter, so keep it in the previous patch. > Does the value of OA_TC6_CTRL_REG_READ matter? Is it also used in > FIELD_PREP etc? If not, taking away the = 0 will emphasise that > OA_TC6_CTRL_REG_WRITE has to be 1. Sorry, I have done a mistake here that's confusing. The define name OA_TC6_CTRL_HEADER_WRITE in the FIELD_PREP is supposed to be OA_TC6_CTRL_HEADER_WRITE_NOT_READ. This bit field in the control command header differentiates the type of operation. If it is 0, then register read command and if it is 1, then register write command. So regop in the FIELD_PREP actually sets the type of operation. The values of both OA_TC6_CTRL_REG_READ and OA_TC6_CTRL_REG_WRITE are matters here. So let's keep the numbering for both as it is now. But I will change the bit field define name as OA_TC6_CTRL_HEADER_WRITE_NOT_READ in the next version. Hope you are fine with it? Best regards, Parthiban V > > Andrew >
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c index 82131f865fe7..35e377577ba4 100644 --- a/drivers/net/ethernet/oa_tc6.c +++ b/drivers/net/ethernet/oa_tc6.c @@ -38,6 +38,7 @@ enum oa_tc6_header_type { }; enum oa_tc6_register_op { + OA_TC6_CTRL_REG_READ = 0, OA_TC6_CTRL_REG_WRITE = 1, }; @@ -113,7 +114,8 @@ static void oa_tc6_prepare_ctrl_spi_buf(struct oa_tc6 *tc6, u32 address, *tx_buf = oa_tc6_prepare_ctrl_header(address, length, reg_op); - oa_tc6_update_ctrl_write_data(tc6, value, length); + if (reg_op == OA_TC6_CTRL_REG_WRITE) + oa_tc6_update_ctrl_write_data(tc6, value, length); } static int oa_tc6_check_ctrl_write_reply(struct oa_tc6 *tc6, u8 size) @@ -132,6 +134,30 @@ static int oa_tc6_check_ctrl_write_reply(struct oa_tc6 *tc6, u8 size) return 0; } +static int oa_tc6_check_ctrl_read_reply(struct oa_tc6 *tc6, u8 size) +{ + u32 *tx_buf = tc6->spi_ctrl_tx_buf; + u32 *rx_buf = tc6->spi_ctrl_rx_buf + OA_TC6_CTRL_IGNORED_SIZE; + + /* The echoed control read header must match with the one that was + * transmitted. + */ + if (*tx_buf != *rx_buf) + return -ENODEV; + + return 0; +} + +static void oa_tc6_copy_ctrl_read_data(struct oa_tc6 *tc6, u32 value[], + u8 length) +{ + __be32 *rx_buf = tc6->spi_ctrl_rx_buf + OA_TC6_CTRL_IGNORED_SIZE + + OA_TC6_CTRL_HEADER_SIZE; + + for (int i = 0; i < length; i++) + value[i] = be32_to_cpu(*rx_buf++); +} + static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 address, u32 value[], u8 length, enum oa_tc6_register_op reg_op) { @@ -152,8 +178,62 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 address, u32 value[], } /* Check echoed/received control write command reply for errors */ - return oa_tc6_check_ctrl_write_reply(tc6, size); + if (reg_op == OA_TC6_CTRL_REG_WRITE) + return oa_tc6_check_ctrl_write_reply(tc6, size); + + /* Check echoed/received control read command reply for errors */ + ret = oa_tc6_check_ctrl_read_reply(tc6, size); + if (ret) + return ret; + + oa_tc6_copy_ctrl_read_data(tc6, value, length); + + return 0; +} + +/** + * oa_tc6_read_registers - function for reading multiple consecutive registers. + * @tc6: oa_tc6 struct. + * @address: address of the first register to be read in the MAC-PHY. + * @value: values to be read from the starting register address @address. + * @length: number of consecutive registers to be read from @address. + * + * Maximum of 128 consecutive registers can be read starting at @address. + * + * Returns 0 on success otherwise failed. + */ +int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 address, u32 value[], + u8 length) +{ + int ret; + + if (!length || length > OA_TC6_CTRL_MAX_REGISTERS) { + dev_err(&tc6->spi->dev, "Invalid register length parameter\n"); + return -EINVAL; + } + + mutex_lock(&tc6->spi_ctrl_lock); + ret = oa_tc6_perform_ctrl(tc6, address, value, length, + OA_TC6_CTRL_REG_READ); + mutex_unlock(&tc6->spi_ctrl_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(oa_tc6_read_registers); + +/** + * oa_tc6_read_register - function for reading a MAC-PHY register. + * @tc6: oa_tc6 struct. + * @address: register address of the MAC-PHY to be read. + * @value: value read from the @address register address of the MAC-PHY. + * + * Returns 0 on success otherwise failed. + */ +int oa_tc6_read_register(struct oa_tc6 *tc6, u32 address, u32 *value) +{ + return oa_tc6_read_registers(tc6, address, value, 1); } +EXPORT_SYMBOL_GPL(oa_tc6_read_register); /** * oa_tc6_write_registers - function for writing multiple consecutive registers. diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h index 99c490f1c8a8..85aeecf87306 100644 --- a/include/linux/oa_tc6.h +++ b/include/linux/oa_tc6.h @@ -15,3 +15,6 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi); int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value); int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 address, u32 value[], u8 length); +int oa_tc6_read_register(struct oa_tc6 *tc6, u32 address, u32 *value); +int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 address, u32 value[], + u8 length);
Implement register read operation according to the control communication specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface document. Control read commands are used by the SPI host to read registers within the MAC-PHY. Each control read commands are composed of a 32 bits control command header. The MAC-PHY ignores all data from the SPI host following the control header for the remainder of the control read command. Control read commands can read either a single register or multiple consecutive registers. When multiple consecutive registers are read, the address is automatically post-incremented by the MAC-PHY. Reading any unimplemented or undefined registers shall return zero. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/ethernet/oa_tc6.c | 84 ++++++++++++++++++++++++++++++++++- include/linux/oa_tc6.h | 3 ++ 2 files changed, 85 insertions(+), 2 deletions(-)