| Message ID | 20211014135636.3644166-1-msp@baylibre.com |
|---|---|
| Headers | show |
| Series | thermal: mediatek: Add mt8365 support | expand |
On 14/10/2021 15:56, Markus Schneider-Pargmann wrote: > From: Fabien Parent <fparent@baylibre.com> > > mt8365 is similar to the other SoCs supported by the driver. It has only > one bank and 3 sensors that can be multiplexed. > > Additionally the buffer has to be enabled and connected to AUXADC > similar to the V2 version but at a different register offset. That's why > I added three new configuration values to define the register, mask and > bits to be set to be able to use it for both V2 and mt8365. > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > [Added apmixed control register logic] > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > --- > drivers/thermal/mtk_thermal.c | 91 ++++++++++++++++++++++++++++++++--- > 1 file changed, 85 insertions(+), 6 deletions(-) > > diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c > index 93ee043d70da..7a75ae8231f2 100644 > --- a/drivers/thermal/mtk_thermal.c > +++ b/drivers/thermal/mtk_thermal.c > @@ -31,6 +31,7 @@ > #define AUXADC_CON2_V 0x010 > #define AUXADC_DATA(channel) (0x14 + (channel) * 4) > > +#define APMIXED_SYS_TS_CON0 0x600 > #define APMIXED_SYS_TS_CON1 0x604 > > /* Thermal Controller Registers */ > @@ -245,6 +246,17 @@ enum mtk_thermal_version { > /* The calibration coefficient of sensor */ > #define MT8183_CALIBRATION 153 > > +/* MT8365 */ > +#define MT8365_TEMP_AUXADC_CHANNEL 11 > +#define MT8365_CALIBRATION 164 > +#define MT8365_NUM_CONTROLLER 1 > +#define MT8365_NUM_BANKS 1 > +#define MT8365_NUM_SENSORS 3 > +#define MT8365_NUM_SENSORS_PER_ZONE 3 You can get rid of these macros by using ARRAY_SIZE. eg. static const int mt8365_bank_data[] = { MT8365_TS1, MT8365_TS2, MT8365_TS3 }; ... .num_sensors = ARRAY_SIZE(mt8365_bank_data); ... > +#define MT8365_TS1 0 > +#define MT8365_TS2 1 > +#define MT8365_TS3 2 > + > struct mtk_thermal; > > struct thermal_bank_cfg { > @@ -271,6 +283,9 @@ struct mtk_thermal_data { > bool need_switch_bank; > struct thermal_bank_cfg bank_data[MAX_NUM_ZONES]; > enum mtk_thermal_version version; > + u32 apmixed_buffer_ctl_reg; > + u32 apmixed_buffer_ctl_mask; > + u32 apmixed_buffer_ctl_set; > }; > > struct mtk_thermal { > @@ -386,6 +401,24 @@ static const int mt7622_mux_values[MT7622_NUM_SENSORS] = { 0, }; > static const int mt7622_vts_index[MT7622_NUM_SENSORS] = { VTS1 }; > static const int mt7622_tc_offset[MT7622_NUM_CONTROLLER] = { 0x0, }; > > +/* MT8365 thermal sensor data */ > +static const int mt8365_bank_data[MT8365_NUM_SENSORS] = { > + MT8365_TS1, MT8365_TS2, MT8365_TS3 > +}; > + > +static const int mt8365_msr[MT8365_NUM_SENSORS_PER_ZONE] = { > + TEMP_MSR0, TEMP_MSR1, TEMP_MSR2 > +}; > + > +static const int mt8365_adcpnp[MT8365_NUM_SENSORS_PER_ZONE] = { > + TEMP_ADCPNP0, TEMP_ADCPNP1, TEMP_ADCPNP2 > +}; > + > +static const int mt8365_mux_values[MT8365_NUM_SENSORS] = { 0, 1, 2 }; > +static const int mt8365_tc_offset[MT8365_NUM_CONTROLLER] = { 0 }; > + > +static const int mt8365_vts_index[MT8365_NUM_SENSORS] = { VTS1, VTS2, VTS3 }; > + > /* > * The MT8173 thermal controller has four banks. Each bank can read up to > * four temperature sensors simultaneously. The MT8173 has a total of 5 > @@ -460,6 +493,40 @@ static const struct mtk_thermal_data mt2701_thermal_data = { > .version = MTK_THERMAL_V1, > }; > > +/* > + * The MT8365 thermal controller has one bank, which can read up to > + * four temperature sensors simultaneously. The MT8365 has a total of 3 > + * temperature sensors. > + * > + * The thermal core only gets the maximum temperature of this one bank, > + * so the bank concept wouldn't be necessary here. However, the SVS (Smart > + * Voltage Scaling) unit makes its decisions based on the same bank > + * data. > + */ > +static const struct mtk_thermal_data mt8365_thermal_data = { > + .auxadc_channel = MT8365_TEMP_AUXADC_CHANNEL, > + .num_banks = MT8365_NUM_BANKS, > + .num_sensors = MT8365_NUM_SENSORS, > + .vts_index = mt8365_vts_index, > + .cali_val = MT8365_CALIBRATION, > + .num_controller = MT8365_NUM_CONTROLLER, > + .controller_offset = mt8365_tc_offset, > + .need_switch_bank = false, > + .bank_data = { > + { > + .num_sensors = MT8365_NUM_SENSORS, > + .sensors = mt8365_bank_data > + }, > + }, > + .msr = mt8365_msr, > + .adcpnp = mt8365_adcpnp, > + .sensor_mux_values = mt8365_mux_values, > + .version = MTK_THERMAL_V1, > + .apmixed_buffer_ctl_reg = APMIXED_SYS_TS_CON0, > + .apmixed_buffer_ctl_mask = ~(u32)GENMASK(29, 28), > + .apmixed_buffer_ctl_set = 0, > +}; > + > /* > * The MT2712 thermal controller has one bank, which can read up to > * four temperature sensors simultaneously. The MT2712 has a total of 4 > @@ -514,6 +581,9 @@ static const struct mtk_thermal_data mt7622_thermal_data = { > .adcpnp = mt7622_adcpnp, > .sensor_mux_values = mt7622_mux_values, > .version = MTK_THERMAL_V2, > + .apmixed_buffer_ctl_reg = APMIXED_SYS_TS_CON1, > + .apmixed_buffer_ctl_mask = ~0x37, > + .apmixed_buffer_ctl_set = 0x1, Please change those literal into macros > }; > > /* > @@ -958,19 +1028,27 @@ static const struct of_device_id mtk_thermal_of_match[] = { > { > .compatible = "mediatek,mt8183-thermal", > .data = (void *)&mt8183_thermal_data, > + }, > + { > + .compatible = "mediatek,mt8365-thermal", > + .data = (void *)&mt8365_thermal_data, Is this cast really needed ? > }, { > }, > }; > MODULE_DEVICE_TABLE(of, mtk_thermal_of_match); > > -static void mtk_thermal_turn_on_buffer(void __iomem *apmixed_base) > +static void mtk_thermal_turn_on_buffer(struct mtk_thermal *mt, > + void __iomem *apmixed_base) > { > int tmp; > > - tmp = readl(apmixed_base + APMIXED_SYS_TS_CON1); > - tmp &= ~(0x37); > - tmp |= 0x1; > - writel(tmp, apmixed_base + APMIXED_SYS_TS_CON1); > + if (!mt->conf->apmixed_buffer_ctl_reg) > + return; > + > + tmp = readl(apmixed_base + mt->conf->apmixed_buffer_ctl_reg); > + tmp &= mt->conf->apmixed_buffer_ctl_mask; > + tmp |= mt->conf->apmixed_buffer_ctl_set; What is the goal of these two bits operations ? > + writel(tmp, apmixed_base + mt->conf->apmixed_buffer_ctl_reg); > udelay(200); > } > > @@ -1070,8 +1148,9 @@ static int mtk_thermal_probe(struct platform_device *pdev) > goto err_disable_clk_auxadc; > } > > + mtk_thermal_turn_on_buffer(mt, apmixed_base); > + > if (mt->conf->version == MTK_THERMAL_V2) { > - mtk_thermal_turn_on_buffer(apmixed_base); > mtk_thermal_release_periodic_ts(mt, auxadc_base); > } > >
Hi Daniel, On Sat, Oct 16, 2021 at 10:06:28PM +0200, Daniel Lezcano wrote: > On 14/10/2021 15:56, Markus Schneider-Pargmann wrote: > > From: Fabien Parent <fparent@baylibre.com> > > > > mt8365 is similar to the other SoCs supported by the driver. It has only > > one bank and 3 sensors that can be multiplexed. > > > > Additionally the buffer has to be enabled and connected to AUXADC > > similar to the V2 version but at a different register offset. That's why > > I added three new configuration values to define the register, mask and > > bits to be set to be able to use it for both V2 and mt8365. > > > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > > [Added apmixed control register logic] > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > --- > > drivers/thermal/mtk_thermal.c | 91 ++++++++++++++++++++++++++++++++--- > > 1 file changed, 85 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c > > index 93ee043d70da..7a75ae8231f2 100644 > > --- a/drivers/thermal/mtk_thermal.c > > +++ b/drivers/thermal/mtk_thermal.c > > @@ -31,6 +31,7 @@ > > #define AUXADC_CON2_V 0x010 > > #define AUXADC_DATA(channel) (0x14 + (channel) * 4) > > > > +#define APMIXED_SYS_TS_CON0 0x600 > > #define APMIXED_SYS_TS_CON1 0x604 > > > > /* Thermal Controller Registers */ > > @@ -245,6 +246,17 @@ enum mtk_thermal_version { > > /* The calibration coefficient of sensor */ > > #define MT8183_CALIBRATION 153 > > > > +/* MT8365 */ > > +#define MT8365_TEMP_AUXADC_CHANNEL 11 > > +#define MT8365_CALIBRATION 164 > > +#define MT8365_NUM_CONTROLLER 1 > > +#define MT8365_NUM_BANKS 1 > > +#define MT8365_NUM_SENSORS 3 > > +#define MT8365_NUM_SENSORS_PER_ZONE 3 > > You can get rid of these macros by using ARRAY_SIZE. > > eg. > > static const int mt8365_bank_data[] = { > MT8365_TS1, > MT8365_TS2, > MT8365_TS3 > }; > > ... > > .num_sensors = ARRAY_SIZE(mt8365_bank_data); > ... > Thanks, done, I also used ARRAY_SIZE() on the other array definitions to enforce that they have the correct size. > > > +#define MT8365_TS1 0 > > +#define MT8365_TS2 1 > > +#define MT8365_TS3 2 > > + > > struct mtk_thermal; > > > > struct thermal_bank_cfg { > > @@ -271,6 +283,9 @@ struct mtk_thermal_data { > > bool need_switch_bank; > > struct thermal_bank_cfg bank_data[MAX_NUM_ZONES]; > > enum mtk_thermal_version version; > > + u32 apmixed_buffer_ctl_reg; > > + u32 apmixed_buffer_ctl_mask; > > + u32 apmixed_buffer_ctl_set; > > }; > > > > struct mtk_thermal { > > @@ -386,6 +401,24 @@ static const int mt7622_mux_values[MT7622_NUM_SENSORS] = { 0, }; > > static const int mt7622_vts_index[MT7622_NUM_SENSORS] = { VTS1 }; > > static const int mt7622_tc_offset[MT7622_NUM_CONTROLLER] = { 0x0, }; > > > > +/* MT8365 thermal sensor data */ > > +static const int mt8365_bank_data[MT8365_NUM_SENSORS] = { > > + MT8365_TS1, MT8365_TS2, MT8365_TS3 > > +}; > > + > > +static const int mt8365_msr[MT8365_NUM_SENSORS_PER_ZONE] = { > > + TEMP_MSR0, TEMP_MSR1, TEMP_MSR2 > > +}; > > + > > +static const int mt8365_adcpnp[MT8365_NUM_SENSORS_PER_ZONE] = { > > + TEMP_ADCPNP0, TEMP_ADCPNP1, TEMP_ADCPNP2 > > +}; > > + > > +static const int mt8365_mux_values[MT8365_NUM_SENSORS] = { 0, 1, 2 }; > > +static const int mt8365_tc_offset[MT8365_NUM_CONTROLLER] = { 0 }; > > + > > +static const int mt8365_vts_index[MT8365_NUM_SENSORS] = { VTS1, VTS2, VTS3 }; > > + > > /* > > * The MT8173 thermal controller has four banks. Each bank can read up to > > * four temperature sensors simultaneously. The MT8173 has a total of 5 > > @@ -460,6 +493,40 @@ static const struct mtk_thermal_data mt2701_thermal_data = { > > .version = MTK_THERMAL_V1, > > }; > > > > +/* > > + * The MT8365 thermal controller has one bank, which can read up to > > + * four temperature sensors simultaneously. The MT8365 has a total of 3 > > + * temperature sensors. > > + * > > + * The thermal core only gets the maximum temperature of this one bank, > > + * so the bank concept wouldn't be necessary here. However, the SVS (Smart > > + * Voltage Scaling) unit makes its decisions based on the same bank > > + * data. > > + */ > > +static const struct mtk_thermal_data mt8365_thermal_data = { > > + .auxadc_channel = MT8365_TEMP_AUXADC_CHANNEL, > > + .num_banks = MT8365_NUM_BANKS, > > + .num_sensors = MT8365_NUM_SENSORS, > > + .vts_index = mt8365_vts_index, > > + .cali_val = MT8365_CALIBRATION, > > + .num_controller = MT8365_NUM_CONTROLLER, > > + .controller_offset = mt8365_tc_offset, > > + .need_switch_bank = false, > > + .bank_data = { > > + { > > + .num_sensors = MT8365_NUM_SENSORS, > > + .sensors = mt8365_bank_data > > + }, > > + }, > > + .msr = mt8365_msr, > > + .adcpnp = mt8365_adcpnp, > > + .sensor_mux_values = mt8365_mux_values, > > + .version = MTK_THERMAL_V1, > > + .apmixed_buffer_ctl_reg = APMIXED_SYS_TS_CON0, > > + .apmixed_buffer_ctl_mask = ~(u32)GENMASK(29, 28), > > + .apmixed_buffer_ctl_set = 0, > > +}; > > + > > /* > > * The MT2712 thermal controller has one bank, which can read up to > > * four temperature sensors simultaneously. The MT2712 has a total of 4 > > @@ -514,6 +581,9 @@ static const struct mtk_thermal_data mt7622_thermal_data = { > > .adcpnp = mt7622_adcpnp, > > .sensor_mux_values = mt7622_mux_values, > > .version = MTK_THERMAL_V2, > > + .apmixed_buffer_ctl_reg = APMIXED_SYS_TS_CON1, > > + .apmixed_buffer_ctl_mask = ~0x37, > > + .apmixed_buffer_ctl_set = 0x1, > > Please change those literal into macros done. > > > }; > > > > /* > > @@ -958,19 +1028,27 @@ static const struct of_device_id mtk_thermal_of_match[] = { > > { > > .compatible = "mediatek,mt8183-thermal", > > .data = (void *)&mt8183_thermal_data, > > + }, > > + { > > + .compatible = "mediatek,mt8365-thermal", > > + .data = (void *)&mt8365_thermal_data, > > Is this cast really needed ? No, removed and created another patch to fix all other ones. Would look weird otherwise. > > > }, { > > }, > > }; > > MODULE_DEVICE_TABLE(of, mtk_thermal_of_match); > > > > -static void mtk_thermal_turn_on_buffer(void __iomem *apmixed_base) > > +static void mtk_thermal_turn_on_buffer(struct mtk_thermal *mt, > > + void __iomem *apmixed_base) > > { > > int tmp; > > > > - tmp = readl(apmixed_base + APMIXED_SYS_TS_CON1); > > - tmp &= ~(0x37); > > - tmp |= 0x1; > > - writel(tmp, apmixed_base + APMIXED_SYS_TS_CON1); > > + if (!mt->conf->apmixed_buffer_ctl_reg) > > + return; > > + > > + tmp = readl(apmixed_base + mt->conf->apmixed_buffer_ctl_reg); > > + tmp &= mt->conf->apmixed_buffer_ctl_mask; > > + tmp |= mt->conf->apmixed_buffer_ctl_set; > > What is the goal of these two bits operations ? mt7622 needs to unset a few bits and set one bit in this register. mt8365 only unsets bits. For this purpose I created a _mask field to unset bits and a _set field to set bits. Would you suggest a different way? Thanks for your feedback. Best, Markus > > > + writel(tmp, apmixed_base + mt->conf->apmixed_buffer_ctl_reg); > > udelay(200); > > } > > > > @@ -1070,8 +1148,9 @@ static int mtk_thermal_probe(struct platform_device *pdev) > > goto err_disable_clk_auxadc; > > } > > > > + mtk_thermal_turn_on_buffer(mt, apmixed_base); > > + > > if (mt->conf->version == MTK_THERMAL_V2) { > > - mtk_thermal_turn_on_buffer(apmixed_base); > > mtk_thermal_release_periodic_ts(mt, auxadc_base); > > } > > > > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog
Hi Markus, On 18/10/2021 17:40, Markus Schneider-Pargmann wrote: [ ... ] >> >>> }, { >>> }, >>> }; >>> MODULE_DEVICE_TABLE(of, mtk_thermal_of_match); >>> >>> -static void mtk_thermal_turn_on_buffer(void __iomem *apmixed_base) >>> +static void mtk_thermal_turn_on_buffer(struct mtk_thermal *mt, >>> + void __iomem *apmixed_base) >>> { >>> int tmp; >>> >>> - tmp = readl(apmixed_base + APMIXED_SYS_TS_CON1); >>> - tmp &= ~(0x37); >>> - tmp |= 0x1; >>> - writel(tmp, apmixed_base + APMIXED_SYS_TS_CON1); >>> + if (!mt->conf->apmixed_buffer_ctl_reg) >>> + return; >>> + >>> + tmp = readl(apmixed_base + mt->conf->apmixed_buffer_ctl_reg); >>> + tmp &= mt->conf->apmixed_buffer_ctl_mask; >>> + tmp |= mt->conf->apmixed_buffer_ctl_set; >> >> What is the goal of these two bits operations ? > > mt7622 needs to unset a few bits and set one bit in this register. > mt8365 only unsets bits. For this purpose I created a _mask field to > unset bits and a _set field to set bits. > > Would you suggest a different way? No, I understand what you wanted to do. I was just unsure if there was a trick because of the tmp |= 0;